Merge branch 'rc-new-members-request-access-service' into 'master'

New `Members::RequestAccessService`

Part of #21979.

See merge request !6265

9 files changed, 107 insertions, 14 deletions

diff --git a/app/models/concerns/access_requestable.rb b/app/models/concerns/access_requestable.rb
index eedd32a729f..62bc6b809f4 100644
--- a/app/models/concerns/access_requestable.rb
+++ b/app/models/concerns/access_requestable.rb
@@ -8,9 +8,6 @@ module AccessRequestable
   extend ActiveSupport::Concern
 
   def request_access(user)
-    members.create(
-      access_level: Gitlab::Access::DEVELOPER,
-      user: user,
-      requested_at: Time.now.utc)
+    Members::RequestAccessService.new(self, user).execute
   end
 end
diff --git a/app/services/members/request_access_service.rb b/app/services/members/request_access_service.rb
new file mode 100644
index 00000000000..2614153d900
--- /dev/null
+++ b/app/services/members/request_access_service.rb
@@ -0,0 +1,25 @@
+module Members
+  class RequestAccessService < BaseService
+    attr_accessor :source
+
+    def initialize(source, current_user)
+      @source = source
+      @current_user = current_user
+    end
+
+    def execute
+      raise Gitlab::Access::AccessDeniedError unless can_request_access?(source)
+
+      source.members.create(
+        access_level: Gitlab::Access::DEVELOPER,
+        user: current_user,
+        requested_at: Time.now.utc)
+    end
+
+    private
+
+    def can_request_access?(source)
+      source && can?(current_user, :request_access, source)
+    end
+  end
+end
diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb
index 7998209b7b0..6703d88e357 100644
--- a/spec/helpers/members_helper_spec.rb
+++ b/spec/helpers/members_helper_spec.rb
@@ -11,7 +11,7 @@ describe MembersHelper do
 
   describe '#remove_member_message' do
     let(:requester) { build(:user) }
-    let(:project) { create(:project) }
+    let(:project) { create(:empty_project, :public) }
     let(:project_member) { build(:project_member, project: project) }
     let(:project_member_invite) { build(:project_member, project: project).tap { |m| m.generate_invite_token! } }
     let(:project_member_request) { project.request_access(requester) }
@@ -32,7 +32,7 @@ describe MembersHelper do
 
   describe '#remove_member_title' do
     let(:requester) { build(:user) }
-    let(:project) { create(:project) }
+    let(:project) { create(:empty_project, :public) }
     let(:project_member) { build(:project_member, project: project) }
     let(:project_member_request) { project.request_access(requester) }
     let(:group) { create(:group) }
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 0363bc74939..2e558018d74 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -402,7 +402,7 @@ describe Notify do
 
     describe 'project access requested' do
       context 'for a project in a user namespace' do
-        let(:project) { create(:project).tap { |p| p.team << [p.owner, :master, p.owner] } }
+        let(:project) { create(:project, :public).tap { |p| p.team << [p.owner, :master, p.owner] } }
         let(:user) { create(:user) }
         let(:project_member) do
           project.request_access(user)
@@ -429,7 +429,7 @@ describe Notify do
       context 'for a project in a group' do
         let(:group_owner) { create(:user) }
         let(:group) { create(:group).tap { |g| g.add_owner(group_owner) } }
-        let(:project) { create(:project, namespace: group) }
+        let(:project) { create(:project, :public, namespace: group) }
         let(:user) { create(:user) }
         let(:project_member) do
           project.request_access(user)
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index 0b1634f654a..c2f4601790d 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -57,7 +57,7 @@ describe Member, models: true do
 
   describe 'Scopes & finders' do
     before do
-      project = create(:empty_project)
+      project = create(:empty_project, :public)
       group = create(:group)
       @owner_user = create(:user).tap { |u| group.add_owner(u) }
       @owner = group.members.find_by(user_id: @owner_user.id)
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 83f61f0af0a..98f5305a855 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -68,7 +68,7 @@ describe Project, models: true do
     it { is_expected.to have_many(:forks).through(:forked_project_links) }
 
     describe '#members & #requesters' do
