Merge branch '220203-gitlab-com-sso-create-no-access-role' into 'master'

Resolve "Gitlab.com SSO - Create "Minimal access" Role"

See merge request gitlab-org/gitlab!39731

app/controllers/admin/groups_controller.rb

@@ -19,7 +19,7 @@ class Admin::GroupsController < Admin::ApplicationController
     # the Group with statistics).
     @group = Group.with_statistics.find(group&.id)
     @members = present_members(
-      @group.members.order("access_level DESC").page(params[:members_page]))
+      group_members.order("access_level DESC").page(params[:members_page]))
     @requesters = present_members(
       AccessRequestsFinder.new(@group).execute(current_user))
     @projects = @group.projects.with_statistics.page(params[:projects_page])
@@ -82,6 +82,10 @@ class Admin::GroupsController < Admin::ApplicationController
     @group ||= Group.find_by_full_path(params[:id])
   end
 
+  def group_members
+    @group.members
+  end
+
   def group_params
     params.require(:group).permit(allowed_group_params)
   end

app/finders/group_members_finder.rb

@@ -27,7 +27,7 @@ class GroupMembersFinder < UnionFinder
     relations << group_members if include_relations.include?(:direct)
 
     if include_relations.include?(:inherited) && group.parent
-      parents_members = GroupMember.non_request
+      parents_members = GroupMember.non_request.non_minimal_access
         .where(source_id: group.ancestors.select(:id))
         .where.not(user_id: group.users.select(:id))
 
@@ -35,7 +35,7 @@ class GroupMembersFinder < UnionFinder
     end
 
     if include_relations.include?(:descendants)
-      descendant_members = GroupMember.non_request
+      descendant_members = GroupMember.non_request.non_minimal_access
         .where(source_id: group.descendants.select(:id))
         .where.not(user_id: group.users.select(:id))
 

app/finders/members_finder.rb

@@ -63,7 +63,7 @@ class MembersFinder
   def direct_group_members(include_descendants)
     requested_relations = [:inherited, :direct]
     requested_relations << :descendants if include_descendants
-    GroupMembersFinder.new(group).execute(include_relations: requested_relations).non_invite # rubocop: disable CodeReuse/Finder
+    GroupMembersFinder.new(group).execute(include_relations: requested_relations).non_invite.non_minimal_access # rubocop: disable CodeReuse/Finder
   end
 
   def project_invited_groups_members
@@ -73,7 +73,7 @@ class MembersFinder
       .public_or_visible_to_user(current_user)
       .select(:id)
 
-    GroupMember.with_source_id(invited_groups_ids_including_ancestors)
+    GroupMember.with_source_id(invited_groups_ids_including_ancestors).non_minimal_access
   end
 
   def distinct_union_of_members(union_members)

app/models/concerns/loaded_in_group_list.rb

@@ -54,6 +54,7 @@ module LoadedInGroupList
         .where(members[:source_type].eq(Namespace.name))
         .where(members[:source_id].eq(namespaces[:id]))
         .where(members[:requested_at].eq(nil))
+        .where(members[:access_level].gt(Gitlab::Access::MINIMAL_ACCESS))
     end
   end
 
@@ -70,7 +71,7 @@ module LoadedInGroupList
   end
 
   def member_count
-    @member_count ||= try(:preloaded_member_count) || users.count
+    @member_count ||= try(:preloaded_member_count) || members.count
   end
 end
 

app/models/group.rb

@@ -20,8 +20,10 @@ class Group < Namespace
 
   UpdateSharedRunnersError = Class.new(StandardError)
 
-  has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
+  has_many :all_group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent
+  has_many :group_members, -> { where(requested_at: nil).where.not(members: { access_level: Gitlab::Access::MINIMAL_ACCESS }) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
   alias_method :members, :group_members
+
   has_many :users, through: :group_members
   has_many :owners,
     -> { where(members: { access_level: Gitlab::Access::OWNER }) },
@@ -395,6 +397,10 @@ class Group < Namespace
     ])
   end
 
+  def users_count
+    members.count
+  end
+
   # Returns all users that are members of projects
   # belonging to the current group or sub-groups
   def project_users_with_descendants
@@ -630,6 +636,7 @@ class Group < Namespace
       .where(group_member_table[:requested_at].eq(nil))
       .where(group_member_table[:source_id].eq(group_group_link_table[:shared_with_group_id]))
       .where(group_member_table[:source_type].eq('Namespace'))
+      .non_minimal_access
   end
 
   def smallest_value_arel(args, column_alias)

app/models/member.rb

