Merge pull request #2558 from alphagov/update-permission-editing-for-…

…other-users

Make editing the permissions of other users the same as editing your own permissions

app/controllers/users/applications_controller.rb

@@ -0,0 +1,20 @@
+class Users::ApplicationsController < ApplicationController
+  layout "admin_layout"
+
+  before_action :authenticate_user!
+
+  def show
+    user = User.find(params[:user_id])
+    authorize user, :edit?
+
+    redirect_to user_applications_path(user)
+  end
+
+  def index
+    @user = User.find(params[:user_id])
+    authorize @user, :edit?
+
+    @applications_with_signin = Doorkeeper::Application.not_api_only.can_signin(@user)
+    @applications_without_signin = Doorkeeper::Application.not_api_only.without_signin_permission_for(@user)
+  end
+end

app/controllers/users/permissions_controller.rb

@@ -0,0 +1,46 @@
+class Users::PermissionsController < ApplicationController
+  layout "admin_layout"
+
+  before_action :authenticate_user!
+  before_action :set_user
+  before_action :set_application
+
+  def show
+    authorize @user, :edit?
+
+    @permissions = @application
+      .sorted_supported_permissions_grantable_from_ui
+      .sort_by { |permission| @user.has_permission?(permission) ? 0 : 1 }
+  end
+
+  def edit
+    authorize UserApplicationPermission.for(@user, @application)
+
+    @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
+  end
+
+  def update
+    authorize UserApplicationPermission.for(@user, @application)
+
+    permission_ids_for_other_applications = @user.supported_permissions.excluding_application(@application).pluck(:id)
+    user_update_params = { supported_permission_ids: permission_ids_for_other_applications + update_params[:supported_permission_ids].map(&:to_i) }
+    UserUpdate.new(@user, user_update_params, current_user, user_ip_address).call
+
+    flash[:application_id] = @application.id
+    redirect_to user_applications_path(@user)
+  end
+
+private
+
+  def update_params
+    params.require(:application).permit(supported_permission_ids: [])
+  end
+
+  def set_user
+    @user = User.find(params[:user_id])
+  end
+
+  def set_application
+    @application = Doorkeeper::Application.with_signin_permission_for(@user).not_api_only.find(params[:application_id])
+  end
+end

app/controllers/users/signin_permissions_controller.rb

@@ -0,0 +1,40 @@
+class Users::SigninPermissionsController < ApplicationController
+  layout "admin_layout"
+
+  before_action :authenticate_user!
+  before_action :set_user
+  before_action :set_application, except: [:create]
+
+  def create
+    application = Doorkeeper::Application.not_api_only.find(params[:application_id])
+    authorize UserApplicationPermission.for(@user, application)
+
+    params = { supported_permission_ids: @user.supported_permissions.map(&:id) + [application.signin_permission.id] }
+    UserUpdate.new(@user, params, current_user, user_ip_address).call
+
+    redirect_to user_applications_path(@user)
+  end
+
+  def delete
+    authorize UserApplicationPermission.for(@user, @application)
+  end
+
+  def destroy
+    authorize UserApplicationPermission.for(@user, @application)
+
+    params = { supported_permission_ids: @user.supported_permissions.map(&:id) - [@application.signin_permission.id] }
+    UserUpdate.new(@user, params, current_user, user_ip_address).call
+
+    redirect_to user_applications_path(@user)
+  end
+
+private
+
+  def set_user
+    @user = User.find(params[:user_id])
+  end
+
+  def set_application
+    @application = Doorkeeper::Application.with_signin_permission_for(@user).not_api_only.find(params[:application_id])
+  end
+end

app/controllers/users_controller.rb

@@ -7,9 +7,8 @@ class UsersController < ApplicationController
 
   before_action :authenticate_user!
   before_action :load_user, except: %i[index]
-  before_action :redirect_to_account_page_if_acting_on_own_user, only: %i[edit manage_permissions]
+  before_action :redirect_to_account_page_if_acting_on_own_user, only: %i[edit]
   before_action :authorize_user, except: %i[index]
