Commit 0acfc4c6 authored by Imre Farkas's avatar Imre Farkas

Merge branch...

Merge branch '355659-inviting-project-bots-project-access-token-users-to-be-members-of-projects-groups' into 'master'

Add error message to member when can not be updated

See merge request gitlab-org/gitlab!84105
parents 8d60a27b 9ee36ff7
......@@ -650,7 +650,6 @@ Layout/LineLength:
- 'app/services/loose_foreign_keys/batch_cleaner_service.rb'
- 'app/services/loose_foreign_keys/cleaner_service.rb'
- 'app/services/members/approve_access_request_service.rb'
- 'app/services/members/create_service.rb'
- 'app/services/members/destroy_service.rb'
- 'app/services/members/invitation_reminder_email_service.rb'
- 'app/services/members/mailgun/process_webhook_service.rb'
......
......@@ -16,6 +16,7 @@ module Members
@errors = []
@invites = invites_from_params
@source = params[:source]
@tasks_to_be_done_members = []
end
def execute
......@@ -25,6 +26,7 @@ module Members
validate_invitable!
add_members
create_tasks_to_be_done
enqueue_onboarding_progress_action
publish_event!
......@@ -40,7 +42,8 @@ module Members
private
attr_reader :source, :errors, :invites, :member_created_namespace_id, :members
attr_reader :source, :errors, :invites, :member_created_namespace_id, :members,
:tasks_to_be_done_members, :member_created_member_task_id
def invites_from_params
return params[:user_ids] if params[:user_ids].is_a?(Array)
......@@ -76,13 +79,15 @@ module Members
)
members.each { |member| process_result(member) }
create_tasks_to_be_done
end
def process_result(member)
if member.invalid?
add_error_for_member(member)
existing_errors = member.errors.full_messages
# calling invalid? clears any errors that were added outside of the
# rails validation process
if member.invalid? || existing_errors.present?
add_error_for_member(member, existing_errors)
else
after_execute(member: member)
@member_created_namespace_id ||= member.namespace_id
......@@ -90,20 +95,29 @@ module Members
end
# overridden
def add_error_for_member(member)
def add_error_for_member(member, existing_errors)
prefix = "#{member.user.username}: " if member.user.present?
errors << "#{prefix}#{member.errors.full_messages.to_sentence}"
errors << "#{prefix}#{all_member_errors(member, existing_errors).to_sentence}"
end
def all_member_errors(member, existing_errors)
existing_errors.concat(member.errors.full_messages).uniq
end
def after_execute(member:)
super
build_tasks_to_be_done_members(member)
track_invite_source(member)
end
def track_invite_source(member)
Gitlab::Tracking.event(self.class.name, 'create_member', label: invite_source, property: tracking_property(member), user: current_user)
Gitlab::Tracking.event(self.class.name,
'create_member',
label: invite_source,
property: tracking_property(member),
user: current_user)
end
def invite_source
......@@ -117,17 +131,28 @@ module Members
member.invite? ? 'net_new_user' : 'existing_user'
end
def create_tasks_to_be_done
return if params[:tasks_to_be_done].blank? || params[:tasks_project_id].blank?
# Only create task issues for existing users. Tasks for new users are created when they signup.
valid_members = members.select { |member| member.valid? && member.member_task.valid? && member.user.present? }
return unless valid_members.present?
def build_tasks_to_be_done_members(member)
return unless tasks_to_be_done?(member)
@tasks_to_be_done_members << member
# We can take the first `member_task` here, since all tasks will have the same attributes needed
# for the `TasksToBeDone::CreateWorker`, ie. `project` and `tasks_to_be_done`.
member_task = valid_members[0].member_task
TasksToBeDone::CreateWorker.perform_async(member_task.id, current_user.id, valid_members.map(&:user_id))
@member_created_member_task_id ||= member.member_task.id
end
def tasks_to_be_done?(member)
return false if params[:tasks_to_be_done].blank? || params[:tasks_project_id].blank?
# Only create task issues for existing users. Tasks for new users are created when they signup.
member.member_task&.valid? && member.user.present?
end
def create_tasks_to_be_done
return unless member_created_member_task_id # signal if there is any work to be done here
TasksToBeDone::CreateWorker.perform_async(member_created_member_task_id,
current_user.id,
tasks_to_be_done_members.map(&:user_id))
end
def user_limit
......
......@@ -4,15 +4,13 @@ module Members
# This class serves as more of an app-wide way we add/create members
# All roads to add members should take this path.
class CreatorService
include Gitlab::Experiment::Dsl
class << self
def parsed_access_level(access_level)
access_levels.fetch(access_level) { access_level.to_i }
end
def access_levels
raise NotImplementedError
Gitlab::Access.sym_options_with_owner
end
end
......@@ -25,7 +23,7 @@ module Members
def execute
find_or_build_member
update_member
commit_member
create_member_task
member
......@@ -33,23 +31,39 @@ module Members
private
delegate :new_record?, to: :member
attr_reader :source, :user, :access_level, :member, :args
def update_member
return unless can_update_member?
def assign_member_attributes
member.attributes = member_attributes
end
if member.request?
approve_request
def commit_member
if can_commit_member?
assign_member_attributes
commit_changes
else
member.save
add_commit_error
end
end
def can_update_member?
def can_commit_member?
# There is no current user for bulk actions, in which case anything is allowed
!current_user # inheriting classes will add more logic
return true if skip_authorization?
if new_record?
can_create_new_member?
else
can_update_existing_member?
end
end
def can_create_new_member?
raise NotImplementedError
end
def can_update_existing_member?
raise NotImplementedError
end
# Populates the attributes of a member.
......@@ -64,6 +78,14 @@ module Members
}
end
def commit_changes
if member.request?
approve_request
else
member.save
end
end
def create_member_task
return unless member.persisted?
return if member_task_attributes.value?(nil)
......@@ -93,6 +115,20 @@ module Members
args[:current_user]
end
def skip_authorization?
!current_user
end
def add_commit_error
msg = if new_record?
_('not authorized to create member')
else
_('not authorized to update member')
end
member.errors.add(:base, msg)
end
def find_or_build_member
@user = parse_user_param
......
......@@ -3,14 +3,14 @@
module Members
module Groups
class CreatorService < Members::CreatorService
def self.access_levels
Gitlab::Access.sym_options_with_owner
end
private
def can_update_member?
super || current_user.can?(:update_group_member, member)
def can_create_new_member?
current_user.can?(:admin_group_member, member.group)
end
def can_update_existing_member?
current_user.can?(:update_group_member, member)
end
end
end
......
......@@ -51,8 +51,8 @@ module Members
end
override :add_error_for_member
def add_error_for_member(member)
errors[invited_object(member)] = member.errors.full_messages.to_sentence
def add_error_for_member(member, existing_errors)
errors[invited_object(member)] = all_member_errors(member, existing_errors).to_sentence
end
def invited_object(member)
......
......@@ -3,19 +3,28 @@
module Members
module Projects
class CreatorService < Members::CreatorService
def self.access_levels
Gitlab::Access.sym_options_with_owner
end
private
def can_update_member?
super || current_user.can?(:update_project_member, member) || adding_the_creator_as_owner_in_a_personal_project?
def can_create_new_member?
# order is important here!
# The `admin_project_member` check has side-effects that causes projects not be created if this area is hit
# during project creation.
# Call that triggers is current_user.can?(:admin_project_member, member.project)
# I tracked back to base_policy.rb admin check and specifically in
# Gitlab::Auth::CurrentUserMode.new(@user).admin_mode? call.
# This calls user.admin? and that specific call causes issues with project creation in
# spec/requests/api/projects_spec.rb specs and others, mostly around project creation.
# https://gitlab.com/gitlab-org/gitlab/-/issues/358931 for investigation
adding_the_creator_as_owner_in_a_personal_project? || current_user.can?(:admin_project_member, member.project)
end
def can_update_existing_member?
current_user.can?(:update_project_member, member)
end
def adding_the_creator_as_owner_in_a_personal_project?
# this condition is reached during testing setup a lot due to use of `.add_user`
member.project.personal_namespace_holder?(member.user) && member.new_record?
member.project.personal_namespace_holder?(member.user)
end
end
end
......
......@@ -45326,6 +45326,12 @@ msgstr ""
msgid "none"
msgstr ""
msgid "not authorized to create member"
msgstr ""
msgid "not authorized to update member"
msgstr ""
msgid "not found"
msgstr ""
......
......@@ -143,6 +143,32 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_
end
end
context 'when adding a project_bot' do
let_it_be(:project_bot) { create(:user, :project_bot) }
let(:user_ids) { project_bot.id }
context 'when project_bot is already a member' do
before do
source.add_developer(project_bot)
end
it 'does not update the member' do
expect(execute_service[:status]).to eq(:error)
expect(execute_service[:message]).to eq("#{project_bot.username}: not authorized to update member")
expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(false)
end
end
context 'when project_bot is not already a member' do
it 'adds the member' do
expect(execute_service[:status]).to eq(:success)
expect(source.users).to include project_bot
expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true)
end
end
end
context 'when tracking the invite source', :snowplow do
context 'when invite_source is not passed' do
let(:additional_params) { {} }
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Members::CreatorService do
let_it_be(:source, reload: true) { create(:group, :public) }
let_it_be(:member_type) { GroupMember }
let_it_be(:user) { create(:user) }
let_it_be(:current_user) { create(:user) }
describe '#execute' do
it 'raises error for new member on authorization check implementation' do
expect do
described_class.new(source, user, :maintainer, current_user: current_user).execute
end.to raise_error(NotImplementedError)
end
it 'raises error for an existing member on authorization check implementation' do
source.add_developer(user)
expect do
described_class.new(source, user, :maintainer, current_user: current_user).execute
end.to raise_error(NotImplementedError)
end
end
end
......@@ -88,19 +88,55 @@ RSpec.shared_examples_for "member creation" do
expect(member).to be_persisted
end
context 'when admin mode is enabled', :enable_admin_mode do
context 'when adding a project_bot' do
let_it_be(:project_bot) { create(:user, :project_bot) }
before_all do
source.add_owner(user)
end
context 'when project_bot is already a member' do
before do
source.add_developer(project_bot)
end
it 'does not update the member' do
member = described_class.new(source, project_bot, :maintainer, current_user: user).execute
expect(source.users.reload).to include(project_bot)
expect(member).to be_persisted
expect(member.access_level).to eq(Gitlab::Access::DEVELOPER)
expect(member.errors.full_messages).to include(/not authorized to update member/)
end
end
context 'when project_bot is not already a member' do
it 'adds the member' do
member = described_class.new(source, project_bot, :maintainer, current_user: user).execute
expect(source.users.reload).to include(project_bot)
expect(member).to be_persisted
end
end
end
context 'when admin mode is enabled', :enable_admin_mode, :aggregate_failures do
it 'sets members.created_by to the given admin current_user' do
member = described_class.new(source, user, :maintainer, current_user: admin).execute
expect(member).to be_persisted
expect(source.users.reload).to include(user)
expect(member.created_by).to eq(admin)
end
end
context 'when admin mode is disabled' do
it 'rejects setting members.created_by to the given admin current_user' do
it 'rejects setting members.created_by to the given admin current_user', :aggregate_failures do
member = described_class.new(source, user, :maintainer, current_user: admin).execute
expect(member.created_by).to be_nil
expect(member).not_to be_persisted
expect(source.users.reload).not_to include(user)
expect(member.errors.full_messages).to include(/not authorized to create member/)
end
end
......@@ -142,7 +178,7 @@ RSpec.shared_examples_for "member creation" do
end
context 'when called with an unknown user id' do
it 'adds the user as a member' do
it 'does not add the user as a member' do
expect(source.users).not_to include(user)
described_class.new(source, non_existing_record_id, :maintainer).execute
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment