From 8951241c60cebd848b4dc16116b7291834e63d55 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Mon, 6 Nov 2023 10:58:31 +0100 Subject: [PATCH 1/6] Add methods to ApplicationController to use new permission checks --- app/controllers/application_controller.rb | 63 +++++++++++++++++++++++ app/models/users/permission_checks.rb | 2 +- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 08b870b8d078..5e06b5186a07 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -231,15 +231,77 @@ def deny_access(not_found: false) end end + def authorize_in_project(ctrl = params[:controller], action = params[:action]) + authorization_project = @project || @projects + allowed = User.current.allowed_in_project?({ controller: ctrl, action: }, authorization_project) + + unless allowed + if @project&.archived? + render_403 message: :notice_not_authorized_archived_project + else + deny_access + end + end + + allowed + end + + def authorize_globally(ctrl = params[:controller], action = params[:action]) + allowed = User.current.allowed_globally?({ controller: ctrl, action: }) + + deny_access unless allowed + + allowed + end + + def authorize_in_any_project(ctrl = params[:controller], action = params[:action]) + allowed = User.current.allowed_in_any_project?({ controller: ctrl, action: }) + + deny_access unless allowed + + allowed + end + + def authorize_in_model(ctrl = params[:controller], action = params[:action]) + model = instance_variable_get("@#{model_object.to_s.underscore}") + + allowed = User.current.allowed_in_entity?({ controller: ctrl, action: }, model, model_object) + + unless allowed + if model.respond_to?(:project) && model.project&.archived? + render_403 message: :notice_not_authorized_archived_project + else + deny_access + end + end + + allowed + end + + def authorize_in_any_entity(ctrl = params[:controller], action = params[:action]) + allowed = if @project + User.current.allowed_in_any_entity?({ controller: ctrl, action: }, in_project: @project) + else + User.current.allowed_in_any_entity?({ controller: ctrl, action: }) + end + + deny_access unless allowed + + allowed + end + # Authorize the user for the requested controller action. # To be used in before_action hooks def authorize(ctrl = params[:controller], action = params[:action]) + OpenProject::Deprecation.deprecate_method(ApplicationController, :authorize) do_authorize({ controller: ctrl, action: }, global: false) end # Authorize the user for the requested controller action outside a project # To be used in before_action hooks def authorize_global + OpenProject::Deprecation.deprecate_method(ApplicationController, :authorize_global) + action = { controller: params[:controller], action: params[:action] } do_authorize(action, global: true) end @@ -250,6 +312,7 @@ def authorize_global # * a parameter-like Hash (eg. { controller: '/projects', action: 'edit' }) # * a permission Symbol (eg. :edit_project) def do_authorize(action, global: false) + OpenProject::Deprecation.deprecate_method(ApplicationController, :do_authorize) context = @project || @projects is_authorized = User.current.allowed_to?(action, context, global:) diff --git a/app/models/users/permission_checks.rb b/app/models/users/permission_checks.rb index 8be654c8b937..707e4bb68800 100644 --- a/app/models/users/permission_checks.rb +++ b/app/models/users/permission_checks.rb @@ -119,7 +119,7 @@ def allowed_based_on_permission_context?(permission, project: nil, entity: nil) # Old allowed_to? interface. Marked as deprecated, should be removed at some point ... Guessing 14.0? def allowed_to?(action, context, global: false) - # OpenProject::Deprecation.deprecate_method(User, :allowed_to?) + OpenProject::Deprecation.deprecate_method(User, :allowed_to?) user_allowed_service.call(action, context, global:) end From 93d6f2a7d7d55ec0782604f5a00896607af6d663 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 7 Nov 2023 09:37:00 +0100 Subject: [PATCH 2/6] Correctl use allowed_in_any_entity --- app/controllers/application_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5e06b5186a07..f8c0d6f37a85 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -278,11 +278,11 @@ def authorize_in_model(ctrl = params[:controller], action = params[:action]) allowed end - def authorize_in_any_entity(ctrl = params[:controller], action = params[:action]) + def authorize_in_any_model(ctrl = params[:controller], action = params[:action]) allowed = if @project - User.current.allowed_in_any_entity?({ controller: ctrl, action: }, in_project: @project) + User.current.allowed_in_any_entity?({ controller: ctrl, action: }, model_object, in_project: @project) else - User.current.allowed_in_any_entity?({ controller: ctrl, action: }) + User.current.allowed_in_any_entity?({ controller: ctrl, action: }, model_object) end deny_access unless allowed From e7396beb0ace75a8280422314dd552aff33c0b00 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 7 Nov 2023 15:15:04 +0100 Subject: [PATCH 3/6] Fix method that finds projects to use new permission check --- app/controllers/application_controller.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f8c0d6f37a85..7bed8e364402 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -352,14 +352,14 @@ def find_optional_project def find_optional_project_and_raise_error @project = Project.find(params[:project_id]) if params[:project_id].present? - allowed = User.current.allowed_to?({ controller: params[:controller], action: params[:action] }, - @project, global: @project.nil?) + allowed = User.current.allowed_based_on_permission_context?({ controller: params[:controller], action: params[:action] }, + project: @project) allowed ? true : deny_access end # Finds and sets @project based on @object.project def find_project_from_association - render_404 unless @object.present? + render_404 if @object.blank? @project = @object.project rescue ActiveRecord::RecordNotFound @@ -370,7 +370,7 @@ def find_model_object(object_id = :id) model = self.class._model_object if model @object = model.find(params[object_id]) - instance_variable_set('@' + controller_name.singularize, @object) if @object + instance_variable_set("@#{controller_name.singularize}", @object) if @object end rescue ActiveRecord::RecordNotFound render_404 @@ -381,7 +381,7 @@ def find_model_object_and_project(object_id = :id) model_object = self.class._model_object instance = model_object.find(params[object_id]) @project = instance.project - instance_variable_set('@' + model_object.to_s.underscore, instance) + instance_variable_set("@#{model_object.to_s.underscore}", instance) else @project = Project.find(params[:project_id]) end From a732ad3bfdb636aa1f6091891f8b5bc5d6695988 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 7 Nov 2023 15:55:13 +0100 Subject: [PATCH 4/6] Try if we can use the allowed_based_on_permission_context method in authorize --- app/controllers/application_controller.rb | 11 +++++++++-- app/models/users/permission_checks.rb | 4 ++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7bed8e364402..c10004abb465 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -313,8 +313,15 @@ def authorize_global # * a permission Symbol (eg. :edit_project) def do_authorize(action, global: false) OpenProject::Deprecation.deprecate_method(ApplicationController, :do_authorize) - context = @project || @projects - is_authorized = User.current.allowed_to?(action, context, global:) + # context = @project || @projects + # is_authorized = User.current.allowed_to?(action, context, global:) + + is_authorized = if global + User.current.allowed_based_on_permission_context?(action) + else + User.current.allowed_based_on_permission_context?(action, project: @project || @projects, + entity: @work_package || @work_packages) + end unless is_authorized if @project&.archived? diff --git a/app/models/users/permission_checks.rb b/app/models/users/permission_checks.rb index 707e4bb68800..2ec18911af1f 100644 --- a/app/models/users/permission_checks.rb +++ b/app/models/users/permission_checks.rb @@ -105,6 +105,10 @@ def allowed_based_on_permission_context?(permission, project: nil, entity: nil) allowed_globally?(perm) elsif perm.work_package? && entity.is_a?(WorkPackage) allowed_in_work_package?(perm, entity) + elsif perm.work_package? && entity.nil? && project.nil? + allowed_in_any_work_package?(perm) + elsif perm.work_package? && project && entity.nil? + allowed_in_any_work_package?(perm, in_project: project) elsif perm.project? && project allowed_in_project?(perm, project) elsif perm.project? && entity && entity.respond_to?(:project) From 96c2646671afef74599556078f09e2a90741b1e3 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 7 Nov 2023 17:02:19 +0100 Subject: [PATCH 5/6] Fix tests --- app/controllers/application_controller.rb | 28 +++++++++++-------- app/models/users/permission_checks.rb | 8 +++--- .../authorization/user_permissible_service.rb | 7 +++-- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index c10004abb465..eaa815270d85 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -293,14 +293,14 @@ def authorize_in_any_model(ctrl = params[:controller], action = params[:action]) # Authorize the user for the requested controller action. # To be used in before_action hooks def authorize(ctrl = params[:controller], action = params[:action]) - OpenProject::Deprecation.deprecate_method(ApplicationController, :authorize) + # OpenProject::Deprecation.deprecate_method(ApplicationController, :authorize) do_authorize({ controller: ctrl, action: }, global: false) end # Authorize the user for the requested controller action outside a project # To be used in before_action hooks def authorize_global - OpenProject::Deprecation.deprecate_method(ApplicationController, :authorize_global) + # OpenProject::Deprecation.deprecate_method(ApplicationController, :authorize_global) action = { controller: params[:controller], action: params[:action] } do_authorize(action, global: true) @@ -311,17 +311,21 @@ def authorize_global # Action can be: # * a parameter-like Hash (eg. { controller: '/projects', action: 'edit' }) # * a permission Symbol (eg. :edit_project) - def do_authorize(action, global: false) - OpenProject::Deprecation.deprecate_method(ApplicationController, :do_authorize) + def do_authorize(action, global: false) # rubocop:disable Metrics/PerceivedComplexity + # OpenProject::Deprecation.deprecate_method(ApplicationController, :do_authorize) # context = @project || @projects - # is_authorized = User.current.allowed_to?(action, context, global:) - - is_authorized = if global - User.current.allowed_based_on_permission_context?(action) - else - User.current.allowed_based_on_permission_context?(action, project: @project || @projects, - entity: @work_package || @work_packages) - end + # old_authorized = User.current.allowed_to?(action, context, global:) + + is_authorized = begin + if global + User.current.allowed_based_on_permission_context?(action) + else + User.current.allowed_based_on_permission_context?(action, project: @project || @projects, + entity: @work_package || @work_packages) + end + rescue Authorization::UnknownPermissionError + false + end unless is_authorized if @project&.archived? diff --git a/app/models/users/permission_checks.rb b/app/models/users/permission_checks.rb index 2ec18911af1f..fe0615a98ca6 100644 --- a/app/models/users/permission_checks.rb +++ b/app/models/users/permission_checks.rb @@ -103,17 +103,17 @@ def allowed_based_on_permission_context?(permission, project: nil, entity: nil) permissions.any? do |perm| if perm.global? allowed_globally?(perm) - elsif perm.work_package? && entity.is_a?(WorkPackage) + elsif perm.work_package? && (entity.is_a?(WorkPackage) || (entity.is_a?(Array) && entity.all? { |e| e.is_a?(WorkPackage) })) allowed_in_work_package?(perm, entity) - elsif perm.work_package? && entity.nil? && project.nil? + elsif perm.work_package? && entity.blank? && project.blank? allowed_in_any_work_package?(perm) - elsif perm.work_package? && project && entity.nil? + elsif perm.work_package? && project && entity.blank? allowed_in_any_work_package?(perm, in_project: project) elsif perm.project? && project allowed_in_project?(perm, project) elsif perm.project? && entity && entity.respond_to?(:project) allowed_in_project?(perm, entity.project) - elsif perm.project? && entity.nil? && project.nil? + elsif perm.project? && entity.blank? && project.blank? allowed_in_any_project?(perm) else false diff --git a/app/services/authorization/user_permissible_service.rb b/app/services/authorization/user_permissible_service.rb index f0a15b776c8d..e158da8531d8 100644 --- a/app/services/authorization/user_permissible_service.rb +++ b/app/services/authorization/user_permissible_service.rb @@ -28,6 +28,7 @@ def allowed_in_project?(permission, projects_to_check) def allowed_in_any_project?(permission) perms = contextual_permissions(permission, :project) + return false unless authorizable_user? return true if admin_and_all_granted_to_admin?(perms) Project.allowed_to(user, perms).exists? @@ -48,15 +49,17 @@ def allowed_in_entity?(permission, entities_to_check, entity_class) def allowed_in_any_entity?(permission, entity_class, in_project: nil) perms = contextual_permissions(permission, context_name(entity_class)) + return false unless authorizable_user? return true if admin_and_all_granted_to_admin?(perms) # entity_class.allowed_to will also check whether the user has the permission via a membership in the project. + # TODO: ^--- thats' actually not the case weirdly... allowed_scope = entity_class.allowed_to(user, perms) if in_project - allowed_scope.exists?(project: in_project) + allowed_in_single_project?(perms, in_project) || allowed_scope.exists?(project: in_project) else - allowed_scope.exists? + allowed_in_any_project?(perms) || allowed_scope.exists? end end From f7fb74dc031950e05c5139a460e76f8b738e1451 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 7 Nov 2023 17:14:37 +0100 Subject: [PATCH 6/6] Add change_status actions to the manage_user permission --- config/initializers/permissions.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/permissions.rb b/config/initializers/permissions.rb index 5ff57501ff8f..69f57581179c 100644 --- a/config/initializers/permissions.rb +++ b/config/initializers/permissions.rb @@ -63,7 +63,7 @@ map.permission :manage_user, { - users: %i[index show edit update], + users: %i[index show edit update change_status change_status_info], 'users/memberships': %i[create update destroy], admin: %i[index] },