Merge branch 'pending-group-member-access-part3' into 'master'

Handle Pending Memberships for Project Authorizations

See merge request gitlab-org/gitlab!79696

app/models/user.rb

@@ -149,6 +149,7 @@ class User < ApplicationRecord
   has_many :members
   has_many :group_members, -> { where(requested_at: nil).where("access_level >= ?", Gitlab::Access::GUEST) }, class_name: 'GroupMember'
   has_many :groups, through: :group_members
+  has_many :groups_with_active_memberships, -> { where(members: { state: ::Member::STATE_ACTIVE }) }, through: :group_members, source: :group
   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
   has_many :developer_groups, -> { where(members: { access_level: ::Gitlab::Access::DEVELOPER }) }, through: :group_members, source: :group
@@ -170,6 +171,7 @@ class User < ApplicationRecord
   has_many :project_members, -> { where(requested_at: nil) }
   has_many :projects,                 through: :project_members
   has_many :created_projects,         foreign_key: :creator_id, class_name: 'Project'
+  has_many :projects_with_active_memberships, -> { where(members: { state: ::Member::STATE_ACTIVE }) }, through: :project_members, source: :project
   has_many :users_star_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :starred_projects, through: :users_star_projects, source: :project
   has_many :project_authorizations, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent

ee/spec/factories/group_members.rb

-# frozen_string_literal: true
-
-FactoryBot.modify do
-  factory :group_member do
-    trait :awaiting do
-      after(:create) do |member|
-        member.wait
-      end
-    end
-
-    trait :active do
-      after(:create) do |member|
-        member.activate
-      end
-    end
-  end
-end

ee/spec/factories/project_members.rb

-# frozen_string_literal: true
-
-FactoryBot.modify do
-  factory :project_member do
-    trait :awaiting do
-      after(:create) do |member|
-        member.wait
-      end
-    end
-
-    trait :active do
-      after(:create) do |member|
-        member.activate
-      end
-    end
-  end
-end

ee/spec/features/pending_group_memberships_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Pending group memberships', :js do
+  let_it_be(:developer) { create(:user) }
+
+  before do
+    sign_in(developer)
+  end
+
+  context 'with a public group' do
+    let_it_be(:group) { create(:group, :public) }
+
+    it 'a pending group member gets a 404 for a private project in the group' do
+      project = create(:project, :private, namespace: group)
+      create(:group_member, :awaiting, :developer, source: group, user: developer)
+
+      visit project_path(project)
+
+      expect(page).to have_content "Page Not Found"
+    end
+  end
+
+  context 'with a subgroup' do
+    let_it_be(:group) { create(:group, :private) }
+    let_it_be(:subgroup) { create(:group, :private, parent: group) }
+
+    it 'a pending member of the root group sees a subgroup project as if not a member' do
+      project = create(:project, :private, namespace: subgroup)
+      create(:group_member, :awaiting, :developer, source: group, user: developer)
+
+      visit project_path(project)
+
+      expect(page).to have_content 'Page Not Found'
+    end
+
+    it 'a pending member of a subgroup sees a root group as if not a member' do
+      create(:group_member, :awaiting, :developer, source: subgroup, user: developer)
+
+      visit group_path(group)
+
+      expect(page).to have_content 'Page Not Found'
+    end
+
+    it 'a pending member of a subgroup sees a project as if not a member' do
+      project = create(:project, :private, namespace: subgroup)
+      create(:group_member, :awaiting, :developer, source: subgroup, user: developer)
+
+      visit project_path(project)
+
+      expect(page).to have_content 'Page Not Found'
+    end
+
+    it 'a member with an active group membership and a pending subgroup membership sees a subgroup project as if the pending membership does not exist' do
+      project = create(:project, :private, namespace: subgroup)
+      create(:group_member, :guest, source: group, user: developer)
+      create(:group_member, :awaiting, :maintainer, source: subgroup, user: developer)
+
+      visit project_path(project)
+
+      expect(page).to have_content project.name
+      expect(page).to have_content 'Project information'
+      expect(page).to have_content 'Issues'
+      expect(page).not_to have_content 'Settings'
+    end
+  end
+
+  context 'with a shared group' do
+    let_it_be(:group) { create(:group, :private) }
+    let_it_be(:other_group) { create(:group, :private) }
+
+    before_all do
+      create(:group_group_link, shared_group: other_group, shared_with_group: group)
+    end
+
+    it 'a pending member of the invited group sees a project in the shared group as if not a member' do
+      project = create(:project, namespace: other_group)
+      create(:group_member, :awaiting, :developer, source: group, user: developer)
+
+      visit project_path(project)
+
+      expect(page).to have_content 'Page Not Found'
+    end
+  end
+
+  context 'with a shared project' do
+    let_it_be(:group) { create(:group, :private) }
+    let_it_be(:other_group) { create(:group, :private) }
+    let_it_be(:project) { create(:project, :private, namespace: other_group) }
+
+    before_all do
+      create(:project_group_link, group: group, project: project)
+    end
+
+    it "a pending member of the invited group sees the shared project's group as if not a member" do
+      create(:group_member, :awaiting, :developer, source: group, user: developer)
+
+      visit group_path(other_group)
+
+      expect(page).to have_content 'Page Not Found'
+    end
+
+    it "a pending member of the invited group sees the shared project as if not a member" do
+      create(:group_member, :awaiting, :developer, source: group, user: developer)
+
+      visit project_path(project)
+
+      expect(page).to have_content 'Page Not Found'
+    end
+  end
+end

