Commit a264524a authored by Nick Thomas's avatar Nick Thomas

Merge branch '42936-improve-ns-factory-avoid-duplicates-ee' into 'master'

EE: Make sure we're not creating duplicated namespace

See merge request gitlab-org/gitlab-ee!5410
parents 048d388f 471457d4
...@@ -102,7 +102,7 @@ module Routable ...@@ -102,7 +102,7 @@ module Routable
# the route. Caching this per request ensures that even if we have multiple instances, # the route. Caching this per request ensures that even if we have multiple instances,
# we will not have to duplicate work, avoiding N+1 queries in some cases. # we will not have to duplicate work, avoiding N+1 queries in some cases.
def full_path def full_path
return uncached_full_path unless RequestStore.active? return uncached_full_path unless RequestStore.active? && id
RequestStore[full_path_key] ||= uncached_full_path RequestStore[full_path_key] ||= uncached_full_path
end end
...@@ -124,6 +124,11 @@ module Routable ...@@ -124,6 +124,11 @@ module Routable
end end
end end
# Group would override this to check from association
def owned_by?(user)
owner == user
end
private private
def set_path_errors def set_path_errors
......
...@@ -141,6 +141,10 @@ class Group < Namespace ...@@ -141,6 +141,10 @@ class Group < Namespace
self[:lfs_enabled] self[:lfs_enabled]
end end
def owned_by?(user)
owners.include?(user)
end
def add_users(users, access_level, current_user: nil, expires_at: nil) def add_users(users, access_level, current_user: nil, expires_at: nil)
GroupMember.add_users( GroupMember.add_users(
self, self,
......
---
title: Fix discussions API setting created_at for notable in a group or notable in
a project in a group with owners
merge_request: 18464
author:
type: fixed
...@@ -2,7 +2,7 @@ require 'spec_helper' ...@@ -2,7 +2,7 @@ require 'spec_helper'
describe Gitlab::LegacyGithubImport::ProjectCreator do describe Gitlab::LegacyGithubImport::ProjectCreator do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:namespace) { create(:group, owner: user) } let(:namespace) { create(:group) }
let(:repo) do let(:repo) do
OpenStruct.new( OpenStruct.new(
......
...@@ -66,8 +66,10 @@ module API ...@@ -66,8 +66,10 @@ module API
authorize! :create_note, noteable authorize! :create_note, noteable
parent = noteable_parent(noteable) parent = noteable_parent(noteable)
if opts[:created_at] if opts[:created_at]
opts.delete(:created_at) unless current_user.admin? || parent.owner == current_user opts.delete(:created_at) unless
current_user.admin? || parent.owned_by?(current_user)
end end
project = parent if parent.is_a?(Project) project = parent if parent.is_a?(Project)
......
...@@ -223,11 +223,12 @@ describe Import::BitbucketController do ...@@ -223,11 +223,12 @@ describe Import::BitbucketController do
end end
context 'user has chosen an existing nested namespace and name for the project', :postgresql do context 'user has chosen an existing nested namespace and name for the project', :postgresql do
let(:parent_namespace) { create(:group, name: 'foo', owner: user) } let(:parent_namespace) { create(:group, name: 'foo') }
let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) } let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
let(:test_name) { 'test_name' } let(:test_name) { 'test_name' }
before do before do
parent_namespace.add_owner(user)
nested_namespace.add_owner(user) nested_namespace.add_owner(user)
end end
...@@ -273,7 +274,7 @@ describe Import::BitbucketController do ...@@ -273,7 +274,7 @@ describe Import::BitbucketController do
context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do
let(:test_name) { 'test_name' } let(:test_name) { 'test_name' }
let!(:parent_namespace) { create(:group, name: 'foo', owner: user) } let!(:parent_namespace) { create(:group, name: 'foo') }
before do before do
parent_namespace.add_owner(user) parent_namespace.add_owner(user)
......
...@@ -196,10 +196,11 @@ describe Import::GitlabController do ...@@ -196,10 +196,11 @@ describe Import::GitlabController do
end end
context 'user has chosen an existing nested namespace for the project', :postgresql do context 'user has chosen an existing nested namespace for the project', :postgresql do
let(:parent_namespace) { create(:group, name: 'foo', owner: user) } let(:parent_namespace) { create(:group, name: 'foo') }
let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) } let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
before do before do
parent_namespace.add_owner(user)
nested_namespace.add_owner(user) nested_namespace.add_owner(user)
end end
...@@ -245,7 +246,7 @@ describe Import::GitlabController do ...@@ -245,7 +246,7 @@ describe Import::GitlabController do
context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do
let(:test_name) { 'test_name' } let(:test_name) { 'test_name' }
let!(:parent_namespace) { create(:group, name: 'foo', owner: user) } let!(:parent_namespace) { create(:group, name: 'foo') }
before do before do
parent_namespace.add_owner(user) parent_namespace.add_owner(user)
......
...@@ -4,7 +4,11 @@ describe Projects::ForksController do ...@@ -4,7 +4,11 @@ describe Projects::ForksController do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, :public, :repository) } let(:project) { create(:project, :public, :repository) }
let(:forked_project) { Projects::ForkService.new(project, user).execute } let(:forked_project) { Projects::ForkService.new(project, user).execute }
let(:group) { create(:group, owner: forked_project.creator) } let(:group) { create(:group) }
before do
group.add_owner(user)
end
describe 'GET index' do describe 'GET index' do
def get_forks def get_forks
......
...@@ -6,6 +6,14 @@ FactoryBot.define do ...@@ -6,6 +6,14 @@ FactoryBot.define do
owner nil owner nil
project_creation_level ::EE::Gitlab::Access::MASTER_PROJECT_ACCESS project_creation_level ::EE::Gitlab::Access::MASTER_PROJECT_ACCESS
after(:create) do |group|
if group.owner
# We could remove this after we have proper constraint:
# https://gitlab.com/gitlab-org/gitlab-ce/issues/43292
raise "Don't set owner for groups, use `group.add_owner(user)` instead"
end
end
trait :public do trait :public do
visibility_level Gitlab::VisibilityLevel::PUBLIC visibility_level Gitlab::VisibilityLevel::PUBLIC
end end
......
...@@ -2,7 +2,23 @@ FactoryBot.define do ...@@ -2,7 +2,23 @@ FactoryBot.define do
factory :namespace do factory :namespace do
sequence(:name) { |n| "namespace#{n}" } sequence(:name) { |n| "namespace#{n}" }
path { name.downcase.gsub(/\s/, '_') } path { name.downcase.gsub(/\s/, '_') }
owner
# This is a workaround to avoid the user creating another namespace via
# User#ensure_namespace_correct. We should try to remove it and then
# we could remove this workaround
association :owner, factory: :user, strategy: :build
before(:create) do |namespace|
owner = namespace.owner
if owner
# We're changing the username here because we want to keep our path,
# and User#ensure_namespace_correct would change the path based on
# username, so we're forced to do this otherwise we'll need to change
# a lot of existing tests.
owner.username = namespace.path
owner.namespace = namespace
end
end
trait :with_build_minutes do trait :with_build_minutes do
namespace_statistics factory: :namespace_statistics, shared_runners_seconds: 400.minutes.to_i namespace_statistics factory: :namespace_statistics, shared_runners_seconds: 400.minutes.to_i
......
...@@ -64,7 +64,7 @@ feature 'New project' do ...@@ -64,7 +64,7 @@ feature 'New project' do
end end
context 'with group namespace' do context 'with group namespace' do
let(:group) { create(:group, :private, owner: user) } let(:group) { create(:group, :private) }
before do before do
group.add_owner(user) group.add_owner(user)
...@@ -81,7 +81,7 @@ feature 'New project' do ...@@ -81,7 +81,7 @@ feature 'New project' do
end end
context 'with subgroup namespace' do context 'with subgroup namespace' do
let(:group) { create(:group, owner: user) } let(:group) { create(:group) }
let(:subgroup) { create(:group, parent: group) } let(:subgroup) { create(:group, parent: group) }
before do before do
......
...@@ -15,7 +15,7 @@ describe Gitlab::BitbucketImport::ProjectCreator do ...@@ -15,7 +15,7 @@ describe Gitlab::BitbucketImport::ProjectCreator do
has_wiki?: false) has_wiki?: false)
end end
let(:namespace) { create(:group, owner: user) } let(:namespace) { create(:group) }
let(:token) { "asdasd12345" } let(:token) { "asdasd12345" }
let(:secret) { "sekrettt" } let(:secret) { "sekrettt" }
let(:access_params) { { bitbucket_access_token: token, bitbucket_access_token_secret: secret } } let(:access_params) { { bitbucket_access_token: token, bitbucket_access_token_secret: secret } }
......
...@@ -12,7 +12,7 @@ describe Gitlab::GitlabImport::ProjectCreator do ...@@ -12,7 +12,7 @@ describe Gitlab::GitlabImport::ProjectCreator do
owner: { name: "john" } owner: { name: "john" }
}.with_indifferent_access }.with_indifferent_access
end end
let(:namespace) { create(:group, owner: user) } let(:namespace) { create(:group) }
let(:token) { "asdffg" } let(:token) { "asdffg" }
let(:access_params) { { gitlab_access_token: token } } let(:access_params) { { gitlab_access_token: token } }
......
...@@ -9,7 +9,7 @@ describe Gitlab::GoogleCodeImport::ProjectCreator do ...@@ -9,7 +9,7 @@ describe Gitlab::GoogleCodeImport::ProjectCreator do
"repositoryUrls" => ["https://vim.googlecode.com/git/"] "repositoryUrls" => ["https://vim.googlecode.com/git/"]
) )
end end
let(:namespace) { create(:group, owner: user) } let(:namespace) { create(:group) }
before do before do
namespace.add_owner(user) namespace.add_owner(user)
......
...@@ -2,7 +2,7 @@ require 'spec_helper' ...@@ -2,7 +2,7 @@ require 'spec_helper'
describe Gitlab::LegacyGithubImport::ProjectCreator do describe Gitlab::LegacyGithubImport::ProjectCreator do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:namespace) { create(:group, owner: user) } let(:namespace) { create(:group) }
let(:repo) do let(:repo) do
OpenStruct.new( OpenStruct.new(
......
...@@ -364,7 +364,7 @@ describe Project do ...@@ -364,7 +364,7 @@ describe Project do
let(:owner) { create(:user, name: 'Gitlab') } let(:owner) { create(:user, name: 'Gitlab') }
let(:namespace) { create(:namespace, path: 'sample-namespace', owner: owner) } let(:namespace) { create(:namespace, path: 'sample-namespace', owner: owner) }
let(:project) { create(:project, path: 'sample-project', namespace: namespace) } let(:project) { create(:project, path: 'sample-project', namespace: namespace) }
let(:group) { create(:group, name: 'Group', path: 'sample-group', owner: owner) } let(:group) { create(:group, name: 'Group', path: 'sample-group') }
context 'when nil argument' do context 'when nil argument' do
it 'returns nil' do it 'returns nil' do
......
...@@ -1192,8 +1192,12 @@ describe User do ...@@ -1192,8 +1192,12 @@ describe User do
end end
context 'with a group route matching the given path' do context 'with a group route matching the given path' do
let!(:group) { create(:group, path: 'group_path') }
context 'when the group namespace has an owner_id (legacy data)' do context 'when the group namespace has an owner_id (legacy data)' do
let!(:group) { create(:group, path: 'group_path', owner: user) } before do
group.update!(owner_id: user.id)
end
it 'returns nil' do it 'returns nil' do
expect(described_class.find_by_full_path('group_path')).to eq(nil) expect(described_class.find_by_full_path('group_path')).to eq(nil)
...@@ -1201,8 +1205,6 @@ describe User do ...@@ -1201,8 +1205,6 @@ describe User do
end end
context 'when the group namespace does not have an owner_id' do context 'when the group namespace does not have an owner_id' do
let!(:group) { create(:group, path: 'group_path') }
it 'returns nil' do it 'returns nil' do
expect(described_class.find_by_full_path('group_path')).to eq(nil) expect(described_class.find_by_full_path('group_path')).to eq(nil)
end end
......
...@@ -32,13 +32,13 @@ describe API::Discussions do ...@@ -32,13 +32,13 @@ describe API::Discussions do
end end
context "when noteable is an Epic" do context "when noteable is an Epic" do
let(:group) { create(:group, :public, owner: user) } let(:group) { create(:group, :public) }
let(:ext_group) { create(:group, :public) } let(:ext_group) { create(:group, :public) }
let(:epic) { create(:epic, group: group, author: user) } let(:epic) { create(:epic, group: group, author: user) }
let!(:epic_note) { create(:discussion_note, noteable: epic, project: project, author: user) } let!(:epic_note) { create(:discussion_note, noteable: epic, project: project, author: user) }
before do before do
group.add_developer(user) group.add_owner(user)
stub_licensed_features(epics: true) stub_licensed_features(epics: true)
end end
......
...@@ -184,13 +184,13 @@ describe API::Notes do ...@@ -184,13 +184,13 @@ describe API::Notes do
end end
context "when noteable is an Epic" do context "when noteable is an Epic" do
let(:group) { create(:group, :public, owner: user) } let(:group) { create(:group, :public) }
let(:ext_group) { create(:group, :public) } let(:ext_group) { create(:group, :public) }
let(:epic) { create(:epic, group: group, author: user) } let(:epic) { create(:epic, group: group, author: user) }
let!(:epic_note) { create(:note, noteable: epic, project: project, author: user) } let!(:epic_note) { create(:note, noteable: epic, project: project, author: user) }
before do before do
group.add_developer(user) group.add_owner(user)
stub_licensed_features(epics: true) stub_licensed_features(epics: true)
end end
......
...@@ -59,8 +59,11 @@ describe Groups::NestedCreateService do ...@@ -59,8 +59,11 @@ describe Groups::NestedCreateService do
describe "#execute" do describe "#execute" do
it 'returns the group if it already existed' do it 'returns the group if it already existed' do
parent = create(:group, path: 'a-group', owner: user) parent = create(:group, path: 'a-group')
child = create(:group, path: 'a-sub-group', parent: parent, owner: user) child = create(:group, path: 'a-sub-group', parent: parent)
parent.add_owner(user)
child.add_owner(user)
expect(service.execute).to eq(child) expect(service.execute).to eq(child)
end end
......
...@@ -56,7 +56,7 @@ shared_examples 'a GitHub-ish import controller: GET status' do ...@@ -56,7 +56,7 @@ shared_examples 'a GitHub-ish import controller: GET status' do
end end
it "assigns variables" do it "assigns variables" do
project = create(:project, import_type: provider, creator_id: user.id) project = create(:project, import_type: provider, namespace: user.namespace)
stub_client(repos: [repo, org_repo], orgs: [org], org_repos: [org_repo]) stub_client(repos: [repo, org_repo], orgs: [org], org_repos: [org_repo])
get :status get :status
...@@ -69,7 +69,7 @@ shared_examples 'a GitHub-ish import controller: GET status' do ...@@ -69,7 +69,7 @@ shared_examples 'a GitHub-ish import controller: GET status' do
end end
it "does not show already added project" do it "does not show already added project" do
project = create(:project, import_type: provider, creator_id: user.id, import_source: 'asd/vim') project = create(:project, import_type: provider, namespace: user.namespace, import_source: 'asd/vim')
stub_client(repos: [repo], orgs: []) stub_client(repos: [repo], orgs: [])
get :status get :status
...@@ -257,11 +257,12 @@ shared_examples 'a GitHub-ish import controller: POST create' do ...@@ -257,11 +257,12 @@ shared_examples 'a GitHub-ish import controller: POST create' do
end end
context 'user has chosen an existing nested namespace and name for the project', :postgresql do context 'user has chosen an existing nested namespace and name for the project', :postgresql do
let(:parent_namespace) { create(:group, name: 'foo', owner: user) } let(:parent_namespace) { create(:group, name: 'foo') }
let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) } let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
let(:test_name) { 'test_name' } let(:test_name) { 'test_name' }
before do before do
parent_namespace.add_owner(user)
nested_namespace.add_owner(user) nested_namespace.add_owner(user)
end end
...@@ -307,7 +308,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do ...@@ -307,7 +308,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do
context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do
let(:test_name) { 'test_name' } let(:test_name) { 'test_name' }
let!(:parent_namespace) { create(:group, name: 'foo', owner: user) } let!(:parent_namespace) { create(:group, name: 'foo') }
before do before do
parent_namespace.add_owner(user) parent_namespace.add_owner(user)
......
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