@@ -25,7 +25,6 @@ class Member < ApplicationRecord
   validates :user_id, uniqueness: { scope: [:source_type, :source_id],
                                     message: "already exists in source",
                                     allow_nil: true }
-  validates :access_level, inclusion: { in: Gitlab::Access.all_values }, presence: true
   validate :higher_access_level_than_group, unless: :importing?
   validates :invite_email,
     presence: {
@@ -60,6 +59,7 @@ class Member < ApplicationRecord
     left_join_users
       .where(user_ok)
       .where(requested_at: nil)
+      .non_minimal_access
       .reorder(nil)
   end
 
@@ -68,6 +68,8 @@ class Member < ApplicationRecord
     left_join_users
       .where(users: { state: 'active' })
       .non_request
+      .non_invite
+      .non_minimal_access
       .reorder(nil)
   end
 
@@ -85,6 +87,7 @@ class Member < ApplicationRecord
   scope :developers, -> { active.where(access_level: DEVELOPER) }
   scope :maintainers, -> { active.where(access_level: MAINTAINER) }
   scope :non_guests, -> { where('members.access_level > ?', GUEST) }
+  scope :non_minimal_access, -> { where('members.access_level > ?', MINIMAL_ACCESS) }
   scope :owners, -> { active.where(access_level: OWNER) }
   scope :owners_and_maintainers, -> { active.where(access_level: [OWNER, MAINTAINER]) }
   scope :with_user, -> (user) { where(user: user) }

app/models/members/group_member.rb

@@ -13,6 +13,9 @@ class GroupMember < Member
   # Make sure group member points only to group as it source
   default_value_for :source_type, SOURCE_TYPE
   validates :source_type, format: { with: /\ANamespace\z/ }
+  validates :access_level, presence: true
+  validate :access_level_inclusion
+
   default_scope { where(source_type: SOURCE_TYPE) } # rubocop:disable Cop/DefaultScope
 
   scope :of_groups, ->(groups) { where(source_id: groups.select(:id)) }
@@ -45,6 +48,12 @@ class GroupMember < Member
 
   private
 
+  def access_level_inclusion
+    return if access_level.in?(Gitlab::Access.all_values)
+
+    errors.add(:access_level, "is not included in the list")
+  end
+
   def send_invite
     run_after_commit_or_now { notification_service.invite_group_member(self, @raw_invite_token) }
 

app/models/user.rb

@@ -120,7 +120,7 @@ class User < ApplicationRecord
 
   # Groups
   has_many :members
-  has_many :group_members, -> { where(requested_at: nil) }, source: 'GroupMember'
+  has_many :group_members, -> { where(requested_at: nil).where("access_level >= ?", Gitlab::Access::GUEST) }, source: 'GroupMember'
   has_many :groups, through: :group_members
   has_many :owned_groups, -> { where(members: { access_level: Gitlab::Access::OWNER }) }, through: :group_members, source: :group
   has_many :maintainers_groups, -> { where(members: { access_level: Gitlab::Access::MAINTAINER }) }, through: :group_members, source: :group

app/views/admin/groups/_group.html.haml

@@ -27,7 +27,7 @@
 
     %span.gl-ml-5
       = sprite_icon('users', css_class: 'gl-vertical-align-text-bottom')
-      = number_with_delimiter(group.users.count)
+      = number_with_delimiter(group.users_count)
 
     %span.gl-ml-5.visibility-icon.has-tooltip{ data: { container: 'body', placement: 'left' }, title: visibility_icon_description(group) }
       = visibility_level_icon(group.visibility_level)

ee/app/models/ee/group.rb

@@ -401,6 +401,23 @@ module EE
       root_ancestor.saml_provider&.prohibited_outer_forks?
     end
 
+    def minimal_access_role_allowed?
+      feature_available?(:minimal_access_role) && !has_parent?
+    end
+
+    override :member?
+    def member?(user, min_access_level = minimal_member_access_level)
+      if min_access_level == ::Gitlab::Access::MINIMAL_ACCESS && minimal_access_role_allowed?
+        all_group_members.find_by(user_id: user.id).present?
+      else
+        super
+      end
+    end
+
+    def minimal_member_access_level
+      minimal_access_role_allowed? ? ::Gitlab::Access::MINIMAL_ACCESS : ::Gitlab::Access::GUEST
+    end
+
     private
 
     def custom_project_templates_group_allowed

ee/app/models/license.rb

@@ -108,6 +108,7 @@ class License < ApplicationRecord
     smartcard_auth
     group_timelogs
     type_of_work_analytics
+    minimal_access_role
     unprotection_restrictions
     ci_project_subscriptions
   ]