ee/spec/models/group_member_spec.rb

@@ -140,6 +140,36 @@ RSpec.describe GroupMember do
     end
   end
 
+  describe '#state' do
+    let!(:group) { create(:group) }
+    let!(:project) { create(:project, group: group) }
+    let!(:user) { create(:user) }
+
+    describe '#activate!' do
+      it "refreshes the user's authorized projects" do
+        membership = create(:group_member, :awaiting, source: group, user: user)
+
+        expect(user.authorized_projects).not_to include(project)
+
+        membership.activate!
+
+        expect(user.authorized_projects.reload).to include(project)
+      end
+    end
+
+    describe '#wait!' do
+      it "refreshes the user's authorized projects" do
+        membership = create(:group_member, source: group, user: user)
+
+        expect(user.authorized_projects).to include(project)
+
+        membership.wait!
+
+        expect(user.authorized_projects.reload).not_to include(project)
+      end
+    end
+  end
+
   context 'group member webhooks', :sidekiq_inline, :saas do
     let_it_be_with_refind(:group) { create(:group_with_plan, plan: :ultimate_plan) }
     let_it_be(:group_hook) { create(:group_hook, group: group, member_events: true) }

ee/spec/policies/project_policy_spec.rb

@@ -1961,4 +1961,58 @@ RSpec.describe ProjectPolicy do
       it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
     end
   end
+
+  describe 'pending member permissions' do
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:group) { create(:group, :public) }
+
+    context 'with a group invited to a project' do
+      let_it_be(:project) { create(:project, :private, public_builds: false) }
+
+      before_all do
+        create(:project_group_link, project: project, group: group)
+      end
+
+      it 'a pending developer in the group has permissions to the project as if the user is not a member' do
+        create(:group_member, :awaiting, :developer, source: group, user: current_user)
+
+        expect_private_project_permissions_as_if_non_member
+      end
+    end
+
+    context 'with a group invited to another group' do
+      let_it_be(:other_group) { create(:group, :public) }
+      let_it_be(:project) { create(:project, :private, public_builds: false, namespace: other_group) }
+
+      before_all do
+        create(:group_group_link, shared_with_group: group, shared_group: other_group)
+      end
+
+      it "a pending developer in the group has permissions to the other group's project as if the user is not a member" do
+        create(:group_member, :awaiting, :developer, source: group, user: current_user)
+
+        expect_private_project_permissions_as_if_non_member
+      end
+    end
+
+    context 'with a subgroup' do
+      let_it_be(:subgroup) { create(:group, :private, parent: group) }
+      let_it_be(:project) { create(:project, :private, public_builds: false, namespace: subgroup) }
+
+      it 'a pending developer in the group has permissions to the subgroup project as if the user is not a member' do
+        create(:group_member, :awaiting, :developer, source: group, user: current_user)
+
+        expect_private_project_permissions_as_if_non_member
+      end
+    end
+
+    def expect_private_project_permissions_as_if_non_member
+      expect_disallowed(*guest_permissions)
+      expect_disallowed(*reporter_permissions)
+      expect_disallowed(*team_member_reporter_permissions)
+      expect_disallowed(*developer_permissions)
+      expect_disallowed(*maintainer_permissions)
+      expect_disallowed(*owner_permissions)
+    end
+  end
 end

