From 40018afda510be1af2ef69100e9683d8fb79c0d4 Mon Sep 17 00:00:00 2001
From: Rutger Wessels <rwessels@gitlab.com>
Date: Mon, 16 Sep 2024 20:20:43 +0000
Subject: [PATCH] Remove remaining 'skip validation' exceptions for
Organization presence validation on Namespace
---
app/services/groups/create_service.rb | 2 +-
app/services/groups/update_service.rb | 2 +-
app/services/users/create_service.rb | 4 +--
.../ee/registrations_controller_spec.rb | 4 ---
ee/spec/features/groups_spec.rb | 11 ++++--
.../combined_registration_spec.rb | 4 +--
...on_with_no_existing_namespace_flow_spec.rb | 4 +--
ee/spec/lib/gitlab/auth/oidc/user_spec.rb | 4 ---
ee/spec/requests/api/service_accounts_spec.rb | 6 ++--
.../services/ee/groups/create_service_spec.rb | 6 ++--
.../services/ee/users/create_service_spec.rb | 4 ++-
spec/controllers/groups_controller_spec.rb | 13 ++++---
spec/features/dashboard/group_spec.rb | 6 +++-
.../file_uploads/group_import_spec.rb | 4 +--
spec/lib/gitlab/auth/ldap/user_spec.rb | 2 +-
spec/lib/gitlab/auth/o_auth/user_spec.rb | 4 ---
spec/services/groups/create_service_spec.rb | 29 +++++++--------
spec/services/users/create_service_spec.rb | 36 ++++++-------------
...able_namespace_organization_validation.yml | 2 --
19 files changed, 65 insertions(+), 82 deletions(-)
diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb
index 8e81329c0dbc5..40c97a7de9f69 100644
--- a/app/services/groups/create_service.rb
+++ b/app/services/groups/create_service.rb
@@ -17,7 +17,7 @@ def execute
@group.name ||= @group.path.dup
create_chat_team
- Namespace.with_disabled_organization_validation { create_group }
+ create_group
return error_response unless @group.persisted?
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index 8072baa5f2e64..6d11660c3093e 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -39,7 +39,7 @@ def execute
group.assign_attributes(params)
begin
- success = Namespace.with_disabled_organization_validation { group.save }
+ success = group.save
after_update if success
diff --git a/app/services/users/create_service.rb b/app/services/users/create_service.rb
index bcae6e8ce3ff9..591d88b275efc 100644
--- a/app/services/users/create_service.rb
+++ b/app/services/users/create_service.rb
@@ -13,9 +13,7 @@ def execute
user = build_class.new(current_user, params).execute
reset_token = user.generate_reset_token if user.recently_sent_password_reset?
- Namespace.with_disabled_organization_validation do
- after_create_hook(user, reset_token) if user.save
- end
+ after_create_hook(user, reset_token) if user.save
user
end
diff --git a/ee/spec/controllers/ee/registrations_controller_spec.rb b/ee/spec/controllers/ee/registrations_controller_spec.rb
index a4cf67b308207..1b78a99c00497 100644
--- a/ee/spec/controllers/ee/registrations_controller_spec.rb
+++ b/ee/spec/controllers/ee/registrations_controller_spec.rb
@@ -124,10 +124,6 @@
let(:params) { {} }
let(:session) { {} }
- around do |example|
- Namespace.with_disabled_organization_validation { example.run }
- end
-
subject(:post_create) { post :create, params: params.merge(user_params), session: session }
def identity_verification_exempt_for_user?
diff --git a/ee/spec/features/groups_spec.rb b/ee/spec/features/groups_spec.rb
index 9adcd7c0ad186..8d28c7ba06b6b 100644
--- a/ee/spec/features/groups_spec.rb
+++ b/ee/spec/features/groups_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe 'Group', feature_category: :groups_and_projects do
+RSpec.describe 'Group', :with_current_organization, feature_category: :groups_and_projects do
include NamespaceStorageHelpers
include FreeUserCapHelpers
@@ -187,7 +187,12 @@
end
describe 'identity verification', :js, :saas do
- let_it_be_with_reload(:user) { create(:user, :identity_verification_eligible, :with_sign_ins) }
+ let_it_be_with_reload(:user) do
+ create(
+ :user, :identity_verification_eligible, :with_sign_ins,
+ organizations: [current_organization]
+ )
+ end
before do
sign_in(user)
@@ -239,7 +244,7 @@
end
context 'when creating a subgroup' do
- let_it_be(:group) { create(:group, path: 'parent', owners: user) }
+ let_it_be(:group) { create(:group, path: 'parent', organization: current_organization, owners: user) }
before do
visit new_group_path(parent_id: group.id, anchor: 'create-group-pane')
diff --git a/ee/spec/features/registrations/combined_registration_spec.rb b/ee/spec/features/registrations/combined_registration_spec.rb
index 3207549788b4c..56a7669af915f 100644
--- a/ee/spec/features/registrations/combined_registration_spec.rb
+++ b/ee/spec/features/registrations/combined_registration_spec.rb
@@ -2,10 +2,10 @@
require 'spec_helper'
-RSpec.describe 'Registration group and project creation flow', :js, feature_category: :onboarding do
+RSpec.describe 'Registration group and project creation flow', :with_current_organization, :js, feature_category: :onboarding do
include SaasRegistrationHelpers
- let_it_be(:user) { create(:user, onboarding_in_progress: true) }
+ let_it_be(:user) { create(:user, onboarding_in_progress: true, organizations: [current_organization]) }
before do
# https://gitlab.com/gitlab-org/gitlab/-/issues/340302
diff --git a/ee/spec/features/trials/saas/creation_with_no_existing_namespace_flow_spec.rb b/ee/spec/features/trials/saas/creation_with_no_existing_namespace_flow_spec.rb
index 90593edf1670b..72d66c27f841b 100644
--- a/ee/spec/features/trials/saas/creation_with_no_existing_namespace_flow_spec.rb
+++ b/ee/spec/features/trials/saas/creation_with_no_existing_namespace_flow_spec.rb
@@ -2,8 +2,8 @@
require 'spec_helper'
-RSpec.describe 'Trial lead submission, group and trial creation', :saas_trial, :js, feature_category: :acquisition do
- let_it_be(:user) { create(:user) } # rubocop:disable Gitlab/RSpec/AvoidSetup
+RSpec.describe 'Trial lead submission, group and trial creation', :with_current_organization, :saas_trial, :js, feature_category: :acquisition do
+ let_it_be(:user) { create(:user, organizations: [current_organization]) } # rubocop:disable Gitlab/RSpec/AvoidSetup -- We need to ensure user is member of current organization
context 'when creating lead, group and applying trial is successful' do
it 'fills out form, testing validations, submits and lands on the group page' do
diff --git a/ee/spec/lib/gitlab/auth/oidc/user_spec.rb b/ee/spec/lib/gitlab/auth/oidc/user_spec.rb
index 94d2d39835dd3..e6db99639e44b 100644
--- a/ee/spec/lib/gitlab/auth/oidc/user_spec.rb
+++ b/ee/spec/lib/gitlab/auth/oidc/user_spec.rb
@@ -30,10 +30,6 @@
extra: extra_hash)
end
- around do |example|
- Namespace.with_disabled_organization_validation { example.run }
- end
-
before do
allow(Gitlab.config.omniauth).to receive_messages(block_auto_created_users: false)
diff --git a/ee/spec/requests/api/service_accounts_spec.rb b/ee/spec/requests/api/service_accounts_spec.rb
index cc792f47de21a..8a7e8b98c25f9 100644
--- a/ee/spec/requests/api/service_accounts_spec.rb
+++ b/ee/spec/requests/api/service_accounts_spec.rb
@@ -2,9 +2,9 @@
require 'spec_helper'
-RSpec.describe API::ServiceAccounts, :aggregate_failures, feature_category: :user_management do
- let(:user) { create(:user) }
- let(:admin) { create(:admin) }
+RSpec.describe API::ServiceAccounts, :with_current_organization, :aggregate_failures, feature_category: :user_management do
+ let(:user) { create(:user, organizations: [current_organization]) }
+ let(:admin) { create(:admin, organizations: [current_organization]) }
let(:license) { create(:license, plan: License::ULTIMATE_PLAN) }
describe "POST /service_accounts" do
diff --git a/ee/spec/services/ee/groups/create_service_spec.rb b/ee/spec/services/ee/groups/create_service_spec.rb
index ad5458addeec3..9081be9dd9306 100644
--- a/ee/spec/services/ee/groups/create_service_spec.rb
+++ b/ee/spec/services/ee/groups/create_service_spec.rb
@@ -4,12 +4,14 @@
RSpec.describe Groups::CreateService, '#execute', feature_category: :groups_and_projects do
let_it_be(:user) { create(:user) }
+ let_it_be(:organization) { create(:organization, users: [user]) }
let(:current_user) { user }
let(:group_params) do
{
name: 'GitLab',
path: 'group_path',
- visibility_level: Gitlab::VisibilityLevel::PUBLIC
+ visibility_level: Gitlab::VisibilityLevel::PUBLIC,
+ organization_id: organization.id
}.merge(extra_params)
end
@@ -46,7 +48,7 @@
end
context 'when created group is a sub-group' do
- let_it_be(:group) { create(:group, owners: user) }
+ let_it_be(:group) { create(:group, organization: organization, owners: user) }
let(:extra_params) { { parent_id: group.id } }
include_examples 'sends streaming audit event'
diff --git a/ee/spec/services/ee/users/create_service_spec.rb b/ee/spec/services/ee/users/create_service_spec.rb
index eb98c2217c04b..5332ed5b3a2ed 100644
--- a/ee/spec/services/ee/users/create_service_spec.rb
+++ b/ee/spec/services/ee/users/create_service_spec.rb
@@ -4,13 +4,15 @@
RSpec.describe Users::CreateService, feature_category: :user_management do
let_it_be(:current_user) { create(:admin) }
+ let_it_be(:organization) { create(:organization) }
let(:params) do
{
name: 'John Doe',
username: 'jduser',
email: 'jd@example.com',
- password: User.random_password
+ password: User.random_password,
+ organization_id: organization.id
}
end
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index 1bb250c8308cf..bf705f753a747 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -2,11 +2,11 @@
require 'spec_helper'
-RSpec.describe GroupsController, factory_default: :keep, feature_category: :code_review_workflow do
+RSpec.describe GroupsController, :with_current_organization, factory_default: :keep, feature_category: :code_review_workflow do
include ExternalAuthorizationServiceHelpers
include AdminModeHelper
- let_it_be(:group_organization) { create(:organization) }
+ let_it_be(:group_organization) { current_organization }
let_it_be_with_refind(:group) { create_default(:group, :public, organization: group_organization) }
let_it_be_with_refind(:project) { create(:project, namespace: group) }
let_it_be(:user) { create(:user) }
@@ -18,6 +18,10 @@
let_it_be(:developer) { group.add_developer(create(:user)).user }
let_it_be(:guest) { group.add_guest(create(:user)).user }
+ before_all do
+ group_organization.users = User.all
+ end
+
before do
enable_admin_mode!(admin_with_admin_mode)
end
@@ -240,7 +244,7 @@
context 'authorization' do
it 'allows an admin to create a group' do
- sign_in(create(:admin))
+ sign_in(admin_without_admin_mode)
expect do
post :create, params: { group: { name: 'new_group', path: 'new_group' } }
@@ -301,10 +305,9 @@
end
end
- context 'when creating a top level group', :with_current_organization do
+ context 'when creating a top level group' do
before do
sign_in(developer)
- Current.organization.users << developer
end
context 'and can_create_group is enabled' do
diff --git a/spec/features/dashboard/group_spec.rb b/spec/features/dashboard/group_spec.rb
index 26adbee049ac2..06922565d5372 100644
--- a/spec/features/dashboard/group_spec.rb
+++ b/spec/features/dashboard/group_spec.rb
@@ -2,10 +2,14 @@
require 'spec_helper'
-RSpec.describe 'Dashboard Group', :js, feature_category: :groups_and_projects do
+RSpec.describe 'Dashboard Group', :with_current_organization, :js, feature_category: :groups_and_projects do
let(:user) { create(:user) }
let(:group) { create(:group) }
+ before do
+ current_organization.users << user
+ end
+
context 'when user has no groups' do
before do
sign_in(user)
diff --git a/spec/features/file_uploads/group_import_spec.rb b/spec/features/file_uploads/group_import_spec.rb
index 02e6488f324bf..436853fa8dbcf 100644
--- a/spec/features/file_uploads/group_import_spec.rb
+++ b/spec/features/file_uploads/group_import_spec.rb
@@ -2,10 +2,10 @@
require 'spec_helper'
-RSpec.describe 'Upload a group export archive', :api, :js, feature_category: :groups_and_projects do
+RSpec.describe 'Upload a group export archive', :with_current_organization, :api, :js, feature_category: :groups_and_projects do
include_context 'file upload requests helpers'
- let_it_be(:user) { create(:user, :admin) }
+ let_it_be(:user) { create(:user, :admin, organizations: [current_organization]) }
let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
let(:api_path) { '/groups/import' }
diff --git a/spec/lib/gitlab/auth/ldap/user_spec.rb b/spec/lib/gitlab/auth/ldap/user_spec.rb
index 1f0b3d4d4b474..2128ceddc4f8c 100644
--- a/spec/lib/gitlab/auth/ldap/user_spec.rb
+++ b/spec/lib/gitlab/auth/ldap/user_spec.rb
@@ -52,7 +52,7 @@
describe '#valid_sign_in?' do
before do
- Namespace.with_disabled_organization_validation { gl_user.save! }
+ gl_user.save!
end
it 'returns true' do
diff --git a/spec/lib/gitlab/auth/o_auth/user_spec.rb b/spec/lib/gitlab/auth/o_auth/user_spec.rb
index 4c72a91b1211c..8c1f90c6beaa0 100644
--- a/spec/lib/gitlab/auth/o_auth/user_spec.rb
+++ b/spec/lib/gitlab/auth/o_auth/user_spec.rb
@@ -31,10 +31,6 @@
let(:ldap_user) { Gitlab::Auth::Ldap::Person.new(Net::LDAP::Entry.new, 'ldapmain') }
let(:ldap_user_2) { Gitlab::Auth::Ldap::Person.new(Net::LDAP::Entry.new, 'ldapmain') }
- around do |example|
- Namespace.with_disabled_organization_validation { example.run }
- end
-
describe '.find_by_uid_and_provider' do
let(:provider) { 'provider' }
let(:uid) { 'uid' }
diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb
index 9d2dab2e08f4c..2702f475422d8 100644
--- a/spec/services/groups/create_service_spec.rb
+++ b/spec/services/groups/create_service_spec.rb
@@ -4,8 +4,13 @@
RSpec.describe Groups::CreateService, '#execute', feature_category: :groups_and_projects do
let_it_be(:user, reload: true) { create(:user) }
+ let_it_be(:organization) { create(:organization, users: [user]) }
let(:current_user) { user }
- let(:group_params) { { path: 'group_path', visibility_level: Gitlab::VisibilityLevel::PUBLIC }.merge(extra_params) }
+ let(:group_params) do
+ { path: 'group_path', visibility_level: Gitlab::VisibilityLevel::PUBLIC,
+ organization_id: organization.id }.merge(extra_params)
+ end
+
let(:extra_params) { {} }
let(:created_group) { response[:group] }
@@ -220,11 +225,11 @@
end
end
- context 'when organization is not set by params', :with_current_organization do
+ context 'when organization is not set by params' do
context 'and the parent of the group has an organization' do
let_it_be(:parent_group) { create(:group, organization: other_organization) }
- let(:extra_params) { { parent_id: parent_group.id } }
+ let(:group_params) { { path: 'with-parent', parent_id: parent_group.id } }
it 'creates group with the parent group organization' do
expect(created_group.organization).to eq(other_organization)
@@ -232,26 +237,18 @@
end
end
- context 'when organization_id is set to nil' do
+ context 'when organization_id is not specified' do
let_it_be(:default_organization) { create(:organization, :default) }
- let(:extra_params) { { organization_id: nil } }
+ let(:group_params) { { path: 'group_path' } }
it 'creates group in default organization' do
expect(created_group.organization).to eq(default_organization)
end
end
-
- context 'when organization is not set at all' do
- it 'creates group without an organization' do
- expect(created_group.organization).to eq(nil)
- # let db default happen even if the organization record itself doesn't exist
- expect(created_group.organization_id).not_to be_nil
- end
- end
end
context 'for a subgroup' do
- let_it_be(:group) { create(:group) }
+ let_it_be(:group) { create(:group, organization: organization) }
let(:extra_params) { { parent_id: group.id } }
context 'as group owner' do
@@ -337,7 +334,7 @@
context 'when there is a group-level exclusion' do
let(:extra_params) { { parent_id: group.id } }
- let_it_be(:group) { create(:group) { |g| g.add_owner(user) } }
+ let_it_be(:group) { create(:group, organization: organization) { |g| g.add_owner(user) } }
let_it_be(:group_integration) do
create(:beyond_identity_integration, group: group, instance: false, active: false)
end
@@ -363,7 +360,7 @@
context 'with an active group-level integration' do
let(:extra_params) { { parent_id: group.id } }
- let_it_be(:group) { create(:group) { |g| g.add_owner(user) } }
+ let_it_be(:group) { create(:group, organization: organization) { |g| g.add_owner(user) } }
let_it_be(:group_integration) do
create(:prometheus_integration, :group, group: group, api_url: 'https://prometheus.group.com/')
end
diff --git a/spec/services/users/create_service_spec.rb b/spec/services/users/create_service_spec.rb
index 8875c4b12463d..45402097d8233 100644
--- a/spec/services/users/create_service_spec.rb
+++ b/spec/services/users/create_service_spec.rb
@@ -4,17 +4,19 @@
RSpec.describe Users::CreateService, feature_category: :user_management do
describe '#execute' do
+ let_it_be(:organization) { create(:organization) }
let(:password) { User.random_password }
let(:admin_user) { create(:admin) }
+ let(:email) { 'jd@example.com' }
+ let(:base_params) do
+ { name: 'John Doe', username: 'jduser', email: email, password: password, organization_id: organization.id }
+ end
context 'with an admin user' do
let(:service) { described_class.new(admin_user, params) }
- let(:email) { 'jd@example.com' }
context 'when required parameters are provided' do
- let(:params) do
- { name: 'John Doe', username: 'jduser', email: email, password: password }
- end
+ let(:params) { base_params }
it 'returns a persisted user' do
expect(service.execute).to be_persisted
@@ -88,9 +90,7 @@
end
context 'when force_random_password parameter is true' do
- let(:params) do
- { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: password, force_random_password: true }
- end
+ let(:params) { base_params.merge(force_random_password: true) }
it 'generates random password' do
user = service.execute
@@ -101,15 +101,7 @@
end
context 'when password_automatically_set parameter is true' do
- let(:params) do
- {
- name: 'John Doe',
- username: 'jduser',
- email: 'jd@example.com',
- password: password,
- password_automatically_set: true
- }
- end
+ let(:params) { base_params.merge(password_automatically_set: true) }
it 'persists the given attributes' do
user = service.execute
@@ -127,9 +119,7 @@
end
context 'when skip_confirmation parameter is true' do
- let(:params) do
- { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: password, skip_confirmation: true }
- end
+ let(:params) { base_params.merge(skip_confirmation: true) }
it 'confirms the user' do
expect(service.execute).to be_confirmed
@@ -137,9 +127,7 @@
end
context 'when reset_password parameter is true' do
- let(:params) do
- { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: password, reset_password: true }
- end
+ let(:params) { base_params.merge(reset_password: true) }
it 'resets password even if a password parameter is given' do
expect(service.execute).to be_recently_sent_password_reset
@@ -158,9 +146,7 @@
end
context 'with nil user' do
- let(:params) do
- { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: password, skip_confirmation: true }
- end
+ let(:params) { base_params.merge(skip_confirmation: true) }
let(:service) { described_class.new(nil, params) }
diff --git a/spec/support/helpers/disable_namespace_organization_validation.yml b/spec/support/helpers/disable_namespace_organization_validation.yml
index d5bda5ca8d89a..d1f8842262164 100644
--- a/spec/support/helpers/disable_namespace_organization_validation.yml
+++ b/spec/support/helpers/disable_namespace_organization_validation.yml
@@ -23,6 +23,4 @@
- spec/lib/gitlab/auth/saml/user_spec.rb
- spec/models/hooks/system_hook_spec.rb
- spec/requests/api/groups_spec.rb
-- spec/services/users/create_service_spec.rb
-- spec/services/groups/create_service_spec.rb
- spec/services/resource_access_tokens/create_service_spec.rb
--
GitLab