THREESCALE-6412: Fix admin member user permissions for services (also THREESCALE-11202)

app/controllers/api/plan_copies_controller.rb

@@ -1,10 +1,10 @@
 # frozen_string_literal: true
 
 class Api::PlanCopiesController < FrontendController
-  before_action :find_plan
-  before_action :find_service
   before_action :authorize_section, only: %i[new create]
   before_action :authorize_action, only: %i[new create]
+  before_action :find_plan
+  before_action :find_service
 
   def create
     @plan = @original.copy(params[@type] || {})

app/models/contract.rb

@@ -62,10 +62,15 @@ def self.issued_by(issuer, *ids)
     where.has { plan_id.in( scope ) }
   end
 
+  # FIXME: this probably shouldn't return `all`, it's dangerous if it is accidentally
+  # used as a separate scope, and not merged with other scopes that filters by account previously
   scope :permitted_for, ->(user) {
-    next all unless user.forbidden_some_services?
+    permitted_services_status = user.permitted_services_status
+    next none if permitted_services_status == :none
 
-    where(service_id: user.member_permission_service_ids)
+    next all if permitted_services_status == :all
+
+    merge(permitted_services_status == :selected ? where(service_id: user.member_permission_service_ids) : {})
   }
 
   # Return contracts bought by given account.

app/models/service.rb

@@ -115,10 +115,14 @@ def cinstance
   scope :permitted_for, ->(user = nil) {
     next all unless user
 
+    permitted_services_status = user.permitted_services_status
+
+    next none if permitted_services_status == :none
+
     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(permitted_services_status == :selected ? where(id: user.member_permission_service_ids) : {})
     )
   }
 

app/models/user/permissions.rb

@@ -5,7 +5,7 @@
 module User::Permissions
   extend ActiveSupport::Concern
 
-  ATTRIBUTES = %I[ role member_permission_ids member_permission_service_ids ]
+  ATTRIBUTES = %I[role member_permission_ids member_permission_service_ids].freeze
 
   included do
     has_many :member_permissions, dependent: :destroy, autosave: true
@@ -16,15 +16,12 @@ module User::Permissions
     alias_attribute :allowed_service_ids, :member_permission_service_ids
   end
 
-  #TODO: this is repeated from bcms_hacks plugins because of some loading problem
   def has_permission?(permission)
-    return true  if account.provider? && admin?
+    return true if account.provider? && admin?
     return false if account.buyer?
 
-    # check = Permission.count(:include => {:groups => :users}, :conditions => ["users.id = ? and permissions.name=?", id, permission]) > 0
-
     admin_sections.include?(permission.to_sym).tap do |check|
-      logger.info "~> #{username} has_permission?(#{permission}) => #{check}"
+      logger.debug "~> #{username} has_permission?(#{permission}) => #{check}"
     end
   end
 
@@ -60,11 +57,11 @@ def member_permission_ids=(roles)
   end
 
   def member_permission_service_ids=(service_ids)
-    if service_ids.present?
-      service_ids = Array(service_ids).compact.map(&:to_i)
+    if service_ids.is_a? Array
+      service_ids = service_ids.compact_blank.map(&:to_i)
       member_permission = services_member_permission || member_permissions.build(admin_section: :services)
       member_permission.service_ids = service_ids & existing_service_ids
-    else
+    elsif service_ids.blank?
       self.member_permissions = member_permissions - [services_member_permission].compact
     end
   ensure
@@ -80,6 +77,7 @@ def existing_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
   end
@@ -89,22 +87,34 @@ def services_member_permission
   end
 
   def has_access_to_service?(service)
-    services_permission = services_member_permission
-    services_permission && services_permission.has_service?(service) || has_access_to_all_services?
+    services_member_permission&.has_service?(service) || has_access_to_all_services?
+  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 is 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
 
-  # 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.
   def has_access_to_all_services?
-    !admin_sections.include?(:services) || admin?
+    permitted_services_status == :all
   end
 
-  def forbidden_some_services?
-    !has_access_to_all_services? && account.provider_can_use?(:service_permissions)
+  # Returns whether the user has access to any service-related permissions (partners, plans or monitoring)
+  def service_permissions_selected?
+    (member_permission_ids & AdminSection::SERVICE_PERMISSIONS).any?
   end
 
   def access_to_service_admin_sections?
-    (member_permission_ids & AdminSection::SERVICE_PERMISSIONS).any? && accessible_services?
+    service_permissions_selected? && accessible_services?
   end
 
   def reload(*)

config/abilities/provider_any.rb

@@ -21,7 +21,8 @@
     cannot %i[destroy update_role], user
 
     # Services
-    can %i[read show edit update], Service, user.accessible_services.where_values_hash
+    user_accessible_services = user.accessible_services
+    can %i[read show edit update], Service, user_accessible_services.where_values_hash unless user_accessible_services.is_a? ActiveRecord::NullRelation
 
     #
     # Events

