From 921d2afc6989dfa8220032984f657210c07e8792 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Thu, 18 Jan 2018 09:31:00 +0000 Subject: [PATCH 1/9] Adds option to push over HTTP to create a new project --- .../projects/git_http_controller.rb | 21 +++++++++++++- lib/gitlab/git_access.rb | 29 +++++++++++++++---- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 71ae60cb8cd..45a1a5cf0de 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -11,6 +11,12 @@ class Projects::GitHttpController < Projects::GitHttpClientController def info_refs log_user_activity if upload_pack? + if project.blank? && params[:service] == 'git-receive-pack' + @project = ::Projects::CreateService.new(access_actor, project_params).execute + + return render_ok if @project.saved? + end + render_ok end @@ -26,6 +32,15 @@ class Projects::GitHttpController < Projects::GitHttpClientController private + def project_params + { + description: "", + path: params[:project_id].gsub("\.git", ''), + namespace_id: namespace.id.to_s, + visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s + } + end + def download_request? upload_pack? end @@ -56,7 +71,11 @@ class Projects::GitHttpController < Projects::GitHttpClientController end def access - @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities, redirected_path: redirected_path) + @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: namespace) + end + + def namespace + @namespace = Namespace.find_by_path_or_name(params[:namespace_id]) end def access_actor diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 56f6febe86d..9427a5e4baa 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -18,21 +18,23 @@ module Gitlab upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.', receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.', read_only: 'The repository is temporarily read-only. Please try again later.', - cannot_push_to_read_only: "You can't push code to a read-only GitLab instance." + cannot_push_to_read_only: "You can't push code to a read-only GitLab instance.", + create: "Creating a repository to that namespace is not allowed." }.freeze DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }.freeze PUSH_COMMANDS = %w{ git-receive-pack }.freeze ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS - attr_reader :actor, :project, :protocol, :authentication_abilities, :redirected_path + attr_reader :actor, :project, :protocol, :authentication_abilities, :redirected_path, :target_namespace - def initialize(actor, project, protocol, authentication_abilities:, redirected_path: nil) + def initialize(actor, project, protocol, authentication_abilities:, redirected_path: nil, target_namespace: nil) @actor = actor @project = project @protocol = protocol @redirected_path = redirected_path @authentication_abilities = authentication_abilities + @target_namespace = target_namespace end def check(cmd, changes) @@ -44,6 +46,7 @@ module Gitlab check_command_disabled!(cmd) check_command_existence!(cmd) check_repository_existence! + check_repository_creation! case cmd when *DOWNLOAD_COMMANDS @@ -96,7 +99,7 @@ module Gitlab end def check_project_accessibility! - if project.blank? || !can_read_project? + if (project.blank? || !can_read_project?) && !can_create_project_in_namespace? raise NotFoundError, ERROR_MESSAGES[:project_not_found] end end @@ -140,11 +143,19 @@ module Gitlab end def check_repository_existence! - unless project.repository.exists? + if (project.blank? || !project.repository.exists?) && !can_create_project_in_namespace? raise UnauthorizedError, ERROR_MESSAGES[:no_repo] end end + def check_repository_creation! + return unless target_namespace + + unless can_create_project_in_namespace? + raise UnauthorizedError, ERROR_MESSAGES[:create] + end + end + def check_download_access! return if deploy_key? @@ -158,6 +169,8 @@ module Gitlab end def check_push_access!(changes) + return if can_create_project_in_namespace? + if project.repository_read_only? raise UnauthorizedError, ERROR_MESSAGES[:read_only] end @@ -234,6 +247,12 @@ module Gitlab end || Guest.can?(:read_project, project) end + def can_create_project_in_namespace? + return unless target_namespace + + actor.can?(:create_projects, target_namespace) + end + def http? protocol == 'http' end From 35882e681b681f68a818bda9a8d2624edfecc219 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Thu, 18 Jan 2018 16:07:07 +0000 Subject: [PATCH 2/9] Adds option to push over SSH to create a new project --- app/controllers/projects/git_http_controller.rb | 10 +++++----- lib/api/internal.rb | 15 ++++++++++++++- lib/gitlab/git_access.rb | 2 +- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 45a1a5cf0de..97c0f5b8c87 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -12,7 +12,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController log_user_activity if upload_pack? if project.blank? && params[:service] == 'git-receive-pack' - @project = ::Projects::CreateService.new(access_actor, project_params).execute + @project = ::Projects::CreateService.new(user, project_params).execute return render_ok if @project.saved? end @@ -34,10 +34,10 @@ class Projects::GitHttpController < Projects::GitHttpClientController def project_params { - description: "", - path: params[:project_id].gsub("\.git", ''), - namespace_id: namespace.id.to_s, - visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s + description: "", + path: params[:project_id].gsub("\.git", ''), + namespace_id: namespace.id.to_s, + visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s } end diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 063f0d6599c..a83f714a1f3 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -43,7 +43,7 @@ module API access_checker_klass = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess access_checker = access_checker_klass - .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities, redirected_path: redirected_path) + .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities, redirected_path: redirected_path, target_namespace: user.namespace) begin access_checker.check(params[:action], params[:changes]) @@ -51,6 +51,19 @@ module API return { status: false, message: e.message } end + if project.blank? && params[:action] == 'git-receive-pack' + project_params = { + description: "", + path: params[:project].split('/').last.gsub("\.git", ''), + namespace_id: user.namespace.id.to_s, + visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s + } + + @project = ::Projects::CreateService.new(user, project_params).execute + + return { status: false, message: "Could not create project" } unless @project.saved? + end + log_user_activity(actor) { diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 9427a5e4baa..7de8a99f9dc 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -250,7 +250,7 @@ module Gitlab def can_create_project_in_namespace? return unless target_namespace - actor.can?(:create_projects, target_namespace) + user.can?(:create_projects, target_namespace) end def http? From 32b2ff26011a5274bdb8a3dd41ad360a67c3148a Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Fri, 19 Jan 2018 13:04:14 +0000 Subject: [PATCH 3/9] Adds remote messsage when project is created in a push over SSH or HTTP --- .../projects/git_http_controller.rb | 38 +++++++----- app/models/project.rb | 4 ++ lib/api/helpers/internal_helpers.rb | 16 +++++ lib/api/internal.rb | 17 ++++-- lib/gitlab/checks/new_project.rb | 60 +++++++++++++++++++ lib/gitlab/git_access.rb | 35 ++++++----- lib/gitlab/git_access_wiki.rb | 4 ++ spec/lib/gitlab/git_access_spec.rb | 18 +++--- 8 files changed, 149 insertions(+), 43 deletions(-) create mode 100644 lib/gitlab/checks/new_project.rb diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 97c0f5b8c87..6010423c243 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -11,10 +11,14 @@ class Projects::GitHttpController < Projects::GitHttpClientController def info_refs log_user_activity if upload_pack? - if project.blank? && params[:service] == 'git-receive-pack' + if user && project.blank? && receive_pack? @project = ::Projects::CreateService.new(user, project_params).execute - return render_ok if @project.saved? + if @project.saved? + Gitlab::Checks::NewProject.new(user, @project, 'http').add_new_project_message + else + raise Gitlab::GitAccess::NotFoundError, 'Could not create project' + end end render_ok @@ -32,15 +36,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController private - def project_params - { - description: "", - path: params[:project_id].gsub("\.git", ''), - namespace_id: namespace.id.to_s, - visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s - } - end - def download_request? upload_pack? end @@ -49,6 +44,10 @@ class Projects::GitHttpController < Projects::GitHttpClientController git_command == 'git-upload-pack' end + def receive_pack? + git_command == 'git-receive-pack' + end + def git_command if action_name == 'info_refs' params[:service] @@ -74,10 +73,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: namespace) end - def namespace - @namespace = Namespace.find_by_path_or_name(params[:namespace_id]) - end - def access_actor return user if user return :ci if ci? @@ -93,6 +88,19 @@ class Projects::GitHttpController < Projects::GitHttpClientController @access_klass ||= wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess end + def project_params + { + description: "", + path: Project.parse_project_id(params[:project_id]), + namespace_id: namespace&.id, + visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s + } + end + + def namespace + @namespace ||= Namespace.find_by_path_or_name(params[:namespace_id]) + end + def log_user_activity Users::ActivityService.new(user, 'pull').execute end diff --git a/app/models/project.rb b/app/models/project.rb index 12d5f28f5ea..a4d83b2a79a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -468,6 +468,10 @@ class Project < ActiveRecord::Base def group_ids joins(:namespace).where(namespaces: { type: 'Group' }).select(:namespace_id) end + + def parse_project_id(project_id) + project_id.gsub("\.git", '') + end end # returns all ancestor-groups upto but excluding the given namespace diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index eb67de81a0d..c0fcae43638 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -29,6 +29,10 @@ module API {} end + def receive_pack? + params[:action] == 'git-receive-pack' + end + def fix_git_env_repository_paths(env, repository_path) if obj_dir_relative = env['GIT_OBJECT_DIRECTORY_RELATIVE'].presence env['GIT_OBJECT_DIRECTORY'] = File.join(repository_path, obj_dir_relative) @@ -62,6 +66,18 @@ module API private + def project_path_regex + @project_regex ||= /\A(?#{Gitlab::PathRegex.full_namespace_route_regex})\/(?#{Gitlab::PathRegex.project_git_route_regex})\z/.freeze + end + + def project_match + @match ||= params[:project].match(project_path_regex).captures + end + + def namespace + @namespace ||= Namespace.find_by_path_or_name(project_match[:namespace_id]) + end + # rubocop:disable Gitlab/ModuleWithInstanceVariables def set_project if params[:gl_repository] diff --git a/lib/api/internal.rb b/lib/api/internal.rb index a83f714a1f3..f641ef457a3 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -43,7 +43,7 @@ module API access_checker_klass = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess access_checker = access_checker_klass - .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities, redirected_path: redirected_path, target_namespace: user.namespace) + .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities, redirected_path: redirected_path, target_namespace: namespace) begin access_checker.check(params[:action], params[:changes]) @@ -51,17 +51,21 @@ module API return { status: false, message: e.message } end - if project.blank? && params[:action] == 'git-receive-pack' + if user && project.blank? && receive_pack? project_params = { description: "", - path: params[:project].split('/').last.gsub("\.git", ''), - namespace_id: user.namespace.id.to_s, + path: Project.parse_project_id(project_match[:project_name]), + namespace_id: namespace&.id, visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s } @project = ::Projects::CreateService.new(user, project_params).execute - return { status: false, message: "Could not create project" } unless @project.saved? + if @project.saved? + Gitlab::Checks::NewProject.new(user, @project, protocol).add_new_project_message + else + return { status: false, message: "Could not create project" } + end end log_user_activity(actor) @@ -221,7 +225,10 @@ module API # key could be used if user redirect_message = Gitlab::Checks::ProjectMoved.fetch_redirect_message(user.id, project.id) + new_project_message = Gitlab::Checks::NewProject.fetch_new_project_message(user.id, project.id) + output[:redirected_message] = redirect_message if redirect_message + output[:new_project_message] = new_project_message if new_project_message end output diff --git a/lib/gitlab/checks/new_project.rb b/lib/gitlab/checks/new_project.rb new file mode 100644 index 00000000000..40d0acefaba --- /dev/null +++ b/lib/gitlab/checks/new_project.rb @@ -0,0 +1,60 @@ +module Gitlab + module Checks + class NewProject + NEW_PROJECT = "new_project".freeze + + def initialize(user, project, protocol) + @user = user + @project = project + @protocol = protocol + end + + def self.fetch_new_project_message(user_id, project_id) + new_project_key = new_project_message_key(user_id, project_id) + + Gitlab::Redis::SharedState.with do |redis| + message = redis.get(new_project_key) + redis.del(new_project_key) + message + end + end + + def add_new_project_message + Gitlab::Redis::SharedState.with do |redis| + key = self.class.new_project_message_key(user.id, project.id) + redis.setex(key, 5.minutes, new_project_message) + end + end + + def new_project_message + <<~MESSAGE.strip_heredoc + + The private project #{project.full_path} was created. + + To configure the remote, run: + git remote add origin #{git_url} + + To view the project, visit: + #{project_url} + + MESSAGE + end + + private + + attr_reader :project, :user, :protocol + + def self.new_project_message_key(user_id, project_id) + "#{NEW_PROJECT}:#{user_id}:#{project_id}" + end + + def project_url + Gitlab::Routing.url_helpers.project_url(project) + end + + def git_url + protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo + end + end + end +end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 7de8a99f9dc..598506aa418 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -13,6 +13,7 @@ module Gitlab 'This deploy key does not have write access to this project.', no_repo: 'A repository for this project does not exist yet.', project_not_found: 'The project you were looking for could not be found.', + namespace_not_found: 'The namespace you were looking for could not be found.', account_blocked: 'Your account has been blocked.', command_not_allowed: "The command you're trying to execute is not allowed.", upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.', @@ -41,18 +42,18 @@ module Gitlab check_protocol! check_valid_actor! check_active_user! - check_project_accessibility! + check_project_accessibility!(cmd) check_project_moved! check_command_disabled!(cmd) check_command_existence!(cmd) - check_repository_existence! - check_repository_creation! + check_repository_existence!(cmd) case cmd when *DOWNLOAD_COMMANDS check_download_access! when *PUSH_COMMANDS - check_push_access!(changes) + check_push_access!(cmd, changes) + check_repository_creation!(cmd) end true @@ -98,8 +99,8 @@ module Gitlab end end - def check_project_accessibility! - if (project.blank? || !can_read_project?) && !can_create_project_in_namespace? + def check_project_accessibility!(cmd) + if (project.blank? || !can_read_project?) && !can_create_project_in_namespace?(cmd) raise NotFoundError, ERROR_MESSAGES[:project_not_found] end end @@ -142,16 +143,20 @@ module Gitlab end end - def check_repository_existence! - if (project.blank? || !project.repository.exists?) && !can_create_project_in_namespace? + def check_repository_existence!(cmd) + if (project.blank? || !project.repository.exists?) && !can_create_project_in_namespace?(cmd) raise UnauthorizedError, ERROR_MESSAGES[:no_repo] end end - def check_repository_creation! - return unless target_namespace + def check_repository_creation!(cmd) + return unless project.blank? - unless can_create_project_in_namespace? + unless target_namespace + raise NotFoundError, ERROR_MESSAGES[:namespace_not_found] + end + + unless can_create_project_in_namespace?(cmd) raise UnauthorizedError, ERROR_MESSAGES[:create] end end @@ -168,8 +173,8 @@ module Gitlab end end - def check_push_access!(changes) - return if can_create_project_in_namespace? + def check_push_access!(cmd, changes) + return if project.blank? && can_create_project_in_namespace?(cmd) if project.repository_read_only? raise UnauthorizedError, ERROR_MESSAGES[:read_only] @@ -247,8 +252,8 @@ module Gitlab end || Guest.can?(:read_project, project) end - def can_create_project_in_namespace? - return unless target_namespace + def can_create_project_in_namespace?(cmd) + return false unless PUSH_COMMANDS.include?(cmd) && target_namespace user.can?(:create_projects, target_namespace) end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index 1c9477e84b2..f679b5e8ed6 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -25,6 +25,10 @@ module Gitlab true end + def check_repository_creation!(cmd) + # Method not used in wiki + end + def push_to_read_only_message ERROR_MESSAGES[:read_only] end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 2009a8ac48c..457e219c1a5 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -501,15 +501,17 @@ describe Gitlab::GitAccess do end aggregate_failures do - matrix.each do |action, allowed| - check = -> { access.send(:check_push_access!, changes[action]) } + Gitlab::GitAccess::ALL_COMMANDS.each do |cmd| + matrix.each do |action, allowed| + check = -> { access.send(:check_push_access!, cmd, changes[action]) } - if allowed - expect(&check).not_to raise_error, - -> { "expected #{action} to be allowed" } - else - expect(&check).to raise_error(Gitlab::GitAccess::UnauthorizedError), - -> { "expected #{action} to be disallowed" } + if allowed + expect(&check).not_to raise_error, + -> { "expected #{action} to be allowed" } + else + expect(&check).to raise_error(Gitlab::GitAccess::UnauthorizedError), + -> { "expected #{action} to be disallowed" } + end end end end From bc78ae6985ee37f9ac2ffc2dbf6f445078d16038 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Mon, 22 Jan 2018 18:10:56 +0000 Subject: [PATCH 4/9] Add specs --- .../projects/git_http_controller.rb | 2 + .../26388-push-to-create-a-new-project.yml | 5 ++ lib/api/helpers/internal_helpers.rb | 17 +++- lib/api/internal.rb | 9 +-- lib/gitlab/checks/new_project.rb | 2 + lib/gitlab/git_access.rb | 11 +-- spec/lib/gitlab/checks/new_project_spec.rb | 46 +++++++++++ spec/lib/gitlab/checks/project_moved_spec.rb | 18 ++--- spec/lib/gitlab/git_access_spec.rb | 79 +++++++++++++++++++ spec/models/project_spec.rb | 6 ++ spec/requests/api/internal_spec.rb | 36 ++++++--- spec/requests/git_http_spec.rb | 38 +++++++-- 12 files changed, 224 insertions(+), 45 deletions(-) create mode 100644 changelogs/unreleased/26388-push-to-create-a-new-project.yml create mode 100644 spec/lib/gitlab/checks/new_project_spec.rb diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 6010423c243..5660a9027d5 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -31,6 +31,8 @@ class Projects::GitHttpController < Projects::GitHttpClientController # POST /foo/bar.git/git-receive-pack" (git push) def git_receive_pack + raise Gitlab::GitAccess::NotFoundError, 'Could not create project' unless project + render_ok end diff --git a/changelogs/unreleased/26388-push-to-create-a-new-project.yml b/changelogs/unreleased/26388-push-to-create-a-new-project.yml new file mode 100644 index 00000000000..f641fcced37 --- /dev/null +++ b/changelogs/unreleased/26388-push-to-create-a-new-project.yml @@ -0,0 +1,5 @@ +--- +title: User can now git push to create a new project +merge_request: 16547 +author: +type: added diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index c0fcae43638..fd568c5ef30 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -64,6 +64,15 @@ module API false end + def project_params + { + description: "", + path: Project.parse_project_id(project_match[:project_id]), + namespace_id: project_namespace&.id, + visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s + } + end + private def project_path_regex @@ -71,11 +80,13 @@ module API end def project_match - @match ||= params[:project].match(project_path_regex).captures + @project_match ||= params[:project].match(project_path_regex) end - def namespace - @namespace ||= Namespace.find_by_path_or_name(project_match[:namespace_id]) + def project_namespace + return unless project_match + + @project_namespace ||= Namespace.find_by_path_or_name(project_match[:namespace_id]) end # rubocop:disable Gitlab/ModuleWithInstanceVariables diff --git a/lib/api/internal.rb b/lib/api/internal.rb index f641ef457a3..b7475af2044 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -43,7 +43,7 @@ module API access_checker_klass = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess access_checker = access_checker_klass - .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities, redirected_path: redirected_path, target_namespace: namespace) + .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities, redirected_path: redirected_path, target_namespace: project_namespace) begin access_checker.check(params[:action], params[:changes]) @@ -52,13 +52,6 @@ module API end if user && project.blank? && receive_pack? - project_params = { - description: "", - path: Project.parse_project_id(project_match[:project_name]), - namespace_id: namespace&.id, - visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s - } - @project = ::Projects::CreateService.new(user, project_params).execute if @project.saved? diff --git a/lib/gitlab/checks/new_project.rb b/lib/gitlab/checks/new_project.rb index 40d0acefaba..488c5c03c32 100644 --- a/lib/gitlab/checks/new_project.rb +++ b/lib/gitlab/checks/new_project.rb @@ -20,6 +20,8 @@ module Gitlab end def add_new_project_message + return unless user.present? && project.present? + Gitlab::Redis::SharedState.with do |redis| key = self.class.new_project_message_key(user.id, project.id) redis.setex(key, 5.minutes, new_project_message) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 598506aa418..38649a4fdef 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -19,8 +19,7 @@ module Gitlab upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.', receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.', read_only: 'The repository is temporarily read-only. Please try again later.', - cannot_push_to_read_only: "You can't push code to a read-only GitLab instance.", - create: "Creating a repository to that namespace is not allowed." + cannot_push_to_read_only: "You can't push code to a read-only GitLab instance." }.freeze DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }.freeze @@ -53,7 +52,7 @@ module Gitlab check_download_access! when *PUSH_COMMANDS check_push_access!(cmd, changes) - check_repository_creation!(cmd) + check_namespace_accessibility!(cmd) end true @@ -149,16 +148,12 @@ module Gitlab end end - def check_repository_creation!(cmd) + def check_namespace_accessibility!(cmd) return unless project.blank? unless target_namespace raise NotFoundError, ERROR_MESSAGES[:namespace_not_found] end - - unless can_create_project_in_namespace?(cmd) - raise UnauthorizedError, ERROR_MESSAGES[:create] - end end def check_download_access! diff --git a/spec/lib/gitlab/checks/new_project_spec.rb b/spec/lib/gitlab/checks/new_project_spec.rb new file mode 100644 index 00000000000..c696e02e41a --- /dev/null +++ b/spec/lib/gitlab/checks/new_project_spec.rb @@ -0,0 +1,46 @@ +require 'rails_helper' + +describe Gitlab::Checks::NewProject, :clean_gitlab_redis_shared_state do + let(:user) { create(:user) } + let(:project) { create(:project) } + + describe '.fetch_new_project_message' do + context 'with a new project message queue' do + let(:new_project) { described_class.new(user, project, 'http') } + + before do + new_project.add_new_project_message + end + + it 'returns new project message' do + expect(described_class.fetch_new_project_message(user.id, project.id)).to eq(new_project.new_project_message) + end + + it 'deletes the new project message from redis' do + expect(Gitlab::Redis::SharedState.with { |redis| redis.get("new_project:#{user.id}:#{project.id}") }).not_to be_nil + described_class.fetch_new_project_message(user.id, project.id) + expect(Gitlab::Redis::SharedState.with { |redis| redis.get("new_project:#{user.id}:#{project.id}") }).to be_nil + end + end + + context 'with no new project message queue' do + it 'returns nil' do + expect(described_class.fetch_new_project_message(1, 2)).to be_nil + end + end + end + + describe '#add_new_project_message' do + it 'queues a new project message' do + new_project = described_class.new(user, project, 'http') + + expect(new_project.add_new_project_message).to eq('OK') + end + + it 'handles anonymous push' do + new_project = described_class.new(user, nil, 'http') + + expect(new_project.add_new_project_message).to be_nil + end + end +end diff --git a/spec/lib/gitlab/checks/project_moved_spec.rb b/spec/lib/gitlab/checks/project_moved_spec.rb index f90c2d6aded..b03a598edd8 100644 --- a/spec/lib/gitlab/checks/project_moved_spec.rb +++ b/spec/lib/gitlab/checks/project_moved_spec.rb @@ -6,14 +6,14 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do describe '.fetch_redirct_message' do context 'with a redirect message queue' do - it 'should return the redirect message' do + it 'returns the redirect message' do project_moved = described_class.new(project, user, 'foo/bar', 'http') project_moved.add_redirect_message expect(described_class.fetch_redirect_message(user.id, project.id)).to eq(project_moved.redirect_message) end - it 'should delete the redirect message from redis' do + it 'deletes the redirect message from redis' do project_moved = described_class.new(project, user, 'foo/bar', 'http') project_moved.add_redirect_message @@ -24,19 +24,19 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do end context 'with no redirect message queue' do - it 'should return nil' do + it 'returns nil' do expect(described_class.fetch_redirect_message(1, 2)).to be_nil end end end describe '#add_redirect_message' do - it 'should queue a redirect message' do + it 'queues a redirect message' do project_moved = described_class.new(project, user, 'foo/bar', 'http') expect(project_moved.add_redirect_message).to eq("OK") end - it 'should handle anonymous clones' do + it 'handles anonymous clones' do project_moved = described_class.new(project, nil, 'foo/bar', 'http') expect(project_moved.add_redirect_message).to eq(nil) @@ -45,7 +45,7 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do describe '#redirect_message' do context 'when the push is rejected' do - it 'should return a redirect message telling the user to try again' do + it 'returns a redirect message telling the user to try again' do project_moved = described_class.new(project, user, 'foo/bar', 'http') message = "Project 'foo/bar' was moved to '#{project.full_path}'." + "\n\nPlease update your Git remote:" + @@ -56,7 +56,7 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do end context 'when the push is not rejected' do - it 'should return a redirect message' do + it 'returns a redirect message' do project_moved = described_class.new(project, user, 'foo/bar', 'http') message = "Project 'foo/bar' was moved to '#{project.full_path}'." + "\n\nPlease update your Git remote:" + @@ -69,7 +69,7 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do describe '#permanent_redirect?' do context 'with a permanent RedirectRoute' do - it 'should return true' do + it 'returns true' do project.route.create_redirect('foo/bar', permanent: true) project_moved = described_class.new(project, user, 'foo/bar', 'http') expect(project_moved.permanent_redirect?).to be_truthy @@ -77,7 +77,7 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do end context 'without a permanent RedirectRoute' do - it 'should return false' do + it 'returns false' do project.route.create_redirect('foo/bar') project_moved = described_class.new(project, user, 'foo/bar', 'http') expect(project_moved.permanent_redirect?).to be_falsy diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 457e219c1a5..3c98c95e301 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -152,6 +152,30 @@ describe Gitlab::GitAccess do expect { push_access_check }.to raise_not_found end end + + context 'when user is allowed to create project in namespace' do + let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: user.namespace) } + + it 'blocks pull access with "not found"' do + expect { pull_access_check }.to raise_not_found + end + + it 'allows push access' do + expect { push_access_check }.not_to raise_error + end + end + + context 'when user is not allowed to create project in namespace' do + let(:user2) { create(:user) } + let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: user2.namespace) } + + it 'blocks push and pull with "not found"' do + aggregate_failures do + expect { pull_access_check }.to raise_not_found + expect { push_access_check }.to raise_not_found + end + end + end end end @@ -311,6 +335,51 @@ describe Gitlab::GitAccess do end end + describe '#check_namespace_accessibility!' do + context 'when project exists' do + context 'when user can pull or push' do + before do + project.add_master(user) + end + + it 'does not block pull or push' do + aggregate_failures do + expect { push_access_check }.not_to raise_error + expect { pull_access_check }.not_to raise_error + end + end + end + end + + context 'when project does not exist' do + context 'when namespace does not exist' do + let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: nil) } + + it 'blocks push and pull' do + aggregate_failures do + expect { push_access_check }.not_to raise_namespace_not_found + expect { pull_access_check }.not_to raise_namespace_not_found + end + end + end + + context 'when namespace exists' do + context 'when user is unable to push to namespace' do + let(:user2) { create(:user) } + let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: user2.namespace) } + + it 'blocks push' do + expect { push_access_check }.to raise_project_create + end + + it 'does not block pull' do + expect { push_access_check }.to raise_error + end + end + end + end + end + describe '#check_download_access!' do it 'allows masters to pull' do project.add_master(user) @@ -773,6 +842,16 @@ describe Gitlab::GitAccess do Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found]) end + def raise_namespace_not_found + raise_error(Gitlab::GitAccess::NotFoundError, + Gitlab::GitAccess::ERROR_MESSAGES[:namespace_not_found]) + end + + def raise_project_create + raise_error(Gitlab::GitAccess::NotFoundError, + Gitlab::GitAccess::ERROR_MESSAGES[:create]) + end + def build_authentication_abilities [ :read_project, diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index da940571bc1..a502a798368 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1079,6 +1079,12 @@ describe Project do end end + describe '.parse_project_id' do + it 'removes .git from the project_id' do + expect(described_class.parse_project_id('new-project.git')).to eq('new-project') + end + end + context 'repository storage by default' do let(:project) { create(:project) } diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 884a258fd12..5f6790312e4 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -366,25 +366,28 @@ describe API::Internal do end end - context 'project as /namespace/project' do + context 'project as namespace/project' do it do - pull(key, project_with_repo_path('/' + project.full_path)) + push(key, project_with_repo_path(project.full_path)) expect(response).to have_gitlab_http_status(200) expect(json_response["status"]).to be_truthy expect(json_response["repository_path"]).to eq(project.repository.path_to_repo) expect(json_response["gl_repository"]).to eq("project-#{project.id}") end - end - context 'project as namespace/project' do - it do - pull(key, project_with_repo_path(project.full_path)) + context 'when project does not exist' do + it 'creates a new project' do + path = "#{user.namespace.path}/notexist.git" - expect(response).to have_gitlab_http_status(200) - expect(json_response["status"]).to be_truthy - expect(json_response["repository_path"]).to eq(project.repository.path_to_repo) - expect(json_response["gl_repository"]).to eq("project-#{project.id}") + expect do + push_with_path(key, path) + end.to change { Project.count }.by(1) + + expect(response).to have_gitlab_http_status(200) + expect(json_response["status"]).to be_truthy + expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(path) + end end end end @@ -818,6 +821,19 @@ describe API::Internal do end end + context 'with new project data' do + it 'returns new project message on the response' do + new_project = Gitlab::Checks::NewProject.new(user, project, 'http') + new_project.add_new_project_message + + post api("/internal/post_receive"), valid_params + + expect(response).to have_gitlab_http_status(200) + expect(json_response["new_project_message"]).to be_present + expect(json_response["new_project_message"]).to eq(new_project.new_project_message) + end + end + context 'with an orphaned write deploy key' do it 'does not try to notify that project moved' do allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(nil) diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 27bd22d6bca..c73db257b05 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -107,15 +107,39 @@ describe 'Git HTTP requests' do let(:user) { create(:user) } context "when the project doesn't exist" do - let(:path) { 'doesnt/exist.git' } + context "when namespace doesn't exist" do + let(:path) { 'doesnt/exist.git' } - it_behaves_like 'pulls require Basic HTTP Authentication' - it_behaves_like 'pushes require Basic HTTP Authentication' + it_behaves_like 'pulls require Basic HTTP Authentication' + it_behaves_like 'pushes require Basic HTTP Authentication' - context 'when authenticated' do - it 'rejects downloads and uploads with 404 Not Found' do - download_or_upload(path, user: user.username, password: user.password) do |response| - expect(response).to have_gitlab_http_status(:not_found) + context 'when authenticated' do + it 'rejects downloads and uploads with 404 Not Found' do + download_or_upload(path, user: user.username, password: user.password) do |response| + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + end + + context 'when namespace exists' do + let(:path) { "#{user.namespace.path}/new-project.git"} + + context 'when authenticated' do + it 'creates a new project under the existing namespace' do + expect do + upload(path, user: user.username, password: user.password) do |response| + expect(response).to have_gitlab_http_status(:ok) + end + end.to change { user.projects.count }.by(1) + end + + it 'rejects upload with 404 Not Found when project is invalid' do + path = "#{user.namespace.path}/new.git" + + upload(path, user: user.username, password: user.password) do |response| + expect(response).to have_gitlab_http_status(:not_found) + end end end end From e42a548f1dac02577d0c1731fef508dab68c45a5 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Thu, 25 Jan 2018 12:26:52 +0000 Subject: [PATCH 5/9] Move new project on push logic to a service --- .../projects/git_http_controller.rb | 33 +++++-------- app/models/project.rb | 4 -- .../projects/create_from_push_service.rb | 37 +++++++++++++++ lib/api/helpers/internal_helpers.rb | 29 +++--------- lib/api/internal.rb | 16 +++---- .../{new_project.rb => project_created.rb} | 24 +++++----- lib/gitlab/git_access.rb | 15 ++++-- lib/gitlab/git_access_wiki.rb | 4 -- lib/gitlab/path_regex.rb | 4 ++ spec/lib/gitlab/checks/new_project_spec.rb | 46 ------------------- .../lib/gitlab/checks/project_created_spec.rb | 46 +++++++++++++++++++ spec/lib/gitlab/git_access_spec.rb | 13 ++---- spec/models/project_spec.rb | 6 --- spec/requests/api/internal_spec.rb | 19 ++++++-- .../projects/create_from_push_service_spec.rb | 34 ++++++++++++++ 15 files changed, 186 insertions(+), 144 deletions(-) create mode 100644 app/services/projects/create_from_push_service.rb rename lib/gitlab/checks/{new_project.rb => project_created.rb} (59%) delete mode 100644 spec/lib/gitlab/checks/new_project_spec.rb create mode 100644 spec/lib/gitlab/checks/project_created_spec.rb create mode 100644 spec/services/projects/create_from_push_service_spec.rb diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 5660a9027d5..90a9079fab3 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -5,21 +5,13 @@ class Projects::GitHttpController < Projects::GitHttpClientController rescue_from Gitlab::GitAccess::UnauthorizedError, with: :render_403 rescue_from Gitlab::GitAccess::NotFoundError, with: :render_404 + rescue_from Gitlab::GitAccess::ProjectCreationError, with: :render_422 # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull) # GET /foo/bar.git/info/refs?service=git-receive-pack (git push) def info_refs log_user_activity if upload_pack? - - if user && project.blank? && receive_pack? - @project = ::Projects::CreateService.new(user, project_params).execute - - if @project.saved? - Gitlab::Checks::NewProject.new(user, @project, 'http').add_new_project_message - else - raise Gitlab::GitAccess::NotFoundError, 'Could not create project' - end - end + create_new_project if receive_pack? && project.blank? render_ok end @@ -31,8 +23,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController # POST /foo/bar.git/git-receive-pack" (git push) def git_receive_pack - raise Gitlab::GitAccess::NotFoundError, 'Could not create project' unless project - render_ok end @@ -58,6 +48,10 @@ class Projects::GitHttpController < Projects::GitHttpClientController end end + def create_new_project + @project = ::Projects::CreateFromPushService.new(user, params[:project_id], namespace, 'http').execute + end + def render_ok set_workhorse_internal_api_content_type render json: Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name) @@ -71,6 +65,10 @@ class Projects::GitHttpController < Projects::GitHttpClientController render plain: exception.message, status: :not_found end + def render_422(exception) + render plain: exception.message, status: :unprocessable_entity + end + def access @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: namespace) end @@ -90,17 +88,8 @@ class Projects::GitHttpController < Projects::GitHttpClientController @access_klass ||= wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess end - def project_params - { - description: "", - path: Project.parse_project_id(params[:project_id]), - namespace_id: namespace&.id, - visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s - } - end - def namespace - @namespace ||= Namespace.find_by_path_or_name(params[:namespace_id]) + @namespace ||= Namespace.find_by_full_path(params[:namespace_id]) end def log_user_activity diff --git a/app/models/project.rb b/app/models/project.rb index a4d83b2a79a..12d5f28f5ea 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -468,10 +468,6 @@ class Project < ActiveRecord::Base def group_ids joins(:namespace).where(namespaces: { type: 'Group' }).select(:namespace_id) end - - def parse_project_id(project_id) - project_id.gsub("\.git", '') - end end # returns all ancestor-groups upto but excluding the given namespace diff --git a/app/services/projects/create_from_push_service.rb b/app/services/projects/create_from_push_service.rb new file mode 100644 index 00000000000..58b3e091438 --- /dev/null +++ b/app/services/projects/create_from_push_service.rb @@ -0,0 +1,37 @@ +module Projects + class CreateFromPushService < BaseService + attr_reader :user, :project_path, :namespace, :protocol + + def initialize(user, project_path, namespace, protocol) + @user = user + @project_path = project_path + @namespace = namespace + @protocol = protocol + end + + def execute + return unless user + + project = Projects::CreateService.new(user, project_params).execute + + if project.saved? + Gitlab::Checks::ProjectCreated.new(user, project, protocol).add_project_created_message + else + raise Gitlab::GitAccess::ProjectCreationError, "Could not create project: #{project.errors.full_messages.join(', ')}" + end + + project + end + + private + + def project_params + { + description: "", + path: project_path.gsub(/\.git$/, ''), + namespace_id: namespace&.id, + visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s + } + end + end +end diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index fd568c5ef30..2340e962918 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -29,10 +29,6 @@ module API {} end - def receive_pack? - params[:action] == 'git-receive-pack' - end - def fix_git_env_repository_paths(env, repository_path) if obj_dir_relative = env['GIT_OBJECT_DIRECTORY_RELATIVE'].presence env['GIT_OBJECT_DIRECTORY'] = File.join(repository_path, obj_dir_relative) @@ -51,6 +47,10 @@ module API ::Users::ActivityService.new(actor, 'Git SSH').execute if commands.include?(params[:action]) end + def receive_pack? + params[:action] == 'git-receive-pack' + end + def merge_request_urls ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) end @@ -64,29 +64,14 @@ module API false end - def project_params - { - description: "", - path: Project.parse_project_id(project_match[:project_id]), - namespace_id: project_namespace&.id, - visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s - } + def project_namespace + @project_namespace ||= project&.namespace || Namespace.find_by_full_path(project_match[:namespace_path]) end private - def project_path_regex - @project_regex ||= /\A(?#{Gitlab::PathRegex.full_namespace_route_regex})\/(?#{Gitlab::PathRegex.project_git_route_regex})\z/.freeze - end - def project_match - @project_match ||= params[:project].match(project_path_regex) - end - - def project_namespace - return unless project_match - - @project_namespace ||= Namespace.find_by_path_or_name(project_match[:namespace_id]) + @project_match ||= params[:project].match(Gitlab::PathRegex.full_project_git_path_regex) end # rubocop:disable Gitlab/ModuleWithInstanceVariables diff --git a/lib/api/internal.rb b/lib/api/internal.rb index b7475af2044..841a34eb67f 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -51,13 +51,11 @@ module API return { status: false, message: e.message } end - if user && project.blank? && receive_pack? - @project = ::Projects::CreateService.new(user, project_params).execute - - if @project.saved? - Gitlab::Checks::NewProject.new(user, @project, protocol).add_new_project_message - else - return { status: false, message: "Could not create project" } + if receive_pack? && project.blank? + begin + @project = ::Projects::CreateFromPushService.new(user, project_match[:project_path], project_namespace, protocol).execute + rescue Gitlab::GitAccess::ProjectCreationError => e + return { status: false, message: e.message } end end @@ -218,10 +216,10 @@ module API # key could be used if user redirect_message = Gitlab::Checks::ProjectMoved.fetch_redirect_message(user.id, project.id) - new_project_message = Gitlab::Checks::NewProject.fetch_new_project_message(user.id, project.id) + project_created_message = Gitlab::Checks::ProjectCreated.fetch_project_created_message(user.id, project.id) output[:redirected_message] = redirect_message if redirect_message - output[:new_project_message] = new_project_message if new_project_message + output[:project_created_message] = project_created_message if project_created_message end output diff --git a/lib/gitlab/checks/new_project.rb b/lib/gitlab/checks/project_created.rb similarity index 59% rename from lib/gitlab/checks/new_project.rb rename to lib/gitlab/checks/project_created.rb index 488c5c03c32..f05e8b4a7e8 100644 --- a/lib/gitlab/checks/new_project.rb +++ b/lib/gitlab/checks/project_created.rb @@ -1,7 +1,7 @@ module Gitlab module Checks - class NewProject - NEW_PROJECT = "new_project".freeze + class ProjectCreated + PROJECT_CREATED = "project_created".freeze def initialize(user, project, protocol) @user = user @@ -9,26 +9,26 @@ module Gitlab @protocol = protocol end - def self.fetch_new_project_message(user_id, project_id) - new_project_key = new_project_message_key(user_id, project_id) + def self.fetch_project_created_message(user_id, project_id) + project_created_key = project_created_message_key(user_id, project_id) Gitlab::Redis::SharedState.with do |redis| - message = redis.get(new_project_key) - redis.del(new_project_key) + message = redis.get(project_created_key) + redis.del(project_created_key) message end end - def add_new_project_message + def add_project_created_message return unless user.present? && project.present? Gitlab::Redis::SharedState.with do |redis| - key = self.class.new_project_message_key(user.id, project.id) - redis.setex(key, 5.minutes, new_project_message) + key = self.class.project_created_message_key(user.id, project.id) + redis.setex(key, 5.minutes, project_created_message) end end - def new_project_message + def project_created_message <<~MESSAGE.strip_heredoc The private project #{project.full_path} was created. @@ -46,8 +46,8 @@ module Gitlab attr_reader :project, :user, :protocol - def self.new_project_message_key(user_id, project_id) - "#{NEW_PROJECT}:#{user_id}:#{project_id}" + def self.project_created_message_key(user_id, project_id) + "#{PROJECT_CREATED}:#{user_id}:#{project_id}" end def project_url diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 38649a4fdef..32a2395a26b 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -4,6 +4,7 @@ module Gitlab class GitAccess UnauthorizedError = Class.new(StandardError) NotFoundError = Class.new(StandardError) + ProjectCreationError = Class.new(StandardError) ProjectMovedError = Class.new(NotFoundError) ERROR_MESSAGES = { @@ -13,13 +14,13 @@ module Gitlab 'This deploy key does not have write access to this project.', no_repo: 'A repository for this project does not exist yet.', project_not_found: 'The project you were looking for could not be found.', - namespace_not_found: 'The namespace you were looking for could not be found.', account_blocked: 'Your account has been blocked.', command_not_allowed: "The command you're trying to execute is not allowed.", upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.', receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.', read_only: 'The repository is temporarily read-only. Please try again later.', - cannot_push_to_read_only: "You can't push code to a read-only GitLab instance." + cannot_push_to_read_only: "You can't push code to a read-only GitLab instance.", + namespace_not_found: 'The namespace you were looking for could not be found.' }.freeze DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }.freeze @@ -52,7 +53,7 @@ module Gitlab check_download_access! when *PUSH_COMMANDS check_push_access!(cmd, changes) - check_namespace_accessibility!(cmd) + check_namespace_accessibility! end true @@ -148,7 +149,7 @@ module Gitlab end end - def check_namespace_accessibility!(cmd) + def check_namespace_accessibility! return unless project.blank? unless target_namespace @@ -248,7 +249,7 @@ module Gitlab end def can_create_project_in_namespace?(cmd) - return false unless PUSH_COMMANDS.include?(cmd) && target_namespace + return false unless push?(cmd) && target_namespace user.can?(:create_projects, target_namespace) end @@ -265,6 +266,10 @@ module Gitlab command == 'git-receive-pack' end + def push?(cmd) + PUSH_COMMANDS.include?(cmd) + end + def upload_pack_disabled_over_http? !Gitlab.config.gitlab_shell.upload_pack end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index f679b5e8ed6..1c9477e84b2 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -25,10 +25,6 @@ module Gitlab true end - def check_repository_creation!(cmd) - # Method not used in wiki - end - def push_to_read_only_message ERROR_MESSAGES[:read_only] end diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 7e5dfd33502..4a2db11a978 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -187,6 +187,10 @@ module Gitlab @full_project_path_regex ||= %r{\A#{full_namespace_route_regex}/#{project_route_regex}/\z} end + def full_project_git_path_regex + @full_project_git_path_regex ||= /\A(\/|)(?#{full_namespace_route_regex})\/(?#{project_git_route_regex})\z/.freeze + end + def full_namespace_format_regex @namespace_format_regex ||= /A#{FULL_NAMESPACE_FORMAT_REGEX}\z/.freeze end diff --git a/spec/lib/gitlab/checks/new_project_spec.rb b/spec/lib/gitlab/checks/new_project_spec.rb deleted file mode 100644 index c696e02e41a..00000000000 --- a/spec/lib/gitlab/checks/new_project_spec.rb +++ /dev/null @@ -1,46 +0,0 @@ -require 'rails_helper' - -describe Gitlab::Checks::NewProject, :clean_gitlab_redis_shared_state do - let(:user) { create(:user) } - let(:project) { create(:project) } - - describe '.fetch_new_project_message' do - context 'with a new project message queue' do - let(:new_project) { described_class.new(user, project, 'http') } - - before do - new_project.add_new_project_message - end - - it 'returns new project message' do - expect(described_class.fetch_new_project_message(user.id, project.id)).to eq(new_project.new_project_message) - end - - it 'deletes the new project message from redis' do - expect(Gitlab::Redis::SharedState.with { |redis| redis.get("new_project:#{user.id}:#{project.id}") }).not_to be_nil - described_class.fetch_new_project_message(user.id, project.id) - expect(Gitlab::Redis::SharedState.with { |redis| redis.get("new_project:#{user.id}:#{project.id}") }).to be_nil - end - end - - context 'with no new project message queue' do - it 'returns nil' do - expect(described_class.fetch_new_project_message(1, 2)).to be_nil - end - end - end - - describe '#add_new_project_message' do - it 'queues a new project message' do - new_project = described_class.new(user, project, 'http') - - expect(new_project.add_new_project_message).to eq('OK') - end - - it 'handles anonymous push' do - new_project = described_class.new(user, nil, 'http') - - expect(new_project.add_new_project_message).to be_nil - end - end -end diff --git a/spec/lib/gitlab/checks/project_created_spec.rb b/spec/lib/gitlab/checks/project_created_spec.rb new file mode 100644 index 00000000000..c79798bc84d --- /dev/null +++ b/spec/lib/gitlab/checks/project_created_spec.rb @@ -0,0 +1,46 @@ +require 'rails_helper' + +describe Gitlab::Checks::ProjectCreated, :clean_gitlab_redis_shared_state do + let(:user) { create(:user) } + let(:project) { create(:project) } + + describe '.fetch_project_created_message' do + context 'with a project created message queue' do + let(:project_created) { described_class.new(user, project, 'http') } + + before do + project_created.add_project_created_message + end + + it 'returns project created message' do + expect(described_class.fetch_project_created_message(user.id, project.id)).to eq(project_created.project_created_message) + end + + it 'deletes the project created message from redis' do + expect(Gitlab::Redis::SharedState.with { |redis| redis.get("project_created:#{user.id}:#{project.id}") }).not_to be_nil + described_class.fetch_project_created_message(user.id, project.id) + expect(Gitlab::Redis::SharedState.with { |redis| redis.get("project_created:#{user.id}:#{project.id}") }).to be_nil + end + end + + context 'with no project created message queue' do + it 'returns nil' do + expect(described_class.fetch_project_created_message(1, 2)).to be_nil + end + end + end + + describe '#add_project_created_message' do + it 'queues a project created message' do + project_created = described_class.new(user, project, 'http') + + expect(project_created.add_project_created_message).to eq('OK') + end + + it 'handles anonymous push' do + project_created = described_class.new(user, nil, 'http') + + expect(project_created.add_project_created_message).to be_nil + end + end +end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 3c98c95e301..e6ad35867c0 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -335,7 +335,7 @@ describe Gitlab::GitAccess do end end - describe '#check_namespace_accessibility!' do + describe '#check_namespace_existence!' do context 'when project exists' do context 'when user can pull or push' do before do @@ -838,18 +838,11 @@ describe Gitlab::GitAccess do end def raise_not_found - raise_error(Gitlab::GitAccess::NotFoundError, - Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found]) + raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found]) end def raise_namespace_not_found - raise_error(Gitlab::GitAccess::NotFoundError, - Gitlab::GitAccess::ERROR_MESSAGES[:namespace_not_found]) - end - - def raise_project_create - raise_error(Gitlab::GitAccess::NotFoundError, - Gitlab::GitAccess::ERROR_MESSAGES[:create]) + raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:namespace_not_found]) end def build_authentication_abilities diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index a502a798368..da940571bc1 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1079,12 +1079,6 @@ describe Project do end end - describe '.parse_project_id' do - it 'removes .git from the project_id' do - expect(described_class.parse_project_id('new-project.git')).to eq('new-project') - end - end - context 'repository storage by default' do let(:project) { create(:project) } diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 5f6790312e4..1e8197653ce 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -366,6 +366,17 @@ describe API::Internal do end end + context 'project as /namespace/project' do + it do + push(key, project_with_repo_path('/' + project.full_path)) + + expect(response).to have_gitlab_http_status(200) + expect(json_response["status"]).to be_truthy + expect(json_response["repository_path"]).to eq(project.repository.path_to_repo) + expect(json_response["gl_repository"]).to eq("project-#{project.id}") + end + end + context 'project as namespace/project' do it do push(key, project_with_repo_path(project.full_path)) @@ -823,14 +834,14 @@ describe API::Internal do context 'with new project data' do it 'returns new project message on the response' do - new_project = Gitlab::Checks::NewProject.new(user, project, 'http') - new_project.add_new_project_message + project_created = Gitlab::Checks::ProjectCreated.new(user, project, 'http') + project_created.add_project_created_message post api("/internal/post_receive"), valid_params expect(response).to have_gitlab_http_status(200) - expect(json_response["new_project_message"]).to be_present - expect(json_response["new_project_message"]).to eq(new_project.new_project_message) + expect(json_response["project_created_message"]).to be_present + expect(json_response["project_created_message"]).to eq(project_created.project_created_message) end end diff --git a/spec/services/projects/create_from_push_service_spec.rb b/spec/services/projects/create_from_push_service_spec.rb new file mode 100644 index 00000000000..8e45697cd85 --- /dev/null +++ b/spec/services/projects/create_from_push_service_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +describe Projects::CreateFromPushService do + let(:user) { create(:user) } + let(:project_path) { "nonexist" } + let(:namespace) { user&.namespace } + let(:protocol) { 'http' } + + subject { described_class.new(user, project_path, namespace, protocol) } + + it 'creates project' do + expect_any_instance_of(Projects::CreateService).to receive(:execute).and_call_original + + expect { subject.execute }.to change { Project.count }.by(1) + end + + it 'raises project creation error when project creation fails' do + allow_any_instance_of(Project).to receive(:saved?).and_return(false) + + expect { subject.execute }.to raise_error(Gitlab::GitAccess::ProjectCreationError) + end + + context 'when user is nil' do + let(:user) { nil } + + subject { described_class.new(user, project_path, namespace, cmd, protocol) } + + it 'returns nil' do + expect_any_instance_of(Projects::CreateService).not_to receive(:execute) + + expect(subject.execute).to be_nil + end + end +end From dc229c076cdc0ef6e7f3f74f6e462c22880ff08c Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Fri, 26 Jan 2018 14:28:08 +0000 Subject: [PATCH 6/9] Abstracts ProjectMoved and ProjectCreated into a BaseProject --- .../projects/create_from_push_service.rb | 2 +- lib/api/helpers/internal_helpers.rb | 8 +++- lib/api/internal.rb | 4 +- lib/gitlab/checks/base_project.rb | 46 +++++++++++++++++++ lib/gitlab/checks/project_created.rb | 39 ++-------------- lib/gitlab/checks/project_moved.rb | 40 ++++------------ lib/gitlab/git_access.rb | 34 +++++--------- lib/gitlab/path_regex.rb | 2 +- .../lib/gitlab/checks/project_created_spec.rb | 22 ++++----- spec/lib/gitlab/checks/project_moved_spec.rb | 40 ++++++++-------- spec/lib/gitlab/git_access_spec.rb | 30 +++--------- spec/requests/api/internal_spec.rb | 24 +++++++--- spec/requests/git_http_spec.rb | 8 ++-- .../projects/create_from_push_service_spec.rb | 2 +- 14 files changed, 142 insertions(+), 159 deletions(-) create mode 100644 lib/gitlab/checks/base_project.rb diff --git a/app/services/projects/create_from_push_service.rb b/app/services/projects/create_from_push_service.rb index 58b3e091438..949a0bfdc09 100644 --- a/app/services/projects/create_from_push_service.rb +++ b/app/services/projects/create_from_push_service.rb @@ -15,7 +15,7 @@ module Projects project = Projects::CreateService.new(user, project_params).execute if project.saved? - Gitlab::Checks::ProjectCreated.new(user, project, protocol).add_project_created_message + Gitlab::Checks::ProjectCreated.new(project, user, protocol).add_message else raise Gitlab::GitAccess::ProjectCreationError, "Could not create project: #{project.errors.full_messages.join(', ')}" end diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 2340e962918..bff245fe9a2 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -1,6 +1,8 @@ module API module Helpers module InternalHelpers + include Gitlab::Utils::StrongMemoize + attr_reader :redirected_path def wiki? @@ -65,13 +67,15 @@ module API end def project_namespace - @project_namespace ||= project&.namespace || Namespace.find_by_full_path(project_match[:namespace_path]) + strong_memoize(:project_namespace) do + project&.namespace || Namespace.find_by_full_path(project_match[:namespace_path]) + end end private def project_match - @project_match ||= params[:project].match(Gitlab::PathRegex.full_project_git_path_regex) + @project_match ||= params[:project].match(Gitlab::PathRegex.full_project_git_path_regex) || {} end # rubocop:disable Gitlab/ModuleWithInstanceVariables diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 841a34eb67f..ed6d022df97 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -215,8 +215,8 @@ module API # A user is not guaranteed to be returned; an orphaned write deploy # key could be used if user - redirect_message = Gitlab::Checks::ProjectMoved.fetch_redirect_message(user.id, project.id) - project_created_message = Gitlab::Checks::ProjectCreated.fetch_project_created_message(user.id, project.id) + redirect_message = Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id) + project_created_message = Gitlab::Checks::ProjectCreated.fetch_message(user.id, project.id) output[:redirected_message] = redirect_message if redirect_message output[:project_created_message] = project_created_message if project_created_message diff --git a/lib/gitlab/checks/base_project.rb b/lib/gitlab/checks/base_project.rb new file mode 100644 index 00000000000..dd6c007b356 --- /dev/null +++ b/lib/gitlab/checks/base_project.rb @@ -0,0 +1,46 @@ +module Gitlab + module Checks + class BaseProject + def initialize(project, user, protocol) + @project = project + @user = user + @protocol = protocol + end + + def self.fetch_message(user_id, project_id) + key = message_key(user_id, project_id) + + Gitlab::Redis::SharedState.with do |redis| + message = redis.get(key) + redis.del(key) + message + end + end + + def add_message + return unless user.present? && project.present? + + Gitlab::Redis::SharedState.with do |redis| + key = self.class.message_key(user.id, project.id) + redis.setex(key, 5.minutes, message) + end + end + + def message + raise NotImplementedError + end + + protected + + attr_reader :project, :user, :protocol + + def self.message_key(user_id, project_id) + raise NotImplementedError + end + + def url_to_repo + protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo + end + end + end +end diff --git a/lib/gitlab/checks/project_created.rb b/lib/gitlab/checks/project_created.rb index f05e8b4a7e8..bd1e204bc81 100644 --- a/lib/gitlab/checks/project_created.rb +++ b/lib/gitlab/checks/project_created.rb @@ -1,40 +1,15 @@ module Gitlab module Checks - class ProjectCreated + class ProjectCreated < BaseProject PROJECT_CREATED = "project_created".freeze - def initialize(user, project, protocol) - @user = user - @project = project - @protocol = protocol - end - - def self.fetch_project_created_message(user_id, project_id) - project_created_key = project_created_message_key(user_id, project_id) - - Gitlab::Redis::SharedState.with do |redis| - message = redis.get(project_created_key) - redis.del(project_created_key) - message - end - end - - def add_project_created_message - return unless user.present? && project.present? - - Gitlab::Redis::SharedState.with do |redis| - key = self.class.project_created_message_key(user.id, project.id) - redis.setex(key, 5.minutes, project_created_message) - end - end - - def project_created_message + def message <<~MESSAGE.strip_heredoc The private project #{project.full_path} was created. To configure the remote, run: - git remote add origin #{git_url} + git remote add origin #{url_to_repo} To view the project, visit: #{project_url} @@ -44,19 +19,13 @@ module Gitlab private - attr_reader :project, :user, :protocol - - def self.project_created_message_key(user_id, project_id) + def self.message_key(user_id, project_id) "#{PROJECT_CREATED}:#{user_id}:#{project_id}" end def project_url Gitlab::Routing.url_helpers.project_url(project) end - - def git_url - protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo - end end end end diff --git a/lib/gitlab/checks/project_moved.rb b/lib/gitlab/checks/project_moved.rb index dfb2f4d4054..eca59e88e24 100644 --- a/lib/gitlab/checks/project_moved.rb +++ b/lib/gitlab/checks/project_moved.rb @@ -1,37 +1,15 @@ module Gitlab module Checks - class ProjectMoved + class ProjectMoved < BaseProject REDIRECT_NAMESPACE = "redirect_namespace".freeze - def initialize(project, user, redirected_path, protocol) - @project = project - @user = user + def initialize(project, user, protocol, redirected_path) @redirected_path = redirected_path - @protocol = protocol + + super(project, user, protocol) end - def self.fetch_redirect_message(user_id, project_id) - redirect_key = redirect_message_key(user_id, project_id) - - Gitlab::Redis::SharedState.with do |redis| - message = redis.get(redirect_key) - redis.del(redirect_key) - message - end - end - - def add_redirect_message - # Don't bother with sending a redirect message for anonymous clones - # because they never see it via the `/internal/post_receive` endpoint - return unless user.present? && project.present? - - Gitlab::Redis::SharedState.with do |redis| - key = self.class.redirect_message_key(user.id, project.id) - redis.setex(key, 5.minutes, redirect_message) - end - end - - def redirect_message(rejected: false) + def message(rejected: false) <<~MESSAGE.strip_heredoc Project '#{redirected_path}' was moved to '#{project.full_path}'. @@ -47,17 +25,17 @@ module Gitlab private - attr_reader :project, :redirected_path, :protocol, :user + attr_reader :redirected_path - def self.redirect_message_key(user_id, project_id) + def self.message_key(user_id, project_id) "#{REDIRECT_NAMESPACE}:#{user_id}:#{project_id}" end def remote_url_message(rejected) if rejected - "git remote set-url origin #{url} and try again." + "git remote set-url origin #{url_to_repo} and try again." else - "git remote set-url origin #{url}" + "git remote set-url origin #{url_to_repo}" end end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 32a2395a26b..3d07e112e2b 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -19,8 +19,7 @@ module Gitlab upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.', receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.', read_only: 'The repository is temporarily read-only. Please try again later.', - cannot_push_to_read_only: "You can't push code to a read-only GitLab instance.", - namespace_not_found: 'The namespace you were looking for could not be found.' + cannot_push_to_read_only: "You can't push code to a read-only GitLab instance." }.freeze DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }.freeze @@ -53,7 +52,6 @@ module Gitlab check_download_access! when *PUSH_COMMANDS check_push_access!(cmd, changes) - check_namespace_accessibility! end true @@ -100,7 +98,7 @@ module Gitlab end def check_project_accessibility!(cmd) - if (project.blank? || !can_read_project?) && !can_create_project_in_namespace?(cmd) + unless can_create_project_in_namespace?(cmd) || can_read_project? raise NotFoundError, ERROR_MESSAGES[:project_not_found] end end @@ -108,12 +106,12 @@ module Gitlab def check_project_moved! return if redirected_path.nil? - project_moved = Checks::ProjectMoved.new(project, user, redirected_path, protocol) + project_moved = Checks::ProjectMoved.new(project, user, protocol, redirected_path) if project_moved.permanent_redirect? - project_moved.add_redirect_message + project_moved.add_message else - raise ProjectMovedError, project_moved.redirect_message(rejected: true) + raise ProjectMovedError, project_moved.message(rejected: true) end end @@ -144,19 +142,11 @@ module Gitlab end def check_repository_existence!(cmd) - if (project.blank? || !project.repository.exists?) && !can_create_project_in_namespace?(cmd) + unless can_create_project_in_namespace?(cmd) || project.repository.exists? raise UnauthorizedError, ERROR_MESSAGES[:no_repo] end end - def check_namespace_accessibility! - return unless project.blank? - - unless target_namespace - raise NotFoundError, ERROR_MESSAGES[:namespace_not_found] - end - end - def check_download_access! return if deploy_key? @@ -170,16 +160,16 @@ module Gitlab end def check_push_access!(cmd, changes) - return if project.blank? && can_create_project_in_namespace?(cmd) + if Gitlab::Database.read_only? + raise UnauthorizedError, push_to_read_only_message + end + + return if can_create_project_in_namespace?(cmd) if project.repository_read_only? raise UnauthorizedError, ERROR_MESSAGES[:read_only] end - if Gitlab::Database.read_only? - raise UnauthorizedError, push_to_read_only_message - end - if deploy_key check_deploy_key_push_access! elsif user @@ -249,7 +239,7 @@ module Gitlab end def can_create_project_in_namespace?(cmd) - return false unless push?(cmd) && target_namespace + return false unless push?(cmd) && target_namespace && project.blank? user.can?(:create_projects, target_namespace) end diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 4a2db11a978..c6a594d38d1 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -188,7 +188,7 @@ module Gitlab end def full_project_git_path_regex - @full_project_git_path_regex ||= /\A(\/|)(?#{full_namespace_route_regex})\/(?#{project_git_route_regex})\z/.freeze + @full_project_git_path_regex ||= /\A\/?(?#{full_namespace_route_regex})\/(?#{project_git_route_regex})\z/.freeze end def full_namespace_format_regex diff --git a/spec/lib/gitlab/checks/project_created_spec.rb b/spec/lib/gitlab/checks/project_created_spec.rb index c79798bc84d..ac02007e111 100644 --- a/spec/lib/gitlab/checks/project_created_spec.rb +++ b/spec/lib/gitlab/checks/project_created_spec.rb @@ -4,43 +4,43 @@ describe Gitlab::Checks::ProjectCreated, :clean_gitlab_redis_shared_state do let(:user) { create(:user) } let(:project) { create(:project) } - describe '.fetch_project_created_message' do + describe '.fetch_message' do context 'with a project created message queue' do - let(:project_created) { described_class.new(user, project, 'http') } + let(:project_created) { described_class.new(project, user, 'http') } before do - project_created.add_project_created_message + project_created.add_message end it 'returns project created message' do - expect(described_class.fetch_project_created_message(user.id, project.id)).to eq(project_created.project_created_message) + expect(described_class.fetch_message(user.id, project.id)).to eq(project_created.message) end it 'deletes the project created message from redis' do expect(Gitlab::Redis::SharedState.with { |redis| redis.get("project_created:#{user.id}:#{project.id}") }).not_to be_nil - described_class.fetch_project_created_message(user.id, project.id) + described_class.fetch_message(user.id, project.id) expect(Gitlab::Redis::SharedState.with { |redis| redis.get("project_created:#{user.id}:#{project.id}") }).to be_nil end end context 'with no project created message queue' do it 'returns nil' do - expect(described_class.fetch_project_created_message(1, 2)).to be_nil + expect(described_class.fetch_message(1, 2)).to be_nil end end end - describe '#add_project_created_message' do + describe '#add_message' do it 'queues a project created message' do - project_created = described_class.new(user, project, 'http') + project_created = described_class.new(project, user, 'http') - expect(project_created.add_project_created_message).to eq('OK') + expect(project_created.add_message).to eq('OK') end it 'handles anonymous push' do - project_created = described_class.new(user, nil, 'http') + project_created = described_class.new(nil, user, 'http') - expect(project_created.add_project_created_message).to be_nil + expect(project_created.add_message).to be_nil end end end diff --git a/spec/lib/gitlab/checks/project_moved_spec.rb b/spec/lib/gitlab/checks/project_moved_spec.rb index b03a598edd8..e263d29656c 100644 --- a/spec/lib/gitlab/checks/project_moved_spec.rb +++ b/spec/lib/gitlab/checks/project_moved_spec.rb @@ -4,65 +4,65 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do let(:user) { create(:user) } let(:project) { create(:project) } - describe '.fetch_redirct_message' do + describe '.fetch_message' do context 'with a redirect message queue' do it 'returns the redirect message' do - project_moved = described_class.new(project, user, 'foo/bar', 'http') - project_moved.add_redirect_message + project_moved = described_class.new(project, user, 'http', 'foo/bar') + project_moved.add_message - expect(described_class.fetch_redirect_message(user.id, project.id)).to eq(project_moved.redirect_message) + expect(described_class.fetch_message(user.id, project.id)).to eq(project_moved.message) end it 'deletes the redirect message from redis' do - project_moved = described_class.new(project, user, 'foo/bar', 'http') - project_moved.add_redirect_message + project_moved = described_class.new(project, user, 'http', 'foo/bar') + project_moved.add_message expect(Gitlab::Redis::SharedState.with { |redis| redis.get("redirect_namespace:#{user.id}:#{project.id}") }).not_to be_nil - described_class.fetch_redirect_message(user.id, project.id) + described_class.fetch_message(user.id, project.id) expect(Gitlab::Redis::SharedState.with { |redis| redis.get("redirect_namespace:#{user.id}:#{project.id}") }).to be_nil end end context 'with no redirect message queue' do it 'returns nil' do - expect(described_class.fetch_redirect_message(1, 2)).to be_nil + expect(described_class.fetch_message(1, 2)).to be_nil end end end - describe '#add_redirect_message' do + describe '#add_message' do it 'queues a redirect message' do - project_moved = described_class.new(project, user, 'foo/bar', 'http') - expect(project_moved.add_redirect_message).to eq("OK") + project_moved = described_class.new(project, user, 'http', 'foo/bar') + expect(project_moved.add_message).to eq("OK") end it 'handles anonymous clones' do - project_moved = described_class.new(project, nil, 'foo/bar', 'http') + project_moved = described_class.new(project, nil, 'http', 'foo/bar') - expect(project_moved.add_redirect_message).to eq(nil) + expect(project_moved.add_message).to eq(nil) end end - describe '#redirect_message' do + describe '#message' do context 'when the push is rejected' do it 'returns a redirect message telling the user to try again' do - project_moved = described_class.new(project, user, 'foo/bar', 'http') + project_moved = described_class.new(project, user, 'http', 'foo/bar') message = "Project 'foo/bar' was moved to '#{project.full_path}'." + "\n\nPlease update your Git remote:" + "\n\n git remote set-url origin #{project.http_url_to_repo} and try again.\n" - expect(project_moved.redirect_message(rejected: true)).to eq(message) + expect(project_moved.message(rejected: true)).to eq(message) end end context 'when the push is not rejected' do it 'returns a redirect message' do - project_moved = described_class.new(project, user, 'foo/bar', 'http') + project_moved = described_class.new(project, user, 'http', 'foo/bar') message = "Project 'foo/bar' was moved to '#{project.full_path}'." + "\n\nPlease update your Git remote:" + "\n\n git remote set-url origin #{project.http_url_to_repo}\n" - expect(project_moved.redirect_message).to eq(message) + expect(project_moved.message).to eq(message) end end end @@ -71,7 +71,7 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do context 'with a permanent RedirectRoute' do it 'returns true' do project.route.create_redirect('foo/bar', permanent: true) - project_moved = described_class.new(project, user, 'foo/bar', 'http') + project_moved = described_class.new(project, user, 'http', 'foo/bar') expect(project_moved.permanent_redirect?).to be_truthy end end @@ -79,7 +79,7 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do context 'without a permanent RedirectRoute' do it 'returns false' do project.route.create_redirect('foo/bar') - project_moved = described_class.new(project, user, 'foo/bar', 'http') + project_moved = described_class.new(project, user, 'http', 'foo/bar') expect(project_moved.permanent_redirect?).to be_falsy end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index e6ad35867c0..6bfac67ddae 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -221,7 +221,7 @@ describe Gitlab::GitAccess do it 'enqueues a redirected message' do push_access_check - expect(Gitlab::Checks::ProjectMoved.fetch_redirect_message(user.id, project.id)).not_to be_nil + expect(Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id)).not_to be_nil end end @@ -335,7 +335,7 @@ describe Gitlab::GitAccess do end end - describe '#check_namespace_existence!' do + describe '#check_namespace_accessibility!' do context 'when project exists' do context 'when user can pull or push' do before do @@ -352,28 +352,16 @@ describe Gitlab::GitAccess do end context 'when project does not exist' do - context 'when namespace does not exist' do - let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: nil) } - - it 'blocks push and pull' do - aggregate_failures do - expect { push_access_check }.not_to raise_namespace_not_found - expect { pull_access_check }.not_to raise_namespace_not_found - end - end - end - context 'when namespace exists' do context 'when user is unable to push to namespace' do let(:user2) { create(:user) } let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: user2.namespace) } - it 'blocks push' do - expect { push_access_check }.to raise_project_create - end - - it 'does not block pull' do - expect { push_access_check }.to raise_error + it 'blocks push and pull' do + aggregate_failures do + expect { push_access_check }.to raise_not_found + expect { pull_access_check }.to raise_not_found + end end end end @@ -841,10 +829,6 @@ describe Gitlab::GitAccess do raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found]) end - def raise_namespace_not_found - raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:namespace_not_found]) - end - def build_authentication_abilities [ :read_project, diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 1e8197653ce..5ef180348aa 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -399,6 +399,18 @@ describe API::Internal do expect(json_response["status"]).to be_truthy expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(path) end + + it 'handles project creation failure' do + path = "#{user.namespace.path}/new.git" + + expect do + push_with_path(key, path) + end.not_to change { Project.count } + + expect(response).to have_gitlab_http_status(200) + expect(json_response["status"]).to be_falsey + expect(json_response["message"]).to eq("Could not create project: Path new is a reserved name") + end end end end @@ -821,27 +833,27 @@ describe API::Internal do context 'with a redirected data' do it 'returns redirected message on the response' do - project_moved = Gitlab::Checks::ProjectMoved.new(project, user, 'foo/baz', 'http') - project_moved.add_redirect_message + project_moved = Gitlab::Checks::ProjectMoved.new(project, user, 'http', 'foo/baz') + project_moved.add_message post api("/internal/post_receive"), valid_params expect(response).to have_gitlab_http_status(200) expect(json_response["redirected_message"]).to be_present - expect(json_response["redirected_message"]).to eq(project_moved.redirect_message) + expect(json_response["redirected_message"]).to eq(project_moved.message) end end context 'with new project data' do it 'returns new project message on the response' do - project_created = Gitlab::Checks::ProjectCreated.new(user, project, 'http') - project_created.add_project_created_message + project_created = Gitlab::Checks::ProjectCreated.new(project, user, 'http') + project_created.add_message post api("/internal/post_receive"), valid_params expect(response).to have_gitlab_http_status(200) expect(json_response["project_created_message"]).to be_present - expect(json_response["project_created_message"]).to eq(project_created.project_created_message) + expect(json_response["project_created_message"]).to eq(project_created.message) end end diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index c73db257b05..8be94459c2e 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -134,12 +134,12 @@ describe 'Git HTTP requests' do end.to change { user.projects.count }.by(1) end - it 'rejects upload with 404 Not Found when project is invalid' do + it 'rejects push with 422 Unprocessable Entity when project is invalid' do path = "#{user.namespace.path}/new.git" - upload(path, user: user.username, password: user.password) do |response| - expect(response).to have_gitlab_http_status(:not_found) - end + push_get(path, user: user.username, password: user.password) + + expect(response).to have_gitlab_http_status(:unprocessable_entity) end end end diff --git a/spec/services/projects/create_from_push_service_spec.rb b/spec/services/projects/create_from_push_service_spec.rb index 8e45697cd85..329279ea5fe 100644 --- a/spec/services/projects/create_from_push_service_spec.rb +++ b/spec/services/projects/create_from_push_service_spec.rb @@ -23,7 +23,7 @@ describe Projects::CreateFromPushService do context 'when user is nil' do let(:user) { nil } - subject { described_class.new(user, project_path, namespace, cmd, protocol) } + subject { described_class.new(user, project_path, namespace, protocol) } it 'returns nil' do expect_any_instance_of(Projects::CreateService).not_to receive(:execute) From 839829a7786dd163eccb470bf251211bfb90bd72 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Wed, 31 Jan 2018 13:52:46 +0000 Subject: [PATCH 7/9] Adds documentation for the feature --- doc/gitlab-basics/create-project.md | 38 +++++++++++++++++++++++++++++ lib/gitlab/git_access.rb | 8 ++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/doc/gitlab-basics/create-project.md b/doc/gitlab-basics/create-project.md index e18711f3392..d491d676884 100644 --- a/doc/gitlab-basics/create-project.md +++ b/doc/gitlab-basics/create-project.md @@ -33,5 +33,43 @@ 1. Click **Create project**. +## Push to create a new project + +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/26388) in GitLab 10.5. + +When you create a new repo locally, instead of going to GitLab to manually +create a new project and then push the repo, you can directly push it to +GitLab to create the new project, all without leaving your terminal. That +will automatically create a new project under a GitLab namespace that you have access to +with its visibility set to private by default (you can later change it). + +This can be done by using either SSH or HTTP: + +``` +## Git push using SSH +git push git@gitlab.com:namespace/nonexistent-project.git branch_name + +## Git push using HTTP +git push https://gitlab.com/namespace/nonexistent-project.git branch_name +``` + +Once the push finishes successfully, a remote message will indicate +the command to set the remote and the URL to the new project: + +``` +remote: +remote: The private project namespace/nonexistent-project was created. +remote: +remote: To configure the remote, run: +remote: git remote add origin https://gitlab.com/namespace/nonexistent-project.git +remote: +remote: To view the project, visit: +remote: https://gitlab.com/namespace/nonexistent-project +remote: +``` + +If the project name is already in use, your push will be rejected +to prevent accidental overwriting the existing project. + [import it]: ../workflow/importing/README.md [reserved]: ../user/reserved_names.md diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 3d07e112e2b..84299dd5790 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -2,6 +2,8 @@ # class return an instance of `GitlabAccessStatus` module Gitlab class GitAccess + include Gitlab::Utils::StrongMemoize + UnauthorizedError = Class.new(StandardError) NotFoundError = Class.new(StandardError) ProjectCreationError = Class.new(StandardError) @@ -239,9 +241,11 @@ module Gitlab end def can_create_project_in_namespace?(cmd) - return false unless push?(cmd) && target_namespace && project.blank? + strong_memoize(:can_create_project_in_namespace) do + return false unless push?(cmd) && target_namespace && project.blank? - user.can?(:create_projects, target_namespace) + user.can?(:create_projects, target_namespace) + end end def http? From 1e56b3f476f9779ec747534e94156a6b8076209c Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Fri, 2 Feb 2018 15:27:30 +0000 Subject: [PATCH 8/9] Moves project creationg to git access check for git push --- GITLAB_SHELL_VERSION | 2 +- .../projects/git_http_controller.rb | 19 +- .../projects/create_from_push_service.rb | 37 --- doc/gitlab-basics/create-project.md | 17 +- lib/api/helpers/internal_helpers.rb | 20 +- lib/api/internal.rb | 15 +- .../{base_project.rb => post_push_message.rb} | 2 +- lib/gitlab/checks/project_created.rb | 6 +- lib/gitlab/checks/project_moved.rb | 4 +- lib/gitlab/git_access.rb | 92 ++++--- lib/gitlab/path_regex.rb | 2 +- lib/gitlab/user_access.rb | 3 +- spec/lib/gitlab/git_access_spec.rb | 226 ++++++++++++++---- spec/requests/api/internal_spec.rb | 26 -- .../projects/create_from_push_service_spec.rb | 34 --- 15 files changed, 272 insertions(+), 233 deletions(-) delete mode 100644 app/services/projects/create_from_push_service.rb rename lib/gitlab/checks/{base_project.rb => post_push_message.rb} (97%) delete mode 100644 spec/services/projects/create_from_push_service_spec.rb diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index 9b9a244206f..090ea9dad19 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -6.0.2 +6.0.3 diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 90a9079fab3..45910a9be44 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -11,7 +11,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController # GET /foo/bar.git/info/refs?service=git-receive-pack (git push) def info_refs log_user_activity if upload_pack? - create_new_project if receive_pack? && project.blank? render_ok end @@ -36,10 +35,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController git_command == 'git-upload-pack' end - def receive_pack? - git_command == 'git-receive-pack' - end - def git_command if action_name == 'info_refs' params[:service] @@ -48,10 +43,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController end end - def create_new_project - @project = ::Projects::CreateFromPushService.new(user, params[:project_id], namespace, 'http').execute - end - def render_ok set_workhorse_internal_api_content_type render json: Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name) @@ -70,7 +61,10 @@ class Projects::GitHttpController < Projects::GitHttpClientController end def access - @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: namespace) + @access ||= access_klass.new(access_actor, project, + 'http', authentication_abilities: authentication_abilities, + namespace_path: params[:namespace_id], project_path: project_path, + redirected_path: redirected_path) end def access_actor @@ -82,14 +76,15 @@ class Projects::GitHttpController < Projects::GitHttpClientController # Use the magic string '_any' to indicate we do not know what the # changes are. This is also what gitlab-shell does. access.check(git_command, '_any') + @project ||= access.project end def access_klass @access_klass ||= wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess end - def namespace - @namespace ||= Namespace.find_by_full_path(params[:namespace_id]) + def project_path + @project_path ||= params[:project_id].sub(/\.git$/, '') end def log_user_activity diff --git a/app/services/projects/create_from_push_service.rb b/app/services/projects/create_from_push_service.rb deleted file mode 100644 index 949a0bfdc09..00000000000 --- a/app/services/projects/create_from_push_service.rb +++ /dev/null @@ -1,37 +0,0 @@ -module Projects - class CreateFromPushService < BaseService - attr_reader :user, :project_path, :namespace, :protocol - - def initialize(user, project_path, namespace, protocol) - @user = user - @project_path = project_path - @namespace = namespace - @protocol = protocol - end - - def execute - return unless user - - project = Projects::CreateService.new(user, project_params).execute - - if project.saved? - Gitlab::Checks::ProjectCreated.new(project, user, protocol).add_message - else - raise Gitlab::GitAccess::ProjectCreationError, "Could not create project: #{project.errors.full_messages.join(', ')}" - end - - project - end - - private - - def project_params - { - description: "", - path: project_path.gsub(/\.git$/, ''), - namespace_id: namespace&.id, - visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s - } - end - end -end diff --git a/doc/gitlab-basics/create-project.md b/doc/gitlab-basics/create-project.md index d491d676884..7b87039da84 100644 --- a/doc/gitlab-basics/create-project.md +++ b/doc/gitlab-basics/create-project.md @@ -39,18 +39,18 @@ When you create a new repo locally, instead of going to GitLab to manually create a new project and then push the repo, you can directly push it to -GitLab to create the new project, all without leaving your terminal. That -will automatically create a new project under a GitLab namespace that you have access to -with its visibility set to private by default (you can later change it). +GitLab to create the new project, all without leaving your terminal. If you have access to that +namespace, we will automatically create a new project under that GitLab namespace with its +visibility set to private by default (you can later change it in the UI). This can be done by using either SSH or HTTP: ``` ## Git push using SSH -git push git@gitlab.com:namespace/nonexistent-project.git branch_name +git push git@gitlab.example.com:namespace/nonexistent-project.git ## Git push using HTTP -git push https://gitlab.com/namespace/nonexistent-project.git branch_name +git push https://gitlab.example.com/namespace/nonexistent-project.git ``` Once the push finishes successfully, a remote message will indicate @@ -61,15 +61,12 @@ remote: remote: The private project namespace/nonexistent-project was created. remote: remote: To configure the remote, run: -remote: git remote add origin https://gitlab.com/namespace/nonexistent-project.git +remote: git remote add origin https://gitlab.example.com/namespace/nonexistent-project.git remote: remote: To view the project, visit: -remote: https://gitlab.com/namespace/nonexistent-project +remote: https://gitlab.example.com/namespace/nonexistent-project remote: ``` -If the project name is already in use, your push will be rejected -to prevent accidental overwriting the existing project. - [import it]: ../workflow/importing/README.md [reserved]: ../user/reserved_names.md diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index bff245fe9a2..cd59da6fc70 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -1,8 +1,6 @@ module API module Helpers module InternalHelpers - include Gitlab::Utils::StrongMemoize - attr_reader :redirected_path def wiki? @@ -49,10 +47,6 @@ module API ::Users::ActivityService.new(actor, 'Git SSH').execute if commands.include?(params[:action]) end - def receive_pack? - params[:action] == 'git-receive-pack' - end - def merge_request_urls ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) end @@ -66,16 +60,18 @@ module API false end - def project_namespace - strong_memoize(:project_namespace) do - project&.namespace || Namespace.find_by_full_path(project_match[:namespace_path]) - end + def project_path + project&.path || project_path_match[:project_path] + end + + def namespace_path + project&.namespace&.full_path || project_path_match[:namespace_path] end private - def project_match - @project_match ||= params[:project].match(Gitlab::PathRegex.full_project_git_path_regex) || {} + def project_path_match + @project_path_match ||= params[:project].match(Gitlab::PathRegex.full_project_git_path_regex) || {} end # rubocop:disable Gitlab/ModuleWithInstanceVariables diff --git a/lib/api/internal.rb b/lib/api/internal.rb index ed6d022df97..9285fb90cdc 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -42,23 +42,18 @@ module API end access_checker_klass = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess - access_checker = access_checker_klass - .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities, redirected_path: redirected_path, target_namespace: project_namespace) + access_checker = access_checker_klass.new(actor, project, + protocol, authentication_abilities: ssh_authentication_abilities, + namespace_path: namespace_path, project_path: project_path, + redirected_path: redirected_path) begin access_checker.check(params[:action], params[:changes]) + @project ||= access_checker.project rescue Gitlab::GitAccess::UnauthorizedError, Gitlab::GitAccess::NotFoundError => e return { status: false, message: e.message } end - if receive_pack? && project.blank? - begin - @project = ::Projects::CreateFromPushService.new(user, project_match[:project_path], project_namespace, protocol).execute - rescue Gitlab::GitAccess::ProjectCreationError => e - return { status: false, message: e.message } - end - end - log_user_activity(actor) { diff --git a/lib/gitlab/checks/base_project.rb b/lib/gitlab/checks/post_push_message.rb similarity index 97% rename from lib/gitlab/checks/base_project.rb rename to lib/gitlab/checks/post_push_message.rb index dd6c007b356..473c0385b34 100644 --- a/lib/gitlab/checks/base_project.rb +++ b/lib/gitlab/checks/post_push_message.rb @@ -1,6 +1,6 @@ module Gitlab module Checks - class BaseProject + class PostPushMessage def initialize(project, user, protocol) @project = project @user = user diff --git a/lib/gitlab/checks/project_created.rb b/lib/gitlab/checks/project_created.rb index bd1e204bc81..cec270d6a58 100644 --- a/lib/gitlab/checks/project_created.rb +++ b/lib/gitlab/checks/project_created.rb @@ -1,12 +1,12 @@ module Gitlab module Checks - class ProjectCreated < BaseProject + class ProjectCreated < PostPushMessage PROJECT_CREATED = "project_created".freeze def message - <<~MESSAGE.strip_heredoc + <<~MESSAGE - The private project #{project.full_path} was created. + The private project #{project.full_path} was successfully created. To configure the remote, run: git remote add origin #{url_to_repo} diff --git a/lib/gitlab/checks/project_moved.rb b/lib/gitlab/checks/project_moved.rb index eca59e88e24..3263790a876 100644 --- a/lib/gitlab/checks/project_moved.rb +++ b/lib/gitlab/checks/project_moved.rb @@ -1,6 +1,6 @@ module Gitlab module Checks - class ProjectMoved < BaseProject + class ProjectMoved < PostPushMessage REDIRECT_NAMESPACE = "redirect_namespace".freeze def initialize(project, user, protocol, redirected_path) @@ -10,7 +10,7 @@ module Gitlab end def message(rejected: false) - <<~MESSAGE.strip_heredoc + <<~MESSAGE Project '#{redirected_path}' was moved to '#{project.full_path}'. Please update your Git remote: diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 84299dd5790..bc1e83f77b2 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -28,32 +28,37 @@ module Gitlab PUSH_COMMANDS = %w{ git-receive-pack }.freeze ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS - attr_reader :actor, :project, :protocol, :authentication_abilities, :redirected_path, :target_namespace + attr_reader :actor, :project, :protocol, :authentication_abilities, :namespace_path, :project_path, :redirected_path - def initialize(actor, project, protocol, authentication_abilities:, redirected_path: nil, target_namespace: nil) + def initialize(actor, project, protocol, authentication_abilities:, namespace_path: nil, project_path: nil, redirected_path: nil) @actor = actor @project = project @protocol = protocol - @redirected_path = redirected_path @authentication_abilities = authentication_abilities - @target_namespace = target_namespace + @namespace_path = namespace_path + @project_path = project_path + @redirected_path = redirected_path end def check(cmd, changes) check_protocol! check_valid_actor! check_active_user! - check_project_accessibility!(cmd) - check_project_moved! check_command_disabled!(cmd) check_command_existence!(cmd) - check_repository_existence!(cmd) + check_db_accessibility!(cmd) + + ensure_project_on_push!(cmd, changes) + + check_project_accessibility! + check_project_moved! + check_repository_existence! case cmd when *DOWNLOAD_COMMANDS check_download_access! when *PUSH_COMMANDS - check_push_access!(cmd, changes) + check_push_access!(changes) end true @@ -99,8 +104,8 @@ module Gitlab end end - def check_project_accessibility!(cmd) - unless can_create_project_in_namespace?(cmd) || can_read_project? + def check_project_accessibility! + if project.blank? || !can_read_project? raise NotFoundError, ERROR_MESSAGES[:project_not_found] end end @@ -143,16 +148,49 @@ module Gitlab end end - def check_repository_existence!(cmd) - unless can_create_project_in_namespace?(cmd) || project.repository.exists? + def check_db_accessibility!(cmd) + return unless receive_pack?(cmd) + + if Gitlab::Database.read_only? + raise UnauthorizedError, push_to_read_only_message + end + end + + def ensure_project_on_push!(cmd, changes) + return if project || deploy_key? + return unless receive_pack?(cmd) && changes == '_any' && authentication_abilities.include?(:push_code) + + namespace = Namespace.find_by_full_path(namespace_path) + + return unless user&.can?(:create_projects, namespace) + + project_params = { + path: project_path, + namespace_id: namespace.id, + visibility_level: Gitlab::VisibilityLevel::PRIVATE + } + + project = Projects::CreateService.new(user, project_params).execute + + unless project.saved? + raise ProjectCreationError, "Could not create project: #{project.errors.full_messages.join(', ')}" + end + + @project = project + user_access.project = @project + + Checks::ProjectCreated.new(project, user, protocol).add_message + end + + def check_repository_existence! + unless project.repository.exists? raise UnauthorizedError, ERROR_MESSAGES[:no_repo] end end def check_download_access! - return if deploy_key? - - passed = user_can_download_code? || + passed = deploy_key? || + user_can_download_code? || build_can_download_code? || guest_can_download_code? @@ -161,13 +199,7 @@ module Gitlab end end - def check_push_access!(cmd, changes) - if Gitlab::Database.read_only? - raise UnauthorizedError, push_to_read_only_message - end - - return if can_create_project_in_namespace?(cmd) - + def check_push_access!(changes) if project.repository_read_only? raise UnauthorizedError, ERROR_MESSAGES[:read_only] end @@ -180,8 +212,6 @@ module Gitlab raise UnauthorizedError, ERROR_MESSAGES[:upload] end - return if changes.blank? # Allow access. - check_change_access!(changes) end @@ -198,6 +228,8 @@ module Gitlab end def check_change_access!(changes) + return if changes.blank? # Allow access. + changes_list = Gitlab::ChangesList.new(changes) # Iterate over all changes to find if user allowed all of them to be applied @@ -240,14 +272,6 @@ module Gitlab end || Guest.can?(:read_project, project) end - def can_create_project_in_namespace?(cmd) - strong_memoize(:can_create_project_in_namespace) do - return false unless push?(cmd) && target_namespace && project.blank? - - user.can?(:create_projects, target_namespace) - end - end - def http? protocol == 'http' end @@ -260,10 +284,6 @@ module Gitlab command == 'git-receive-pack' end - def push?(cmd) - PUSH_COMMANDS.include?(cmd) - end - def upload_pack_disabled_over_http? !Gitlab.config.gitlab_shell.upload_pack end diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index c6a594d38d1..1fc0363904a 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -188,7 +188,7 @@ module Gitlab end def full_project_git_path_regex - @full_project_git_path_regex ||= /\A\/?(?#{full_namespace_route_regex})\/(?#{project_git_route_regex})\z/.freeze + @full_project_git_path_regex ||= %r{\A\/?(?#{full_namespace_route_regex})\/(?#{project_route_regex})\.git\z} end def full_namespace_format_regex diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index f357488ac61..15eb1c41213 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -6,7 +6,8 @@ module Gitlab [user&.id, project&.id] end - attr_reader :user, :project + attr_reader :user + attr_accessor :project def initialize(user, project: nil) @user = user diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 6bfac67ddae..cc48373a0c0 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -5,11 +5,19 @@ describe Gitlab::GitAccess do let(:actor) { user } let(:project) { create(:project, :repository) } + let(:project_path) { project.path } + let(:namespace_path) { project&.namespace&.path } let(:protocol) { 'ssh' } let(:authentication_abilities) { %i[read_project download_code push_code] } let(:redirected_path) { nil } - let(:access) { described_class.new(actor, project, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path) } + let(:access) do + described_class.new(actor, project, + protocol, authentication_abilities: authentication_abilities, + namespace_path: namespace_path, project_path: project_path, + redirected_path: redirected_path) + end + let(:push_access_check) { access.check('git-receive-pack', '_any') } let(:pull_access_check) { access.check('git-upload-pack', '_any') } @@ -145,6 +153,7 @@ describe Gitlab::GitAccess do context 'when the project is nil' do let(:project) { nil } + let(:project_path) { "new-project" } it 'blocks push and pull with "not found"' do aggregate_failures do @@ -154,7 +163,13 @@ describe Gitlab::GitAccess do end context 'when user is allowed to create project in namespace' do - let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: user.namespace) } + let(:namespace_path) { user.namespace.path } + let(:access) do + described_class.new(actor, nil, + protocol, authentication_abilities: authentication_abilities, + project_path: project_path, namespace_path: namespace_path, + redirected_path: redirected_path) + end it 'blocks pull access with "not found"' do expect { pull_access_check }.to raise_not_found @@ -167,7 +182,13 @@ describe Gitlab::GitAccess do context 'when user is not allowed to create project in namespace' do let(:user2) { create(:user) } - let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: user2.namespace) } + let(:namespace_path) { user2.namespace.path } + let(:access) do + described_class.new(actor, nil, + protocol, authentication_abilities: authentication_abilities, + project_path: project_path, namespace_path: namespace_path, + redirected_path: redirected_path) + end it 'blocks push and pull with "not found"' do aggregate_failures do @@ -297,6 +318,52 @@ describe Gitlab::GitAccess do end end + describe '#check_authentication_abilities!' do + before do + project.add_master(user) + end + + context 'when download' do + let(:authentication_abilities) { [] } + + it 'raises unauthorized with download error' do + expect { pull_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:download]) + end + + context 'when authentication abilities include download code' do + let(:authentication_abilities) { [:download_code] } + + it 'does not raise any errors' do + expect { pull_access_check }.not_to raise_error + end + end + + context 'when authentication abilities include build download code' do + let(:authentication_abilities) { [:build_download_code] } + + it 'does not raise any errors' do + expect { pull_access_check }.not_to raise_error + end + end + end + + context 'when upload' do + let(:authentication_abilities) { [] } + + it 'raises unauthorized with push error' do + expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) + end + + context 'when authentication abilities include push code' do + let(:authentication_abilities) { [:push_code] } + + it 'does not raise any errors' do + expect { push_access_check }.not_to raise_error + end + end + end + end + describe '#check_command_disabled!' do before do project.add_master(user) @@ -335,34 +402,112 @@ describe Gitlab::GitAccess do end end - describe '#check_namespace_accessibility!' do - context 'when project exists' do - context 'when user can pull or push' do - before do - project.add_master(user) + describe '#check_db_accessibility!' do + context 'when in a read-only GitLab instance' do + before do + create(:protected_branch, name: 'feature', project: project) + allow(Gitlab::Database).to receive(:read_only?) { true } + end + + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:cannot_push_to_read_only]) } + end + end + + describe '#ensure_project_on_push!' do + let(:access) do + described_class.new(actor, project, + protocol, authentication_abilities: authentication_abilities, + project_path: project_path, namespace_path: namespace_path, + redirected_path: redirected_path) + end + + context 'when push' do + let(:cmd) { 'git-receive-pack' } + + context 'when project does not exist' do + let(:project_path) { "nonexistent" } + let(:project) { nil } + + context 'when changes is _any' do + let(:changes) { '_any' } + + context 'when authentication abilities include push code' do + let(:authentication_abilities) { [:push_code] } + + context 'when user can create project in namespace' do + let(:namespace_path) { user.namespace.path } + + it 'creates a new project' do + expect { access.send(:ensure_project_on_push!, cmd, changes) }.to change { Project.count }.by(1) + end + end + + context 'when user cannot create project in namespace' do + let(:user2) { create(:user) } + let(:namespace_path) { user2.namespace.path } + + it 'does not create a new project' do + expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count } + end + end + end + + context 'when authentication abilities do not include push code' do + let(:authentication_abilities) { [] } + + context 'when user can create project in namespace' do + let(:namespace_path) { user.namespace.path } + + it 'does not create a new project' do + expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count } + end + end + end end - it 'does not block pull or push' do - aggregate_failures do - expect { push_access_check }.not_to raise_error - expect { pull_access_check }.not_to raise_error + context 'when check contains actual changes' do + let(:changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch" } + + it 'does not create a new project' do + expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count } end end end + + context 'when project exists' do + let(:changes) { '_any' } + let!(:project) { create(:project) } + + it 'does not create a new project' do + expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count } + end + end + + context 'when deploy key is used' do + let(:key) { create(:deploy_key, user: user) } + let(:actor) { key } + let(:project_path) { "nonexistent" } + let(:project) { nil } + let(:namespace_path) { user.namespace.path } + let(:changes) { '_any' } + + it 'does not create a new project' do + expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count } + end + end end - context 'when project does not exist' do - context 'when namespace exists' do - context 'when user is unable to push to namespace' do - let(:user2) { create(:user) } - let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: user2.namespace) } + context 'when pull' do + let(:cmd) { 'git-upload-pack' } + let(:changes) { '_any' } - it 'blocks push and pull' do - aggregate_failures do - expect { push_access_check }.to raise_not_found - expect { pull_access_check }.to raise_not_found - end - end + context 'when project does not exist' do + let(:project_path) { "new-project" } + let(:namespace_path) { user.namespace.path } + let(:project) { nil } + + it 'does not create a new project' do + expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count } end end end @@ -395,7 +540,9 @@ describe Gitlab::GitAccess do context 'when project is public' do let(:public_project) { create(:project, :public, :repository) } - let(:access) { described_class.new(nil, public_project, 'web', authentication_abilities: []) } + let(:project_path) { public_project.path } + let(:namespace_path) { public_project.namespace.path } + let(:access) { described_class.new(nil, public_project, 'web', authentication_abilities: [:download_code], project_path: project_path, namespace_path: namespace_path) } context 'when repository is enabled' do it 'give access to download code' do @@ -558,17 +705,15 @@ describe Gitlab::GitAccess do end aggregate_failures do - Gitlab::GitAccess::ALL_COMMANDS.each do |cmd| - matrix.each do |action, allowed| - check = -> { access.send(:check_push_access!, cmd, changes[action]) } + matrix.each do |action, allowed| + check = -> { access.send(:check_push_access!, changes[action]) } - if allowed - expect(&check).not_to raise_error, - -> { "expected #{action} to be allowed" } - else - expect(&check).to raise_error(Gitlab::GitAccess::UnauthorizedError), - -> { "expected #{action} to be disallowed" } - end + if allowed + expect(&check).not_to raise_error, + -> { "expected #{action} to be allowed" } + else + expect(&check).to raise_error(Gitlab::GitAccess::UnauthorizedError), + -> { "expected #{action} to be disallowed" } end end end @@ -697,19 +842,6 @@ describe Gitlab::GitAccess do admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false })) end end - - context "when in a read-only GitLab instance" do - before do - create(:protected_branch, name: 'feature', project: project) - allow(Gitlab::Database).to receive(:read_only?) { true } - end - - # Only check admin; if an admin can't do it, other roles can't either - matrix = permissions_matrix[:admin].dup - matrix.each { |key, _| matrix[key] = false } - - run_permission_checks(admin: matrix) - end end describe 'build authentication abilities' do diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 5ef180348aa..ea6b0a71849 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -386,32 +386,6 @@ describe API::Internal do expect(json_response["repository_path"]).to eq(project.repository.path_to_repo) expect(json_response["gl_repository"]).to eq("project-#{project.id}") end - - context 'when project does not exist' do - it 'creates a new project' do - path = "#{user.namespace.path}/notexist.git" - - expect do - push_with_path(key, path) - end.to change { Project.count }.by(1) - - expect(response).to have_gitlab_http_status(200) - expect(json_response["status"]).to be_truthy - expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(path) - end - - it 'handles project creation failure' do - path = "#{user.namespace.path}/new.git" - - expect do - push_with_path(key, path) - end.not_to change { Project.count } - - expect(response).to have_gitlab_http_status(200) - expect(json_response["status"]).to be_falsey - expect(json_response["message"]).to eq("Could not create project: Path new is a reserved name") - end - end end end end diff --git a/spec/services/projects/create_from_push_service_spec.rb b/spec/services/projects/create_from_push_service_spec.rb deleted file mode 100644 index 329279ea5fe..00000000000 --- a/spec/services/projects/create_from_push_service_spec.rb +++ /dev/null @@ -1,34 +0,0 @@ -require 'spec_helper' - -describe Projects::CreateFromPushService do - let(:user) { create(:user) } - let(:project_path) { "nonexist" } - let(:namespace) { user&.namespace } - let(:protocol) { 'http' } - - subject { described_class.new(user, project_path, namespace, protocol) } - - it 'creates project' do - expect_any_instance_of(Projects::CreateService).to receive(:execute).and_call_original - - expect { subject.execute }.to change { Project.count }.by(1) - end - - it 'raises project creation error when project creation fails' do - allow_any_instance_of(Project).to receive(:saved?).and_return(false) - - expect { subject.execute }.to raise_error(Gitlab::GitAccess::ProjectCreationError) - end - - context 'when user is nil' do - let(:user) { nil } - - subject { described_class.new(user, project_path, namespace, protocol) } - - it 'returns nil' do - expect_any_instance_of(Projects::CreateService).not_to receive(:execute) - - expect(subject.execute).to be_nil - end - end -end From 8b4280cb25cd27c3b2c1cbdfb7ee871a7ebaa6d3 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Tue, 6 Feb 2018 17:25:36 +0000 Subject: [PATCH 9/9] Check ability ability before proceeding with project specific checks --- GITLAB_SHELL_VERSION | 2 +- lib/gitlab/git_access.rb | 41 +++++++++++++++++------------- spec/lib/gitlab/git_access_spec.rb | 14 +++++----- spec/requests/git_http_spec.rb | 4 +-- 4 files changed, 33 insertions(+), 28 deletions(-) diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index 090ea9dad19..9b9a244206f 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -6.0.3 +6.0.2 diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index bc1e83f77b2..8ec3386184a 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -12,8 +12,9 @@ module Gitlab ERROR_MESSAGES = { upload: 'You are not allowed to upload code for this project.', download: 'You are not allowed to download code from this project.', - deploy_key_upload: - 'This deploy key does not have write access to this project.', + auth_upload: 'You are not allowed to upload code.', + auth_download: 'You are not allowed to download code.', + deploy_key_upload: 'This deploy key does not have write access to this project.', no_repo: 'A repository for this project does not exist yet.', project_not_found: 'The project you were looking for could not be found.', account_blocked: 'Your account has been blocked.', @@ -44,6 +45,7 @@ module Gitlab check_protocol! check_valid_actor! check_active_user! + check_authentication_abilities!(cmd) check_command_disabled!(cmd) check_command_existence!(cmd) check_db_accessibility!(cmd) @@ -104,6 +106,19 @@ module Gitlab end end + def check_authentication_abilities!(cmd) + case cmd + when *DOWNLOAD_COMMANDS + unless authentication_abilities.include?(:download_code) || authentication_abilities.include?(:build_download_code) + raise UnauthorizedError, ERROR_MESSAGES[:auth_download] + end + when *PUSH_COMMANDS + unless authentication_abilities.include?(:push_code) + raise UnauthorizedError, ERROR_MESSAGES[:auth_upload] + end + end + end + def check_project_accessibility! if project.blank? || !can_read_project? raise NotFoundError, ERROR_MESSAGES[:project_not_found] @@ -205,31 +220,21 @@ module Gitlab end if deploy_key - check_deploy_key_push_access! + unless deploy_key.can_push_to?(project) + raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload] + end elsif user - check_user_push_access! + # User access is verified in check_change_access! else raise UnauthorizedError, ERROR_MESSAGES[:upload] end + return if changes.blank? # Allow access this is needed for EE. + check_change_access!(changes) end - def check_user_push_access! - unless authentication_abilities.include?(:push_code) - raise UnauthorizedError, ERROR_MESSAGES[:upload] - end - end - - def check_deploy_key_push_access! - unless deploy_key.can_push_to?(project) - raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload] - end - end - def check_change_access!(changes) - return if changes.blank? # Allow access. - changes_list = Gitlab::ChangesList.new(changes) # Iterate over all changes to find if user allowed all of them to be applied diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index cc48373a0c0..3c3697e7aa9 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -119,7 +119,7 @@ describe Gitlab::GitAccess do end it 'does not block pushes with "not found"' do - expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) + expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload]) end end end @@ -327,7 +327,7 @@ describe Gitlab::GitAccess do let(:authentication_abilities) { [] } it 'raises unauthorized with download error' do - expect { pull_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:download]) + expect { pull_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_download]) end context 'when authentication abilities include download code' do @@ -351,7 +351,7 @@ describe Gitlab::GitAccess do let(:authentication_abilities) { [] } it 'raises unauthorized with push error' do - expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) + expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload]) end context 'when authentication abilities include push code' do @@ -852,26 +852,26 @@ describe Gitlab::GitAccess do project.add_reporter(user) end - it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) } + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload]) } end context 'when unauthorized' do context 'to public project' do let(:project) { create(:project, :public, :repository) } - it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) } + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload]) } end context 'to internal project' do let(:project) { create(:project, :internal, :repository) } - it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:upload]) } + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload]) } end context 'to private project' do let(:project) { create(:project, :private, :repository) } - it { expect { push_access_check }.to raise_not_found } + it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:auth_upload]) } end end end diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 8be94459c2e..2e2dccdafad 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -620,7 +620,7 @@ describe 'Git HTTP requests' do push_get(path, env) expect(response).to have_gitlab_http_status(:forbidden) - expect(response.body).to eq(git_access_error(:upload)) + expect(response.body).to eq(git_access_error(:auth_upload)) end # We are "authenticated" as CI using a valid token here. But we are @@ -660,7 +660,7 @@ describe 'Git HTTP requests' do push_get path, env expect(response).to have_gitlab_http_status(:forbidden) - expect(response.body).to eq(git_access_error(:upload)) + expect(response.body).to eq(git_access_error(:auth_upload)) end end