From 61e2b88dd11431ea865455ec8ced3d4f6735a67d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 28 Oct 2016 11:03:08 +0200 Subject: Allow Members::ApproveAccessRequestService to accept a new :force param MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This param allows to bypass permission check. It is useful for LDAP-sync where even owners don't have the :admin_group_member permission. See https://gitlab.com/gitlab-org/gitlab-ee/blob/6081c37123abae4570f78831b33c2f45f92c2765/app/policies/group_policy.rb#L38 and https://gitlab.com/gitlab-org/gitlab-ee/issues/1159 Signed-off-by: Rémy Coutable --- .../members/approve_access_request_service.rb | 21 ++++++-- .../members/approve_access_request_service_spec.rb | 63 +++++++++++++++++++--- 2 files changed, 73 insertions(+), 11 deletions(-) diff --git a/app/services/members/approve_access_request_service.rb b/app/services/members/approve_access_request_service.rb index 416aee2ab51..c13f289f61e 100644 --- a/app/services/members/approve_access_request_service.rb +++ b/app/services/members/approve_access_request_service.rb @@ -4,17 +4,25 @@ module Members attr_accessor :source + # source - The source object that respond to `#requesters` (i.g. project or group) + # current_user - The user that performs the access request approval + # params - A hash of parameters + # :user_id - User ID used to retrieve the access requester + # :id - Member ID used to retrieve the access requester + # :access_level - Optional access level set when the request is accepted def initialize(source, current_user, params = {}) @source = source @current_user = current_user - @params = params + @params = params.slice(:user_id, :id, :access_level) end - def execute + # opts - A hash of options + # :force - Bypass permission check: current_user can be nil in that case + def execute(opts = {}) condition = params[:user_id] ? { user_id: params[:user_id] } : { id: params[:id] } access_requester = source.requesters.find_by!(condition) - raise Gitlab::Access::AccessDeniedError unless can_update_access_requester?(access_requester) + raise Gitlab::Access::AccessDeniedError unless can_update_access_requester?(access_requester, opts) access_requester.access_level = params[:access_level] if params[:access_level] access_requester.accept_request @@ -24,8 +32,11 @@ module Members private - def can_update_access_requester?(access_requester) - access_requester && can?(current_user, action_member_permission(:update, access_requester), access_requester) + def can_update_access_requester?(access_requester, opts = {}) + access_requester && ( + opts[:force] || + can?(current_user, action_member_permission(:update, access_requester), access_requester) + ) end end end diff --git a/spec/services/members/approve_access_request_service_spec.rb b/spec/services/members/approve_access_request_service_spec.rb index 03e296259f9..7b090343a3e 100644 --- a/spec/services/members/approve_access_request_service_spec.rb +++ b/spec/services/members/approve_access_request_service_spec.rb @@ -5,36 +5,37 @@ describe Members::ApproveAccessRequestService, services: true do let(:access_requester) { create(:user) } let(:project) { create(:project, :public) } let(:group) { create(:group, :public) } + let(:opts) { {} } shared_examples 'a service raising ActiveRecord::RecordNotFound' do it 'raises ActiveRecord::RecordNotFound' do - expect { described_class.new(source, user, params).execute }.to raise_error(ActiveRecord::RecordNotFound) + expect { described_class.new(source, user, params).execute(opts) }.to raise_error(ActiveRecord::RecordNotFound) end end shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do it 'raises Gitlab::Access::AccessDeniedError' do - expect { described_class.new(source, user, params).execute }.to raise_error(Gitlab::Access::AccessDeniedError) + expect { described_class.new(source, user, params).execute(opts) }.to raise_error(Gitlab::Access::AccessDeniedError) end end shared_examples 'a service approving an access request' do it 'succeeds' do - expect { described_class.new(source, user, params).execute }.to change { source.requesters.count }.by(-1) + expect { described_class.new(source, user, params).execute(opts) }.to change { source.requesters.count }.by(-1) end it 'returns a Member' do - member = described_class.new(source, user, params).execute + member = described_class.new(source, user, params).execute(opts) expect(member).to be_a "#{source.class}Member".constantize expect(member.requested_at).to be_nil end context 'with a custom access level' do - let(:params) { { user_id: access_requester.id, access_level: Gitlab::Access::MASTER } } + let(:params2) { params.merge(user_id: access_requester.id, access_level: Gitlab::Access::MASTER) } it 'returns a ProjectMember with the custom access level' do - member = described_class.new(source, user, params).execute + member = described_class.new(source, user, params2).execute(opts) expect(member.access_level).to eq Gitlab::Access::MASTER end @@ -60,6 +61,56 @@ describe Members::ApproveAccessRequestService, services: true do end let(:params) { { user_id: access_requester.id } } + context 'when current user is nil' do + let(:user) { nil } + + context 'and :force option is not given' 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 'and :force option is false' do + let(:opts) { { force: false } } + + 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 'and :force option is true' do + let(:opts) { { force: true } } + + it_behaves_like 'a service approving an access request' do + let(:source) { project } + end + + it_behaves_like 'a service approving an access request' do + let(:source) { group } + end + end + + context 'and :force param is true' do + let(:params) { { user_id: access_requester.id, force: true } } + + 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 + end + context 'when current user cannot approve access request to the project' do it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do let(:source) { project } -- cgit v1.2.1