config/abilities/provider_member.rb

@@ -28,10 +28,11 @@
 
     # Using historical optimized way and leave canonical way (through plan) commented out below
     # The resulting hash presently is something like {"type"=>"Cinstance", "service_id"=>[ids..]}
-    can %i[read show edit update], Cinstance, Cinstance.permitted_for(user).where_values_hash
+    permitted_cinstances = Cinstance.permitted_for(user)
+    can %i[read show edit update], Cinstance, permitted_cinstances.where_values_hash unless permitted_cinstances.is_a? ActiveRecord::NullRelation
     # can %i[read show edit update], Cinstance, user.accessible_cinstances do |cinstance|
     #   cinstance.plan&.issuer_type == "Service" && cinstance.plan.issuer.account == user.account &&
-    #     (!user.forbidden_some_services? || user.member_permission_service_ids.include?(cinstance.plan.issuer.id))
+    #     (user.permitted_services_status == :selected || user.member_permission_service_ids.include?(cinstance.plan.issuer.id))
     # end
 
     # abilities for buyer users

doc/active_docs/account_management_api.json

@@ -2812,7 +2812,7 @@
                   },
                   "allowed_service_ids[]": {
                     "type": "array",
-                    "description": "IDs of the services that need to be enabled, URL-encoded array. To disable all services, set the value to '[]'. To enable all services, add parameter 'allowed_service_ids[]' with no value to the 'curl' command (can't be done in ActiveDocs)",
+                    "description": "IDs of the services that need to be enabled, URL-encoded array. To disable all services, set the value to '[]'. To enable all services, add parameter 'allowed_service_ids' with no value to the 'curl' command (can't be done in ActiveDocs)",
                     "items": {
                       "type": "integer"
                     }

spec/acceptance/api/member_permissions_spec.rb

@@ -28,7 +28,7 @@
 
   shared_context "all services disabled" do
     before do
-      user.member_permission_service_ids = "[]"
+      user.member_permission_service_ids = []
     end
   end
 

test/integration/api/application_plans_controller_test.rb

@@ -239,13 +239,13 @@ class ProviderMemberTest < self
       get new_admin_service_application_plan_path(service)
       assert_response :forbidden
 
-      post admin_service_application_plans_path(service), params: { application_plan:{ name: 'planName' } }
+      post admin_service_application_plans_path(service), params: { application_plan: { name: 'planName' } }
       assert_response :forbidden
 
       get edit_admin_application_plan_path(plan)
       assert_response :forbidden
 
-      put admin_application_plan_path(plan), params: { application_plan:{ name: 'New plan name' } }
+      put admin_application_plan_path(plan), params: { application_plan: { name: 'New plan name' } }
       assert_response :forbidden
 
       post masterize_admin_service_application_plans_path(service)
@@ -274,13 +274,13 @@ class ProviderMemberTest < self
       get new_admin_service_application_plan_path(service)
       assert_response :success
 
-      post admin_service_application_plans_path(service), params: { application_plan:{ name: 'planName' } }
+      post admin_service_application_plans_path(service), params: { application_plan: { name: 'planName' } }
       assert_response :redirect
 
       get edit_admin_application_plan_path(plan)
       assert_response :success
 
-      put admin_application_plan_path(plan), params: { application_plan:{ name: 'New plan name' } }
+      put admin_application_plan_path(plan), params: { application_plan: { name: 'New plan name' } }
       assert_response :redirect
 
       post masterize_admin_service_application_plans_path(service)

test/integration/api/backend_usages_controller_test.rb

@@ -109,15 +109,13 @@ class Api::BackendUsagesControllerTest < ActionDispatch::IntegrationTest
     get admin_service_backend_usages_path(service)
     assert_response :success
 
-    member.member_permission_service_ids = []
-    member.save!
+    member.update(member_permission_service_ids: [])
 
     get admin_service_backend_usages_path(service)
     assert_response :success
 
     other_service = FactoryBot.create(:simple_service, account: provider)
-    member.member_permission_service_ids = [other_service.id]
-    member.save!
+    member.update(member_permission_service_ids: [other_service.id])
 
     get admin_service_backend_usages_path(service)
     assert_response :not_found

test/integration/api/integrations_controller_test.rb

@@ -23,7 +23,7 @@ def setup
     login! provider, user: member
 
     get admin_service_integration_path(service_id: service.id)
-    assert_response 403
+    assert_response 404
 
     member.member_permissions.create!(admin_section: 'plans')
     get admin_service_integration_path(service_id: service.id)

test/integration/api/policies_controller_test.rb

@@ -55,4 +55,15 @@ def setup
     get edit_admin_service_policies_path(@service)
     assert_response :service_unavailable
   end
+
+  test 'policies edit for members with no permissions' do
+    Policies::PoliciesListService.unstub(:call!)
+    Policies::PoliciesListService.expects(:call!).never
+    member = FactoryBot.create(:member, account: @provider, state: 'active')
+    logout! && login!(@provider, user: member)
+
+    get edit_admin_service_policies_path(@service)
+
+    assert_response :not_found
+  end
 end

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

test/integration/buyers/accounts_controller_test.rb

@@ -79,17 +79,17 @@ def test_show
       cinstance = service.cinstances.last
       cinstance.update(name: 'Alaska Application App')
 
-      User.any_instance.expects(:has_access_to_all_services?).returns(true).at_least_once
+      assert_nil user.member_permission_service_ids
       get admin_buyers_account_path(buyer)
       assert_response :success
       assert_match 'Alaska Application App', response.body
 
-      User.any_instance.expects(:has_access_to_all_services?).returns(false).at_least_once
+      user.update(member_permission_service_ids: [])
       get admin_buyers_account_path(buyer)
       assert_response :success
       assert_not_match 'Alaska Application App', response.body
 
-      User.any_instance.expects(:member_permission_service_ids).returns([service.id]).at_least_once
+      user.update(member_permission_service_ids: [service.id])
       get admin_buyers_account_path(buyer)
       assert_response :success
       assert_match 'Alaska Application App', response.body

test/integration/master/providers/plans_controller_test.rb

@@ -52,7 +52,7 @@ def setup
   end
 
   test '#update for a member with permission partners but without the service' do
-    master_member.update!({member_permission_ids: ['partners'], member_permission_service_ids: '[]'})
+    master_member.update!({member_permission_ids: ['partners'], member_permission_service_ids: []})
     login! master_account, user: master_member
 
     put master_provider_plan_path(tenant), params: { plan_id: new_plan.id, format: :js }

test/integration/stats/applications_test.rb

@@ -8,37 +8,33 @@ def setup
     @service = @provider.default_service
     @plan = FactoryBot.create(:simple_application_plan, issuer: @service)
     @application = FactoryBot.create(:simple_cinstance, plan: @plan)
+    @member = FactoryBot.create(:member, account: @provider, member_permission_ids: %i[partners plans], state: 'active')
 
     host! @provider.external_admin_domain
-    login_provider @provider
+    login_provider @provider, user: @member
   end
 
   test '#show nonexistent application does not check permissions' do
-    User.any_instance.expects(:has_access_to_all_services?).never
     User.any_instance.expects(:member_permission_service_ids).never
 
     get admin_buyers_stats_application_path(id: 'foo')
     assert_response :not_found
   end
 
-  test '#show does not check member permission with access to all services' do
-    User.any_instance.expects(:has_access_to_all_services?).returns(true).at_least_once
-    User.any_instance.expects(:member_permission_service_ids).never
+  test '#show succeeds with access to all services' do
+    assert_nil @member.member_permission_service_ids
     get admin_buyers_stats_application_path(id: @application.id)
     assert_response :success
   end
 
-  test '#show needs member permission' do
-    User.any_instance.expects(:has_access_to_all_services?).returns(false)
-    User.any_instance.expects(:access_to_service_admin_sections?).returns(false)
-    User.any_instance.expects(:member_permission_service_ids).returns([@service.id]).at_least_once
+  test '#show succeeds with permission for a specific service' do
+    @member.update(member_permission_service_ids: [@service.id])
     get admin_buyers_stats_application_path(id: @application.id)
     assert_response :success
   end
 
   test '#show is forbidden without member permission' do
-    User.any_instance.expects(:has_access_to_all_services?).returns(false)
-    User.any_instance.expects(:member_permission_service_ids).returns([]).at_least_once
+    @member.update(member_permission_service_ids: [])
     get admin_buyers_stats_application_path(id: @application.id)
     assert_response :forbidden
   end

test/integration/stats/data/requests_to_api_test.rb

@@ -35,21 +35,19 @@ def setup
 
     token.scopes = ['stats']
     token.save!
-    member.admin_sections = []
-    member.save!
+
     # member does not have the right permission
+    member.update(member_permission_ids: [])
     get usage_stats_data_applications_path(@application, format: :json), params: params
     assert_response :forbidden
 
-    member.admin_sections = ['monitoring']
-    member.save!
-    User.any_instance.expects(:has_access_to_all_services?).returns(false).at_least_once
     # the service is not accessible for the member
+    member.update(member_permission_ids: [:monitoring], member_permission_service_ids: [])
     get usage_stats_data_applications_path(@application, format: :json), params: params
     assert_response :forbidden
 
-    User.any_instance.expects(:member_permission_service_ids).returns([@service.id]).at_least_once
     # the service is accessible for the member
+    member.update(member_permission_service_ids: [@service.id])
     get usage_stats_data_applications_path(@application, format: :json), params: params
     assert_response :success
   end