ee/lib/ee/gitlab/access.rb

@@ -15,6 +15,12 @@ module EE
         def vulnerability_access_levels
           @vulnerability_access_levels ||= options_with_owner.except('Guest')
         end
+
+        def options_with_minimal_access
+          options_with_owner.merge(
+            "Minimal Access" => ::Gitlab::Access::MINIMAL_ACCESS
+          )
+        end
       end
     end
   end

ee/spec/models/group_spec.rb

@@ -661,13 +661,78 @@ RSpec.describe Group do
           stub_licensed_features(group_project_templates: false)
         end
 
-        it 'returns false unlicensed instance' do
+        it 'returns false for unlicensed instance' do
           is_expected.to be false
         end
       end
     end
   end
 
+  describe '#minimal_access_role_allowed?' do
+    subject { group.minimal_access_role_allowed? }
+
+    context 'licensed' do
+      before do
+        stub_licensed_features(minimal_access_role: true)
+      end
+
+      it 'returns true for licensed instance' do
+        is_expected.to be true
+      end
+
+      it 'returns false for subgroup in licensed instance' do
+        expect(create(:group, parent: group).minimal_access_role_allowed?).to be false
+      end
+    end
+
+    context 'unlicensed' do
+      before do
+        stub_licensed_features(minimal_access_role: false)
+      end
+
+      it 'returns false unlicensed instance' do
+        is_expected.to be false
+      end
+    end
+  end
+
+  describe '#member?' do
+    subject { group.member?(user) }
+
+    let(:group) { create(:group) }
+    let(:user) { create(:user) }
+
+    context 'with `minimal_access_role` not licensed' do
+      before do
+        stub_licensed_features(minimal_access_role: false)
+        create(:group_member, :minimal_access, user: user, group: group)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'with `minimal_access_role` licensed' do
+      before do
+        stub_licensed_features(minimal_access_role: true)
+        create(:group_member, :minimal_access, user: user, source: group)
+      end
+
+      context 'when group is a subgroup' do
+        let(:group) { create(:group, parent: create(:group)) }
+
+        it { is_expected.to be_falsey }
+      end
+
+      context 'when group is a top-level group' do
+        it { is_expected.to be_truthy }
+
+        it 'accepts higher level as argument' do
+          expect(group.member?(user, ::Gitlab::Access::DEVELOPER)).to be_falsey
+        end
+      end
+    end
+  end
+
   describe '#saml_discovery_token' do
     it 'returns existing tokens' do
       group = create(:group, saml_discovery_token: 'existing')

lib/gitlab/access.rb

@@ -9,12 +9,13 @@ module Gitlab
   module Access
     AccessDeniedError = Class.new(StandardError)
 
-    NO_ACCESS  = 0
-    GUEST      = 10
-    REPORTER   = 20
-    DEVELOPER  = 30
-    MAINTAINER = 40
-    OWNER      = 50
+    NO_ACCESS      = 0
+    MINIMAL_ACCESS = 5
+    GUEST          = 10
+    REPORTER       = 20
+    DEVELOPER      = 30
+    MAINTAINER     = 40
+    OWNER          = 50
 
     # Branch protection settings
     PROTECTION_NONE          = 0

lib/gitlab/project_authorizations.rb

@@ -99,6 +99,7 @@ module Gitlab
         .and(members[:source_type].eq('Namespace'))
         .and(members[:requested_at].eq(nil))
         .and(members[:user_id].eq(user.id))
+        .and(members[:access_level].gt(Gitlab::Access::MINIMAL_ACCESS))
 
       Arel::Nodes::OuterJoin.new(members, Arel::Nodes::On.new(cond))
     end
@@ -119,6 +120,7 @@ module Gitlab
                     .and(members[:source_type].eq('Namespace'))
                     .and(members[:requested_at].eq(nil))
                     .and(members[:user_id].eq(user.id))
+                    .and(members[:access_level].gt(Gitlab::Access::MINIMAL_ACCESS))
       Arel::Nodes::InnerJoin.new(members, Arel::Nodes::On.new(cond))
     end
 

spec/factories/group_members.rb

@@ -28,5 +28,11 @@ FactoryBot.define do
     trait :blocked do
       after(:build) { |group_member, _| group_member.user.block! }
     end
+
+    trait :minimal_access do
+      to_create { |instance| instance.save!(validate: false) }
+
+      access_level { GroupMember::MINIMAL_ACCESS }
+    end
   end
 end

spec/finders/group_members_finder_spec.rb

