refactor: standardize permission layers

app/controllers/internal/log_access_controller.rb

@@ -6,18 +6,20 @@ class LogAccessController < RestController::BaseController
     def lookup(guid)
       check_read_permissions!
 
-      if roles.admin? || roles.admin_read_only? || roles.global_auditor?
+      if permission_queryer.can_read_globally?
         found = LogAccessFetcher.new.app_exists?(guid)
       else
-        allowed_space_guids = membership.authorized_space_guids_subquery(Permissions::ROLES_FOR_SPACE_READING)
+        allowed_space_guids = permission_queryer.readable_space_guids_query
         found = LogAccessFetcher.new.app_exists_by_space?(guid, allowed_space_guids)
       end
 
       found ? HTTP::OK : HTTP::NOT_FOUND
     end
 
-    def membership
-      @membership ||= Membership.new(current_user)
+    private
+
+    def permission_queryer
+      @permission_queryer ||= Permissions.new(SecurityContext.current_user)
     end
   end
 end

app/controllers/v3/mixins/service_permissions.rb

@@ -16,15 +16,15 @@ def visible_in_readable_orgs?(service_plans)
     end
 
     def writable_space_scoped?(space)
-      space && space.has_developer?(current_user)
+      space && permission_queryer.can_write_to_active_space?(space.id)
     end
 
     def current_user_can_write?(resource)
       permission_queryer.can_write_globally? || writable_space_scoped?(resource.service_broker.space)
     end
 
     def visible_space_scoped?(space)
-      current_user && space && (space.has_member?(current_user) || space.has_supporter?(current_user))
+      current_user && space && permission_queryer.can_read_from_space_as_space_member?(space.id)
     end
 
     def visible_to_current_user?(service: nil, plan: nil)

app/controllers/v3/roles_controller.rb

@@ -166,29 +166,19 @@ def create_uaa_shadow_user(message)
   end
 
   def readable_users
-    current_user.readable_users(permission_queryer.can_read_globally?)
+    permission_queryer.readable_users_query
   end
 
   def readable_roles
     if permission_queryer.can_read_globally?
       Role
     else
-      # This rather complex filter should be applied to the overall Role dataset and is thus using the (virtual row)
-      # block syntax.
-      readable_by_space = { space_id: visible_space_ids }
-      readable_by_org = { organization_id: visible_org_ids }
+      readable_by_space = { space_id: permission_queryer.readable_spaces_query.select(:id) }
+      readable_by_org = { organization_id: permission_queryer.readable_orgs_query.select(:id) }
       Role.where { Sequel.|(readable_by_space, readable_by_org) }
     end
   end
 
-  def visible_space_ids
-    Space.user_visibility_filter(current_user)[:spaces__id]
-  end
-
-  def visible_org_ids
-    Organization.user_visibility_filter(current_user)[:id]
-  end
-
   def unprocessable_space!
     unprocessable!('Invalid space. Ensure that the space exists and you have access to it.')
   end

app/controllers/v3/users_controller.rb

@@ -93,13 +93,11 @@ def destroy
   private
 
   def fetch_readable_users(message)
-    admin_roles = permission_queryer.can_read_globally?
-    UserListFetcher.fetch_all(message, current_user.readable_users(admin_roles))
+    UserListFetcher.fetch_all(message, permission_queryer.readable_users_query)
   end
 
   def fetch_user_if_readable(desired_guid)
-    readable_users = current_user.readable_users(permission_queryer.can_read_globally?)
-    readable_users.first(guid: desired_guid)
+    permission_queryer.readable_users_query.first(guid: desired_guid)
   end
 
   def user_not_found!

lib/cloud_controller/membership.rb

@@ -68,6 +68,17 @@ def member_org_ids(roles, extra_filters={})
       end&.select(:organization_id)
     end
 
+    def visible_user_ids_in_orgs(org_roles)
+      org_ids = member_org_ids(org_roles)
+      return nil unless org_ids
+
+      OrganizationUser.where(organization_id: org_ids).select(:user_id).
+        union(OrganizationManager.where(organization_id: org_ids).select(:user_id), from_self: false).
+        union(OrganizationAuditor.where(organization_id: org_ids).select(:user_id), from_self: false).
+        union(OrganizationBillingManager.where(organization_id: org_ids).select(:user_id), from_self: false).
+        select(:user_id)
+    end
+
     private
 
     def role_applies_to_space?(roles, space_id)

lib/cloud_controller/permissions.rb

@@ -98,7 +98,7 @@ def can_write_globally?
   end
 
   def is_org_manager?
-    VCAP::CloudController::OrganizationManager.where(user_id: @user.id).any?
+    membership.authorized_org_guids_subquery(VCAP::CloudController::Membership::ORG_MANAGER).any?
   end
 
   def readable_org_guids
@@ -178,6 +178,10 @@ def can_read_from_space?(space_id, org_id)
     can_read_globally? || membership.role_applies?(ROLES_FOR_SPACE_READING, space_id, org_id)
   end
 
