Commit f21797e1 authored by Robert Speicher's avatar Robert Speicher Committed by Ruben Davila

Merge branch '21457-not-create-groups-for-unallowed-users-when-importing-projects' into 'master'

Don't create groups for unallowed users when importing projects

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/21457

See merge request !1990
parent ebf74a34
...@@ -11,6 +11,9 @@ v 8.11.4 ...@@ -11,6 +11,9 @@ v 8.11.4
- Fix sorting issues by "last updated" doesn't work after import from GitHub - Fix sorting issues by "last updated" doesn't work after import from GitHub
- Creating an issue through our API now emails label subscribers !5720 - Creating an issue through our API now emails label subscribers !5720
- Block concurrent updates for Pipeline - Block concurrent updates for Pipeline
- Fix resolving conflicts on forks
- Fix diff commenting on merge requests created prior to 8.10
- Don't create groups for unallowed users when importing projects
- Fix issue boards leak private label names and descriptions - Fix issue boards leak private label names and descriptions
v 8.11.3 v 8.11.3
......
...@@ -10,21 +10,24 @@ ...@@ -10,21 +10,24 @@
ImporterStatus.prototype.initStatusPage = function() { ImporterStatus.prototype.initStatusPage = function() {
$('.js-add-to-import').off('click').on('click', (function(_this) { $('.js-add-to-import').off('click').on('click', (function(_this) {
return function(e) { return function(e) {
var $btn, $namespace_input, $target_field, $tr, id, new_namespace; var $btn, $namespace_input, $target_field, $tr, id, target_namespace;
$btn = $(e.currentTarget); $btn = $(e.currentTarget);
$tr = $btn.closest('tr'); $tr = $btn.closest('tr');
$target_field = $tr.find('.import-target'); $target_field = $tr.find('.import-target');
$namespace_input = $target_field.find('input'); $namespace_input = $target_field.find('input');
id = $tr.attr('id').replace('repo_', ''); id = $tr.attr('id').replace('repo_', '');
new_namespace = null; target_namespace = null;
if ($namespace_input.length > 0) { if ($namespace_input.length > 0) {
new_namespace = $namespace_input.prop('value'); target_namespace = $namespace_input.prop('value');
$target_field.empty().append(new_namespace + "/" + ($target_field.data('project_name'))); $target_field.empty().append(target_namespace + "/" + ($target_field.data('project_name')));
} }
$btn.disable().addClass('is-loading'); $btn.disable().addClass('is-loading');
return $.post(_this.import_url, { return $.post(_this.import_url, {
repo_id: id, repo_id: id,
new_namespace: new_namespace target_namespace: target_namespace
}, { }, {
dataType: 'script' dataType: 'script'
}); });
...@@ -70,7 +73,7 @@ ...@@ -70,7 +73,7 @@
if ($('.js-importer-status').length) { if ($('.js-importer-status').length) {
var jobsImportPath = $('.js-importer-status').data('jobs-import-path'); var jobsImportPath = $('.js-importer-status').data('jobs-import-path');
var importPath = $('.js-importer-status').data('import-path'); var importPath = $('.js-importer-status').data('import-path');
new ImporterStatus(jobsImportPath, importPath); new ImporterStatus(jobsImportPath, importPath);
} }
}); });
......
class Import::BaseController < ApplicationController class Import::BaseController < ApplicationController
private private
def get_or_create_namespace def find_or_create_namespace(name, owner)
return current_user.namespace if name == owner
return current_user.namespace unless current_user.can_create_group?
begin begin
namespace = Group.create!(name: @target_namespace, path: @target_namespace, owner: current_user) name = params[:target_namespace].presence || name
namespace = Group.create!(name: name, path: name, owner: current_user)
namespace.add_owner(current_user) namespace.add_owner(current_user)
namespace
rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid
namespace = Namespace.find_by_path_or_name(@target_namespace) Namespace.find_by_path_or_name(name)
unless current_user.can?(:create_projects, namespace)
@already_been_taken = true
return false
end
end end
namespace
end end
end end
...@@ -35,23 +35,20 @@ class Import::BitbucketController < Import::BaseController ...@@ -35,23 +35,20 @@ class Import::BitbucketController < Import::BaseController
end end
def create def create
@repo_id = params[:repo_id] || "" @repo_id = params[:repo_id].to_s
repo = client.project(@repo_id.gsub("___", "/")) repo = client.project(@repo_id.gsub('___', '/'))
@project_name = repo["slug"] @project_name = repo['slug']
@target_namespace = find_or_create_namespace(repo['owner'], client.user['user']['username'])
repo_owner = repo["owner"]
repo_owner = current_user.username if repo_owner == client.user["user"]["username"]
@target_namespace = params[:new_namespace].presence || repo_owner
namespace = get_or_create_namespace || (render and return)
unless Gitlab::BitbucketImport::KeyAdder.new(repo, current_user, access_params).execute unless Gitlab::BitbucketImport::KeyAdder.new(repo, current_user, access_params).execute
@access_denied = true render 'deploy_key' and return
render
return
end end
@project = Gitlab::BitbucketImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute if current_user.can?(:create_projects, @target_namespace)
@project = Gitlab::BitbucketImport::ProjectCreator.new(repo, @target_namespace, current_user, access_params).execute
else
render 'unauthorized'
end
end end
private private
......
...@@ -41,14 +41,13 @@ class Import::GithubController < Import::BaseController ...@@ -41,14 +41,13 @@ class Import::GithubController < Import::BaseController
@repo_id = params[:repo_id].to_i @repo_id = params[:repo_id].to_i
repo = client.repo(@repo_id) repo = client.repo(@repo_id)
@project_name = repo.name @project_name = repo.name
@target_namespace = find_or_create_namespace(repo.owner.login, client.user.login)
repo_owner = repo.owner.login if current_user.can?(:create_projects, @target_namespace)
repo_owner = current_user.username if repo_owner == client.user.login @project = Gitlab::GithubImport::ProjectCreator.new(repo, @target_namespace, current_user, access_params).execute
@target_namespace = params[:new_namespace].presence || repo_owner else
render 'unauthorized'
namespace = get_or_create_namespace || (render and return) end
@project = Gitlab::GithubImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute
end end
private private
......
...@@ -26,15 +26,14 @@ class Import::GitlabController < Import::BaseController ...@@ -26,15 +26,14 @@ class Import::GitlabController < Import::BaseController
def create def create
@repo_id = params[:repo_id].to_i @repo_id = params[:repo_id].to_i
repo = client.project(@repo_id) repo = client.project(@repo_id)
@project_name = repo["name"] @project_name = repo['name']
@target_namespace = find_or_create_namespace(repo['namespace']['path'], client.user['username'])
repo_owner = repo["namespace"]["path"] if current_user.can?(:create_projects, @target_namespace)
repo_owner = current_user.username if repo_owner == client.user["username"] @project = Gitlab::GitlabImport::ProjectCreator.new(repo, @target_namespace, current_user, access_params).execute
@target_namespace = params[:new_namespace].presence || repo_owner else
render 'unauthorized'
namespace = get_or_create_namespace || (render and return) end
@project = Gitlab::GitlabImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute
end end
private private
......
module ImportHelper module ImportHelper
def import_project_target(owner, name)
namespace = current_user.can_create_group? ? owner : current_user.namespace_path
"#{namespace}/#{name}"
end
def github_project_link(path_with_namespace) def github_project_link(path_with_namespace)
link_to path_with_namespace, github_project_url(path_with_namespace), target: '_blank' link_to path_with_namespace, github_project_url(path_with_namespace), target: '_blank'
end end
......
- if @already_been_taken - if @project.persisted?
:plain
tr = $("tr#repo_#{@repo_id}")
target_field = tr.find(".import-target")
import_button = tr.find(".btn-import")
origin_target = target_field.text()
project_name = "#{@project_name}"
origin_namespace = "#{@target_namespace}"
target_field.empty()
target_field.append("<p class='alert alert-danger'>This namespace already been taken! Please choose another one</p>")
target_field.append("<input type='text' name='target_namespace' />")
target_field.append("/" + project_name)
target_field.data("project_name", project_name)
target_field.find('input').prop("value", origin_namespace)
import_button.enable().removeClass('is-loading')
- elsif @access_denied
:plain
job = $("tr#repo_#{@repo_id}")
job.find(".import-actions").html("<p class='alert alert-danger'>Access denied! Please verify you can add deploy keys to this repository.</p>")
- elsif @project.persisted?
:plain :plain
job = $("tr#repo_#{@repo_id}") job = $("tr#repo_#{@repo_id}")
job.attr("id", "project_#{@project.id}") job.attr("id", "project_#{@project.id}")
......
:plain
tr = $("tr#repo_#{@repo_id}")
target_field = tr.find(".import-target")
import_button = tr.find(".btn-import")
origin_target = target_field.text()
project_name = "#{@project_name}"
origin_namespace = "#{@target_namespace.path}"
target_field.empty()
target_field.append("<p class='alert alert-danger'>This namespace has already been taken! Please choose another one.</p>")
target_field.append("<input type='text' name='target_namespace' />")
target_field.append("/" + project_name)
target_field.data("project_name", project_name)
target_field.find('input').prop("value", origin_namespace)
import_button.enable().removeClass('is-loading')
:plain
job = $("tr#repo_#{@repo_id}")
job.find(".import-actions").html("<p class='alert alert-danger'>Access denied! Please verify you can add deploy keys to this repository.</p>")
...@@ -51,7 +51,7 @@ ...@@ -51,7 +51,7 @@
%td %td
= link_to "#{repo["owner"]}/#{repo["slug"]}", "https://bitbucket.org/#{repo["owner"]}/#{repo["slug"]}", target: "_blank" = link_to "#{repo["owner"]}/#{repo["slug"]}", "https://bitbucket.org/#{repo["owner"]}/#{repo["slug"]}", target: "_blank"
%td.import-target %td.import-target
= "#{repo["owner"]}/#{repo["slug"]}" = import_project_target(repo['owner'], repo['slug'])
%td.import-actions.job-status %td.import-actions.job-status
= button_tag class: "btn btn-import js-add-to-import" do = button_tag class: "btn btn-import js-add-to-import" do
Import Import
......
...@@ -45,7 +45,7 @@ ...@@ -45,7 +45,7 @@
%td %td
= github_project_link(repo.full_name) = github_project_link(repo.full_name)
%td.import-target %td.import-target
= repo.full_name = import_project_target(repo.owner.login, repo.name)
%td.import-actions.job-status %td.import-actions.job-status
= button_tag class: "btn btn-import js-add-to-import" do = button_tag class: "btn btn-import js-add-to-import" do
Import Import
......
...@@ -45,7 +45,7 @@ ...@@ -45,7 +45,7 @@
%td %td
= link_to repo["path_with_namespace"], "https://gitlab.com/#{repo["path_with_namespace"]}", target: "_blank" = link_to repo["path_with_namespace"], "https://gitlab.com/#{repo["path_with_namespace"]}", target: "_blank"
%td.import-target %td.import-target
= repo["path_with_namespace"] = import_project_target(repo['namespace']['path'], repo['name'])
%td.import-actions.job-status %td.import-actions.job-status
= button_tag class: "btn btn-import js-add-to-import" do = button_tag class: "btn btn-import js-add-to-import" do
Import Import
......
...@@ -146,21 +146,42 @@ describe Import::BitbucketController do ...@@ -146,21 +146,42 @@ describe Import::BitbucketController do
end end
context "when a namespace with the Bitbucket user's username doesn't exist" do context "when a namespace with the Bitbucket user's username doesn't exist" do
it "creates the namespace" do context "when current user can create namespaces" do
expect(Gitlab::BitbucketImport::ProjectCreator). it "creates the namespace" do
to receive(:new).and_return(double(execute: true)) expect(Gitlab::BitbucketImport::ProjectCreator).
to receive(:new).and_return(double(execute: true))
post :create, format: :js expect { post :create, format: :js }.to change(Namespace, :count).by(1)
end
it "takes the new namespace" do
expect(Gitlab::BitbucketImport::ProjectCreator).
to receive(:new).with(bitbucket_repo, an_instance_of(Group), user, access_params).
and_return(double(execute: true))
expect(Namespace.where(name: other_username).first).not_to be_nil post :create, format: :js
end
end end
it "takes the new namespace" do context "when current user can't create namespaces" do
expect(Gitlab::BitbucketImport::ProjectCreator). before do
to receive(:new).with(bitbucket_repo, an_instance_of(Group), user, access_params). user.update_attribute(:can_create_group, false)
and_return(double(execute: true)) end
post :create, format: :js it "doesn't create the namespace" do
expect(Gitlab::BitbucketImport::ProjectCreator).
to receive(:new).and_return(double(execute: true))
expect { post :create, format: :js }.not_to change(Namespace, :count)
end
it "takes the current user's namespace" do
expect(Gitlab::BitbucketImport::ProjectCreator).
to receive(:new).with(bitbucket_repo, user.namespace, user, access_params).
and_return(double(execute: true))
post :create, format: :js
end
end end
end end
end end
......
...@@ -181,21 +181,42 @@ describe Import::GithubController do ...@@ -181,21 +181,42 @@ describe Import::GithubController do
end end
context "when a namespace with the GitHub user's username doesn't exist" do context "when a namespace with the GitHub user's username doesn't exist" do
it "creates the namespace" do context "when current user can create namespaces" do
expect(Gitlab::GithubImport::ProjectCreator). it "creates the namespace" do
to receive(:new).and_return(double(execute: true)) expect(Gitlab::GithubImport::ProjectCreator).
to receive(:new).and_return(double(execute: true))
post :create, format: :js expect { post :create, format: :js }.to change(Namespace, :count).by(1)
end
it "takes the new namespace" do
expect(Gitlab::GithubImport::ProjectCreator).
to receive(:new).with(github_repo, an_instance_of(Group), user, access_params).
and_return(double(execute: true))
expect(Namespace.where(name: other_username).first).not_to be_nil post :create, format: :js
end
end end
it "takes the new namespace" do context "when current user can't create namespaces" do
expect(Gitlab::GithubImport::ProjectCreator). before do
to receive(:new).with(github_repo, an_instance_of(Group), user, access_params). user.update_attribute(:can_create_group, false)
and_return(double(execute: true)) end
post :create, format: :js it "doesn't create the namespace" do
expect(Gitlab::GithubImport::ProjectCreator).
to receive(:new).and_return(double(execute: true))
expect { post :create, format: :js }.not_to change(Namespace, :count)
end
it "takes the current user's namespace" do
expect(Gitlab::GithubImport::ProjectCreator).
to receive(:new).with(github_repo, user.namespace, user, access_params).
and_return(double(execute: true))
post :create, format: :js
end
end end
end end
end end
......
...@@ -136,21 +136,42 @@ describe Import::GitlabController do ...@@ -136,21 +136,42 @@ describe Import::GitlabController do
end end
context "when a namespace with the GitLab.com user's username doesn't exist" do context "when a namespace with the GitLab.com user's username doesn't exist" do
it "creates the namespace" do context "when current user can create namespaces" do
expect(Gitlab::GitlabImport::ProjectCreator). it "creates the namespace" do
to receive(:new).and_return(double(execute: true)) expect(Gitlab::GitlabImport::ProjectCreator).
to receive(:new).and_return(double(execute: true))
post :create, format: :js expect { post :create, format: :js }.to change(Namespace, :count).by(1)
end
it "takes the new namespace" do
expect(Gitlab::GitlabImport::ProjectCreator).
to receive(:new).with(gitlab_repo, an_instance_of(Group), user, access_params).
and_return(double(execute: true))
expect(Namespace.where(name: other_username).first).not_to be_nil post :create, format: :js
end
end end
it "takes the new namespace" do context "when current user can't create namespaces" do
expect(Gitlab::GitlabImport::ProjectCreator). before do
to receive(:new).with(gitlab_repo, an_instance_of(Group), user, access_params). user.update_attribute(:can_create_group, false)
and_return(double(execute: true)) end
post :create, format: :js it "doesn't create the namespace" do
expect(Gitlab::GitlabImport::ProjectCreator).
to receive(:new).and_return(double(execute: true))
expect { post :create, format: :js }.not_to change(Namespace, :count)
end
it "takes the current user's namespace" do
expect(Gitlab::GitlabImport::ProjectCreator).
to receive(:new).with(gitlab_repo, user.namespace, user, access_params).
and_return(double(execute: true))
post :create, format: :js
end
end end
end end
end end
......
require 'rails_helper' require 'rails_helper'
describe ImportHelper do describe ImportHelper do
describe '#import_project_target' do
let(:user) { create(:user) }
before do
allow(helper).to receive(:current_user).and_return(user)
end
context 'when current user can create namespaces' do
it 'returns project namespace' do
user.update_attribute(:can_create_group, true)
expect(helper.import_project_target('asd', 'vim')).to eq 'asd/vim'
end
end
context 'when current user can not create namespaces' do
it "takes the current user's namespace" do
user.update_attribute(:can_create_group, false)
expect(helper.import_project_target('asd', 'vim')).to eq "#{user.namespace_path}/vim"
end
end
end
describe '#github_project_link' do describe '#github_project_link' do
context 'when provider does not specify a custom URL' do context 'when provider does not specify a custom URL' do
it 'uses default GitHub URL' do it 'uses default GitHub URL' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment