Merge pull request #2370 from alphagov/allow-publishing-managers-to-m…

…anage-their-apps

Allow Publishing Managers to manage their apps

app/controllers/account/applications_controller.rb

@@ -4,13 +4,13 @@ class Account::ApplicationsController < ApplicationController
   before_action :authenticate_user!
 
   def show
-    authorize :account_applications
+    authorize [:account, Doorkeeper::Application]
 
     redirect_to account_applications_path
   end
 
   def index
-    authorize :account_applications
+    authorize [:account, Doorkeeper::Application]
 
     @applications_with_signin = Doorkeeper::Application.can_signin(current_user)
     @applications_without_signin = Doorkeeper::Application.not_retired.without_signin_permission_for(current_user)

app/controllers/account/permissions_controller.rb

@@ -8,7 +8,7 @@ class Account::PermissionsController < ApplicationController
   def index
     @application = Doorkeeper::Application.not_retired.find(params[:application_id])
 
-    authorize current_user, :view_permissions?
+    authorize [:account, @application], :view_permissions?
 
     @permissions = permissions_for(@application).sort_by { |permission| current_user.has_permission?(permission) ? 0 : 1 }
   end

app/controllers/account/signin_permissions_controller.rb

@@ -4,7 +4,7 @@ class Account::SigninPermissionsController < ApplicationController
   before_action :authenticate_user!
 
   def create
-    authorize current_user, :grant_signin_permission?
+    authorize [:account, Doorkeeper::Application], :grant_signin_permission?
 
     application = Doorkeeper::Application.not_retired.find(params[:application_id])
 
@@ -17,13 +17,13 @@ def create
   def delete
     @application = Doorkeeper::Application.not_retired.find(params[:application_id])
 
-    authorize current_user, :remove_signin_permission?
+    authorize [:account, @application], :remove_signin_permission?
   end
 
   def destroy
     application = Doorkeeper::Application.not_retired.find(params[:application_id])
 
-    authorize current_user, :remove_signin_permission?
+    authorize [:account, application], :remove_signin_permission?
 
     params = { supported_permission_ids: current_user.supported_permissions.map(&:id) - [application.signin_permission.id] }
     UserUpdate.new(current_user, params, current_user, user_ip_address).call

app/controllers/application_controller.rb

@@ -59,7 +59,7 @@ def api_user_via_token_has_signin_permission_on_app?
 
   def user_not_authorized(_exception)
     flash[:alert] = "You do not have permission to perform this action."
-    redirect_to root_path
+    redirect_back_or_to root_path
   end
 
   def notify_bad_request(_exception)

app/policies/account/application_policy.rb

@@ -0,0 +1,20 @@
+class Account::ApplicationPolicy < BasePolicy
+  def index?
+    current_user.govuk_admin? || current_user.publishing_manager?
+  end
+
+  alias_method :show?, :index?
+  alias_method :view_permissions?, :index?
+
+  def grant_signin_permission?
+    current_user.govuk_admin?
+  end
+
+  def remove_signin_permission?
+    current_user.has_access_to?(record) &&
+      (
+        current_user.govuk_admin? ||
+        current_user.publishing_manager? && record.signin_permission.delegatable?
+      )
+  end
+end

app/policies/user_policy.rb

@@ -35,12 +35,6 @@ def edit?
   alias_method :resend?, :edit?
   alias_method :event_logs?, :edit?
 
-  def grant_signin_permission?
-    current_user.govuk_admin?
-  end
-  alias_method :remove_signin_permission?, :grant_signin_permission?
-  alias_method :view_permissions?, :grant_signin_permission?
-
   def edit_email_or_password?
     allow_self_only
   end

app/views/account/applications/index.html.erb

@@ -31,15 +31,17 @@
         <td class="govuk-table__cell"><%= application.name %></td>
         <td class="govuk-table__cell"><%= application.description %></td>
         <td class="govuk-table__cell govuk-!-text-align-right">
-          <%= link_to account_application_permissions_path(application) do %>
+          <%= link_to account_application_permissions_path(application), class: "govuk-link" do %>
               View permissions<span class="govuk-visually-hidden"> for <%= application.name %></span>
           <% end %>
         </td>
         <td class="govuk-table__cell govuk-!-text-align-right">
-          <%= link_to delete_account_application_signin_permission_path(application),
-                      class: "govuk-button govuk-button--warning govuk-!-margin-0",
-                      data: { module: "govuk-button" } do %>
-              Remove access<span class="govuk-visually-hidden"> to <%= application.name %></span>
+          <% if policy([:account, application]).remove_signin_permission? %>
+            <%= link_to delete_account_application_signin_permission_path(application),
+                        class: "govuk-button govuk-button--warning govuk-!-margin-0",
+                        data: { module: "govuk-button" } do %>
+                Remove access<span class="govuk-visually-hidden"> to <%= application.name %></span>
+            <% end %>
           <% end %>
         </td>
       </tr>
@@ -63,10 +65,12 @@
         <td class="govuk-table__cell"><%= application.name %></td>
         <td class="govuk-table__cell"><%= application.description %></td>
         <td class="govuk-table__cell govuk-!-text-align-right">
-          <%= button_to account_application_signin_permission_path(application),
-                        class: "govuk-button govuk-!-margin-0",
-                        data: { module: "govuk-button" } do %>
-              Grant access<span class="govuk-visually-hidden"> to <%= application.name %></span>
+          <% if policy([:account, Doorkeeper::Application]).grant_signin_permission? %>
+            <%= button_to account_application_signin_permission_path(application),
+                          class: "govuk-button govuk-!-margin-0",
+                          data: { module: "govuk-button" } do %>
+                Grant access<span class="govuk-visually-hidden"> to <%= application.name %></span>
+            <% end %>
           <% end %>
         </td>
       </tr>

