diff --git a/app/services/groups/update_roles_service.rb b/app/services/groups/update_roles_service.rb index 70509dded8..dc9ddcb933 100644 --- a/app/services/groups/update_roles_service.rb +++ b/app/services/groups/update_roles_service.rb @@ -78,15 +78,20 @@ module Groups WHERE member_roles.member_id = :member_id ), -- delete all roles assigned to users that group no longer has but keep those that the user - -- has independently of the group (not inherited) + -- has independently of the group (not inherited) or inherited from a different group remove_roles AS ( - DELETE FROM #{MemberRole.table_name} + DELETE FROM #{MemberRole.table_name} delete_member_roles USING #{MemberRole.table_name} user_member_roles JOIN user_members ON user_members.id = user_member_roles.member_id - LEFT JOIN group_member_roles ON user_member_roles.role_id = group_member_roles.role_id - WHERE user_member_roles.inherited_from IS NOT NULL AND group_member_roles.role_id IS NULL - AND member_roles.id = user_member_roles.id - RETURNING #{MemberRole.table_name}.id, #{MemberRole.table_name}.member_id, #{MemberRole.table_name}.role_id + LEFT JOIN #{MemberRole.table_name} inheriting_member_roles + ON user_member_roles.role_id = inheriting_member_roles.role_id + AND user_member_roles.inherited_from = inheriting_member_roles.id + WHERE user_member_roles.inherited_from IS NOT NULL AND inheriting_member_roles.id IS NULL + AND delete_member_roles.id = user_member_roles.id + RETURNING + delete_member_roles.id, + delete_member_roles.member_id, + delete_member_roles.role_id ), -- add all roles to the user memberships add_roles AS ( diff --git a/db/migrate/20220426132637_refix_inherited_group_member_roles.rb b/db/migrate/20220426132637_refix_inherited_group_member_roles.rb new file mode 100644 index 0000000000..a87558f94d --- /dev/null +++ b/db/migrate/20220426132637_refix_inherited_group_member_roles.rb @@ -0,0 +1,14 @@ +class RefixInheritedGroupMemberRoles < ActiveRecord::Migration[6.1] + def up + # When the FixInheritedGroupMemberRoles ran initially, Members + # where the MemberRoles were inherited from more than one Group where + # applied incorrectly. Only the MemberRoles of the last Group where kept. + require Rails.root.join('db/migrate/20200625133727_fix_inherited_group_member_roles.rb') + + # created_on has been renamed to created_at + Member.reset_column_information + Principal.reset_column_information + + FixInheritedGroupMemberRoles.new.up + end +end diff --git a/spec/services/groups/update_roles_service_integration_spec.rb b/spec/services/groups/update_roles_service_integration_spec.rb index 6e1078fe6d..82366efbe6 100644 --- a/spec/services/groups/update_roles_service_integration_spec.rb +++ b/spec/services/groups/update_roles_service_integration_spec.rb @@ -360,6 +360,47 @@ describe Groups::UpdateRolesService, 'integration', type: :model do end end + context 'when adding a role and the user has a role already granted by a different group' do + let(:other_role) { create(:role) } + + let!(:second_group) do + create(:group, + members: users).tap do |group| + create(:member, + project: project, + principal: group, + roles: [other_role]) + + ::Groups::AddUsersService + .new(group, current_user: User.system, contract_class: EmptyContract) + .call(ids: users.map(&:id)) + end + end + + let(:users) { [create(:user)] } + let(:added_role) { create(:role) } + + before do + member.roles << added_role + end + + it 'is successful' do + expect(service_call) + .to be_success + end + + it 'keeps the roles the user already had before and adds the new one' do + service_call + + expect(Member.find_by(principal: users.first).roles.uniq) + .to match_array([role, other_role, added_role]) + end + + it_behaves_like 'sends notification' do + let(:user) { users } + end + end + context 'when not allowed' do let(:current_user) { User.anonymous }