From dc229c076cdc0ef6e7f3f74f6e462c22880ff08c Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Fri, 26 Jan 2018 14:28:08 +0000 Subject: [PATCH] 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)