diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index c3f743ced5bb1968bf9448507fadb7a9d5ae8af0..f597124357f74ba453cc7248fc67fa603ec1a623 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -133,6 +133,10 @@ def plain_source_type
raise NotImplementedError
end
+ def source
+ raise NotImplementedError
+ end
+
def members_and_requesters
membershipable.members_and_requesters
end
@@ -142,7 +146,7 @@ def requesters
end
def update_params
- params.require(root_params_key).permit(:access_level, :expires_at)
+ params.require(root_params_key).permit(:access_level, :expires_at).merge({ source: source })
end
def requested_relations(inherited_permissions = :with_inherited_permissions)
diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb
index 2f8efedbc268c7a335b5e7cbc46af2e9de9113f1..47c1f07acd6b3531ec0e93866a4f7e11fc4d1149 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/app/controllers/groups/group_members_controller.rb
@@ -115,6 +115,10 @@ def source_type
_("group")
end
+ def source
+ group
+ end
+
def members_page_url
polymorphic_url([group, :members])
end
diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb
index 168ae402898de1507fa2ae3396a7167413d0f103..b0bfce96f6ce850924fb348d1688dfdd02d7d8d3 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/app/controllers/projects/project_members_controller.rb
@@ -58,6 +58,10 @@ def source_type
_("project")
end
+ def source
+ project
+ end
+
def members_page_url
project_project_members_path(project)
end
diff --git a/app/graphql/mutations/members/bulk_update_base.rb b/app/graphql/mutations/members/bulk_update_base.rb
index fe24e6b48a6035f30a4927bffa2404f638fa8715..0a54597f6c638525ee7e3a7f000699cec6f35ce7 100644
--- a/app/graphql/mutations/members/bulk_update_base.rb
+++ b/app/graphql/mutations/members/bulk_update_base.rb
@@ -26,8 +26,10 @@ class BulkUpdateBase < BaseMutation
INVALID_MEMBERS_ERROR = 'Only access level of direct members can be updated.'
def resolve(**args)
+ source = authorized_find!(source_id: args[source_id_param_name])
+
result = ::Members::UpdateService
- .new(current_user, args.except(:user_ids, source_id_param_name))
+ .new(current_user, args.except(:user_ids, source_id_param_name).merge({ source: source }))
.execute(@updatable_members)
present_result(result)
diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb
index e126f062be14dc55d6c2d2194c7a3b1461af5ccc..a13a96910846d6b4bf704c9549ddcc11d49d80de 100644
--- a/app/services/members/update_service.rb
+++ b/app/services/members/update_service.rb
@@ -2,9 +2,17 @@
module Members
class UpdateService < Members::BaseService
+ def initialize(*args)
+ super
+
+ @source = params[:source]
+ end
+
# @param members [Member, Array<Member>]
# returns the updated member(s)
def execute(members, permission: :update)
+ validate_source_type!
+
members = Array.wrap(members)
old_access_level_expiry_map = members.to_h do |member|
@@ -24,6 +32,12 @@ def execute(members, permission: :update)
private
+ attr_reader :source
+
+ def validate_source_type!
+ raise "Unknown source type: #{source.class}!" unless source.is_a?(Group) || source.is_a?(Project)
+ end
+
def update_members(members, permission)
# `filter_map` avoids the `post_update` call for the member that resulted in no change
Member.transaction do
diff --git a/ee/app/controllers/ee/groups/group_members_controller.rb b/ee/app/controllers/ee/groups/group_members_controller.rb
index 6b46743fc371426b9645a6bbf65463bd80d22ab3..93917a7fa0a46055feed95cd8a3b4f65e8c000a8 100644
--- a/ee/app/controllers/ee/groups/group_members_controller.rb
+++ b/ee/app/controllers/ee/groups/group_members_controller.rb
@@ -141,7 +141,7 @@ def authorize_ban_group_member!
end
def override_params
- params.require(:group_member).permit(:override)
+ params.require(:group_member).permit(:override).merge({ source: source })
end
override :membershipable_members
diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb
index d19c71139f59043f6b97d1a920e346e8bc87973b..a16d815227f2173691a0ab5426ee798567a8fcd4 100644
--- a/ee/lib/ee/api/members.rb
+++ b/ee/lib/ee/api/members.rb
@@ -17,10 +17,11 @@ module Members
requires :user_id, type: Integer, desc: 'The user ID of the member'
end
post ":id/members/:user_id/override", feature_category: :user_management do
+ group = find_group!(params[:id])
member = find_member(params)
result = ::Members::UpdateService
- .new(current_user, { override: true })
+ .new(current_user, { override: true, source: group })
.execute(member, permission: :override)
updated_member = result[:members].first
@@ -39,10 +40,11 @@ module Members
requires :user_id, type: Integer, desc: 'The user ID of the member'
end
delete ":id/members/:user_id/override", feature_category: :user_management do
+ group = find_group!(params[:id])
member = find_member(params)
result = ::Members::UpdateService
- .new(current_user, { override: false })
+ .new(current_user, { override: false, source: group })
.execute(member, permission: :override)
updated_member = result[:members].first
diff --git a/ee/spec/services/ee/members/update_service_spec.rb b/ee/spec/services/ee/members/update_service_spec.rb
index 8ca6c7723839664daa4c786cd131f0d177a4ce13..889a640338ccc8bfeec2ed84f50a8a5cfe3747d4 100644
--- a/ee/spec/services/ee/members/update_service_spec.rb
+++ b/ee/spec/services/ee/members/update_service_spec.rb
@@ -31,7 +31,8 @@
let(:current_user) { user }
let(:member) { group_member }
- let(:params) { { access_level: new_access_level, expires_at: new_expiration } }
+ let(:source) { group }
+ let(:params) { { access_level: new_access_level, expires_at: new_expiration, source: source } }
let(:service) { described_class.new(current_user, params) }
@@ -177,6 +178,7 @@
end
it_behaves_like 'does not log an audit event' do
+ let(:source) { project }
let(:member) { project_member }
end
end
@@ -211,7 +213,7 @@
let_it_be(:member_role_guest) { create(:member_role, :guest, namespace: group) }
let_it_be(:member_role_reporter) { create(:member_role, :reporter, namespace: group) }
- let(:params) { { member_role_id: target_member_role&.id } }
+ let(:params) { { member_role_id: target_member_role&.id, source: source } }
before do
stub_licensed_features(custom_roles: true)
@@ -277,7 +279,9 @@
end
context 'when invalid access_level is provided' do
- let(:params) { { member_role_id: target_member_role&.id, access_level: GroupMember::DEVELOPER } }
+ let(:params) do
+ { member_role_id: target_member_role&.id, access_level: GroupMember::DEVELOPER, source: source }
+ end
it 'returns error' do
expect(update_member[:status]).to eq(:error)
@@ -307,7 +311,7 @@
create(:member_role, namespace: root_ancestor, admin_group_member: true)
end
- let(:params) { { access_level: role } }
+ let(:params) { { access_level: role, source: source } }
shared_examples 'updating members using custom permission' do
let_it_be(:member, reload: true) do
diff --git a/lib/api/invitations.rb b/lib/api/invitations.rb
index c10c7ff59e5ea5ca55abea3f6c853775cb15acad..10eedacf5516655dda81dd7fb434ae48ea58eb75 100644
--- a/lib/api/invitations.rb
+++ b/lib/api/invitations.rb
@@ -101,7 +101,7 @@ class Invitations < ::API::Base
bad_request! unless update_params.any?
result = ::Members::UpdateService
- .new(current_user, update_params)
+ .new(current_user, update_params.merge({ source: source }))
.execute(invite)
updated_member = result[:members].first
diff --git a/lib/api/members.rb b/lib/api/members.rb
index a72b8958038818aa0d9254c79651f26985d57ddc..e56c8f548230be706019f8fdfdf083a115aa0dd4 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -149,7 +149,7 @@ class Members < ::API::Base
authorize_update_source_member!(source_type, member)
result = ::Members::UpdateService
- .new(current_user, declared_params(include_missing: false))
+ .new(current_user, declared_params(include_missing: false).merge({ source: source }))
.execute(member)
present_put_membership_response(result)
diff --git a/spec/controllers/concerns/membership_actions_spec.rb b/spec/controllers/concerns/membership_actions_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..3abdb7500abd66b319850239e4c299336679e877
--- /dev/null
+++ b/spec/controllers/concerns/membership_actions_spec.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MembershipActions, feature_category: :user_management do
+ let(:controller) do
+ klass = Class.new do
+ def self.before_action(action = nil, params = nil); end
+
+ include MembershipActions
+
+ def get_source
+ source
+ end
+ end
+
+ klass.new
+ end
+
+ describe '#source' do
+ it 'raises an error if not implemented' do
+ expect { controller.get_source }.to raise_error(NotImplementedError)
+ end
+ end
+end
diff --git a/spec/requests/api/invitations_spec.rb b/spec/requests/api/invitations_spec.rb
index d75e6da0fcf3318db6c9d4c7a3e83f70b9a4d91c..5a86b6706bd9171b0ae9225aacb3c8cf55e1d75a 100644
--- a/spec/requests/api/invitations_spec.rb
+++ b/spec/requests/api/invitations_spec.rb
@@ -661,13 +661,15 @@ def invite_api(source, user, email)
end
end
- shared_examples 'PUT /:source_type/:id/invitations/:email' do |source_type|
+ describe 'PUT /groups/:id/invitations' do
+ let(:source) { group }
+
def update_api(source, user, email)
api("/#{source.model_name.plural}/#{source.id}/invitations/#{email}", user)
end
- context "with :source_type == #{source_type.pluralize}" do
- let!(:invite) { invite_member_by_email(source, source_type, developer.email, maintainer) }
+ context "with :source_type == 'groups'" do
+ let!(:invite) { invite_member_by_email(source, 'group', developer.email, maintainer) }
it_behaves_like 'a 404 response when source is private' do
let(:route) do
@@ -736,7 +738,7 @@ def update_api(source, user, email)
end
context 'updating access expiry date' do
- subject do
+ subject(:put_request) do
put update_api(source, maintainer, invite.invite_email), params: { expires_at: expires_at }
end
@@ -744,7 +746,7 @@ def update_api(source, user, email)
let(:expires_at) { 2.days.ago.to_date }
it 'does not update the member' do
- subject
+ put_request
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['message']).to eq({ 'expires_at' => ['cannot be a date in the past'] })
@@ -755,7 +757,7 @@ def update_api(source, user, email)
let(:expires_at) { 2.days.from_now.to_date }
it 'updates the member' do
- subject
+ put_request
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['expires_at']).to eq(expires_at.to_s)
@@ -764,10 +766,4 @@ def update_api(source, user, email)
end
end
end
-
- describe 'PUT /groups/:id/invitations' do
- it_behaves_like 'PUT /:source_type/:id/invitations/:email', 'group' do
- let(:source) { group }
- end
- end
end
diff --git a/spec/requests/projects/merge_requests_discussions_spec.rb b/spec/requests/projects/merge_requests_discussions_spec.rb
index 24b6fb2f640d2a309edcbdeabaf0c067c9c01aa2..ac295b765df7b82bf8ab727821d8ab7ed851b584 100644
--- a/spec/requests/projects/merge_requests_discussions_spec.rb
+++ b/spec/requests/projects/merge_requests_discussions_spec.rb
@@ -236,7 +236,7 @@ def send_request
context 'when author role changes' do
before do
- Members::UpdateService.new(owner, access_level: Gitlab::Access::GUEST).execute(author_membership)
+ Members::UpdateService.new(owner, access_level: Gitlab::Access::GUEST, source: project).execute(author_membership)
end
it_behaves_like 'cache miss' do
@@ -246,7 +246,7 @@ def send_request
context 'when current_user role changes' do
before do
- Members::UpdateService.new(owner, access_level: Gitlab::Access::GUEST).execute(project.member(user))
+ Members::UpdateService.new(owner, access_level: Gitlab::Access::GUEST, source: project).execute(project.member(user))
end
it_behaves_like 'cache miss' do
diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb
index 0ebe6503a9d572c10ec74e0ab08ee9feea598ac4..1382095e84afc66c5fb6a6c5f616cbc861a6b0f2 100644
--- a/spec/services/members/update_service_spec.rb
+++ b/spec/services/members/update_service_spec.rb
@@ -13,7 +13,7 @@
let_it_be(:access_level) { Gitlab::Access::MAINTAINER }
let(:members) { source.members_and_requesters.where(user_id: member_users).to_a }
let(:update_service) { described_class.new(current_user, params) }
- let(:params) { { access_level: access_level } }
+ let(:params) { { access_level: access_level, source: source } }
let(:updated_members) { subject[:members] }
before do
@@ -47,7 +47,7 @@
end
shared_examples 'returns error status when params are invalid' do
- let_it_be(:params) { { expires_at: 2.days.ago } }
+ let_it_be(:params) { { expires_at: 2.days.ago, source: source } }
specify do
expect(subject[:status]).to eq(:error)
@@ -111,20 +111,20 @@
end
context 'with Gitlab::Access::GUEST level as a string' do
- let_it_be(:params) { { access_level: Gitlab::Access::GUEST.to_s } }
+ let_it_be(:params) { { access_level: Gitlab::Access::GUEST.to_s, source: source } }
it_behaves_like 'schedules to delete confidential todos'
end
context 'with Gitlab::Access::GUEST level as an integer' do
- let_it_be(:params) { { access_level: Gitlab::Access::GUEST } }
+ let_it_be(:params) { { access_level: Gitlab::Access::GUEST, source: source } }
it_behaves_like 'schedules to delete confidential todos'
end
end
context 'when access_level is invalid' do
- let_it_be(:params) { { access_level: 'invalid' } }
+ let_it_be(:params) { { access_level: 'invalid', source: source } }
it 'raises an error' do
expect { described_class.new(current_user, params) }
@@ -133,7 +133,7 @@
end
context 'when members update results in no change' do
- let(:params) { { access_level: members.first.access_level } }
+ let(:params) { { access_level: members.first.access_level, source: source } }
it 'does not invoke update! and post_update' do
expect(update_service).not_to receive(:save!)
@@ -221,7 +221,7 @@
end
context 'when project members expiration date is updated with expiry_notified_at' do
- let_it_be(:params) { { expires_at: 20.days.from_now } }
+ let_it_be(:params) { { expires_at: 20.days.from_now, source: source } }
before do
group_project.group.add_owner(current_user)
@@ -254,7 +254,7 @@
end
context 'when group members expiration date is updated' do
- let_it_be(:params) { { expires_at: 20.days.from_now } }
+ let_it_be(:params) { { expires_at: 20.days.from_now, source: source } }
let(:notification_service) { instance_double(NotificationService) }
before do
@@ -271,7 +271,7 @@
end
context 'when group members expiration date is updated with expiry_notified_at' do
- let_it_be(:params) { { expires_at: 20.days.from_now } }
+ let_it_be(:params) { { expires_at: 20.days.from_now, source: source } }
before do
members.each do |member|
@@ -321,11 +321,20 @@
end
end
+ context 'when passing an invalid source' do
+ let_it_be(:source) { Object.new }
+
+ it 'raises a RuntimeError' do
+ expect { update_service.execute([]) }.to raise_error(RuntimeError, 'Unknown source type: Object!')
+ end
+ end
+
it_behaves_like 'current user cannot update the given members'
it_behaves_like 'updating a project'
it_behaves_like 'updating a group'
context 'with a single member' do
+ let_it_be(:source) { group }
let(:members) { create(:group_member, group: group) }
before do