-  before_action :allow_no_application_access, only: [:update]
   before_action :redirect_legacy_filters, only: [:index]
   helper_method :applications_and_permissions, :filter_params
   respond_to :html
@@ -33,18 +32,9 @@ def index
 
   def edit; end
 
-  def manage_permissions
-    @application_permissions = all_applications_and_permissions_for(@user)
-  end
-
   def update
-    updater = UserUpdate.new(@user, user_params, current_user, user_ip_address)
-    if updater.call
-      redirect_to users_path, notice: "Updated user #{@user.email} successfully"
-    else
-      @application_permissions = all_applications_and_permissions_for(@user)
-      render :manage_permissions
-    end
+    UserUpdate.new(@user, user_params, current_user, user_ip_address).call
+    redirect_to users_path, notice: "Updated user #{@user.email} successfully"
   end
 
   def event_logs
@@ -79,28 +69,13 @@ def export
     end
   end
 
-  # When no permissions are selected for a user, we set the value to [] so
-  # a user can have no permissions
-  def allow_no_application_access
-    params[:user] ||= {}
-    params[:user][:supported_permission_ids] ||= []
-  end
-
   def user_params
-    if permitted_user_params[:skip_update_user_permissions]
-      permitted_user_params[:supported_permission_ids] = @user.supported_permission_ids
-    end
-
     UserParameterSanitiser.new(
-      user_params: permitted_user_params,
+      user_params: params.require(:user).permit(:require_2sv).to_h,
       current_user_role: current_user.role.to_sym,
     ).sanitise
   end
 