lib/gitlab/project_authorizations.rb

@@ -19,7 +19,7 @@ module Gitlab
 
       relations = [
         # The project a user has direct access to.
-        user.projects.select_for_project_authorization,
+        user.projects_with_active_memberships.select_for_project_authorization,
 
         # The personal projects of the user.
         user.personal_projects.select_as_maintainer_for_project_authorization,
@@ -65,7 +65,7 @@ module Gitlab
       group_group_links = GroupGroupLink.arel_table
 
       # Namespaces the user is a member of.
-      cte << user.groups
+      cte << user.groups_with_active_memberships
         .select([namespaces[:id], members[:access_level]])
         .except(:order)
 
@@ -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[:state].eq(::Member::STATE_ACTIVE))
         .and(members[:access_level].gt(Gitlab::Access::MINIMAL_ACCESS))
 
       Arel::Nodes::OuterJoin.new(members, Arel::Nodes::On.new(cond))
@@ -120,6 +121,7 @@ module Gitlab
                     .and(members[:source_type].eq('Namespace'))
                     .and(members[:requested_at].eq(nil))
                     .and(members[:user_id].eq(user.id))
+                    .and(members[:state].eq(::Member::STATE_ACTIVE))
                     .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

@@ -35,6 +35,18 @@ FactoryBot.define do
       access_level { GroupMember::MINIMAL_ACCESS }
     end
 
+    trait :awaiting do
+      after(:create) do |member|
+        member.update!(state: ::Member::STATE_AWAITING)
+      end
+    end
+
+    trait :active do
+      after(:create) do |member|
+        member.update!(state: ::Member::STATE_ACTIVE)
+      end
+    end
+
     transient do
       tasks_to_be_done { [] }
     end

spec/factories/project_members.rb

@@ -24,6 +24,18 @@ FactoryBot.define do
       after(:build) { |project_member, _| project_member.user.block! }
     end
 
+    trait :awaiting do
+      after(:create) do |member|
+        member.update!(state: ::Member::STATE_AWAITING)
+      end
+    end
+
+    trait :active do
+      after(:create) do |member|
+        member.update!(state: ::Member::STATE_ACTIVE)
+      end
+    end
+
     transient do
       tasks_to_be_done { [] }
     end

spec/lib/gitlab/project_authorizations_spec.rb

@@ -345,4 +345,76 @@ RSpec.describe Gitlab::ProjectAuthorizations do
       end
     end
   end
+
+  context 'with pending memberships' 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, :developer, :awaiting, user: user, group: 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, :developer, :awaiting, user: user, group: group)
+      end
+
+      it 'does not create authorization' do
+        expect(mapping[sub_group_project.id]).to be_nil
+      end
+    end
+
+    context 'project membership' do
+      let!(:group_project) { create(:project, namespace: group) }
+
+      before do
+        create(:project_member, :developer, :awaiting, user: user, project: group_project)
+      end
+
+      it 'does not create authorization' do
+        expect(mapping[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, :developer, :awaiting, user: user, group: 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, :developer, :awaiting, user: user, group: group)
+      end
+
+      it 'does not create authorization' do
+        expect(mapping[shared_project.id]).to be_nil
+      end
+    end
+  end
 end