From fb4f5f603c3fc614e281ba568315245ba2fe9efb Mon Sep 17 00:00:00 2001 From: Daria Mayorova Date: Thu, 17 Oct 2024 13:06:10 +0200 Subject: [PATCH] Fix Service permitted_for(user) scope --- app/models/service.rb | 6 ++- app/models/user/permissions.rb | 48 +++++++++++++++++-- .../api/policies_controller_test.rb | 12 ++++- .../api/proxy_configs_controller_test.rb | 10 ++++ 4 files changed, 68 insertions(+), 8 deletions(-) diff --git a/app/models/service.rb b/app/models/service.rb index d65494d2a8..f995605e34 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -115,10 +115,12 @@ def cinstance scope :permitted_for, ->(user = nil) { next all unless user + next none if user.no_services_allowed? + account = user.account account_services = (account.provider? ? account : account.provider_account).services - self.merge( - account_services.merge(user.forbidden_some_services? ? where(id: user.member_permission_service_ids) : {}) + merge( + account_services.merge(user.selected_services_allowed? ? where(id: user.member_permission_service_ids) : {}) ) } diff --git a/app/models/user/permissions.rb b/app/models/user/permissions.rb index 77e2727dcd..a548b44425 100644 --- a/app/models/user/permissions.rb +++ b/app/models/user/permissions.rb @@ -79,13 +79,18 @@ def existing_service_ids # but then it is much harder to test MemberPermission#service_ids= # returns [] if no services are enabled, and nil if all (current and future) services are enabled def member_permission_service_ids - return nil if admin? || !services_member_permission - permitted_service_ids = services_member_permission.try(:service_ids) || [] - permitted_service_ids & existing_service_ids + return @member_permission_service_ids if defined?(@member_permission_service_ids) + + if admin? || !services_member_permission + @member_permission_service_ids = nil + else + permitted_service_ids = services_member_permission.try(:service_ids) || [] + @member_permission_service_ids = permitted_service_ids & existing_service_ids + end end def services_member_permission - member_permissions.find { |permission| permission.admin_section == :services } + @services_member_permission ||= member_permissions.find { |permission| permission.admin_section == :services } end def has_access_to_service?(service) @@ -95,16 +100,49 @@ def has_access_to_service?(service) # Lack of the services section means it is the old permission system where everyone had access # to every service. So to limit the scope only for new users, we start adding this permission. + # TODO: check if it can be replaced with other methods def has_access_to_all_services? !admin_sections.include?(:services) || admin? end + # Returns: + # :none - if no services are allowed + # :all - if all services are allowed for the selected service-related permissions + # :selected - if a subset of services are allowed for the selected service-related permissions + def permitted_services_status + if admin? || (service_permissions_selected? && member_permission_service_ids.nil?) + :all + elsif service_permissions_selected? && member_permission_service_ids&.any? + :selected + else + :none + end + end + + def no_services_allowed? + permitted_services_status == :none + end + + def all_services_allowed? + permitted_services_status == :all + end + + def selected_services_allowed? + permitted_services_status == :selected + end + + # TODO: check if it can be replaced with other methods def forbidden_some_services? !has_access_to_all_services? && account.provider_can_use?(:service_permissions) end + def service_permissions_selected? + @service_permissions_selected ||= (member_permission_ids & AdminSection::SERVICE_PERMISSIONS).any? + end + + # TODO: check if it can be replaced with other methods def access_to_service_admin_sections? - (member_permission_ids & AdminSection::SERVICE_PERMISSIONS).any? && accessible_services? + service_permissions_selected? && accessible_services? end def reload(*) diff --git a/test/integration/api/policies_controller_test.rb b/test/integration/api/policies_controller_test.rb index c3cca9f8e4..ba468cfcbe 100644 --- a/test/integration/api/policies_controller_test.rb +++ b/test/integration/api/policies_controller_test.rb @@ -7,7 +7,7 @@ def setup @provider = FactoryBot.create(:provider_account) @service = @provider.default_service login! @provider - Policies::PoliciesListService.expects(:call!).returns({}) + Policies::PoliciesListService.stubs(:call!).returns({}) end @@ -55,4 +55,14 @@ def setup get edit_admin_service_policies_path(@service) assert_response :service_unavailable end + + test 'policies edit for members with no permissions' do + member = FactoryBot.create(:member, account: @provider, state: 'active') + logout! && login!(@provider, user: member) + + get edit_admin_service_policies_path(@service) + + # TODO: maybe this should be be a :forbidden + assert_response :not_found + end end diff --git a/test/integration/api/proxy_configs_controller_test.rb b/test/integration/api/proxy_configs_controller_test.rb index d8444199a9..28db9bf25e 100644 --- a/test/integration/api/proxy_configs_controller_test.rb +++ b/test/integration/api/proxy_configs_controller_test.rb @@ -56,4 +56,14 @@ def setup assert_response :success assert_equal '{"foo":"bar"}', response.body end + + test 'proxy configs index for members with no permissions' do + member = FactoryBot.create(:member, account: @provider, state: 'active') + logout! && login!(@provider, user: member) + + get admin_service_proxy_configs_path(service_id: service, environment: 'sandbox') + + # TODO: maybe this should be be a :forbidden + assert_response :not_found + end end