From d08ca285f37c4b338f8449d5ad86cf739587f9b2 Mon Sep 17 00:00:00 2001 From: Akshay Gupta Date: Wed, 2 Sep 2020 17:07:17 +0530 Subject: [PATCH] Allow Admins to Edit accesses (#1252) We need to be able to edit permissions of existing users that you can manage. Hooks up the edit admin page with our new access tree and pre-selects all the permissions they have along with the permissions you can provide. --- app/assets/javascripts/invite_admin.js | 15 ++++ app/controllers/admins_controller.rb | 87 +++++++++++++++---- app/helpers/admin_access_helper.rb | 11 ++- app/models/user.rb | 3 + app/models/user_access.rb | 31 +++++-- app/models/user_access_tree.rb | 53 ++++++----- app/views/admins/_form.html.erb | 23 +++++ .../invitations/_access_tree.html.erb | 26 +++++- .../invitations/new.html.erb | 5 +- 9 files changed, 199 insertions(+), 55 deletions(-) diff --git a/app/assets/javascripts/invite_admin.js b/app/assets/javascripts/invite_admin.js index 2b7c3a3445..4e60ef4f0d 100644 --- a/app/assets/javascripts/invite_admin.js +++ b/app/assets/javascripts/invite_admin.js @@ -2,12 +2,27 @@ // loads at page refresh // window.addEventListener("DOMContentLoaded", inviteAdmin); +window.addEventListener("DOMContentLoaded", editAdmin); + function inviteAdmin() { checkboxItemListener() resourceRowCollapseListener() } +function editAdmin() { + const SELECTOR = "input.access-input" + const facilityAccessDiv = document.getElementById("facility-access") + + // list of all checkboxes under facilityAccessDiv + const checkboxes = nodeListToArray(SELECTOR, facilityAccessDiv) + const checkedCheckboxes = checkboxes.filter(check => check.checked) + + for (let checkbox of checkedCheckboxes) { + updateParentCheckedState(checkbox, SELECTOR) + } +} + // // listeners // diff --git a/app/controllers/admins_controller.rb b/app/controllers/admins_controller.rb index aa85b4f4b6..bf741a546d 100644 --- a/app/controllers/admins_controller.rb +++ b/app/controllers/admins_controller.rb @@ -2,9 +2,13 @@ class AdminsController < AdminController include Pagination include SearchHelper - before_action :set_admin, only: [:show, :edit, :update, :destroy] - before_action :verify_params, only: [:update] + before_action :set_admin, only: [:show, :edit, :update, :destroy], unless: -> { Flipper.enabled?(:new_permissions_system_aug_2020, current_admin) } + before_action :🆕set_admin, only: [:show, :edit, :update], if: -> { Flipper.enabled?(:new_permissions_system_aug_2020, current_admin) } + before_action :verify_params, only: [:update], unless: -> { Flipper.enabled?(:new_permissions_system_aug_2020, current_admin) } + before_action :🆕verify_params, only: [:update], if: -> { Flipper.enabled?(:new_permissions_system_aug_2020, current_admin) } after_action :verify_policy_scoped, only: :index + skip_after_action :verify_authorized, if: -> { Flipper.enabled?(:new_permissions_system_aug_2020, current_admin) } + skip_after_action :verify_policy_scoped, if: -> { Flipper.enabled?(:new_permissions_system_aug_2020, current_admin) } def index authorize([:manage, :admin, User]) @@ -22,23 +26,36 @@ def show end def edit + unless Flipper.enabled?(:new_permissions_system_aug_2020, current_admin) + authorize([:manage, :admin, current_admin]) + end end def update - User.transaction do - @admin.update!(user_params) - next unless permission_params.present? - - @admin.user_permissions.delete_all - permission_params.each do |attributes| - @admin.user_permissions.create!(attributes.permit( - :permission_slug, - :resource_id, - :resource_type - )) + if Flipper.enabled?(:new_permissions_system_aug_2020, current_admin) + User.transaction do + @admin.update!(user_params) + current_admin.grant_access(@admin, selected_facilities) end + + redirect_to admins_url, notice: "Admin was successfully updated." + else + User.transaction do + @admin.update!(user_params) + next unless permission_params.present? + + @admin.user_permissions.delete_all + permission_params.each do |attributes| + @admin.user_permissions.create!(attributes.permit( + :permission_slug, + :resource_id, + :resource_type + )) + end + end + + render json: {}, status: :ok end - render json: {}, status: :ok end def destroy @@ -57,19 +74,53 @@ def verify_params end end + # + # This is a temporary `verify_params` method that will exist until we migrate fully to the new permissions system + # + def 🆕verify_params + if selected_facilities.blank? + redirect_to edit_admin_path(@admin), + alert: "At least one facility should be selected for access before inviting an Admin." + + return + end + + @admin.assign_attributes(user_params) + + if @admin.invalid? + redirect_to edit_admin_path, + alert: @admin.errors.full_messages.join("") + end + end + def set_admin @admin = User.find(params[:id]) authorize([:manage, :admin, @admin]) end + def 🆕set_admin + @admin = authorize1 { current_admin.accessible_admins(:manage).find(params[:id]) } + end + + def current_admin + AdminAccessPresenter.new(super) + end + def permission_params params[:permissions] end + def selected_facilities + params[:facilities].flatten + end + def user_params - {full_name: params[:full_name], - role: params[:role], - organization_id: params[:organization_id], - device_updated_at: Time.current}.compact + { + full_name: params[:full_name], + role: params[:role], + access_level: params[:access_level], + organization_id: params[:organization_id], + device_updated_at: Time.current + }.compact end end diff --git a/app/helpers/admin_access_helper.rb b/app/helpers/admin_access_helper.rb index 66deae6e3f..80fa12e961 100644 --- a/app/helpers/admin_access_helper.rb +++ b/app/helpers/admin_access_helper.rb @@ -12,7 +12,14 @@ def access_fraction(name, available, total) # end end - def access_checkbox(form, name, resource) - form.check_box(name, {id: resource.id, class: "access-input", label: resource.name.to_s}, resource.id, nil) + def access_checkbox(form, name, resource, page: :edit, checked_fn: -> { false }) + opts = { + id: resource.id, + class: "access-input", + label: resource.name.to_s, + checked: page.eql?(:edit) && checked_fn.call + } + + form.check_box("#{name}[]", opts, resource.id, nil) end end diff --git a/app/models/user.rb b/app/models/user.rb index 9c0d080495..a3cb893581 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -63,6 +63,8 @@ def app_capabilities ->(term) { joins(:phone_number_authentications).merge(PhoneNumberAuthentication.search_by_phone(term)) } scope :search_by_name_or_email, ->(term) { search_by_name(term).union(search_by_email(term)) } scope :search_by_name_or_phone, ->(term) { search_by_name(term).union(search_by_phone(term)) } + scope :non_admins, -> { joins(:phone_number_authentications).where.not(phone_number_authentications: {id: nil}) } + scope :admins, -> { joins(:email_authentications).where.not(email_authentications: {id: nil}) } validates :full_name, presence: true validates :role, presence: true, if: -> { email_authentication.present? } @@ -91,6 +93,7 @@ def app_capabilities :accessible_facilities, :accessible_facility_groups, :accessible_users, + :accessible_admins, :grant_access, :permitted_access_levels, to: :user_access, allow_nil: false diff --git a/app/models/user_access.rb b/app/models/user_access.rb index bdf7cb7994..d627fb08e1 100644 --- a/app/models/user_access.rb +++ b/app/models/user_access.rb @@ -1,4 +1,6 @@ class UserAccess < Struct.new(:user) + include Memery + class NotAuthorizedError < StandardError; end class AuthorizationNotPerformedError < StandardError; end @@ -39,6 +41,7 @@ class AuthorizationNotPerformedError < StandardError; end } }.freeze + ANY_ACTION = :any ACTION_TO_LEVEL = { manage_overdue_list: [:manager, :viewer_all, :call_center], view_reports: [:manager, :viewer_all, :viewer_reports_only], @@ -46,28 +49,33 @@ class AuthorizationNotPerformedError < StandardError; end manage: [:manager] }.freeze - def accessible_organizations(action) + memoize def accessible_organizations(action) resources_for(Organization, action) end - def accessible_facility_groups(action) + memoize def accessible_facility_groups(action) resources_for(FacilityGroup, action) .union(FacilityGroup.where(organization: accessible_organizations(action))) .includes(:organization) end - def accessible_facilities(action) + memoize def accessible_facilities(action) resources_for(Facility, action) .union(Facility.where(facility_group: accessible_facility_groups(action))) .includes(facility_group: :organization) end - def accessible_users - facilities = accessible_facilities(:manage) + memoize def accessible_admins(action) + return User.admins if bypass? + return User.none if action_to_level(action).include?(:manage) + + User.admins.where(organization: user.organization) + end - User.joins(:phone_number_authentications) - .where.not(phone_number_authentications: {id: nil}) - .where(phone_number_authentications: {registration_facility_id: facilities}) + memoize def accessible_users + User + .non_admins + .where(phone_number_authentications: {registration_facility_id: accessible_facilities(:manage)}) end def permitted_access_levels @@ -95,7 +103,7 @@ def grant_access(new_user, selected_facility_ids) def resources_for(resource_model, action) return resource_model.all if bypass? - return resource_model.none unless ACTION_TO_LEVEL.fetch(action).include?(user.access_level.to_sym) + return resource_model.none unless action_to_level(action).include?(user.access_level.to_sym) resource_ids = user @@ -145,4 +153,9 @@ def prepare_grantable_resources(selected_facility_ids) def bypass? user.power_user? end + + def action_to_level(action) + ACTION_TO_LEVEL.values.flatten.uniq if action == ANY_ACTION + ACTION_TO_LEVEL[action] + end end diff --git a/app/models/user_access_tree.rb b/app/models/user_access_tree.rb index f55e6132df..93055c884b 100644 --- a/app/models/user_access_tree.rb +++ b/app/models/user_access_tree.rb @@ -6,7 +6,7 @@ class UserAccessTree < Struct.new(:user) include Memery - def facilities + memoize def facilities visible_facilities.map { |facility| info = { visible: true @@ -16,45 +16,58 @@ def facilities }.to_h end - def facility_groups + memoize def facility_groups facilities .group_by { |facility, _| facility.facility_group } .map { |facility_group, facilities| - info = { - accessible_facility_count: facilities.length, - visible: visible_facility_groups.include?(facility_group), - facilities: facilities - } + info = { + accessible_facility_count: facilities.length, + visible: visible_facility_groups.include?(facility_group), + facilities: facilities + } - [facility_group, info] - }.to_h + [facility_group, info] + }.to_h end - def organizations + memoize def organizations facility_groups .group_by { |facility_group, _| facility_group.organization } .map { |organization, facility_groups| - info = { - accessible_facility_count: facility_groups.sum { |_, info| info[:accessible_facility_count] }, - visible: visible_organizations.include?(organization), - facility_groups: facility_groups - } + info = { + accessible_facility_count: facility_groups.sum { |_, info| info[:accessible_facility_count] }, + visible: visible_organizations.include?(organization), + facility_groups: facility_groups + } - [organization, info] - }.to_h + [organization, info] + }.to_h + end + + def visible?(model, record) + case model + when :facility + facilities.dig(record, :visible) + when :facility_group + facility_groups.dig(record, :visible) + when :organization + organizations.dig(record, :visible) + else + raise ArgumentError, "#{model} is unsupported." + end end private memoize def visible_facility_groups - user.accessible_facility_groups(:view) + user.accessible_facility_groups(:any_access) end memoize def visible_facilities - user.accessible_facilities(:view) + user.accessible_facilities(:any_access) end memoize def visible_organizations - user.accessible_organizations(:view) + user.accessible_organizations(:any_access) end end diff --git a/app/views/admins/_form.html.erb b/app/views/admins/_form.html.erb index 74323be6f9..403da2789a 100644 --- a/app/views/admins/_form.html.erb +++ b/app/views/admins/_form.html.erb @@ -1,3 +1,25 @@ +<% if Flipper.enabled?(:new_permissions_system_aug_2020, current_admin) %> +
+
+ <%= bootstrap_form_with(url: admin_path(admin), method: 'PATCH', local: true, autocomplete: "off", label_errors: true) do |form| %> + <%= form.text_field :full_name, value: @admin.full_name, id: :full_name, required: true, autocomplete: "off", label: "Full Name *" %> + <%= form.text_field :email, value: @admin.email, id: :email, required: true, disabled: true, label: "Email *" %> + <%= form.text_field :role, value: @admin.role, id: :role, required: true, help: "CVHO, STS, State Official etc.", label: 'Job Title *' %> + <%= form.select :access_level, current_admin.permitted_access_levels_info, value: @admin.access_level, id: :access_levels, required: true, label: "Access *" %> + + <%= form.label "Facility Access *" %> +
+ <%= render "email_authentications/invitations/access_tree", + access_tree: current_admin.access_tree.organizations, + user_being_edited: AdminAccessPresenter.new(@admin), + page: :edit, + form: form %> +
+ <%= form.primary("Save") %> + <% end %> +
+
+<% else %> <%= javascript_include_tag "standalone/react_components" %> <% user_permissions = current_admin.user_permissions.pluck(:permission_slug) %> @@ -15,3 +37,4 @@ allow_email_edit: false, submit_text: 'Update Admin', submit_method: 'PATCH' %> +<% end %> diff --git a/app/views/email_authentications/invitations/_access_tree.html.erb b/app/views/email_authentications/invitations/_access_tree.html.erb index 95e4ecf991..d13155028d 100644 --- a/app/views/email_authentications/invitations/_access_tree.html.erb +++ b/app/views/email_authentications/invitations/_access_tree.html.erb @@ -6,8 +6,14 @@
+ <%= access_checkbox( + form, + "organizations[]", + organization, + page: page, + checked_fn: -> { user_being_edited.access_tree.visible?(:organization, organization) } + ) %> - <%= access_checkbox(form, "organizations[]", organization) %> <%= access_fraction("facility groups", organization_metadata[:accessible_facility_count], 0) %> @@ -23,7 +29,13 @@ - <%= access_checkbox(form, "facility_groups[]", facility_group) %> + <%= access_checkbox( + form, + "facility_groups[]", + facility_group, + page: page, + checked_fn: -> { user_being_edited.access_tree.visible?(:facility_group, facility_group) } + ) %>