@@ -16,6 +16,7 @@ RSpec.describe GroupMembersFinder, '#execute' do
     member1 = group.add_maintainer(user1)
     member2 = group.add_maintainer(user2)
     member3 = group.add_maintainer(user3)
+    create(:group_member, :minimal_access, user: create(:user), source: group)
 
     result = described_class.new(group).execute
 

spec/finders/groups_finder_spec.rb

@@ -150,6 +150,14 @@ RSpec.describe GroupsFinder do
               end
             end
           end
+
+          context 'being minimal access member of parent group' do
+            it 'do not return group with minimal_access access' do
+              create(:group_member, :minimal_access, user: user, source: parent_group)
+
+              is_expected.to contain_exactly(public_subgroup, internal_subgroup)
+            end
+          end
         end
       end
     end

spec/finders/members_finder_spec.rb

@@ -45,6 +45,18 @@ RSpec.describe MembersFinder, '#execute' do
     expect(result).to contain_exactly(member1)
   end
 
+  it 'does not return members of parent group with minimal access' do
+    nested_group.request_access(user1)
+    member1 = group.add_maintainer(user2)
+    member2 = nested_group.add_maintainer(user3)
+    member3 = project.add_maintainer(user4)
+    create(:group_member, :minimal_access, user: create(:user), source: group)
+
+    result = described_class.new(project, user2).execute
+
+    expect(result).to contain_exactly(member1, member2, member3)
+  end
+
   it 'includes only non-invite members if user do not have amdin permissions on project' do
     create(:project_member, :invited, project: project, invite_email: create(:user).email)
     member1 = project.add_maintainer(user1)

spec/lib/gitlab/project_authorizations_spec.rb

@@ -115,6 +115,66 @@ RSpec.describe Gitlab::ProjectAuthorizations do
     end
   end
 
+  context 'user with minimal access to group' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:user) { create(:user) }
+
+    subject(:mapping) { map_access_levels(authorizations) }
+
+    context 'group membership' do
+      let!(:group_project) { create(:project, namespace: group) }
+
+      before do
+        create(:group_member, :minimal_access, user: user, source: group)
+      end
+
+      it 'does not create authorization' do
+        expect(mapping[group_project.id]).to be_nil
+      end
+    end
+
+    context 'inherited group membership' do
+      let!(:sub_group) { create(:group, parent: group) }
+      let!(:sub_group_project) { create(:project, namespace: sub_group) }
+
+      before do
+        create(:group_member, :minimal_access, user: user, source: group)
+      end
+
+      it 'does not create authorization' do
+        expect(mapping[sub_group_project.id]).to be_nil
+      end
+    end
+
+    context 'shared group' do
+      let!(:shared_group) { create(:group) }
+      let!(:shared_group_project) { create(:project, namespace: shared_group) }
+
+      before do
+        create(:group_group_link, shared_group: shared_group, shared_with_group: group)
+        create(:group_member, :minimal_access, user: user, source: group)
+      end
+
+      it 'does not create authorization' do
+        expect(mapping[shared_group_project.id]).to be_nil
+      end
+    end
+
+    context 'shared project' do
+      let!(:another_group) { create(:group) }
+      let!(:shared_project) { create(:project, namespace: another_group) }
+
+      before do
+        create(:project_group_link, group: group, project: shared_project)
+        create(:group_member, :minimal_access, user: user, source: group)
+      end
+
+      it 'does not create authorization' do
+        expect(mapping[shared_project.id]).to be_nil
+      end
+    end
+  end
+
   context 'with nested groups' do
     let(:group) { create(:group) }
     let!(:nested_group) { create(:group, parent: group) }

spec/models/group_spec.rb

@@ -692,6 +692,7 @@ RSpec.describe Group do
     before do
       create(:group_member, user: user, group: group_parent, access_level: parent_group_access_level)
       create(:group_member, user: user, group: group, access_level: group_access_level)
+      create(:group_member, :minimal_access, user: create(:user), source: group)
       create(:group_member, user: user, group: group_child, access_level: child_group_access_level)
     end
 

spec/models/member_spec.rb

@@ -16,7 +16,6 @@ RSpec.describe Member do
 
     it { is_expected.to validate_presence_of(:user) }
     it { is_expected.to validate_presence_of(:source) }
-    it { is_expected.to validate_inclusion_of(:access_level).in_array(Gitlab::Access.all_values) }
 
     it_behaves_like 'an object with email-formated attributes', :invite_email do
       subject { build(:project_member) }
@@ -150,6 +149,7 @@ RSpec.describe Member do
 
       accepted_request_user = create(:user).tap { |u| project.request_access(u) }
       @accepted_request_member = project.requesters.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request }