+  def can_read_from_space_as_space_member?(space_id)
+    can_read_globally? || membership.role_applies?(SPACE_ROLES, space_id)
+  end
+
   def can_download_droplet?(space_id, org_id)
     can_read_globally? || membership.role_applies?(ROLES_FOR_DROPLET_DOWLOAD, space_id, org_id)
   end
@@ -236,11 +240,8 @@ def readable_space_scoped_spaces_query
   def can_read_route?(space_id)
     return true if can_read_globally?
 
-    space = VCAP::CloudController::Space.where(id: space_id).first
-
-    space.has_member?(@user) || space.has_supporter?(@user) ||
-      @user.managed_organizations.map(&:id).include?(space.organization_id) ||
-      @user.audited_organizations.map(&:id).include?(space.organization_id)
+    org_id = VCAP::CloudController::Space.where(id: space_id).get(:organization_id)
+    membership.role_applies?(ROLES_FOR_ROUTE_READING, space_id, org_id)
   end
 
   def space_guids_with_readable_routes_query
@@ -260,25 +261,75 @@ def can_read_system_environment_variables?(space_id, org_id)
   end
 
   def readable_app_guids
-    VCAP::CloudController::AppModel.user_visible(@user, can_read_globally?).select_map(:guid)
+    if can_read_globally?
+      VCAP::CloudController::AppModel.select_map(:guid)
+    elsif @user
+      visible_space_guids = membership.authorized_space_guids_subquery(ROLES_FOR_SPACE_READING)
+      VCAP::CloudController::AppModel.where(space_guid: visible_space_guids).select_map(:guid)
+    else
+      []
+    end
   end
 
   def readable_space_quota_guids
-    VCAP::CloudController::SpaceQuotaDefinition.user_visible(@user, can_read_globally?).map(&:guid)
+    if can_read_globally?
+      VCAP::CloudController::SpaceQuotaDefinition.select_map(:guid)
+    elsif @user
+      visible_space_ids = membership.authorized_spaces_subquery(SPACE_ROLES).select(:id)
+      org_manager_org_ids = membership.authorized_orgs_subquery(VCAP::CloudController::Membership::ORG_MANAGER).select(:id)
+
+      VCAP::CloudController::SpaceQuotaDefinition.where(
+        Sequel.or([
+          [:id, VCAP::CloudController::Space.where(id: visible_space_ids).
+                                            exclude(space_quota_definition_id: nil).
+                                            select(:space_quota_definition_id)],
+          [:organization_id, org_manager_org_ids]
+        ])
+      ).select_map(:guid)
+    else
+      []
+    end
   end
 
   def readable_security_group_guids
     readable_security_group_guids_query.select_map(:guid)
   end
 
   def readable_security_group_guids_query
-    VCAP::CloudController::SecurityGroup.user_visible(@user, can_read_globally?).select(:guid)
+    if can_read_globally?
+      VCAP::CloudController::SecurityGroup.dataset.select(:guid)
+    elsif @user
+      visible_space_ids = membership.authorized_spaces_subquery(ROLES_FOR_SPACE_READING).select(:id)
+
+      VCAP::CloudController::SecurityGroup.where(
+        Sequel.or([
+          [:running_default, true],
+          [:staging_default, true],
+          [:id, VCAP::CloudController::SecurityGroupsSpace.where(space_id: visible_space_ids).select(:security_group_id).
+                union(
+                  VCAP::CloudController::StagingSecurityGroupsSpace.where(staging_space_id: visible_space_ids).select(:staging_security_group_id),
+                  from_self: false
+                )]
+        ])
+      ).select(:guid)
+    else
+      VCAP::CloudController::SecurityGroup.where(id: nil).select(:guid)
+    end
   end
 
   def can_update_build_state?
     can_write_globally? || roles.build_state_updater?
   end
 
+  def readable_users_query
+    if can_read_globally?
+      VCAP::CloudController::User.dataset
+    else
+      visible_user_ids = membership.visible_user_ids_in_orgs(ROLES_FOR_ORG_READING)
+      VCAP::CloudController::User.where(id: visible_user_ids).or(id: @user.id)
+    end
+  end
+
   def readable_event_dataset
     return VCAP::CloudController::Event.dataset if can_read_globally?
 

spec/unit/lib/cloud_controller/membership_spec.rb

@@ -489,6 +489,54 @@ module VCAP::CloudController
       end
     end
 
