Simplify some permissions logic with extra helper method

7 files changed, 33 insertions, 11 deletions

diff --git a/perllib/FixMyStreet/App/Controller/Admin.pm b/perllib/FixMyStreet/App/Controller/Admin.pm
index 3c02c1318..66b46877f 100644
--- a/perllib/FixMyStreet/App/Controller/Admin.pm
+++ b/perllib/FixMyStreet/App/Controller/Admin.pm
@@ -982,9 +982,8 @@ 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->from_body &&
-                         $c->user->from_body->id eq $body_id &&
-                         $c->user->has_permission_to('template_edit', $body_id);
+    my $has_permission = $c->user->has_body_permission_to('template_edit') &&
+                         $c->user->from_body->id eq $body_id;
 
     unless ( $c->user->is_superuser || $zurich_user || $has_permission ) {
         $c->detach( '/page_error_404_not_found' );
@@ -1212,7 +1211,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_permission_to('user_edit', $c->user->from_body->id) ) ) {
+    unless ( $c->user->is_superuser || $c->user->has_body_permission_to('user_edit') ) {
         $c->detach('/page_error_403_access_denied', []);
     }
 
@@ -1249,7 +1248,7 @@ sub user_edit : Path('user_edit') : Args(1) {
         # set from_body to the same value as their own from_body.
         if ( $c->user->is_superuser ) {
             $user->from_body( $c->get_param('body') || undef );
-        } elsif ( $c->user->has_permission_to('user_assign_body', $c->user->from_body->id ) &&
+        } elsif ( $c->user->has_body_permission_to('user_assign_body') &&
                   $c->get_param('body') && $c->get_param('body') eq $c->user->from_body->id ) {
             $user->from_body( $c->user->from_body );
         } else {
diff --git a/perllib/FixMyStreet/App/Controller/My.pm b/perllib/FixMyStreet/App/Controller/My.pm
index b15750c98..b7fabcf4c 100644
--- a/perllib/FixMyStreet/App/Controller/My.pm
+++ b/perllib/FixMyStreet/App/Controller/My.pm
@@ -41,7 +41,7 @@ sub planned : Local : Args(0) {
     my ( $self, $c ) = @_;
 
     $c->detach('/page_error_403_access_denied', [])
-        unless $c->user->from_body && $c->user->has_permission_to('planned_reports', $c->user->from_body->id);
+        unless $c->user->has_body_permission_to('planned_reports');
 
     $c->stash->{problems_rs} = $c->user->active_planned_reports;
     $c->forward('get_problems');
diff --git a/perllib/FixMyStreet/DB/Result/User.pm b/perllib/FixMyStreet/DB/Result/User.pm
index 697cfedf6..6444cfe6a 100644
--- a/perllib/FixMyStreet/DB/Result/User.pm
+++ b/perllib/FixMyStreet/DB/Result/User.pm
@@ -254,6 +254,29 @@ sub has_permission_to {
     return $permission ? 1 : undef;
 }
 
+=head2 has_body_permission_to
+
+Checks if the User has a from_body set, and the specified permission on that body.
+
+Instead of saying:
+
+    ($user->from_body && $user->has_permission_to('user_edit', $user->from_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.
+
+=cut
+
+sub has_body_permission_to {
+    my ($self, $permission_type) = @_;
+    return unless $self->from_body;
+
+    return $self->has_permission_to($permission_type, $self->from_body->id);
+}
+
 sub contributing_as {
     my ($self, $other, $c, $bodies) = @_;
     $bodies = join(',', keys %$bodies) if ref $bodies eq 'HASH';
diff --git a/t/app/controller/admin_permissions.t b/t/app/controller/admin_permissions.t
index 3809a9d67..2c271ba4c 100644
--- a/t/app/controller/admin_permissions.t
+++ b/t/app/controller/admin_permissions.t
@@ -167,7 +167,7 @@ FixMyStreet::override_config {
             "permissions[user_assign_areas]" => undef,
         } } );
 
-        ok $user2->has_permission_to("moderate", $user2->from_body->id), "user2 has been granted moderate permission";
+        ok $user2->has_body_permission_to("moderate"), "user2 has been granted moderate permission";
     };
 
     $oxfordshireuser->user_body_permissions->create({
diff --git a/templates/web/base/admin/user-form.html b/templates/web/base/admin/user-form.html
index c22480011..b05f15355 100644
--- a/templates/web/base/admin/user-form.html
+++ b/templates/web/base/admin/user-form.html
@@ -49,7 +49,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.has_permission_to('user_assign_body', c.user.from_body.id) %]>
+            [% 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') %]>
           </li>
         [% END %]
 
@@ -116,7 +116,7 @@
                 [% FOREACH permission IN group.value %]
                   <li>
                     <label>
-                      <input type="checkbox" id="perms_[% permission.key %]" name="permissions[[% permission.key %]]" [% "checked" IF user.has_permission_to(permission.key, user.from_body.id) %]>
+                      <input type="checkbox" id="perms_[% permission.key %]" name="permissions[[% permission.key %]]" [% "checked" IF 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 19a3de03c..dfff77ee6 100644
--- a/templates/web/base/admin/users.html
+++ b/templates/web/base/admin/users.html
@@ -25,7 +25,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.from_body AND user.has_permission_to('moderate', user.from_body.id) %] * [% END %]
+            [% IF 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>
diff --git a/templates/web/oxfordshire/header.html b/templates/web/oxfordshire/header.html
index e347c8677..d5884272b 100644
--- a/templates/web/oxfordshire/header.html
+++ b/templates/web/oxfordshire/header.html
@@ -42,7 +42,7 @@
                             <[% IF c.req.uri.path == '/my' OR ( c.req.uri.path == '/auth' AND c.req.params.r == 'my' ) %]span[% ELSE %]a href="/my"[% END
                              %]>[% c.user_exists ? loc("Your account") : loc("Sign in") %]</[% ( c.req.uri.path == '/my' OR ( c.req.uri.path == '/auth' AND c.req.params.r == 'my' ) ) ? 'span' : 'a' %]>
                         </li>
-                        [% IF c.user_exists AND c.user.has_permission_to('planned_reports', c.user.from_body.id) %]
+                        [% IF c.user_exists AND c.user.has_body_permission_to('planned_reports') %]
                         <li>
                             <[% IF c.req.uri.path == '/my/planned' %]span[% ELSE %]a href="/my/planned"[% END
                              %]>[% loc('Planned reports') %]</[% c.req.uri.path == '/my/planned' ? 'span' : 'a' %]>