+      @member_with_minimal_access = create(:group_member, :minimal_access, source: group)
     end
 
     describe '.access_for_user_ids' do
@@ -180,6 +180,15 @@ RSpec.describe Member do
       it { expect(described_class.non_invite).to include @accepted_request_member }
     end
 
+    describe '.non_minimal_access' do
+      it { expect(described_class.non_minimal_access).to include @maintainer }
+      it { expect(described_class.non_minimal_access).to include @invited_member }
+      it { expect(described_class.non_minimal_access).to include @accepted_invite_member }
+      it { expect(described_class.non_minimal_access).to include @requested_member }
+      it { expect(described_class.non_minimal_access).to include @accepted_request_member }
+      it { expect(described_class.non_minimal_access).not_to include @member_with_minimal_access }
+    end
+
     describe '.request' do
       it { expect(described_class.request).not_to include @maintainer }
       it { expect(described_class.request).not_to include @invited_member }
@@ -257,6 +266,34 @@ RSpec.describe Member do
       it { is_expected.not_to include @blocked_maintainer }
       it { is_expected.not_to include @blocked_developer }
     end
+
+    describe '.active' do
+      subject { described_class.active.to_a }
+
+      it { is_expected.to include @owner }
+      it { is_expected.to include @maintainer }
+      it { is_expected.to include @invited_member }
+      it { is_expected.to include @accepted_invite_member }
+      it { is_expected.not_to include @requested_member }
+      it { is_expected.to include @accepted_request_member }
+      it { is_expected.not_to include @blocked_maintainer }
+      it { is_expected.not_to include @blocked_developer }
+      it { is_expected.not_to include @member_with_minimal_access }
+    end
+
+    describe '.active_without_invites_and_requests' do
+      subject { described_class.active_without_invites_and_requests.to_a }
+
+      it { is_expected.to include @owner }
+      it { is_expected.to include @maintainer }
+      it { is_expected.not_to include @invited_member }
+      it { is_expected.to include @accepted_invite_member }
+      it { is_expected.not_to include @requested_member }
+      it { is_expected.to include @accepted_request_member }
+      it { is_expected.not_to include @blocked_maintainer }
+      it { is_expected.not_to include @blocked_developer }
+      it { is_expected.not_to include @member_with_minimal_access }
+    end
   end
 
   describe "Delegate methods" do

spec/services/authorized_project_update/project_create_service_spec.rb

@@ -81,6 +81,7 @@ RSpec.describe AuthorizedProjectUpdate::ProjectCreateService do
         before do
           create(:group_member, access_level: Gitlab::Access::REPORTER, group: group, user: group_user)
           create(:group_member, access_level: Gitlab::Access::MAINTAINER, group: shared_with_group, user: group_user)
+          create(:group_member, :minimal_access, source: shared_with_group, user: create(:user))
 
           create(:group_group_link, shared_group: group, shared_with_group: shared_with_group, group_access: Gitlab::Access::DEVELOPER)
 
@@ -97,6 +98,11 @@ RSpec.describe AuthorizedProjectUpdate::ProjectCreateService do
             access_level: Gitlab::Access::DEVELOPER)
           expect(project_authorization).to exist
         end
+
+        it 'does not create project authorization for user with minimal access' do
+          expect { service.execute }.to(
+            change { ProjectAuthorization.count }.from(0).to(1))
+        end
       end
     end
 
@@ -118,6 +124,17 @@ RSpec.describe AuthorizedProjectUpdate::ProjectCreateService do
       end
     end
 
+    context 'member with minimal access' do
+      before do
+        create(:group_member, :minimal_access, user: group_user, source: group)
+      end
+
+      it 'does not create project authorization' do
+        expect { service.execute }.not_to(
+          change { ProjectAuthorization.count }.from(0))
+      end
+    end
+
     context 'project has more user than BATCH_SIZE' do
       let(:batch_size) { 2 }
       let(:users) { create_list(:user, batch_size + 1 ) }

spec/services/authorized_project_update/project_group_link_create_service_spec.rb

@@ -112,6 +112,17 @@ RSpec.describe AuthorizedProjectUpdate::ProjectGroupLinkCreateService do
       end
     end
 
+    context 'minimal access member' do
+      before do
+        create(:group_member, :minimal_access, user: group_user, source: group)
+      end
+
+      it 'does not create project authorization' do
+        expect { service.execute }.not_to(
+          change { ProjectAuthorization.count }.from(0))
+      end
+    end
+
     context 'project has more users than BATCH_SIZE' do
       let(:batch_size) { 2 }
       let(:users) { create_list(:user, batch_size + 1 ) }