Merge branch 'explicit-requesters-scope' into 'master'

Exclude requesters from Project#members, Group#members and User#members

## What does this MR do?

It excludes requesters from the `Project#members`, `Group#members` and `User#members` associations, and adds new `Project#requesters` and `Group#requesters` associations.

## Are there points in the code the reviewer needs to double check?

No.

## Why was this MR needed?

Without this, if you call `project.members`, requesters are included in the results! This is at best misleading, and at worst can lead to security issues. By excluding requesters from the `#members` associations, we avoid introducing security inadvertently since you have to call the `#requesters` association explicitly to get requesters.

## What are the relevant issue numbers?

This is something I realized while fixing the security issue #19102.

## Does this MR meet the acceptance criteria?

- [x] I don't think this needs a CHANGELOG since this is an internal change
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !4946

4 files changed, 63 insertions, 20 deletions

diff --git a/spec/models/concerns/access_requestable_spec.rb b/spec/models/concerns/access_requestable_spec.rb
index 98307876962..96eee0e8bdd 100644
--- a/spec/models/concerns/access_requestable_spec.rb
+++ b/spec/models/concerns/access_requestable_spec.rb
@@ -16,7 +16,7 @@ describe AccessRequestable do
 
       before { group.request_access(user) }
 
-      it { expect(group.members.request.exists?(user_id: user)).to be_truthy }
+      it { expect(group.requesters.exists?(user_id: user)).to be_truthy }
     end
   end
 
@@ -34,7 +34,7 @@ describe AccessRequestable do
 
       before { project.request_access(user) }
 
-      it { expect(project.members.request.exists?(user_id: user)).to be_truthy }
+      it { expect(project.requesters.exists?(user_id: user)).to be_truthy }
     end
   end
 end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 2c19aa3f67f..a878ff1b227 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -7,9 +7,38 @@ describe Group, models: true do
     it { is_expected.to have_many :projects }
     it { is_expected.to have_many(:group_members).dependent(:destroy) }
     it { is_expected.to have_many(:users).through(:group_members) }
+    it { is_expected.to have_many(:owners).through(:group_members) }
+    it { is_expected.to have_many(:requesters).dependent(:destroy) }
     it { is_expected.to have_many(:project_group_links).dependent(:destroy) }
     it { is_expected.to have_many(:shared_projects).through(:project_group_links) }
     it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
+
+    describe '#members & #requesters' do
+      let(:requester) { create(:user) }
+      let(:developer) { create(:user) }
+      before do
+        group.request_access(requester)
+        group.add_developer(developer)
+      end
+
+      describe '#members' do
+        it 'includes members and exclude requesters' do
+          member_user_ids = group.members.pluck(:user_id)
+
+          expect(member_user_ids).to include(developer.id)
+          expect(member_user_ids).not_to include(requester.id)
+        end
+      end
+
+      describe '#requesters' do
+        it 'does not include requesters' do
+          requester_user_ids = group.requesters.pluck(:user_id)
+
+          expect(requester_user_ids).to include(requester.id)
+          expect(requester_user_ids).not_to include(developer.id)
+        end
+      end
+    end
   end
 
   describe 'modules' do
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index e9134a3d283..40181a8b906 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -73,10 +73,10 @@ describe Member, models: true do
       @accepted_invite_member = project.members.invite.find_by_invite_email('toto2@example.com').tap { |u| u.accept_invite!(accepted_invite_user) }
 
       requested_user = create(:user).tap { |u| project.request_access(u) }
-      @requested_member = project.members.request.find_by(user_id: requested_user.id)
+      @requested_member = project.requesters.find_by(user_id: requested_user.id)
 
       accepted_request_user = create(:user).tap { |u| project.request_access(u) }
-      @accepted_request_member = project.members.request.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request }
+      @accepted_request_member = project.requesters.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request }
     end
 
     describe '.invite' do
@@ -103,22 +103,6 @@ describe Member, models: true do
       it { expect(described_class.request).not_to include @accepted_request_member }
     end
 
-    describe '.non_request' do
-      it { expect(described_class.non_request).to include @master }
-      it { expect(described_class.non_request).to include @invited_member }
-      it { expect(described_class.non_request).to include @accepted_invite_member }
-      it { expect(described_class.non_request).not_to include @requested_member }
-      it { expect(described_class.non_request).to include @accepted_request_member }
-    end
-
-    describe '.non_pending' do
-      it { expect(described_class.non_pending).to include @master }
-      it { expect(described_class.non_pending).not_to include @invited_member }
-      it { expect(described_class.non_pending).to include @accepted_invite_member }
-      it { expect(described_class.non_pending).not_to include @requested_member }
-      it { expect(described_class.non_pending).to include @accepted_request_member }
-    end
-
     describe '.owners_and_masters' do
       it { expect(described_class.owners_and_masters).to include @owner }
       it { expect(described_class.owners_and_masters).to include @master }
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 42308035d8c..a8c777d1e3e 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -11,6 +11,8 @@ describe Project, models: true do
     it { is_expected.to have_many(:issues).dependent(:destroy) }
     it { is_expected.to have_many(:milestones).dependent(:destroy) }
     it { is_expected.to have_many(:project_members).dependent(:destroy) }
+    it { is_expected.to have_many(:users).through(:project_members) }
+    it { is_expected.to have_many(:requesters).dependent(:destroy) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
     it { is_expected.to have_many(:snippets).class_name('ProjectSnippet').dependent(:destroy) }
     it { is_expected.to have_many(:deploy_keys_projects).dependent(:destroy) }
@@ -31,6 +33,34 @@ describe Project, models: true do
     it { is_expected.to have_many(:environments).dependent(:destroy) }
     it { is_expected.to have_many(:deployments).dependent(:destroy) }
     it { is_expected.to have_many(:todos).dependent(:destroy) }
+
+    describe '#members & #requesters' do
+      let(:project) { create(:project) }
+      let(:requester) { create(:user) }
+      let(:developer) { create(:user) }
+      before do
+        project.request_access(requester)
+        project.team << [developer, :developer]
+      end
+
+      describe '#members' do
+        it 'includes members and exclude requesters' do
+          member_user_ids = project.members.pluck(:user_id)
+
+          expect(member_user_ids).to include(developer.id)
+          expect(member_user_ids).not_to include(requester.id)
+        end
+      end
+
+      describe '#requesters' do
+        it 'does not include requesters' do
+          requester_user_ids = project.requesters.pluck(:user_id)
+
+          expect(requester_user_ids).to include(requester.id)
+          expect(requester_user_ids).not_to include(developer.id)
+        end
+      end
+    end
   end
 
   describe 'modules' do