-      let(:project) { create(:project) }
+      let(:project) { create(:project, :public) }
       let(:requester) { create(:user) }
       let(:developer) { create(:user) }
       before do
diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb
index f979d66c88c..e0f2dadf189 100644
--- a/spec/models/project_team_spec.rb
+++ b/spec/models/project_team_spec.rb
@@ -137,7 +137,7 @@ describe ProjectTeam, models: true do
 
   describe '#find_member' do
     context 'personal project' do
-      let(:project) { create(:empty_project) }
+      let(:project) { create(:empty_project, :public) }
       let(:requester) { create(:user) }
 
       before do
@@ -200,7 +200,7 @@ describe ProjectTeam, models: true do
     let(:requester) { create(:user) }
 
     context 'personal project' do
-      let(:project) { create(:empty_project) }
+      let(:project) { create(:empty_project, :public) }
 
       context 'when project is not shared with group' do
         before do
diff --git a/spec/requests/api/access_requests_spec.rb b/spec/requests/api/access_requests_spec.rb
index d78494b76fa..905a7311372 100644
--- a/spec/requests/api/access_requests_spec.rb
+++ b/spec/requests/api/access_requests_spec.rb
@@ -64,12 +64,12 @@ describe API::AccessRequests, api: true  do
       context 'when authenticated as a member' do
         %i[developer master].each do |type|
           context "as a #{type}" do
-            it 'returns 400' do
+            it 'returns 403' do
               expect do
                 user = public_send(type)
                 post api("/#{source_type.pluralize}/#{source.id}/access_requests", user)
 
-                expect(response).to have_http_status(400)
+                expect(response).to have_http_status(403)
               end.not_to change { source.requesters.count }
             end
           end
@@ -87,6 +87,20 @@ describe API::AccessRequests, api: true  do
       end
 
       context 'when authenticated as a stranger' do
+        context "when access request is disabled for the #{source_type}" do
+          before do
+            source.update(request_access_enabled: false)
+          end
+
+          it 'returns 403' do
+            expect do
+              post api("/#{source_type.pluralize}/#{source.id}/access_requests", stranger)
+
+              expect(response).to have_http_status(403)
+            end.not_to change { source.requesters.count }
+          end
+        end
+
         it 'returns 201' do
           expect do
             post api("/#{source_type.pluralize}/#{source.id}/access_requests", stranger)
diff --git a/spec/services/members/request_access_service_spec.rb b/spec/services/members/request_access_service_spec.rb
new file mode 100644
index 00000000000..dff5b4917ae
--- /dev/null
+++ b/spec/services/members/request_access_service_spec.rb
@@ -0,0 +1,57 @@
+require 'spec_helper'
+
+describe Members::RequestAccessService, services: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :private) }
+  let(:group) { create(:group, :private) }
+
+  shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
+    it 'raises Gitlab::Access::AccessDeniedError' do
+      expect { described_class.new(source, user).execute }.to raise_error(Gitlab::Access::AccessDeniedError)
+    end
+  end
+
+  shared_examples 'a service creating a access request' do
+    it 'succeeds' do
+      expect { described_class.new(source, user).execute }.to change { source.requesters.count }.by(1)
+    end
+
+    it 'returns a <Source>Member' do
+      member = described_class.new(source, user).execute
+
+      expect(member).to be_a "#{source.class.to_s}Member".constantize
+      expect(member.requested_at).to be_present
+    end
+  end
+
+  context 'when source is nil' do
+    it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+      let(:source) { nil }
+    end
+  end
+
+  context 'when current user cannot request access to the project' do
+    it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+      let(:source) { project }
+    end
+
+    it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+      let(:source) { group }
+    end
+  end
+
+  context 'when current user can request access to the project' do
+    before do
+      project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+      group.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+    end
+
+    it_behaves_like 'a service creating a access request' do
+      let(:source) { project }
+    end
+
+    it_behaves_like 'a service creating a access request' do
+      let(:source) { group }
+    end
+  end
+end