Merge pull request #3929 from 3scale/fix-member-permissions-issues

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

@@ -63,9 +63,14 @@ def self.issued_by(issuer, *ids)
   end
 
   scope :permitted_for, ->(user) {
-    next all unless user.forbidden_some_services?
-
-    where(service_id: user.member_permission_service_ids)
+    case user.permitted_services_status
+    when :none
+      none
+    when :all
+      merge(provided_by(user.account))
+    else
+      merge(where(service_id: user.member_permission_service_ids))
+    end
   }
 
   # 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,12 @@ 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
+      # remove all non-integer values
+      service_ids = service_ids.map { Integer(_1, exception: false) }.compact_blank
       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 +78,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 +88,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?
+    has_access_to_all_services? || services_member_permission&.has_service?(service)
+  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.present?
+      :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, send `allowed_service_ids[]` with an empty value, e.g. `allowed_service_ids%5B%5D=` (can't be done in ActiveDocs). `<allowed_service_ids/>` or `\"allowed_service_ids\":[]` in the response indicate that no services are allowed. To enable all services, send 'allowed_service_ids' with no value to the 'curl' command, e.g. `allowed_service_ids=` (can't be done in ActiveDocs). Missing `<allowed_service_ids>` or `\"allowed_service_ids\":null` indicate that all services are allowed.",
                     "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/admin/api/member_permissions_controller_test.rb

@@ -90,7 +90,7 @@ class Admin::Api::MemberPermissionsControllerTest < ActionDispatch::IntegrationT
   test "PUT: enable 'settings', but disable all services" do
     user.update({ allowed_service_ids: [service1_id] })
     # allowed_sections%5B%5D=settings&allowed_service_ids%5B%5D=%5B%5D
-    params = { allowed_sections: ['settings'], allowed_service_ids: ["[]"], access_token: token }
+    params = { allowed_sections: ['settings'], allowed_service_ids: [""], access_token: token }
 
     put admin_api_permissions_path(id: user.id, format: :json), params: params
 
@@ -103,6 +103,21 @@ class Admin::Api::MemberPermissionsControllerTest < ActionDispatch::IntegrationT
     assert_empty user.allowed_service_ids
   end
 
+  # This way was previously documented in ActiveDocs, so keeping it for backwards compatibility
+  test "PUT: disable all services with `[]` value" do
+    user.update({ allowed_service_ids: [service1_id] })
+    # allowed_service_ids%5B%5D=%5B%5D
+    params = { allowed_service_ids: ["[]"], access_token: token }
+
+    put admin_api_permissions_path(id: user.id, format: :json), params: params
+
+    assert_not_nil(permissions = JSON.parse(response.body)['permissions'])
+    assert_empty permissions['allowed_service_ids']
+
+    user.member_permissions.reload
+    assert_equal [], user.allowed_service_ids
+  end
+
   test "updating admin's permissions is not allowed" do
     user.update_attribute :role, 'admin'
     params = { allowed_sections: ['monitoring'], allowed_service_ids: [service1_id], access_token: token }

test/integration/admin/api/service_plans_controller_test.rb

@@ -78,8 +78,7 @@ class ProviderMemberTest < self
       @forbidden_service = FactoryBot.create(:simple_service, account: provider)
       @forbidden_service_plan = FactoryBot.create(:service_plan, name: 'Forbidden Plan', state: 'published', service: forbidden_service)
 
-      @member = FactoryBot.create(:member, account: provider, member_permission_ids: ['partners'])
-      member.member_permission_service_ids = [service.id]
+      @member = FactoryBot.create(:member, account: provider, member_permission_ids: ['partners'], member_permission_service_ids: [service.id])
       member.activate!
 
       login!(provider, user: member)

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

@@ -106,20 +106,26 @@ class Api::BackendUsagesControllerTest < ActionDispatch::IntegrationTest
     logout!
     login! provider, user: member
 
+    # a specific service allowed
     get admin_service_backend_usages_path(service)
     assert_response :success
 
-    member.member_permission_service_ids = []
-    member.save!
+    # no services allowed
+    member.update(member_permission_service_ids: [])
 
     get admin_service_backend_usages_path(service)
-    assert_response :success
+    assert_response :not_found
 
+    # a different service allowed
     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
+
+    # all services allowed
+    member.update(member_permission_service_ids: nil)
+    get admin_service_backend_usages_path(service)
+    assert_response :success
   end
 end

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/provider/admin/account/users_controller_test.rb

@@ -46,22 +46,31 @@ def test_update_blank_member_permission_ids
     assert_equal [], @user.admin_sections.to_a
   end
 
-  # Test for https://app.bugsnag.com/3scale-networks-sl/system/errors/623154afea8d6b0008c052c5
   test '#update member_permission_service_ids' do
-    rolling_updates_on
-    Account.any_instance.expects(:provider_can_use?).with(:service_permissions).returns(false)
+    User.any_instance.stubs(:existing_service_ids).returns([1,2,3])
 
     put provider_admin_account_user_path(@user), params: { user: { member_permission_service_ids: '' } }
     assert_response :redirect
+    assert_nil @user.reload.member_permission_service_ids
 
+    # old way of setting empty services list
+    put provider_admin_account_user_path(@user), params: { user: { member_permission_service_ids: ['[]'] } }
+    assert_response :redirect
+    assert_equal [], @user.reload.member_permission_service_ids
+
+    # this parameter value doesn't change the service ids
+    @user.update(member_permission_service_ids: [1])
     put provider_admin_account_user_path(@user), params: { user: { member_permission_service_ids: '[]' } }
     assert_response :redirect
+    assert_equal [1], @user.reload.member_permission_service_ids
 
     put provider_admin_account_user_path(@user), params: { user: { member_permission_service_ids: [''] } }
     assert_response :redirect
+    assert_equal [], @user.reload.member_permission_service_ids
 
     put provider_admin_account_user_path(@user), params: { user: { member_permission_service_ids: %w[1 2] } }
     assert_response :redirect
+    assert_equal [1, 2], @user.reload.member_permission_service_ids
   end
 
   test '#update with empty user' do