+    describe '#visible_user_ids_in_orgs' do
+      let(:org_roles) { Permissions::ROLES_FOR_ORG_READING }
+
+      context 'when the user has an org role' do
+        let(:other_user) { User.make }
+        let(:manager_user) { User.make }
+        let(:auditor_user) { User.make }
+        let(:billing_user) { User.make }
+
+        before do
+          organization.add_user(user)
+          organization.add_user(other_user)
+          organization.add_manager(manager_user)
+          organization.add_auditor(auditor_user)
+          organization.add_billing_manager(billing_user)
+        end
+
+        it 'returns user ids from all org role types in the org' do
+          result = membership.visible_user_ids_in_orgs(org_roles)
+          user_ids = result.select_map(:user_id)
+          expect(user_ids).to include(user.id, other_user.id, manager_user.id, auditor_user.id, billing_user.id)
+        end
+
+        it 'does not include users from other orgs' do
+          other_org = Organization.make
+          unrelated_user = User.make
+          other_org.add_user(unrelated_user)
+
+          user_ids = membership.visible_user_ids_in_orgs(org_roles).select_map(:user_id)
+          expect(user_ids).not_to include(unrelated_user.id)
+        end
+      end
+
+      context 'when the user has no org role' do
+        it 'returns an empty result set' do
+          result = membership.visible_user_ids_in_orgs(org_roles)
+          expect(result.select_map(:user_id)).to be_empty
+        end
+      end
+
+      context 'when roles contain only space roles (no org subqueries)' do
+        it 'returns nil' do
+          result = membership.visible_user_ids_in_orgs([Membership::SPACE_DEVELOPER])
+          expect(result).to be_nil
+        end
+      end
+    end
+
     describe '#authorized_space_guids' do
       let(:user) { User.make }
 

spec/unit/lib/cloud_controller/permissions_spec.rb

@@ -1157,6 +1157,118 @@ module VCAP::CloudController
       end
     end
 
+    describe '#readable_users_query' do
+      context 'when user is an admin' do
+        it 'returns all users' do
+          set_current_user(user, { admin: true })
+          other_user = User.make
+
+          result = permissions.readable_users_query
+          guids = result.select_map(:guid)
+          expect(guids).to include(user.guid)
+          expect(guids).to include(other_user.guid)
+        end
+      end
+
+      context 'when user is a read-only admin' do
+        it 'returns all users' do
+          set_current_user(user, { admin_read_only: true })
+          other_user = User.make
+
+          guids = permissions.readable_users_query.select_map(:guid)
+          expect(guids).to include(user.guid)
+          expect(guids).to include(other_user.guid)
+        end
+      end
+
+      context 'when user has an org membership' do
+        let(:other_user) { User.make }
+        let(:unrelated_user) { User.make }
+        let(:other_org) { Organization.make }
+
+        before do
+          org.add_user(user)
+          org.add_user(other_user)
+          other_org.add_user(unrelated_user)
+        end
+
+        it 'returns users from the same org plus self' do
+          guids = permissions.readable_users_query.select_map(:guid)
+          expect(guids).to include(user.guid)
+          expect(guids).to include(other_user.guid)
+          expect(guids).not_to include(unrelated_user.guid)
+        end
+      end
+
+      context 'when user has no org membership' do
+        it 'returns only the current user' do
+          guids = permissions.readable_users_query.select_map(:guid)
+          expect(guids).to contain_exactly(user.guid)
+        end
+      end
+    end
+
+    describe '#can_read_from_space_as_space_member?' do
+      context 'user has no membership' do
+        context 'and user is an admin' do
+          it 'returns true' do
+            set_current_user(user, { admin: true })
+            expect(permissions.can_read_from_space_as_space_member?(space.id)).to be true
+          end
+        end
+
+        context 'and user is a read-only admin' do
+          it 'returns true' do
+            set_current_user(user, { admin_read_only: true })
+            expect(permissions.can_read_from_space_as_space_member?(space.id)).to be true
+          end
+        end
+
+        context 'and user is a global auditor' do
+          it 'returns true' do
+            set_current_user_as_global_auditor
+            expect(permissions.can_read_from_space_as_space_member?(space.id)).to be true
+          end
+        end
+
+        context 'and user is not an admin' do
+          it 'returns false' do
+            set_current_user(user)
+            expect(permissions.can_read_from_space_as_space_member?(space.id)).to be false
+          end
+        end
+      end
+
+      context 'user has valid membership' do
+        before { org.add_user(user) }
+
+        it 'returns true for space developer' do
+          space.add_developer(user)
+          expect(permissions.can_read_from_space_as_space_member?(space.id)).to be true
+        end
+
+        it 'returns true for space manager' do
+          space.add_manager(user)
+          expect(permissions.can_read_from_space_as_space_member?(space.id)).to be true
+        end
+
+        it 'returns true for space auditor' do
+          space.add_auditor(user)
+          expect(permissions.can_read_from_space_as_space_member?(space.id)).to be true
+        end
+
+        it 'returns true for space supporter' do
+          space.add_supporter(user)
+          expect(permissions.can_read_from_space_as_space_member?(space.id)).to be true
+        end
+
+        it 'returns false for org manager (not a space member role)' do
+          org.add_manager(user)
+          expect(permissions.can_read_from_space_as_space_member?(space.id)).to be false
+        end
+      end
+    end
+
     describe '#readable_space_quota_guids' do
       let(:org1) { Organization.make }
       let(:org2) { Organization.make }