From fc41a443137d47aaff4ad941e08325632041cf42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 16 Sep 2016 17:54:21 +0200 Subject: [PATCH 1/8] Port of gitlab-org/gitlab-ce!6393 to EE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changes include: - Ensure Member.add_user is not called directly when not necessary - New GroupMember.add_users_to_group to have the same abstraction level as for Project - Refactor Member.add_user to take a source instead of an array of members - Fix Rubocop offenses - Always use Project#add_user instead of project.team.add_user - Factorize users addition as members in Member.add_users_to_source - Make access_level a keyword argument in GroupMember.add_users_to_group and ProjectMember.add_users_to_projects - Destroy any requester before adding them as a member - Improve the way we handle access requesters in Member.add_user Instead of removing the requester and creating a new member, we now simply accepts their access request. This way, they will receive a "access request granted" email. - Fix error that was previously silently ignored - Stop raising when access level is invalid in Member, let Rails validation do their work Signed-off-by: Rémy Coutable --- app/models/group.rb | 54 ++-- app/models/member.rb | 85 +++--- app/models/members/group_member.rb | 18 ++ app/models/members/project_member.rb | 44 ++-- app/models/project.rb | 5 +- app/models/project_team.rb | 14 +- db/fixtures/development/06_teams.rb | 7 +- lib/api/members.rb | 17 +- lib/gitlab/access.rb | 4 + .../projects/templates_controller_spec.rb | 2 +- spec/factories/project_members.rb | 23 +- .../owner_cannot_leave_project_spec.rb | 4 +- ...nnot_request_access_to_his_project_spec.rb | 4 +- spec/features/projects_spec.rb | 6 +- spec/finders/joined_groups_finder_spec.rb | 2 +- spec/finders/projects_finder_spec.rb | 2 +- .../gitlab/template/issue_template_spec.rb | 6 +- .../template/merge_request_template_spec.rb | 6 +- spec/mailers/notify_spec.rb | 50 ++-- spec/models/issue_spec.rb | 2 +- spec/models/member_spec.rb | 243 +++++++++++++++--- spec/models/members/group_member_spec.rb | 27 ++ spec/models/members/project_member_spec.rb | 50 ++-- spec/models/project_spec.rb | 2 +- spec/requests/api/merge_requests_spec.rb | 4 +- spec/services/projects/update_service_spec.rb | 2 +- 26 files changed, 449 insertions(+), 234 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 59718668a5296f..6f8e186ddcb073 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -117,42 +117,48 @@ def lfs_enabled? self[:lfs_enabled] end - def add_users(user_ids, access_level, current_user: nil, skip_notification: false, expires_at: nil, ldap: false) - user_ids.each do |user_id| - Member.add_user( - self.group_members, - user_id, - access_level, - current_user: current_user, - skip_notification: skip_notification, - expires_at: expires_at, - ldap: ldap - ) - end - end - - def add_user(user, access_level, current_user: nil, skip_notification: false, expires_at: nil) - add_users([user], access_level, current_user: current_user, skip_notification: skip_notification, expires_at: expires_at) - end - - def add_owner(user, current_user = nil, skip_notification: false) - add_user(user, Gitlab::Access::OWNER, current_user: current_user, skip_notification: skip_notification) + def add_users(users, access_level, current_user: nil, expires_at: nil, skip_notification: false, ldap: false) + GroupMember.add_users_to_group( + self, + users, + access_level, + current_user: current_user, + expires_at: expires_at, + skip_notification: skip_notification, + ldap: ldap + ) + end + + def add_user(user, access_level, current_user: nil, expires_at: nil, skip_notification: false, ldap: false) + GroupMember.add_user( + self, + user, + access_level, + current_user: current_user, + expires_at: expires_at, + skip_notification: skip_notification, + ldap: ldap + ) end def add_guest(user, current_user = nil) - add_user(user, Gitlab::Access::GUEST, current_user: current_user) + add_user(user, :guest, current_user: current_user) end def add_reporter(user, current_user = nil) - add_user(user, Gitlab::Access::REPORTER, current_user: current_user) + add_user(user, :reporter, current_user: current_user) end def add_developer(user, current_user = nil) - add_user(user, Gitlab::Access::DEVELOPER, current_user: current_user) + add_user(user, :developer, current_user: current_user) end def add_master(user, current_user = nil) - add_user(user, Gitlab::Access::MASTER, current_user: current_user) + add_user(user, :master, current_user: current_user) + end + + def add_owner(user, current_user = nil, skip_notification: false) + add_user(user, :owner, current_user: current_user, skip_notification: skip_notification) end def has_owner?(user) diff --git a/app/models/member.rb b/app/models/member.rb index d968394037a5e1..5ff9e5f1510814 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -81,51 +81,74 @@ def find_by_invite_token(invite_token) find_by(invite_token: invite_token) end - # This method is used to find users that have been entered into the "Add members" field. - # These can be the User objects directly, their IDs, their emails, or new emails to be invited. - def user_for_id(user_id) - return user_id if user_id.is_a?(User) - - user = User.find_by(id: user_id) - user ||= User.find_by(email: user_id) - user ||= user_id - user - end - - def add_user(members, user_id, access_level, current_user: nil, skip_notification: false, expires_at: nil, ldap: false) - user = user_for_id(user_id) + def add_user(source, user, access_level, current_user: nil, expires_at: nil, skip_notification: false, ldap: false) + user = retrieve_user(user) + access_level = retrieve_access_level(access_level) # `user` can be either a User object or an email to be invited - if user.is_a?(User) - member = members.find_or_initialize_by(user_id: user.id) + member = + if user.is_a?(User) + source.members.find_by(user_id: user.id) || + source.requesters.find_by(user_id: user.id) || + source.members.build(user_id: user.id) + else + source.members.build(invite_email: user) + end + + return member unless can_update_member?(current_user, member) + + member.attributes = { + created_by: member.created_by || current_user, + access_level: access_level, + expires_at: expires_at, + skip_notification: skip_notification, + ldap: ldap + } + + if member.request? + ::Members::ApproveAccessRequestService.new(source, current_user, id: member.id).execute else - member = members.build - member.invite_email = user + member.save end - if can_update_member?(current_user, member) || project_creator?(member, access_level) - member.created_by ||= current_user - member.access_level = access_level - member.expires_at = expires_at - member.skip_notification = skip_notification - member.ldap = ldap + member + end - member.save - end + def access_levels + Gitlab::Access.sym_options end private + # This method is used to find users that have been entered into the "Add members" field. + # These can be the User objects directly, their IDs, their emails, or new emails to be invited. + def retrieve_user(user) + return user if user.is_a?(User) + + User.find_by(id: user) || User.find_by(email: user) || user + end + + def retrieve_access_level(access_level) + access_levels.fetch(access_level) { access_level.to_i } + end + def can_update_member?(current_user, member) # There is no current user for bulk actions, in which case anything is allowed - !current_user || - current_user.can?(:update_group_member, member) || - current_user.can?(:update_project_member, member) + !current_user || current_user.can?(:"update_#{member.type.underscore}", member) end - def project_creator?(member, access_level) - member.new_record? && member.owner? && - access_level.to_i == ProjectMember::MASTER + def add_users_to_source(source, users, access_level, current_user: nil, expires_at: nil, skip_notification: false, ldap: false) + users.each do |user| + add_user( + source, + user, + access_level, + current_user: current_user, + expires_at: expires_at, + skip_notification: skip_notification, + ldap: ldap + ) + end end end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 57d7f3d8bab92c..6953673427c9b8 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -17,6 +17,24 @@ def self.access_level_roles Gitlab::Access.options_with_owner end + def self.access_levels + Gitlab::Access.sym_options_with_owner + end + + def self.add_users_to_group(group, users, access_level, current_user: nil, expires_at: nil, skip_notification: false, ldap: false) + self.transaction do + add_users_to_source( + group, + users, + access_level, + current_user: current_user, + expires_at: expires_at, + skip_notification: skip_notification, + ldap: ldap + ) + end + end + def group source end diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb index 3dfc01f6b58c2a..610bbd824fcd61 100644 --- a/app/models/members/project_member.rb +++ b/app/models/members/project_member.rb @@ -35,36 +35,20 @@ class << self # :master # ) # - def add_users_to_projects(project_ids, user_ids, access, current_user: nil, expires_at: nil) - access_level = if roles_hash.has_key?(access) - roles_hash[access] - elsif roles_hash.values.include?(access.to_i) - access - else - raise "Non valid access" - end - - users = user_ids.map { |user_id| Member.user_for_id(user_id) } - - ProjectMember.transaction do + def add_users_to_projects(project_ids, users, access_level, current_user: nil, expires_at: nil) + self.transaction do project_ids.each do |project_id| project = Project.find(project_id) - users.each do |user| - Member.add_user( - project.project_members, - user, - access_level, - current_user: current_user, - expires_at: expires_at - ) - end + add_users_to_source( + project, + users, + access_level, + current_user: current_user, + expires_at: expires_at + ) end end - - true - rescue - false end def truncate_teams(project_ids) @@ -85,13 +69,15 @@ def truncate_team(project) truncate_teams [project.id] end - def roles_hash - Gitlab::Access.sym_options - end - def access_level_roles Gitlab::Access.options end + + private + + def can_update_member?(current_user, member) + super || (member.owner? && member.new_record?) + end end def access_field diff --git a/app/models/project.rb b/app/models/project.rb index 082e5e4eeacacc..10c85b1f4e9bbc 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -151,6 +151,7 @@ def set_last_activity_at delegate :name, to: :owner, allow_nil: true, prefix: true delegate :members, to: :team, prefix: true + delegate :add_user, to: :team # Validations validates :creator, presence: true, on: :create @@ -1127,10 +1128,6 @@ def project_member(user) project_members.find_by(user_id: user) end - def add_user(user, access_level, current_user: nil, expires_at: nil) - team.add_user(user, access_level, current_user: current_user, expires_at: expires_at) - end - def default_branch @default_branch ||= repository.root_ref if repository.exists? end diff --git a/app/models/project_team.rb b/app/models/project_team.rb index bb22f0b74a9195..665a0fa1a20314 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -33,20 +33,26 @@ def find_member(user_id) member end - def add_users(users, access, current_user: nil, expires_at: nil) + def add_users(users, access_level, current_user: nil, expires_at: nil) return false if group_member_lock ProjectMember.add_users_to_projects( [project.id], users, - access, + access_level, current_user: current_user, expires_at: expires_at ) end - def add_user(user, access, current_user: nil, expires_at: nil) - add_users([user], access, current_user: current_user, expires_at: expires_at) + def add_user(user, access_level, current_user: nil, expires_at: nil) + ProjectMember.add_user( + project, + user, + access_level, + current_user: current_user, + expires_at: expires_at + ) end # Remove all users from project team diff --git a/db/fixtures/development/06_teams.rb b/db/fixtures/development/06_teams.rb index 3e8cdcd67b4a66..84b1e4b79e96f2 100644 --- a/db/fixtures/development/06_teams.rb +++ b/db/fixtures/development/06_teams.rb @@ -1,11 +1,8 @@ Gitlab::Seeder.quiet do Group.all.each do |group| User.all.sample(4).each do |user| - if group.add_users([user.id], Gitlab::Access.values.sample) - print '.' - else - print 'F' - end + group.add_users([user.id], Gitlab::Access.values.sample) + print '.' end end diff --git a/lib/api/members.rb b/lib/api/members.rb index 99cdf6e8c2a9b1..38b8e85a9649a4 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -65,13 +65,6 @@ class Members < Grape::API end ## EE specific - access_requester = source.requesters.find_by(user_id: params[:user_id]) - if access_requester - # We pass current_user = access_requester so that the requester doesn't - # receive a "access denied" email - ::Members::DestroyService.new(access_requester, access_requester.user).execute - end - member = source.members.find_by(user_id: params[:user_id]) # This is to ensure back-compatibility but 409 behavior should be used @@ -79,18 +72,12 @@ class Members < Grape::API conflict!('Member already exists') if source_type == 'group' && member unless member - source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at]) - member = source.members.find_by(user_id: params[:user_id]) + member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at]) end - if member + if member.persisted? && member.valid? present member.user, with: Entities::Member, member: member else - # Since `source.add_user` doesn't return a member object, we have to - # build a new one and populate its errors in order to render them. - member = source.members.build(attributes_for_keys([:user_id, :access_level, :expires_at])) - member.valid? # populate the errors - # This is to ensure back-compatibility but 400 behavior should be used # for all validation errors in 9.0! render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level) diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index a533bac26925ff..9b484a2ecfd9dd 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -53,6 +53,10 @@ def sym_options } end + def sym_options_with_owner + sym_options.merge(owner: OWNER) + end + def protection_options { "Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE, diff --git a/spec/controllers/projects/templates_controller_spec.rb b/spec/controllers/projects/templates_controller_spec.rb index 7b3a26d7ca7736..19a152bcb055b7 100644 --- a/spec/controllers/projects/templates_controller_spec.rb +++ b/spec/controllers/projects/templates_controller_spec.rb @@ -13,7 +13,7 @@ end before do - project.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) end diff --git a/spec/factories/project_members.rb b/spec/factories/project_members.rb index cf3659ba275065..1ddb305a8af4a7 100644 --- a/spec/factories/project_members.rb +++ b/spec/factories/project_members.rb @@ -4,24 +4,9 @@ project master - trait :guest do - access_level ProjectMember::GUEST - end - - trait :reporter do - access_level ProjectMember::REPORTER - end - - trait :developer do - access_level ProjectMember::DEVELOPER - end - - trait :master do - access_level ProjectMember::MASTER - end - - trait :owner do - access_level ProjectMember::OWNER - end + trait(:guest) { access_level ProjectMember::GUEST } + trait(:reporter) { access_level ProjectMember::REPORTER } + trait(:developer) { access_level ProjectMember::DEVELOPER } + trait(:master) { access_level ProjectMember::MASTER } end end diff --git a/spec/features/projects/members/owner_cannot_leave_project_spec.rb b/spec/features/projects/members/owner_cannot_leave_project_spec.rb index 67811b1048e6c5..6e948b7a616366 100644 --- a/spec/features/projects/members/owner_cannot_leave_project_spec.rb +++ b/spec/features/projects/members/owner_cannot_leave_project_spec.rb @@ -1,12 +1,10 @@ require 'spec_helper' feature 'Projects > Members > Owner cannot leave project', feature: true do - let(:owner) { create(:user) } let(:project) { create(:project) } background do - project.team << [owner, :owner] - login_as(owner) + login_as(project.owner) visit namespace_project_path(project.namespace, project) end diff --git a/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb b/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb index 0e54c4fdf20b4c..4ca9272b9c1073 100644 --- a/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb +++ b/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb @@ -1,12 +1,10 @@ require 'spec_helper' feature 'Projects > Members > Owner cannot request access to his project', feature: true do - let(:owner) { create(:user) } let(:project) { create(:project) } background do - project.team << [owner, :owner] - login_as(owner) + login_as(project.owner) visit namespace_project_path(project.namespace, project) end diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index 2242cb6236a602..c30d38b650891b 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -82,7 +82,7 @@ before do login_with(user) - project.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) visit namespace_project_path(project.namespace, project) end @@ -101,8 +101,8 @@ context 'on issues page', js: true do before do login_with(user) - project.team.add_user(user, Gitlab::Access::MASTER) - project2.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) + project2.add_user(user, Gitlab::Access::MASTER) visit namespace_project_issue_path(project.namespace, project, issue) end diff --git a/spec/finders/joined_groups_finder_spec.rb b/spec/finders/joined_groups_finder_spec.rb index f90a8e007c8e10..29a47e005a6693 100644 --- a/spec/finders/joined_groups_finder_spec.rb +++ b/spec/finders/joined_groups_finder_spec.rb @@ -43,7 +43,7 @@ context 'if profile visitor is in one of the private group projects' do before do project = create(:project, :private, group: private_group, name: 'B', path: 'B') - project.team.add_user(profile_visitor, Gitlab::Access::DEVELOPER) + project.add_user(profile_visitor, Gitlab::Access::DEVELOPER) end it 'shows group' do diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index 7a3a74335e899a..13bda5f7c5ad5e 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -38,7 +38,7 @@ describe 'with private projects' do before do - private_project.team.add_user(user, Gitlab::Access::MASTER) + private_project.add_user(user, Gitlab::Access::MASTER) end it do diff --git a/spec/lib/gitlab/template/issue_template_spec.rb b/spec/lib/gitlab/template/issue_template_spec.rb index f770857e958817..d2d334e64131d4 100644 --- a/spec/lib/gitlab/template/issue_template_spec.rb +++ b/spec/lib/gitlab/template/issue_template_spec.rb @@ -10,7 +10,7 @@ let(:file_path_3) { '.gitlab/issue_templates/feature_proposal.md' } before do - project.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false) project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false) @@ -53,7 +53,7 @@ context 'when repo is bare or empty' do let(:empty_project) { create(:empty_project) } - before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + before { empty_project.add_user(user, Gitlab::Access::MASTER) } it "returns empty array" do templates = subject.by_category('', empty_project) @@ -78,7 +78,7 @@ context "when repo is empty" do let(:empty_project) { create(:empty_project) } - before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + before { empty_project.add_user(user, Gitlab::Access::MASTER) } it "raises file not found" do issue_template = subject.new('.gitlab/issue_templates/not_existent.md', empty_project) diff --git a/spec/lib/gitlab/template/merge_request_template_spec.rb b/spec/lib/gitlab/template/merge_request_template_spec.rb index bb0f68043fa0ef..ddf68c4cf78bfc 100644 --- a/spec/lib/gitlab/template/merge_request_template_spec.rb +++ b/spec/lib/gitlab/template/merge_request_template_spec.rb @@ -10,7 +10,7 @@ let(:file_path_3) { '.gitlab/merge_request_templates/feature_proposal.md' } before do - project.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false) project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false) @@ -53,7 +53,7 @@ context 'when repo is bare or empty' do let(:empty_project) { create(:empty_project) } - before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + before { empty_project.add_user(user, Gitlab::Access::MASTER) } it "returns empty array" do templates = subject.by_category('', empty_project) @@ -78,7 +78,7 @@ context "when repo is empty" do let(:empty_project) { create(:empty_project) } - before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + before { empty_project.add_user(user, Gitlab::Access::MASTER) } it "raises file not found" do issue_template = subject.new('.gitlab/merge_request_templates/not_existent.md', empty_project) diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 55c01311565f52..9ff34ade108029 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -551,21 +551,22 @@ end end - def invite_to_project(project:, email:, inviter:) - Member.add_user( - project.project_members, - 'toto@example.com', - Gitlab::Access::DEVELOPER, - current_user: inviter + def invite_to_project(project, inviter:) + create( + :project_member, + :developer, + project: project, + invite_token: '1234', + invite_email: 'toto@example.com', + user: nil, + created_by: inviter ) - - project.project_members.invite.last end describe 'project invitation' do let(:project) { create(:project) } let(:master) { create(:user).tap { |u| project.team << [u, :master] } } - let(:project_member) { invite_to_project(project: project, email: 'toto@example.com', inviter: master) } + let(:project_member) { invite_to_project(project, inviter: master) } subject { Notify.member_invited_email('project', project_member.id, project_member.invite_token) } @@ -584,10 +585,10 @@ def invite_to_project(project:, email:, inviter:) describe 'project invitation accepted' do let(:project) { create(:project) } - let(:invited_user) { create(:user) } + let(:invited_user) { create(:user, name: 'invited user') } let(:master) { create(:user).tap { |u| project.team << [u, :master] } } let(:project_member) do - invitee = invite_to_project(project: project, email: 'toto@example.com', inviter: master) + invitee = invite_to_project(project, inviter: master) invitee.accept_invite!(invited_user) invitee end @@ -611,7 +612,7 @@ def invite_to_project(project:, email:, inviter:) let(:project) { create(:project) } let(:master) { create(:user).tap { |u| project.team << [u, :master] } } let(:project_member) do - invitee = invite_to_project(project: project, email: 'toto@example.com', inviter: master) + invitee = invite_to_project(project, inviter: master) invitee.decline_invite! invitee end @@ -803,21 +804,22 @@ def invite_to_project(project:, email:, inviter:) end end - def invite_to_group(group:, email:, inviter:) - Member.add_user( - group.group_members, - 'toto@example.com', - Gitlab::Access::DEVELOPER, - current_user: inviter + def invite_to_group(group, inviter:) + create( + :group_member, + :developer, + group: group, + invite_token: '1234', + invite_email: 'toto@example.com', + user: nil, + created_by: inviter ) - - group.group_members.invite.last end describe 'group invitation' do let(:group) { create(:group) } let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } } - let(:group_member) { invite_to_group(group: group, email: 'toto@example.com', inviter: owner) } + let(:group_member) { invite_to_group(group, inviter: owner) } subject { Notify.member_invited_email('group', group_member.id, group_member.invite_token) } @@ -836,10 +838,10 @@ def invite_to_group(group:, email:, inviter:) describe 'group invitation accepted' do let(:group) { create(:group) } - let(:invited_user) { create(:user) } + let(:invited_user) { create(:user, name: 'invited user') } let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } } let(:group_member) do - invitee = invite_to_group(group: group, email: 'toto@example.com', inviter: owner) + invitee = invite_to_group(group, inviter: owner) invitee.accept_invite!(invited_user) invitee end @@ -863,7 +865,7 @@ def invite_to_group(group:, email:, inviter:) let(:group) { create(:group) } let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } } let(:group_member) do - invitee = invite_to_group(group: group, email: 'toto@example.com', inviter: owner) + invitee = invite_to_group(group, inviter: owner) invitee.decline_invite! invitee end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 3259f79529647c..3b8b743af2d475 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -494,7 +494,7 @@ context 'with an admin user' do let(:project) { create(:empty_project) } - let(:user) { create(:user, admin: true) } + let(:user) { create(:admin) } it 'returns true for a regular issue' do issue = build(:issue, project: project) diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 0b1634f654af88..a9d3fcc6587bbf 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -74,22 +74,17 @@ @blocked_master = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::MASTER) @blocked_developer = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::DEVELOPER) - Member.add_user( - project.members, - 'toto1@example.com', - Gitlab::Access::DEVELOPER, - current_user: @master_user - ) - @invited_member = project.members.invite.find_by_invite_email('toto1@example.com') + @invited_member = create(:project_member, :developer, + project: project, + invite_token: '1234', + invite_email: 'toto1@example.com') accepted_invite_user = build(:user, state: :active) - Member.add_user( - project.members, - 'toto2@example.com', - Gitlab::Access::DEVELOPER, - current_user: @master_user - ) - @accepted_invite_member = project.members.invite.find_by_invite_email('toto2@example.com').tap { |u| u.accept_invite!(accepted_invite_user) } + @accepted_invite_member = create(:project_member, :developer, + project: project, + invite_token: '1234', + invite_email: 'toto2@example.com'). + tap { |u| u.accept_invite!(accepted_invite_user) } requested_user = create(:user).tap { |u| project.request_access(u) } @requested_member = project.requesters.find_by(user_id: requested_user.id) @@ -176,39 +171,209 @@ it { is_expected.to respond_to(:user_email) } end - describe ".add_user" do - let!(:user) { create(:user) } - let(:project) { create(:project) } + describe '.add_user' do + %w[project group].each do |source_type| + context "when source is a #{source_type}" do + let!(:source) { create(source_type) } + let!(:user) { create(:user) } + let!(:admin) { create(:admin) } - context "when called with a user id" do - it "adds the user as a member" do - Member.add_user(project.project_members, user.id, ProjectMember::MASTER) + it 'returns a Member object' do + member = described_class.add_user(source, user, :master) - expect(project.users).to include(user) - end - end + expect(member).to be_a "#{source_type.classify}Member".constantize + expect(member).to be_persisted + end - context "when called with a user object" do - it "adds the user as a member" do - Member.add_user(project.project_members, user, ProjectMember::MASTER) + it 'sets members.created_by to the given current_user' do + member = described_class.add_user(source, user, :master, current_user: admin) - expect(project.users).to include(user) - end - end + expect(member.created_by).to eq(admin) + end - context "when called with a known user email" do - it "adds the user as a member" do - Member.add_user(project.project_members, user.email, ProjectMember::MASTER) + it 'sets members.expires_at to the given expires_at' do + member = described_class.add_user(source, user, :master, expires_at: Date.new(2016, 9, 22)) - expect(project.users).to include(user) - end - end + expect(member.expires_at).to eq(Date.new(2016, 9, 22)) + end + + described_class.access_levels.each do |sym_key, int_access_level| + it "accepts the :#{sym_key} symbol as access level" do + expect(source.users).not_to include(user) + + member = described_class.add_user(source, user.id, sym_key) + + expect(member.access_level).to eq(int_access_level) + expect(source.users.reload).to include(user) + end + + it "accepts the #{int_access_level} integer as access level" do + expect(source.users).not_to include(user) + + member = described_class.add_user(source, user.id, int_access_level) + + expect(member.access_level).to eq(int_access_level) + expect(source.users.reload).to include(user) + end + end + + context 'with no current_user' do + context 'when called with a known user id' do + it 'adds the user as a member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, user.id, :master) + + expect(source.users.reload).to include(user) + end + end + + context 'when called with an unknown user id' do + it 'adds the user as a member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, 42, :master) + + expect(source.users.reload).not_to include(user) + end + end + + context 'when called with a user object' do + it 'adds the user as a member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, user, :master) + + expect(source.users.reload).to include(user) + end + end + + context 'when called with a requester user object' do + before do + source.request_access(user) + end + + it 'adds the requester as a member' do + expect(source.users).not_to include(user) + expect(source.requesters.exists?(user_id: user)).to be_truthy + + expect { described_class.add_user(source, user, :master) }. + to raise_error(Gitlab::Access::AccessDeniedError) + + expect(source.users.reload).not_to include(user) + expect(source.requesters.reload.exists?(user_id: user)).to be_truthy + end + end + + context 'when called with a known user email' do + it 'adds the user as a member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, user.email, :master) + + expect(source.users.reload).to include(user) + end + end + + context 'when called with an unknown user email' do + it 'creates an invited member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, 'user@example.com', :master) + + expect(source.members.invite.pluck(:invite_email)).to include('user@example.com') + end + end + end + + context 'when current_user can update member' do + it 'creates the member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, user, :master, current_user: admin) + + expect(source.users.reload).to include(user) + end + + context 'when called with a requester user object' do + before do + source.request_access(user) + end + + it 'adds the requester as a member' do + expect(source.users).not_to include(user) + expect(source.requesters.exists?(user_id: user)).to be_truthy + + described_class.add_user(source, user, :master, current_user: admin) + + expect(source.users.reload).to include(user) + expect(source.requesters.reload.exists?(user_id: user)).to be_falsy + end + end + end + + context 'when current_user cannot update member' do + it 'does not create the member' do + expect(source.users).not_to include(user) + + member = described_class.add_user(source, user, :master, current_user: user) + + expect(source.users.reload).not_to include(user) + expect(member).not_to be_persisted + end + + context 'when called with a requester user object' do + before do + source.request_access(user) + end + + it 'does not destroy the requester' do + expect(source.users).not_to include(user) + expect(source.requesters.exists?(user_id: user)).to be_truthy + + described_class.add_user(source, user, :master, current_user: user) + + expect(source.users.reload).not_to include(user) + expect(source.requesters.exists?(user_id: user)).to be_truthy + end + end + end + + context 'when member already exists' do + before do + source.add_user(user, :developer) + end + + context 'with no current_user' do + it 'updates the member' do + expect(source.users).to include(user) + + described_class.add_user(source, user, :master) + + expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::MASTER) + end + end + + context 'when current_user can update member' do + it 'updates the member' do + expect(source.users).to include(user) + + described_class.add_user(source, user, :master, current_user: admin) + + expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::MASTER) + end + end + + context 'when current_user cannot update member' do + it 'does not update the member' do + expect(source.users).to include(user) - context "when called with an unknown user email" do - it "adds a member invite" do - Member.add_user(project.project_members, "user@example.com", ProjectMember::MASTER) + described_class.add_user(source, user, :master, current_user: user) - expect(project.project_members.invite.pluck(:invite_email)).to include("user@example.com") + expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::DEVELOPER) + end + end + end end end end diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index 56fa7fa61347cc..370aeb9e0a920a 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -1,6 +1,33 @@ require 'spec_helper' describe GroupMember, models: true do + describe '.access_level_roles' do + it 'returns Gitlab::Access.options_with_owner' do + expect(described_class.access_level_roles).to eq(Gitlab::Access.options_with_owner) + end + end + + describe '.access_levels' do + it 'returns Gitlab::Access.options_with_owner' do + expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner) + end + end + + describe '.add_users_to_group' do + it 'adds the given users to the given group' do + group = create(:group) + users = create_list(:user, 2) + + described_class.add_users_to_group( + group, + [users.first.id, users.second], + described_class::MASTER + ) + + expect(group.users).to include(users.first, users.second) + end + end + describe 'notifications' do describe "#after_create" do it "sends email to user" do diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 805c15a4e5ecd8..d85a1c1e3b2e32 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -15,6 +15,26 @@ it { is_expected.to include_module(Gitlab::ShellAdapter) } end + describe '.access_level_roles' do + it 'returns Gitlab::Access.options' do + expect(described_class.access_level_roles).to eq(Gitlab::Access.options) + end + end + + describe '.add_user' do + context 'when called with the project owner' do + it 'adds the user as a member' do + project = create(:empty_project) + + expect(project.users).not_to include(project.owner) + + described_class.add_user(project, project.owner, :master, current_user: project.owner) + + expect(project.users.reload).to include(project.owner) + end + end + end + describe '#real_source_type' do subject { create(:project_member).real_source_type } @@ -50,7 +70,7 @@ end end - describe :import_team do + describe '.import_team' do before do @project_1 = create :project @project_2 = create :project @@ -81,25 +101,21 @@ end describe '.add_users_to_projects' do - before do - @project_1 = create :project - @project_2 = create :project + it 'adds the given users to the given projects' do + projects = create_list(:empty_project, 2) + users = create_list(:user, 2) - @user_1 = create :user - @user_2 = create :user - - ProjectMember.add_users_to_projects( - [@project_1.id, @project_2.id], - [@user_1.id, @user_2.id], - ProjectMember::MASTER - ) - end + described_class.add_users_to_projects( + [projects.first.id, projects.second], + [users.first.id, users.second], + described_class::MASTER) - it { expect(@project_1.users).to include(@user_1) } - it { expect(@project_1.users).to include(@user_2) } + expect(projects.first.users).to include(users.first) + expect(projects.first.users).to include(users.second) - it { expect(@project_2.users).to include(@user_1) } - it { expect(@project_2.users).to include(@user_2) } + expect(projects.second.users).to include(users.first) + expect(projects.second.users).to include(users.second) + end end describe '.truncate_teams' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 8e63baafb9ce36..14465bac352135 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -960,7 +960,7 @@ def create_pipeline describe 'when a user has access to a project' do before do - project.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) end it { is_expected.to eq([project]) } diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 626af06821b104..4b6825d9fbec06 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -15,7 +15,7 @@ let(:milestone) { create(:milestone, title: '1.0.0', project: project) } before do - project.team << [user, :reporters] + project.team << [user, :reporter] end describe "GET /projects/:id/merge_requests" do @@ -299,7 +299,7 @@ let!(:unrelated_project) { create(:project, namespace: create(:user).namespace, creator_id: user2.id) } before :each do |each| - fork_project.team << [user2, :reporters] + fork_project.team << [user2, :reporter] end it "returns merge_request" do diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index ec68875bf3e6ec..8618d60fe656d9 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -140,7 +140,7 @@ end describe 'repository_storage' do - let(:admin_user) { create(:user, admin: true) } + let(:admin_user) { create(:admin) } let(:user) { create(:user) } let(:project) { create(:project, repository_storage: 'a') } let(:opts) { { repository_storage: 'b' } } -- GitLab From cb08a1f4f9a216818dadb9a556e5017b33cdbd2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 20 Sep 2016 16:03:56 +0200 Subject: [PATCH 2/8] Remove the temporary fix to make LDAP sync handle access requester MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/ee/gitlab/ldap/sync/group.rb | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/lib/ee/gitlab/ldap/sync/group.rb b/lib/ee/gitlab/ldap/sync/group.rb index c7acd2ce8a6ff2..548207e9067bed 100644 --- a/lib/ee/gitlab/ldap/sync/group.rb +++ b/lib/ee/gitlab/ldap/sync/group.rb @@ -172,18 +172,9 @@ def add_or_update_user_membership(user, group, access) if access < ::Gitlab::Access::OWNER && group.last_owner?(user) warn_cannot_remove_last_owner(user, group) else - # Temporarily handle access requests until - # gitlab-org/gitlab-ee#825 is properly resolved. - member = group.requesters.find_by(user_id: user.id) - if member.present? - member.access_level = access - member.requested_at = nil - member.save - else - # If you pass the user object, instead of just user ID, - # it saves an extra user database query. - group.add_users([user], access, skip_notification: true, ldap: true) - end + # If you pass the user object, instead of just user ID, + # it saves an extra user database query. + group.add_user(user, access, skip_notification: true, ldap: true) end end -- GitLab From 223126243c0c8b7ec0f3109fddd22852c309be11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 21 Sep 2016 11:46:46 +0200 Subject: [PATCH 3/8] Fix a line in spec that was silently failing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- spec/requests/api/ldap_group_links_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/ldap_group_links_spec.rb b/spec/requests/api/ldap_group_links_spec.rb index ec9b1263e53ec4..64f96cdf720554 100644 --- a/spec/requests/api/ldap_group_links_spec.rb +++ b/spec/requests/api/ldap_group_links_spec.rb @@ -16,7 +16,7 @@ before do group_with_ldap_links.add_owner owner - group_with_ldap_links.add_user user, group_access: Gitlab::Access::DEVELOPER + group_with_ldap_links.add_user user, Gitlab::Access::DEVELOPER end describe "POST /groups/:id/ldap_group_links" do -- GitLab From 0ab7a52fed5ddc6f08e5cfb89fc6b672eea662fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 21 Sep 2016 17:14:51 +0200 Subject: [PATCH 4/8] Remove unnecessary options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/group.rb | 13 +++++-------- app/models/member.rb | 10 ++++------ app/models/members/group_member.rb | 6 ++---- lib/ee/gitlab/ldap/sync/group.rb | 2 +- spec/lib/ee/gitlab/ldap/sync/group_spec.rb | 13 ++++++------- 5 files changed, 18 insertions(+), 26 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 6f8e186ddcb073..4a8ccb6dc0631b 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -117,26 +117,23 @@ def lfs_enabled? self[:lfs_enabled] end - def add_users(users, access_level, current_user: nil, expires_at: nil, skip_notification: false, ldap: false) + def add_users(users, access_level, current_user: nil, expires_at: nil) GroupMember.add_users_to_group( self, users, access_level, current_user: current_user, - expires_at: expires_at, - skip_notification: skip_notification, - ldap: ldap + expires_at: expires_at ) end - def add_user(user, access_level, current_user: nil, expires_at: nil, skip_notification: false, ldap: false) + def add_user(user, access_level, current_user: nil, expires_at: nil, ldap: false) GroupMember.add_user( self, user, access_level, current_user: current_user, expires_at: expires_at, - skip_notification: skip_notification, ldap: ldap ) end @@ -157,8 +154,8 @@ def add_master(user, current_user = nil) add_user(user, :master, current_user: current_user) end - def add_owner(user, current_user = nil, skip_notification: false) - add_user(user, :owner, current_user: current_user, skip_notification: skip_notification) + def add_owner(user, current_user = nil) + add_user(user, :owner, current_user: current_user) end def has_owner?(user) diff --git a/app/models/member.rb b/app/models/member.rb index 5ff9e5f1510814..06e9b36ec44fa8 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -81,7 +81,7 @@ def find_by_invite_token(invite_token) find_by(invite_token: invite_token) end - def add_user(source, user, access_level, current_user: nil, expires_at: nil, skip_notification: false, ldap: false) + def add_user(source, user, access_level, current_user: nil, expires_at: nil, ldap: false) user = retrieve_user(user) access_level = retrieve_access_level(access_level) @@ -101,7 +101,7 @@ def add_user(source, user, access_level, current_user: nil, expires_at: nil, ski created_by: member.created_by || current_user, access_level: access_level, expires_at: expires_at, - skip_notification: skip_notification, + skip_notification: ldap, ldap: ldap } @@ -137,16 +137,14 @@ def can_update_member?(current_user, member) !current_user || current_user.can?(:"update_#{member.type.underscore}", member) end - def add_users_to_source(source, users, access_level, current_user: nil, expires_at: nil, skip_notification: false, ldap: false) + def add_users_to_source(source, users, access_level, current_user: nil, expires_at: nil) users.each do |user| add_user( source, user, access_level, current_user: current_user, - expires_at: expires_at, - skip_notification: skip_notification, - ldap: ldap + expires_at: expires_at ) end end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 6953673427c9b8..ae6c831d5d78f8 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -21,16 +21,14 @@ def self.access_levels Gitlab::Access.sym_options_with_owner end - def self.add_users_to_group(group, users, access_level, current_user: nil, expires_at: nil, skip_notification: false, ldap: false) + def self.add_users_to_group(group, users, access_level, current_user: nil, expires_at: nil) self.transaction do add_users_to_source( group, users, access_level, current_user: current_user, - expires_at: expires_at, - skip_notification: skip_notification, - ldap: ldap + expires_at: expires_at ) end end diff --git a/lib/ee/gitlab/ldap/sync/group.rb b/lib/ee/gitlab/ldap/sync/group.rb index 548207e9067bed..d4cb28a8e26462 100644 --- a/lib/ee/gitlab/ldap/sync/group.rb +++ b/lib/ee/gitlab/ldap/sync/group.rb @@ -174,7 +174,7 @@ def add_or_update_user_membership(user, group, access) else # If you pass the user object, instead of just user ID, # it saves an extra user database query. - group.add_user(user, access, skip_notification: true, ldap: true) + group.add_user(user, access, ldap: true) end end diff --git a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb index 3b2c2f7fdc8963..bdd2f617dc248b 100644 --- a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb +++ b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb @@ -160,7 +160,7 @@ def execute it 'downgrades existing member access' do # Create user with higher access group.add_users([user], - ::Gitlab::Access::MASTER, skip_notification: true) + ::Gitlab::Access::MASTER) sync_group.update_permissions @@ -171,7 +171,7 @@ def execute it 'upgrades existing member access' do # Create user with lower access group.add_users([user], - ::Gitlab::Access::GUEST, skip_notification: true) + ::Gitlab::Access::GUEST) sync_group.update_permissions @@ -182,8 +182,7 @@ def execute it 'sets an existing member ldap attribute to true' do group.add_users( [user], - ::Gitlab::Access::DEVELOPER, - skip_notification: true + ::Gitlab::Access::DEVELOPER ) sync_group.update_permissions @@ -213,7 +212,7 @@ def execute it 'removes the user from the group' do group.add_users([user], - Gitlab::Access::MASTER, skip_notification: true) + Gitlab::Access::MASTER) sync_group.update_permissions @@ -222,7 +221,7 @@ def execute it 'refuses to delete the last owner' do group.add_users([user], - Gitlab::Access::OWNER, skip_notification: true) + Gitlab::Access::OWNER) sync_group.update_permissions @@ -242,7 +241,7 @@ def execute create(:identity, user: user1, extern_uid: user_dn(user1.username)) create(:identity, user: user2, extern_uid: user_dn(user2.username)) group.add_users([user1, user2], - Gitlab::Access::OWNER, skip_notification: true) + Gitlab::Access::OWNER) sync_group.update_permissions -- GitLab From c902d8d48409a4d46f7669fc5fc85b9ba86e71d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 3 Oct 2016 11:06:52 +0200 Subject: [PATCH 5/8] Make Member#add_user set access_level for requesters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/member.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/models/member.rb b/app/models/member.rb index 06e9b36ec44fa8..0bc62b98544119 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -106,7 +106,12 @@ def add_user(source, user, access_level, current_user: nil, expires_at: nil, lda } if member.request? - ::Members::ApproveAccessRequestService.new(source, current_user, id: member.id).execute + ::Members::ApproveAccessRequestService.new( + source, + current_user, + id: member.id, + access_level: access_level + ).execute else member.save end -- GitLab From 4d56877fe8ad969b171146d8f8c43e2bfe286b69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 3 Oct 2016 11:15:58 +0200 Subject: [PATCH 6/8] Ensure LDAP group sync correctly transforms access requesters into members MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/ee/gitlab/ldap/sync/group.rb | 27 ++++++++++++++++++---- spec/lib/ee/gitlab/ldap/sync/group_spec.rb | 21 +++++++++-------- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/lib/ee/gitlab/ldap/sync/group.rb b/lib/ee/gitlab/ldap/sync/group.rb index d4cb28a8e26462..30ad92f40562a6 100644 --- a/lib/ee/gitlab/ldap/sync/group.rb +++ b/lib/ee/gitlab/ldap/sync/group.rb @@ -99,6 +99,8 @@ def dn_for_uid(uid) def update_existing_group_membership(group, access_levels) logger.debug { "Updating existing membership for '#{group.name}' group" } + first_group_owner = group.members.owners.first.try(:user) + select_and_preload_group_members(group).each do |member| user = member.user identity = user.identities.select(:id, :extern_uid) @@ -137,7 +139,12 @@ def update_existing_group_membership(group, access_levels) next if member.ldap? && member.override? - add_or_update_user_membership(user, group, desired_access) + add_or_update_user_membership( + user, + group, + desired_access, + current_user: first_group_owner + ) elsif group.last_owner?(user) warn_cannot_remove_last_owner(user, group) else @@ -149,11 +156,18 @@ def update_existing_group_membership(group, access_levels) def add_new_members(group, access_levels) logger.debug { "Adding new members to '#{group.name}' group" } + first_group_owner = group.members.owners.first.try(:user) + access_levels.each do |member_dn, access_level| user = ::Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider) if user.present? - add_or_update_user_membership(user, group, access_level) + add_or_update_user_membership( + user, + group, + access_level, + current_user: first_group_owner + ) else logger.debug do <<-MSG.strip_heredoc.tr("\n", ' ') @@ -167,14 +181,19 @@ def add_new_members(group, access_levels) end end - def add_or_update_user_membership(user, group, access) + def add_or_update_user_membership(user, group, access, current_user: nil) # Prevent the last owner of a group from being demoted if access < ::Gitlab::Access::OWNER && group.last_owner?(user) warn_cannot_remove_last_owner(user, group) else # If you pass the user object, instead of just user ID, # it saves an extra user database query. - group.add_user(user, access, ldap: true) + group.add_user( + user, + access, + current_user: current_user, + ldap: true + ) end end diff --git a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb index bdd2f617dc248b..748cbae47848be 100644 --- a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb +++ b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb @@ -112,8 +112,13 @@ def execute end describe '#update_permissions' do - before { group.start_ldap_sync } - after { group.finish_ldap_sync } + before do + group.start_ldap_sync + end + + after do + group.finish_ldap_sync + end let(:group) do create(:group_with_ldap_group_link, @@ -142,17 +147,15 @@ def execute end it 'converts an existing membership access request to a real member' do - group.members.create( - user: user, - access_level: ::Gitlab::Access::MASTER, - requested_at: DateTime.now - ) + group.add_owner(create(:user)) + access_requester = group.request_access(user) + access_requester.update(access_level: ::Gitlab::Access::MASTER) # Validate that the user is properly created as a requester first. - expect(group.requesters.pluck(:user_id)).to include(user.id) + expect(group.requesters.pluck(:id)).to include(access_requester.id) sync_group.update_permissions - expect(group.members.pluck(:user_id)).to include(user.id) + expect(group.members.pluck(:id)).to include(access_requester.id) expect(group.members.find_by(user_id: user.id).access_level) .to eq(::Gitlab::Access::DEVELOPER) end -- GitLab From 9e337725a23d24c755e361eb4e7c6c4988d6cdb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 28 Jul 2016 19:31:17 +0200 Subject: [PATCH 7/8] Improve Members::DestroyService (EE port) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- .../concerns/membership_actions.rb | 15 ++- .../groups/group_members_controller.rb | 5 +- .../projects/project_members_controller.rb | 6 +- .../members/authorized_destroy_service.rb | 2 + app/services/members/destroy_service.rb | 39 ++++-- lib/api/access_requests.rb | 5 +- lib/api/members.rb | 10 +- .../groups/group_members_controller_spec.rb | 4 +- .../project_members_controller_spec.rb | 4 +- spec/requests/api/access_requests_spec.rb | 14 ++- spec/services/members/destroy_service_spec.rb | 115 ++++++++++++------ 11 files changed, 149 insertions(+), 70 deletions(-) diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index 3c1156bf551fcc..20122aed58761f 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -17,21 +17,20 @@ def approve_access_request end def leave - @member = membershipable.members.find_by(user_id: current_user) || - membershipable.requesters.find_by(user_id: current_user) - Members::DestroyService.new(@member, current_user).execute + member = Members::DestroyService.new(membershipable, current_user, user_id: current_user.id). + execute(:all) - source_type = @member.real_source_type.humanize(capitalize: false) + source_type = membershipable.class.to_s.humanize(capitalize: false) notice = - if @member.request? + if member.request? "Your access request to the #{source_type} has been withdrawn." else - "You left the \"#{@member.source.human_name}\" #{source_type}." + "You left the \"#{membershipable.human_name}\" #{source_type}." end - log_audit_event(@member, action: :destroy) unless @member.request? + log_audit_event(member, action: :destroy) unless member.request? - redirect_path = @member.request? ? @member.source : [:dashboard, @member.real_source_type.tableize] + redirect_path = member.request? ? member.source : [:dashboard, membershipable.class.to_s.tableize] redirect_to redirect_path, notice: notice end diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index a22ebe5f9916f4..3afc5a77813120 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -50,10 +50,7 @@ def update end def destroy - @group_member = @group.members.find_by(id: params[:id]) || - @group.requesters.find_by(id: params[:id]) - - Members::DestroyService.new(@group_member, current_user).execute + Members::DestroyService.new(@group, current_user, id: params[:id]).execute(:all) log_audit_event(@group_member, action: :destroy) respond_to do |format| diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index 9e3626f1a6e761..d2fbbb8c0ca564 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -65,10 +65,8 @@ def update end def destroy - @project_member = @project.members.find_by(id: params[:id]) || - @project.requesters.find_by(id: params[:id]) - - Members::DestroyService.new(@project_member, current_user).execute + Members::DestroyService.new(@project, current_user, params). + execute(:all) log_audit_event(@project_member, action: :destroy) diff --git a/app/services/members/authorized_destroy_service.rb b/app/services/members/authorized_destroy_service.rb index ca9db59cac7067..b7a244c2029282 100644 --- a/app/services/members/authorized_destroy_service.rb +++ b/app/services/members/authorized_destroy_service.rb @@ -14,6 +14,8 @@ def execute if member.request? && member.user != user notification_service.decline_access_request(member) end + + member end end end diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index 9a2bf82ef516e2..431da8372c96d0 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -1,17 +1,42 @@ module Members class DestroyService < BaseService - attr_accessor :member, :current_user + include MembersHelper - def initialize(member, current_user) - @member = member + attr_accessor :source + + ALLOWED_SCOPES = %i[members requesters all] + + def initialize(source, current_user, params = {}) + @source = source @current_user = current_user + @params = params end - def execute - unless member && can?(current_user, "destroy_#{member.type.underscore}".to_sym, member) - raise Gitlab::Access::AccessDeniedError - end + def execute(scope = :members) + raise "scope :#{scope} is not allowed!" unless ALLOWED_SCOPES.include?(scope) + + member = find_member!(scope) + + raise Gitlab::Access::AccessDeniedError unless can_destroy_member?(member) + AuthorizedDestroyService.new(member, current_user).execute end + + private + + def find_member!(scope) + condition = params[:user_id] ? { user_id: params[:user_id] } : { id: params[:id] } + case scope + when :all + source.members.find_by(condition) || + source.requesters.find_by!(condition) + else + source.public_send(scope).find_by!(condition) + end + end + + def can_destroy_member?(member) + member && can?(current_user, action_member_permission(:destroy, member), member) + end end end diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index 7b9de7c9598bf3..d3db77408302c9 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -75,9 +75,8 @@ class AccessRequests < Grape::API required_attributes! [:user_id] source = find_source(source_type, params[:id]) - access_requester = source.requesters.find_by!(user_id: params[:user_id]) - - ::Members::DestroyService.new(access_requester, current_user).execute + ::Members::DestroyService.new(source, current_user, params). + execute(:requesters) end end end diff --git a/lib/api/members.rb b/lib/api/members.rb index 38b8e85a9649a4..73755dca16c6ba 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -71,6 +71,14 @@ class Members < Grape::API # for both project and group members in 9.0! conflict!('Member already exists') if source_type == 'group' && member + access_requester = source.requesters.find_by(user_id: params[:user_id]) + if access_requester + # We delete a potential access requester before creating the new member. + # We pass current_user = access_requester so that the requester doesn't + # receive a "access denied" email. + ::Members::DestroyService.new(source, access_requester.user, params).execute(:requesters) + end + unless member member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at]) end @@ -140,7 +148,7 @@ class Members < Grape::API if member.nil? { message: "Access revoked", id: params[:user_id].to_i } else - ::Members::DestroyService.new(member, current_user).execute + ::Members::DestroyService.new(source, current_user, params).execute present member.user, with: Entities::Member, member: member end diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb index 92b97bf3d0cc19..a0870891cf4a4c 100644 --- a/spec/controllers/groups/group_members_controller_spec.rb +++ b/spec/controllers/groups/group_members_controller_spec.rb @@ -87,10 +87,10 @@ context 'when member is not found' do before { sign_in(user) } - it 'returns 403' do + it 'returns 404' do delete :leave, group_id: group - expect(response).to have_http_status(403) + expect(response).to have_http_status(404) end end diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb index 19d05aef0a6fcb..c660b887e69cbe 100644 --- a/spec/controllers/projects/project_members_controller_spec.rb +++ b/spec/controllers/projects/project_members_controller_spec.rb @@ -135,11 +135,11 @@ context 'when member is not found' do before { sign_in(user) } - it 'returns 403' do + it 'returns 404' do delete :leave, namespace_id: project.namespace, project_id: project - expect(response).to have_http_status(403) + expect(response).to have_http_status(404) end end diff --git a/spec/requests/api/access_requests_spec.rb b/spec/requests/api/access_requests_spec.rb index d78494b76fac2a..aeb76a7a6e7496 100644 --- a/spec/requests/api/access_requests_spec.rb +++ b/spec/requests/api/access_requests_spec.rb @@ -181,7 +181,7 @@ end context 'when authenticated as the access requester' do - it 'returns 200' do + it 'deletes the access requester' do expect do delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", access_requester) @@ -191,7 +191,7 @@ end context 'when authenticated as a master/owner' do - it 'returns 200' do + it 'deletes the access requester' do expect do delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", master) @@ -199,6 +199,16 @@ end.to change { source.requesters.count }.by(-1) end + context 'user_id matches a member, not an access requester' do + it 'returns 404' do + expect do + delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{developer.id}", master) + + expect(response).to have_http_status(404) + end.not_to change { source.requesters.count } + end + end + context 'user_id does not match an existing access requester' do it 'returns 404' do expect do diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 2395445e7fddcb..9995f3488af78d 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -2,70 +2,111 @@ describe Members::DestroyService, services: true do let(:user) { create(:user) } - let(:project) { create(:project) } - let!(:member) { create(:project_member, source: project) } + let(:member_user) { create(:user) } + let(:project) { create(:project, :public) } + let(:group) { create(:group, :public) } - context 'when member is nil' do - before do - project.team << [user, :developer] + 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) end + end - it 'does not destroy the member' do - expect { destroy_member(nil, user) }.to raise_error(Gitlab::Access::AccessDeniedError) + 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) end end - context 'when current user cannot destroy the given member' do - before do - project.team << [user, :developer] + shared_examples 'a service destroying a member' do + it 'destroys the member' do + expect { described_class.new(source, user, params).execute }.to change { source.members.count }.by(-1) + end + + context 'when the given member is an access requester' do + before do + source.members.find_by(user_id: member_user).destroy + source.request_access(member_user) + end + let(:access_requester) { source.requesters.find_by(user_id: member_user) } + + it_behaves_like 'a service raising ActiveRecord::RecordNotFound' + + %i[requesters all].each do |scope| + context "and #{scope} scope is passed" do + it 'destroys the access requester' do + expect { described_class.new(source, user, params).execute(scope) }.to change { source.requesters.count }.by(-1) + end + + it 'calls Member#after_decline_request' do + expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(access_requester) + + described_class.new(source, user, params).execute(scope) + end + + context 'when current user is the member' do + it 'does not call Member#after_decline_request' do + expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(access_requester) + + described_class.new(source, member_user, params).execute(scope) + end + end + end + end end + end + + context 'when no member are found' do + let(:params) { { user_id: 42 } } - it 'does not destroy the member' do - expect { destroy_member(member, user) }.to raise_error(Gitlab::Access::AccessDeniedError) + it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do + let(:source) { project } + end + + it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do + let(:source) { group } end end - context 'when current user can destroy the given member' do + context 'when a member is found' do before do - project.team << [user, :master] + project.team << [member_user, :developer] + group.add_developer(member_user) end + let(:params) { { user_id: member_user.id } } - it 'destroys the member' do - destroy_member(member, user) + context 'when current user cannot destroy the given member' do + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let(:source) { project } + end - expect(member).to be_destroyed + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let(:source) { group } + end end - context 'when the given member is a requester' do + context 'when current user can destroy the given member' do before do - member.update_column(:requested_at, Time.now) + project.team << [user, :master] + group.add_owner(user) end - it 'calls Member#after_decline_request' do - expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member) - - destroy_member(member, user) + it_behaves_like 'a service destroying a member' do + let(:source) { project } end - context 'when current user is the member' do - it 'does not call Member#after_decline_request' do - expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) - - destroy_member(member, member.user) - end + it_behaves_like 'a service destroying a member' do + let(:source) { group } end - context 'when current user is the member and ' do - it 'does not call Member#after_decline_request' do - expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) + context 'when given a :id' do + let(:params) { { id: project.members.find_by!(user_id: user.id).id } } - destroy_member(member, member.user) + it 'destroys the member' do + expect { described_class.new(project, user, params).execute }. + to change { project.members.count }.by(-1) end end end end - - def destroy_member(member, user) - Members::DestroyService.new(member, user).execute - end end -- GitLab From 0b498003cb3be82cf4b0502686ee38c59673ad1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 3 Oct 2016 18:47:18 +0200 Subject: [PATCH 8/8] Fix broken EE-specific parts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/controllers/concerns/membership_actions.rb | 6 ++++++ app/controllers/groups/group_members_controller.rb | 7 +++++-- app/controllers/projects/project_members_controller.rb | 6 ++++-- lib/api/members.rb | 8 -------- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index 20122aed58761f..d6af279ae8ac56 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -11,7 +11,9 @@ def request_access def approve_access_request member = Members::ApproveAccessRequestService.new(membershipable, current_user, params).execute + ## EE specific log_audit_event(member, action: :create) + ## EE specific redirect_to polymorphic_url([membershipable, :members]) end @@ -28,7 +30,9 @@ def leave "You left the \"#{membershipable.human_name}\" #{source_type}." end + ## EE specific log_audit_event(member, action: :destroy) unless member.request? + ## EE specific redirect_path = member.request? ? member.source : [:dashboard, membershipable.class.to_s.tableize] @@ -41,8 +45,10 @@ def membershipable raise NotImplementedError end + ## EE specific def log_audit_event(member, options = {}) AuditEventService.new(current_user, membershipable, options). for_member(member).security_event end + ## EE specific end diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index 3afc5a77813120..9df8d8392ff6f8 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -50,8 +50,11 @@ def update end def destroy - Members::DestroyService.new(@group, current_user, id: params[:id]).execute(:all) - log_audit_event(@group_member, action: :destroy) + member = Members::DestroyService.new(@group, current_user, id: params[:id]).execute(:all) + + ## EE specific + log_audit_event(member, action: :destroy) + ## EE specific respond_to do |format| format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' } diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index d2fbbb8c0ca564..535e93ff7fc497 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -65,10 +65,12 @@ def update end def destroy - Members::DestroyService.new(@project, current_user, params). + member = Members::DestroyService.new(@project, current_user, params). execute(:all) - log_audit_event(@project_member, action: :destroy) + ## EE specific + log_audit_event(member, action: :destroy) + ## EE specific respond_to do |format| format.html do diff --git a/lib/api/members.rb b/lib/api/members.rb index 73755dca16c6ba..3d359c3fbd00bb 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -71,14 +71,6 @@ class Members < Grape::API # for both project and group members in 9.0! conflict!('Member already exists') if source_type == 'group' && member - access_requester = source.requesters.find_by(user_id: params[:user_id]) - if access_requester - # We delete a potential access requester before creating the new member. - # We pass current_user = access_requester so that the requester doesn't - # receive a "access denied" email. - ::Members::DestroyService.new(source, access_requester.user, params).execute(:requesters) - end - unless member member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at]) end -- GitLab