From bc78ae6985ee37f9ac2ffc2dbf6f445078d16038 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Mon, 22 Jan 2018 18:10:56 +0000 Subject: Add specs --- app/controllers/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 -- cgit v1.2.1