Commit 44fe4f60 authored by Rémy Coutable's avatar Rémy Coutable
Browse files

Allow Member.add_user to handle access requesters



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: default avatarRémy Coutable <remy@rymai.me>
parent 993bb98ce803
......@@ -102,16 +102,14 @@
self[:lfs_enabled]
end
def add_users(user_ids, access_level, current_user: nil, expires_at: nil)
user_ids.each do |user_id|
Member.add_user(
self.group_members,
user_id,
access_level,
current_user: current_user,
expires_at: expires_at
)
end
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
)
end
def add_user(user, access_level, current_user: nil, expires_at: nil)
......@@ -115,7 +113,13 @@
end
def add_user(user, access_level, current_user: nil, expires_at: nil)
add_users([user], access_level, current_user: current_user, expires_at: expires_at)
GroupMember.add_user(
self,
user,
access_level,
current_user: current_user,
expires_at: expires_at
)
end
def add_guest(user, current_user = nil)
......@@ -119,7 +123,7 @@
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)
......@@ -123,7 +127,7 @@
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)
......@@ -127,7 +131,7 @@
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)
......@@ -131,7 +135,7 @@
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)
......@@ -135,7 +139,7 @@
end
def add_owner(user, current_user = nil)
add_user(user, Gitlab::Access::OWNER, current_user: current_user)
add_user(user, :owner, current_user: current_user)
end
def has_owner?(user)
......
......@@ -80,18 +80,8 @@
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, expires_at: nil)
user = user_for_id(user_id)
def add_user(source, user, access_level, current_user: nil, expires_at: nil)
user = retrieve_user(user)
access_level = retrieve_access_level(access_level)
# `user` can be either a User object or an email to be invited
......@@ -96,5 +86,22 @@
# `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
}
if member.request?
member.accept_request
else
......@@ -100,5 +107,4 @@
else
member = members.build
member.invite_email = user
member.save
end
......@@ -103,7 +109,5 @@
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
end
......@@ -109,7 +113,7 @@
member.save
end
def access_levels
Gitlab::Access.sym_options
end
private
......@@ -112,6 +116,18 @@
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
......@@ -116,7 +132,5 @@
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
......@@ -121,8 +135,15 @@
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)
users.each do |user|
add_user(
source,
user,
access_level,
current_user: current_user,
expires_at: expires_at
)
end
end
end
......
......@@ -12,6 +12,22 @@
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)
self.transaction do
add_users_to_source(
group,
users,
access_level,
current_user: current_user,
expires_at: expires_at
)
end
end
def group
source
end
......
......@@ -34,18 +34,8 @@
# :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)
......@@ -49,14 +39,12 @@
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
......@@ -61,9 +49,5 @@
end
end
true
rescue
false
end
def truncate_teams(project_ids)
......@@ -84,7 +68,7 @@
truncate_teams [project.id]
end
def roles_hash
Gitlab::Access.sym_options
def access_level_roles
Gitlab::Access.options
end
......@@ -89,7 +73,9 @@
end
def access_level_roles
Gitlab::Access.options
private
def can_update_member?(current_user, member)
super || (member.owner? && member.new_record?)
end
end
......
......@@ -146,6 +146,7 @@
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
......@@ -1016,10 +1017,6 @@
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
......
......@@ -33,7 +33,7 @@
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)
ProjectMember.add_users_to_projects(
[project.id],
users,
......@@ -37,9 +37,9 @@
ProjectMember.add_users_to_projects(
[project.id],
users,
access,
access_level,
current_user: current_user,
expires_at: expires_at
)
end
......@@ -41,10 +41,16 @@
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
......
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)
if group.add_user(user, Gitlab::Access.values.sample).persisted?
print '.'
else
print 'F'
......
......@@ -59,13 +59,6 @@
authorize_admin_source!(source_type, source)
required_attributes! [:user_id, :access_level]
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
......@@ -73,7 +66,6 @@
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
......@@ -78,5 +70,5 @@
end
if member
if member.persisted? && member.valid?
present member.user, with: Entities::Member, member: member
else
......@@ -81,10 +73,5 @@
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)
......
......@@ -53,6 +53,10 @@
}
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,
......
......@@ -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
......
......@@ -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
require 'spec_helper'
feature 'Projects > Members > Owner cannot leave project', feature: true do
let(:owner) { create(:user) }
let(:project) { create(:project) }
background do
......@@ -5,8 +4,7 @@
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
......
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
......@@ -5,8 +4,7 @@
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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -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)
......
......@@ -492,10 +492,13 @@
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
)
......@@ -501,8 +504,6 @@
)
project.project_members.invite.last
end