app/views/account/permissions/index.html.erb

@@ -22,7 +22,7 @@
 <table class="govuk-table">
   <thead class="govuk-table__head">
     <tr class="govuk-table__row">
-      <th scope="col" class="govuk-table__header">Name</th>
+      <th scope="col" class="govuk-table__header govuk-!-width-three-quarters">Name</th>
       <th scope="col" class="govuk-table__header">Has this permission?</th>
     </tr>
   </thead>

test/controllers/account/applications_controller_test.rb

@@ -22,4 +22,31 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
       assert_redirected_to "/account/applications"
     end
   end
+
+  context "#index" do
+    context "logged in as a publishing manager" do
+      should "not display the button to grant access to an application" do
+        application = create(:application, name: "app-name")
+        sign_in create(:organisation_admin_user)
+
+        get :index
+
+        assert_select "tr td", text: "app-name"
+        assert_select "form[action='#{account_application_signin_permission_path(application)}']", count: 0
+      end
+
+      should "not display the button to remove access to an application" do
+        application = create(:application, name: "app-name")
+        application.signin_permission.update!(delegatable: false)
+        user = create(:organisation_admin_user, with_signin_permissions_for: [application])
+
+        sign_in user
+
+        get :index
+
+        assert_select "tr td", text: "app-name"
+        assert_select "a[href='#{delete_account_application_signin_permission_path(application)}']", count: 0
+      end
+    end
+  end
 end

test/policies/account/application_policy_test.rb

@@ -0,0 +1,190 @@
+require "test_helper"
+require "support/policy_helpers"
+
+class Account::ApplicationPolicyTest < ActiveSupport::TestCase
+  include PolicyHelpers
+
+  context "accessing index?" do
+    %i[superadmin admin super_organisation_admin organisation_admin].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = FactoryBot.build(:"#{user_role}_user")
+        end
+
+        should "be permitted" do
+          assert permit?(@current_user, nil, :index)
+        end
+      end
+    end
+
+    %i[normal].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = FactoryBot.build(:"#{user_role}_user")
+        end
+
+        should "be forbidden" do
+          assert forbid?(@current_user, nil, :index)
+        end
+      end
+    end
+  end
+
+  context "show?" do
+    %i[superadmin admin super_organisation_admin organisation_admin].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = build(:"#{user_role}_user")
+        end
+
+        should "be permitted" do
+          assert permit?(@current_user, nil, :show)
+        end
+      end
+    end
+
+    %i[normal].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = build(:"#{user_role}_user")
+        end
+
+        should "be forbidden" do
+          assert forbid?(@current_user, nil, :show)
+        end
+      end
+    end
+  end
+
+  context "#grant_signin_permission?" do
+    %i[superadmin admin].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = build(:"#{user_role}_user")
+        end
+
+        should "be permitted" do
+          assert permit?(@current_user, nil, :grant_signin_permission)
+        end
+      end
+    end
+
+    %i[super_organisation_admin organisation_admin normal].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = build(:"#{user_role}_user")
+        end
+
+        should "be forbidden" do
+          assert forbid?(@current_user, nil, :grant_signin_permission)
+        end
+      end
+    end
+  end
+
+  context "#remove_signin_permission?" do
+    %i[superadmin admin].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = create(:"#{user_role}_user")
+          @application = create(:application)
+        end
+
+        context "when the user has signin permission for the app" do
+          setup do
+            @current_user.grant_application_signin_permission(@application)
+          end
+
+          should "be permitted" do
+            assert permit?(@current_user, @application, :remove_signin_permission)
+          end
+        end
+
+        context "when the user does not have the signin permission for the app" do
+          should "be forbidden" do
+            assert forbid?(@current_user, @application, :remove_signin_permission)
+          end
+        end
+      end
+    end
+
+    %i[super_organisation_admin organisation_admin].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = create(:"#{user_role}_user")
+          @application = create(:application)
+        end
+
+        context "when the user has signin permission for the app" do
+          setup do
+            @current_user.grant_application_signin_permission(@application)
+          end
+
+          context "and the application has delegatable permissions" do
+            setup do
+              @application.signin_permission.update!(delegatable: true)
+            end
+
+            should "be permitted" do
+              assert permit?(@current_user, @application, :remove_signin_permission)
+            end
+          end
+
+          context "and the application does not have delegatable permissions" do
+            setup do
+              @application.signin_permission.update!(delegatable: false)
+            end
+
+            should "not be permitted" do
+              assert forbid?(@current_user, @application, :remove_signin_permission)
+            end
+          end
+        end
+
+        context "when the user does not have the signin permission for the app" do
+          should "be forbidden" do
+            assert forbid?(@current_user, @application, :remove_signin_permission)
+          end
+        end
+      end
+    end
+
+    %i[normal].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = build(:"#{user_role}_user")
+        end
+
+        should "be forbidden" do
+          assert forbid?(@current_user, nil, :remove_signin_permission)
+        end
+      end
+    end
+  end
+
+  context "#view_permissions?" do
+    %i[superadmin admin super_organisation_admin organisation_admin].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = build(:"#{user_role}_user")
+        end
+
+        should "be permitted" do
+          assert permit?(@current_user, nil, :view_permissions)
+        end
+      end
+    end
+
+    %i[normal].each do |user_role|
+      context "for #{user_role} users" do
+        setup do
+          @current_user = build(:"#{user_role}_user")
+        end
+
+        should "be forbidden" do
+          assert forbid?(@current_user, nil, :view_permissions)
+        end
+      end
+    end
+  end
+end