-  def permitted_user_params
-    @permitted_user_params ||= params.require(:user).permit(:require_2sv, :skip_update_user_permissions, supported_permission_ids: []).to_h
-  end
-
   def filter_params
     params.permit(
       :filter, :page, :format, :"option-select-filter",

app/helpers/application_permissions_helper.rb

@@ -0,0 +1,25 @@
+module ApplicationPermissionsHelper
+  def message_for_success(application_id, user = current_user)
+    application = Doorkeeper::Application.find_by(id: application_id)
+    return nil unless application
+
+    additional_permissions = user.permissions_for(application).reject { |permission| permission == SupportedPermission::SIGNIN_NAME }
+
+    if additional_permissions.any?
+      prefix = user == current_user ? "You now have" : "#{user.name} now has"
+      paragraph = tag.p("#{prefix} the following permissions for #{application.name}:", class: "govuk-body")
+      list = tag.ul(class: "govuk-list govuk-list--bullet")
+      additional_permissions.map { |permission| list << tag.li(permission) }
+    else
+      string = if user == current_user
+                 "You can access #{application.name} but you do not have any additional permissions."
+               else
+                 "#{user.name} can access #{application.name} but does not have any additional permissions."
+               end
+      paragraph = tag.p(string, class: "govuk-body")
+      list = nil
+    end
+
+    paragraph + list
+  end
+end

app/models/user_application_permission.rb

@@ -8,6 +8,10 @@ class UserApplicationPermission < ApplicationRecord
 
   before_validation :assign_application_id
 
+  def self.for(user, application)
+    new(user:, application:)
+  end
+
 private
 
   # application_id is duplicated across supported_permissions and user_application_permissions

app/policies/user_application_permission_policy.rb

@@ -0,0 +1,15 @@
+class UserApplicationPermissionPolicy < BasePolicy
+  def create?
+    return false unless Pundit.policy(current_user, user).edit?
+
+    return true if current_user.govuk_admin?
+
+    current_user.publishing_manager? && current_user.has_access_to?(application) && application.signin_permission.delegatable?
+  end
+  alias_method :destroy?, :create?
+  alias_method :delete?, :create?
+  alias_method :update?, :create?
+  alias_method :edit?, :create?
+
+  delegate :user, :application, to: :record
+end

app/policies/user_policy.rb

@@ -39,7 +39,6 @@ def edit?
   alias_method :reset_two_step_verification?, :edit?
   alias_method :resend_email_change?, :edit?
   alias_method :cancel_email_change?, :edit?
-  alias_method :manage_permissions?, :edit?
 
   def assign_role?
     current_user.superadmin?

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

@@ -0,0 +1,103 @@
+<% content_for :title_caption, "Manage other users" %>
+<% content_for :title, "#{@user.name}'s applications" %>
+
+<% content_for :breadcrumbs,
+   render("govuk_publishing_components/components/breadcrumbs", {
+     collapse_on_mobile: true,
+     breadcrumbs: [
+       {
+         title: "Dashboard",
+         url: root_path,
+       },
+       {
+         title: "Users",
+         url: users_path,
+       },
+       {
+         title: @user.name,
+         url: edit_user_path(@user),
+       },
+       {
+        title: "#{@user.name}'s applications",
+       }
+     ]
+   })
+%>
+
+<% if flash[:application_id] %>
+<%= render "govuk_publishing_components/components/success_alert", {
+    message: "Permissions updated",
+    description: message_for_success(flash[:application_id], @user),
+} %>
+<% end %>
+
+<table class="govuk-table">
+  <caption class="govuk-table__caption govuk-table__caption--m">Apps <%= @user.name %> has access to</caption>
+  <thead class="govuk-table__head">
+    <tr class="govuk-table__row">
+      <th scope="col" class="govuk-table__header govuk-!-width-one-quarter">Name</th>
+      <th scope="col" class="govuk-table__header govuk-!-width-one-third">Description</th>
+      <th scope="col" class="govuk-table__header"><span class="govuk-visually-hidden">Permissions</span></th>
+      <th scope="col" class="govuk-table__header"><span class="govuk-visually-hidden">Remove access</span></th>
+    </tr>
+  </thead>
+  <tbody class="govuk-table__body">
+    <% @applications_with_signin.each do |application| %>
+      <tr class="govuk-table__row">
+        <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">
+          <% if policy(UserApplicationPermission.for(@user, application)).edit? %>
+            <% unless application.sorted_supported_permissions_grantable_from_ui(include_signin: false).empty? %>
+              <%= link_to edit_user_application_permissions_path(@user, application), class: "govuk-link" do %>
+                Update permissions<span class="govuk-visually-hidden"> for <%= application.name %></span>
+              <% end %>
+            <% end %>
+          <% else %>
+            <%= link_to user_application_permissions_path(@user, application), class: "govuk-link" do %>
+              View permissions<span class="govuk-visually-hidden"> for <%= application.name %></span>
+            <% end %>
+          <% end %>
+        </td>
+        <td class="govuk-table__cell govuk-!-text-align-right">
+          <% if policy(UserApplicationPermission.for(@user, application)).delete? %>
+            <%= link_to delete_user_application_signin_permission_path(@user, 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>
+    <% end %>
+  </tbody>
+</table>
+
+<h2 class="govuk-heading-m" id="other-apps-table-heading">Apps <%= @user.name %> does not have access to</h2>
+
+<table class="govuk-table" aria-labelledby="other-apps-table-heading">
+  <thead class="govuk-table__head">
+    <tr class="govuk-table__row">
+      <th scope="col" class="govuk-table__header govuk-!-width-one-quarter">Name</th>
+      <th scope="col" class="govuk-table__header govuk-!-width-one-third">Description</th>
+      <th scope="col" class="govuk-table__header"><span class="govuk-visually-hidden">Grant access</span></th>
+    </tr>
+  </thead>
+  <tbody class="govuk-table__body">
+    <% @applications_without_signin.each do |application| %>
+      <tr class="govuk-table__row">
+        <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">
+          <% if policy(UserApplicationPermission.for(@user, application)).create? %>
+            <%= button_to user_application_signin_permission_path(@user, 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>
+    <% end %>
+  </tbody>
+</table>

app/views/users/edit.html.erb

@@ -58,7 +58,7 @@
     <section>
       <%= render "govuk_publishing_components/components/button", {
         text: "Manage permissions",
-        href: manage_permissions_user_path(@user)
+        href: user_applications_path(@user)
       } %>
     </section>
   </div>