Update has_body_permission_to to allow superusers.

6 files changed, 22 insertions, 21 deletions

diff --git a/perllib/FixMyStreet/App/Controller/Admin.pm b/perllib/FixMyStreet/App/Controller/Admin.pm
index 592d37d4e..d8c5cdf6d 100644
--- a/perllib/FixMyStreet/App/Controller/Admin.pm
+++ b/perllib/FixMyStreet/App/Controller/Admin.pm
@@ -1006,10 +1006,9 @@ sub load_template_body : Private {
     my ($self, $c, $body_id) = @_;
 
     my $zurich_user = $c->user->from_body && $c->cobrand->moniker eq 'zurich';
-    my $has_permission = $c->user->has_body_permission_to('template_edit') &&
-                         $c->user->from_body->id eq $body_id;
+    my $has_permission = $c->user->has_body_permission_to('template_edit', $body_id);
 
-    unless ( $c->user->is_superuser || $zurich_user || $has_permission ) {
+    unless ( $zurich_user || $has_permission ) {
         $c->detach( '/page_error_404_not_found', [] );
     }
 
@@ -1235,7 +1234,7 @@ sub user_edit : Path('user_edit') : Args(1) {
     my $user = $c->cobrand->users->find( { id => $id } );
     $c->detach( '/page_error_404_not_found', [] ) unless $user;
 
-    unless ( $c->user->is_superuser || $c->user->has_body_permission_to('user_edit') || $c->cobrand->moniker eq 'zurich' ) {
+    unless ( $c->user->has_body_permission_to('user_edit') || $c->cobrand->moniker eq 'zurich' ) {
         $c->detach('/page_error_403_access_denied', []);
     }
 
diff --git a/perllib/FixMyStreet/App/Controller/Admin/ResponsePriorities.pm b/perllib/FixMyStreet/App/Controller/Admin/ResponsePriorities.pm
index 032e593c6..a6c13c117 100644
--- a/perllib/FixMyStreet/App/Controller/Admin/ResponsePriorities.pm
+++ b/perllib/FixMyStreet/App/Controller/Admin/ResponsePriorities.pm
@@ -92,10 +92,9 @@ sub edit : Path : Args(2) {
 sub load_user_body : Private {
     my ($self, $c, $body_id) = @_;
 
-    my $has_permission = $c->user->has_body_permission_to('responsepriority_edit') &&
-                         $c->user->from_body->id eq $body_id;
+    my $has_permission = $c->user->has_body_permission_to('responsepriority_edit', $body_id);
 
-    unless ( $c->user->is_superuser || $has_permission ) {
+    unless ( $has_permission ) {
         $c->detach( '/page_error_404_not_found' );
     }
 
diff --git a/perllib/FixMyStreet/Cobrand/Default.pm b/perllib/FixMyStreet/Cobrand/Default.pm
index 27111deb2..61982c47a 100644
--- a/perllib/FixMyStreet/Cobrand/Default.pm
+++ b/perllib/FixMyStreet/Cobrand/Default.pm
@@ -646,27 +646,27 @@ sub admin_pages {
         $pages->{config} = [ _('Configuration'), 9];
     };
     # And some that need special permissions
-    if ( $user->is_superuser || $user->has_body_permission_to('category_edit') ) {
+    if ( $user->has_body_permission_to('category_edit') ) {
         my $page_title = $user->is_superuser ? _('Bodies') : _('Categories');
         $pages->{bodies} = [ $page_title, 1 ];
         $pages->{body} = [ undef, undef ];
     }
-    if ( $user->is_superuser || $user->has_body_permission_to('report_edit') ) {
+    if ( $user->has_body_permission_to('report_edit') ) {
         $pages->{reports} = [ _('Reports'), 2 ];
         $pages->{report_edit} = [ undef, undef ];
         $pages->{update_edit} = [ undef, undef ];
         $pages->{abuse_edit} = [ undef, undef ];
     }
-    if ( $user->is_superuser || $user->has_body_permission_to('template_edit') ) {
+    if ( $user->has_body_permission_to('template_edit') ) {
         $pages->{templates} = [ _('Templates'), 3 ];
         $pages->{template_edit} = [ undef, undef ];
     };
-    if ( $user->is_superuser || $user->has_body_permission_to('responsepriority_edit') ) {
+    if ( $user->has_body_permission_to('responsepriority_edit') ) {
         $pages->{responsepriorities} = [ _('Priorities'), 4 ];
         $pages->{responsepriority_edit} = [ undef, undef ];
     };
 
-    if ( $user->is_superuser || $user->has_body_permission_to('user_edit') ) {
+    if ( $user->has_body_permission_to('user_edit') ) {
         $pages->{users} = [ _('Users'), 6 ];
         $pages->{user_edit} = [ undef, undef ];
     }
diff --git a/perllib/FixMyStreet/DB/Result/User.pm b/perllib/FixMyStreet/DB/Result/User.pm
index 72acb6940..135f9b4a5 100644
--- a/perllib/FixMyStreet/DB/Result/User.pm
+++ b/perllib/FixMyStreet/DB/Result/User.pm
@@ -287,23 +287,26 @@ sub has_permission_to {
 
 =head2 has_body_permission_to
 
-Checks if the User has a from_body set, and the specified permission on that body.
+Checks if the User has a from_body set, the specified permission on that body,
+and optionally that their from_body is one particular body.
 
 Instead of saying:
 
-    ($user->from_body && $user->has_permission_to('user_edit', $user->from_body->id))
+    ($user->from_body && $user->from_body->id == $body_id && $user->has_permission_to('user_edit', $body_id))
 
 You can just say:
 
-    $user->has_body_permission_to('user_edit')
-
-NB unlike has_permission_to, this doesn't blindly return 1 if the user is a superuser.
+    $user->has_body_permission_to('user_edit', $body_id)
 
 =cut
 
 sub has_body_permission_to {
-    my ($self, $permission_type) = @_;
+    my ($self, $permission_type, $body_id) = @_;
+
+    return 1 if $self->is_superuser;
+
     return unless $self->from_body;
+    return if $body_id && $self->from_body->id != $body_id;
 
     return $self->has_permission_to($permission_type, $self->from_body->id);
 }
diff --git a/templates/web/base/admin/user-form.html b/templates/web/base/admin/user-form.html
index 17230e940..0f5452b0a 100644
--- a/templates/web/base/admin/user-form.html
+++ b/templates/web/base/admin/user-form.html
@@ -47,7 +47,7 @@
                 [% loc("Staff users have permission to log in to the admin.") %]
               </p>
             </div>
-            [% loc('Staff:') %] <input type="checkbox" id="body" name="body" value="[% c.user.from_body.id %]" [% user.from_body.id == c.user.from_body.id ? ' checked' : '' %] [% 'disabled' UNLESS c.user.is_superuser OR c.user.has_body_permission_to('user_assign_body') %]>
+            [% loc('Staff:') %] <input type="checkbox" id="body" name="body" value="[% c.user.from_body.id %]" [% user.from_body.id == c.user.from_body.id ? ' checked' : '' %] [% 'disabled' UNLESS c.user.has_body_permission_to('user_assign_body') %]>
           </li>
         [% END %]
 
@@ -162,7 +162,7 @@
                 [% FOREACH permission IN group.value %]
                   <li>
                     <label class="inline">
-                      <input type="checkbox" id="perms_[% permission.key %]" name="permissions[[% permission.key %]]" [% "checked" IF user.has_body_permission_to(permission.key) %]>
+                      <input type="checkbox" id="perms_[% permission.key %]" name="permissions[[% permission.key %]]" [% "checked" IF NOT user.is_superuser AND user.has_body_permission_to(permission.key) %]>
                       [% permission.value %]
                     </label>
                   </li>
diff --git a/templates/web/base/admin/users.html b/templates/web/base/admin/users.html
index 757046bcf..7bbe04e1e 100644
--- a/templates/web/base/admin/users.html
+++ b/templates/web/base/admin/users.html
@@ -26,7 +26,7 @@
         <td>[% PROCESS value_or_nbsp value=user.name %]</td> 
         <td><a href="[% c.uri_for( 'reports', search => user.email ) %]">[% PROCESS value_or_nbsp value=user.email %]</a></td> 
         <td>[% PROCESS value_or_nbsp value=user.from_body.name %]
-            [% IF user.has_body_permission_to('moderate') %] * [% END %]
+            [% IF NOT user.is_superuser AND user.has_body_permission_to('moderate') %] * [% END %]
         </td>
       [% IF c.cobrand.moniker != 'zurich' %]
         <td>[% user.flagged == 2 ? loc('(Email in abuse table)') : user.flagged ? loc('Yes') : '&nbsp;' %]</td>