From f2275e3377083b0eaa1993310804d859f4b6872e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Sanz=20Garc=C3=ADa?= Date: Fri, 29 Jul 2022 13:19:49 +0200 Subject: [PATCH 1/2] Fix warning when user is unable to create project A vague warning, `Namespace is not valid`, was shown if a user tries to create a project when his maximum number of projects has been reached. After this change, we reuse 2 more specific errors: * `Personal project creation is not allowed. Please contact your administrator with questions`, if the user has a limit of 0. * `Your project limit is %{limit} projects! Please contact your administrator to increase it`, if user has reached the limit but the limit is different than 0. Changelog: changed --- app/services/projects/create_service.rb | 28 ++++++++----------- spec/services/projects/create_service_spec.rb | 26 +++++++++++++++++ 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 9bc8bb428fb05e..6381ee67ce750c 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -26,7 +26,7 @@ def execute return ::Projects::CreateFromTemplateService.new(current_user, params).execute end - @project = Project.new(params) + @project = Project.new(params.merge(creator: current_user)) validate_import_source_enabled! @@ -45,20 +45,14 @@ def execute set_project_name_from_path # get namespace id - namespace_id = params[:namespace_id] - - if namespace_id - # Find matching namespace and check if it allowed - # for current user if namespace_id passed. - unless current_user.can?(:create_projects, parent_namespace) - @project.namespace_id = nil - deny_namespace - return @project - end - else - # Set current user namespace if namespace_id is nil - @project.namespace_id = current_user.namespace_id - end + namespace_id = params[:namespace_id] || current_user.namespace_id + @project.namespace_id = namespace_id.to_i + + @project.check_personal_projects_limit + return @project if @project.errors.any? + + validate_create_permissions + return @project if @project.errors.any? @relations_block&.call(@project) yield(@project) if block_given? @@ -92,7 +86,9 @@ def execute protected - def deny_namespace + def validate_create_permissions + return if current_user.can?(:create_projects, parent_namespace) + @project.errors.add(:namespace, "is not valid") end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 59dee209ff99d0..5391ad932b0fe5 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -254,6 +254,32 @@ end end + context 'user with project limit' do + let(:user_with_projects_limit) { create(:user, projects_limit: 0) } + let!(:group) do + create(:group).tap do |group| + group.add_owner(user_with_projects_limit) + end + end + + it "can't create a project under personal namespace" do + params = opts.merge!(namespace_id: user_with_projects_limit.namespace.id) + project = create_project(user_with_projects_limit, params) + + expect(project.errors.errors.length).to eq 1 + expect(project.errors.messages[:limit_reached].first).to eq(_('Personal project creation is not allowed. Please contact your administrator with questions')) + end + + it 'can still create a project under a group namespace' do + params = opts.merge!(namespace_id: group.id) + project = create_project(user_with_projects_limit, params) + + expect(project).to be_valid + expect(project).to be_saved + expect(project.errors.errors.length).to eq 0 + end + end + context 'membership overrides', :sidekiq_inline do let_it_be(:group) { create(:group, :private) } let_it_be(:subgroup_for_projects) { create(:group, :private, parent: group) } -- GitLab From d8fea4b1e2c155a007c29926e39d160182fa6191 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Sanz=20Garc=C3=ADa?= Date: Mon, 1 Aug 2022 17:28:59 +0200 Subject: [PATCH 2/2] Apply suggestions from code review --- spec/services/projects/create_service_spec.rb | 41 +++++++++++-------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 5391ad932b0fe5..9694ce40fdb4f1 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -255,28 +255,35 @@ end context 'user with project limit' do - let(:user_with_projects_limit) { create(:user, projects_limit: 0) } - let!(:group) do - create(:group).tap do |group| - group.add_owner(user_with_projects_limit) - end - end + let_it_be(:user_with_projects_limit) { create(:user, projects_limit: 0) } + + let(:params) { opts.merge!(namespace_id: target_namespace.id) } - it "can't create a project under personal namespace" do - params = opts.merge!(namespace_id: user_with_projects_limit.namespace.id) - project = create_project(user_with_projects_limit, params) + subject(:project) { create_project(user_with_projects_limit, params) } - expect(project.errors.errors.length).to eq 1 - expect(project.errors.messages[:limit_reached].first).to eq(_('Personal project creation is not allowed. Please contact your administrator with questions')) + context 'under personal namespace' do + let(:target_namespace) { user_with_projects_limit.namespace } + + it 'cannot create a project' do + expect(project.errors.errors.length).to eq 1 + expect(project.errors.messages[:limit_reached].first).to eq(_('Personal project creation is not allowed. Please contact your administrator with questions')) + end end - it 'can still create a project under a group namespace' do - params = opts.merge!(namespace_id: group.id) - project = create_project(user_with_projects_limit, params) + context 'under group namespace' do + let_it_be(:group) do + create(:group).tap do |group| + group.add_owner(user_with_projects_limit) + end + end - expect(project).to be_valid - expect(project).to be_saved - expect(project.errors.errors.length).to eq 0 + let(:target_namespace) { group } + + it 'can create a project' do + expect(project).to be_valid + expect(project).to be_saved + expect(project.errors.errors.length).to eq 0 + end end end -- GitLab