From 2770fc9dc249754c7b069bd088738e5635e4c712 Mon Sep 17 00:00:00 2001 From: ulferts Date: Mon, 10 Jun 2024 15:30:19 +0200 Subject: [PATCH] move code to more central context --- app/models/users/permission_checks.rb | 24 ++++--- .../authorization/user_permissible_service.rb | 20 +----- lib/open_project/access_control/permission.rb | 23 ++++++- .../access_control/permission_spec.rb | 62 +++++++++++++++++++ spec/models/users/permission_checks_spec.rb | 62 +++++++++++++------ 5 files changed, 145 insertions(+), 46 deletions(-) diff --git a/app/models/users/permission_checks.rb b/app/models/users/permission_checks.rb index 9fe321cb7d78..3c60d3377de8 100644 --- a/app/models/users/permission_checks.rb +++ b/app/models/users/permission_checks.rb @@ -106,14 +106,24 @@ def member_of?(project) roles_for_project(project).any?(&:member?) end + # Returns all permissions the user may have for a given context. + # "May" because this method does not check e.g. whether the module + # the permission belongs to is active. def all_permissions_for(context) - Authorization - .roles(self, context) - .includes(:role_permissions) - .pluck(:permission) - .compact - .map(&:to_sym) - .uniq + if admin? + OpenProject::AccessControl + .permissions + .select { |p| p.permissible_on?(context) && p.grant_to_admin? } + .map(&:name) + else + Authorization + .roles(self, context) + .includes(:role_permissions) + .pluck(:permission) + .compact + .map(&:to_sym) + .uniq + end end # Helper method to be used in places where we just throw anything into the permission check and don't know what diff --git a/app/services/authorization/user_permissible_service.rb b/app/services/authorization/user_permissible_service.rb index ef53622f3d05..a3c8e2629b14 100644 --- a/app/services/authorization/user_permissible_service.rb +++ b/app/services/authorization/user_permissible_service.rb @@ -59,25 +59,7 @@ def allowed_in_any_entity?(permission, entity_class, in_project: nil) def cached_permissions(context) @cached_permissions ||= Hash.new do |hash, context_key| - hash[context_key] = if user.admin? - permissible_key = case context_key - when WorkPackage - :work_package - when Project - :project - when nil - :global - else - raise "Unknown context key: #{context_key}" - end - - OpenProject::AccessControl - .permissions - .select { |p| p.permissible_on?(permissible_key) && p.grant_to_admin? } - .map(&:name) - else - user.all_permissions_for(context_key) - end + hash[context_key] = user.all_permissions_for(context_key) end @cached_permissions[context] diff --git a/lib/open_project/access_control/permission.rb b/lib/open_project/access_control/permission.rb index 73157c9e1b3a..5f95837f49b8 100644 --- a/lib/open_project/access_control/permission.rb +++ b/lib/open_project/access_control/permission.rb @@ -89,7 +89,28 @@ def project_query? end def permissible_on?(context_type) - @permissible_on.include?(context_type) + # Sometimes the context_type passed in is a decorated object. + # Most of the times, this would then be an 'EagerLoadingWrapper' instance. + # We need to unwrap the object to get the actual object. + # Checking for `context_type.is_a?(SimpleDelegator)` fails for unknown reasons. + context_type = context_type.__getobj__ if context_type.class.ancestors.include?(SimpleDelegator) + + context_symbol = case context_type + when WorkPackage + :work_package + when Project + :project + when Queries::Projects::ProjectQuery + :project_query + when Symbol + context_type + when nil + :global + else + raise "Unknown context: #{context_type}" + end + + @permissible_on.include?(context_symbol) end def grant_to_admin? diff --git a/spec/lib/open_project/access_control/permission_spec.rb b/spec/lib/open_project/access_control/permission_spec.rb index de9eb7ea7b59..964bf621b88f 100644 --- a/spec/lib/open_project/access_control/permission_spec.rb +++ b/spec/lib/open_project/access_control/permission_spec.rb @@ -79,6 +79,68 @@ end end + describe "#permissible_on?" do + context "when marked as permissible on work package roles" do + subject(:permission) do + described_class.new(:perm, { cont: [:action] }, permissible_on: :work_package) + end + + it { expect(permission).to be_permissible_on(WorkPackage.new) } + it { expect(permission).not_to be_permissible_on(Project.new) } + it { expect(permission).not_to be_permissible_on(nil) } + it { expect(permission).not_to be_permissible_on(Queries::Projects::ProjectQuery.new) } + it { expect(permission).to be_permissible_on(:work_package) } + it { expect(permission).not_to be_permissible_on(:project) } + it { expect(permission).not_to be_permissible_on(:global) } + it { expect(permission).not_to be_permissible_on(:project_query) } + end + + context "when marked as permissible on project roles" do + subject(:permission) do + described_class.new(:perm, { cont: [:action] }, permissible_on: :project) + end + + it { expect(permission).not_to be_permissible_on(WorkPackage.new) } + it { expect(permission).to be_permissible_on(Project.new) } + it { expect(permission).not_to be_permissible_on(nil) } + it { expect(permission).not_to be_permissible_on(Queries::Projects::ProjectQuery.new) } + it { expect(permission).not_to be_permissible_on(:work_package) } + it { expect(permission).to be_permissible_on(:project) } + it { expect(permission).not_to be_permissible_on(:global) } + it { expect(permission).not_to be_permissible_on(:project_query) } + end + + context "when marked as permissible on global roles" do + subject(:permission) do + described_class.new(:perm, { cont: [:action] }, permissible_on: :global) + end + + it { expect(permission).not_to be_permissible_on(WorkPackage.new) } + it { expect(permission).not_to be_permissible_on(Project.new) } + it { expect(permission).to be_permissible_on(nil) } + it { expect(permission).not_to be_permissible_on(Queries::Projects::ProjectQuery.new) } + it { expect(permission).not_to be_permissible_on(:work_package) } + it { expect(permission).not_to be_permissible_on(:project) } + it { expect(permission).to be_permissible_on(:global) } + it { expect(permission).not_to be_permissible_on(:project_query) } + end + + context "when marked as permissible on project queries" do + subject(:permission) do + described_class.new(:perm, { cont: [:action] }, permissible_on: :project_query) + end + + it { expect(permission).not_to be_permissible_on(WorkPackage.new) } + it { expect(permission).not_to be_permissible_on(Project.new) } + it { expect(permission).not_to be_permissible_on(nil) } + it { expect(permission).to be_permissible_on(Queries::Projects::ProjectQuery.new) } + it { expect(permission).not_to be_permissible_on(:work_package) } + it { expect(permission).not_to be_permissible_on(:project) } + it { expect(permission).not_to be_permissible_on(:global) } + it { expect(permission).to be_permissible_on(:project_query) } + end + end + describe "marking it as permissible on multiple role types" do subject(:permission) do described_class.new(:perm, { cont: [:action] }, permissible_on: %i[work_package project]) diff --git a/spec/models/users/permission_checks_spec.rb b/spec/models/users/permission_checks_spec.rb index 765b04560cd9..3a5a9082548f 100644 --- a/spec/models/users/permission_checks_spec.rb +++ b/spec/models/users/permission_checks_spec.rb @@ -179,31 +179,55 @@ let(:public_permissions) { OpenProject::AccessControl.public_permissions.map(&:name) } - subject do - create(:user, global_permissions: [:create_user], - member_with_permissions: { project => %i[view_work_packages edit_work_packages] }) - end + context "for a non admin user" do + subject do + create(:user, global_permissions: [:create_user], + member_with_permissions: { project => %i[view_work_packages edit_work_packages] }) + end - it "returns all permissions given on the project" do - expect(subject.all_permissions_for(project)).to match_array(%i[view_work_packages edit_work_packages] + public_permissions) - end + it "returns all permissions given on the project" do + expect(subject.all_permissions_for(project)) + .to match_array(%i[view_work_packages edit_work_packages] + public_permissions) + end - it "returns non-member permissions given on the project the user is not a member of" do - expect(subject.all_permissions_for(other_project)).to match_array(%i[view_work_packages - manage_members] + public_permissions) - end + it "returns non-member permissions given on the project the user is not a member of" do + expect(subject.all_permissions_for(other_project)).to match_array(%i[view_work_packages + manage_members] + public_permissions) + end - it "returns all global permissions" do - skip "Current implementation of the Authorization.roles query returns ALL permissions the user has, not only global ones. " \ - "We should change this in the fututre, thats why this test is already in here." + it "returns all global permissions" do + skip "Current implementation of the Authorization.roles query returns ALL permissions the user has, " \ + "not only global ones. We should change this in the future, that's why this test is already in here." + + expect(subject.all_permissions_for(nil)).to match_array(%i[create_user]) + end - expect(subject.all_permissions_for(nil)).to match_array(%i[create_user]) + it "returns all permissions the user has (with project and global permissions)" do + expect(subject.all_permissions_for(nil)).to match_array(%i[create_user + view_work_packages edit_work_packages + manage_members] + public_permissions) + end end - it "returns all permissions the user has (with project and global permissions)" do - expect(subject.all_permissions_for(nil)).to match_array(%i[create_user - view_work_packages edit_work_packages - manage_members] + public_permissions) + context "for an admin user" do + subject do + create(:admin) + end + + it "returns all permissions given on the project" do + expect(subject.all_permissions_for(project)) + .to include(:view_work_packages, :edit_work_packages, *public_permissions) + end + + it "returns all permissions given globally" do + expect(subject.all_permissions_for(nil)) + .to include(:create_user) + end + + it "returns all permissions given on a work package" do + expect(subject.all_permissions_for(WorkPackage.new)) + .to include(:view_work_packages, :edit_work_packages) + end end end