New Members::RequestAccessService

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/controllers/concerns/membership_actions.rb

@@ -3,7 +3,7 @@ module MembershipActions
   include MembersHelper
 
   def request_access
-    membershipable.request_access(current_user)
+    Members::RequestAccessService.new(membershipable, current_user).execute
 
     redirect_to polymorphic_path(membershipable),
                 notice: 'Your request for access has been queued for review.'

app/models/concerns/access_requestable.rb

-# == AccessRequestable concern
-#
-# Contains functionality related to objects that can receive request for access.
-#
-# Used by Project, and Group.
-#
-module AccessRequestable
-  extend ActiveSupport::Concern
-
-  def request_access(user)
-    members.create(
-      access_level: Gitlab::Access::DEVELOPER,
-      user: user,
-      requested_at: Time.now.utc)
-  end
-end

app/models/group.rb

@@ -3,7 +3,6 @@ require 'carrierwave/orm/activerecord'
 class Group < Namespace
   include Gitlab::ConfigHelper
   include Gitlab::VisibilityLevel
-  include AccessRequestable
   include Referable
 
   has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember'
@@ -102,6 +101,10 @@ class Group < Namespace
     self[:lfs_enabled]
   end
 
+  def request_access(user)
+    Members::RequestAccessService.new(self, user).execute
+  end
+
   def add_users(user_ids, access_level, current_user: nil, expires_at: nil)
     user_ids.each do |user_id|
       Member.add_user(

app/models/project.rb

@@ -5,7 +5,6 @@ class Project < ActiveRecord::Base
   include Gitlab::ShellAdapter
   include Gitlab::VisibilityLevel
   include Gitlab::CurrentSettings
-  include AccessRequestable
   include Referable
   include Sortable
   include AfterCommitQueue
@@ -1012,6 +1011,10 @@ class Project < ActiveRecord::Base
       update_all(updated_at: Time.now)
   end
 
+  def request_access(user)
+    Members::RequestAccessService.new(self, user).execute
+  end
+
   def project_member(user)
     project_members.find_by(user_id: user)
   end

app/services/members/request_access_service.rb

+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 if cannot_request_access?(source)
+
+      source.members.create(
+        access_level: Gitlab::Access::DEVELOPER,
+        user: current_user,
+        requested_at: Time.now.utc)
+    end
+
+    private
+
+    def cannot_request_access?(source)
+      !source || !can?(current_user, :request_access, source)
+    end
+  end
+end

lib/api/access_requests.rb

@@ -33,7 +33,7 @@ module API
         #  POST /projects/:id/access_requests
         post ":id/access_requests" do
           source = find_source(source_type, params[:id])
-          access_requester = source.request_access(current_user)
+          access_requester = ::Members::RequestAccessService.new(source, current_user).execute
 
           if access_requester.persisted?
             present access_requester.user, with: Entities::AccessRequester, access_requester: access_requester

spec/models/concerns/access_requestable_spec.rb

-require 'spec_helper'
-
-describe AccessRequestable do
-  describe 'Group' do
-    describe '#request_access' do
-      let(:group) { create(:group, :public) }
-      let(:user) { create(:user) }
-
-      it { expect(group.request_access(user)).to be_a(GroupMember) }
-      it { expect(group.request_access(user).user).to eq(user) }
-    end
-
-    describe '#access_requested?' do
-      let(:group) { create(:group, :public) }
-      let(:user) { create(:user) }
-
-      before { group.request_access(user) }
-
-      it { expect(group.requesters.exists?(user_id: user)).to be_truthy }
-    end
-  end
-
-  describe 'Project' do
-    describe '#request_access' do
-      let(:project) { create(:empty_project, :public) }
-      let(:user) { create(:user) }
-
-      it { expect(project.request_access(user)).to be_a(ProjectMember) }
-    end
-
-    describe '#access_requested?' do
-      let(:project) { create(:empty_project, :public) }
-      let(:user) { create(:user) }
-
-      before { project.request_access(user) }
-
-      it { expect(project.requesters.exists?(user_id: user)).to be_truthy }
-    end
-  end
-end

spec/models/group_spec.rb

@@ -105,6 +105,13 @@ describe Group, models: true do
     it { expect(group.human_name).to eq(group.name) }
   end
 
+  describe '#request_access' do
+    let(:user) { create(:user) }
+
+    it { expect(group.request_access(user)).to be_a(GroupMember) }
+    it { expect(group.request_access(user).user).to eq(user) }
+  end
+
   describe '#add_user' do
     let(:user) { create(:user) }
     before { group.add_user(user, GroupMember::MASTER) }

spec/models/project_spec.rb

@@ -942,6 +942,14 @@ describe Project, models: true do
     end
   end
 
+  describe '#request_access' do
+    let(:project) { create(:empty_project, :public) }
+    let(:user) { create(:user) }
+
+    it { expect(project.request_access(user)).to be_a(ProjectMember) }
+    it { expect(project.request_access(user).user).to eq(user) }
+  end
+
   describe '.search' do
     let(:project) { create(:project, description: 'kitten mittens') }
 

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)

spec/services/members/request_access_service_spec.rb

+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