diff --git a/.rubocop_todo/layout/class_structure.yml b/.rubocop_todo/layout/class_structure.yml index dded187d89e7b6e745b832b525b24c4f000cc23e..e638fe947a2d56d1ac60896627dedc5bd92dae31 100644 --- a/.rubocop_todo/layout/class_structure.yml +++ b/.rubocop_todo/layout/class_structure.yml @@ -452,7 +452,6 @@ Layout/ClassStructure: - 'lib/gitlab/metrics/requests_rack_middleware.rb' - 'lib/gitlab/metrics/subscribers/active_record.rb' - 'lib/gitlab/metrics/subscribers/load_balancing.rb' - - 'lib/gitlab/middleware/organizations/current.rb' - 'lib/gitlab/middleware/same_site_cookies.rb' - 'lib/gitlab/multi_destination_logger.rb' - 'lib/gitlab/pagination/keyset/order.rb' diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7f4b27c960150cce40e47cf83d0c7c3dccf13c3a..d7586f1759f775c232d1ddd6a6b03b8c66720e90 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -28,6 +28,7 @@ class ApplicationController < BaseActionController include StrongPaginationParams before_action :authenticate_user!, except: [:route_not_found] + before_action :set_current_organization before_action :enforce_terms!, if: :should_enforce_terms? before_action :check_password_expiration, if: :html_request? before_action :ldap_security_check @@ -524,6 +525,15 @@ def allow_gitaly_ref_name_caching def context_user auth_user if strong_memoized?(:auth_user) end + + def set_current_organization + return if ::Current.lock_organization + + ::Current.organization = Gitlab::Current::Organization.new( + params: params.permit(:controller, :namespace_id, :group_id, :id), + user: current_user + ).organization + end end ApplicationController.prepend_mod diff --git a/app/models/organizations/organization.rb b/app/models/organizations/organization.rb index 0c301d7340d54a4359110586e13b88ebd79d4a1f..fd751d24ea3940e0acc2729b9ea6cda0fcd6bfc5 100644 --- a/app/models/organizations/organization.rb +++ b/app/models/organizations/organization.rb @@ -9,6 +9,12 @@ class Organization < MainClusterwide::ApplicationRecord DEFAULT_ORGANIZATION_ID = 1 scope :without_default, -> { where.not(id: DEFAULT_ORGANIZATION_ID) } + scope :with_namespace_path, ->(path) { + joins(namespaces: :route).where(route: { path: path.to_s }) + } + scope :with_user, ->(user) { + joins(:users).where(users: user).order(Arel.sql('organization_users.id asc')) + } before_destroy :check_if_default_organization diff --git a/config/initializers/current_organization.rb b/config/initializers/current_organization.rb deleted file mode 100644 index 9c9e9ac09d14d64ce4f40a63a65b3b15ac6989f3..0000000000000000000000000000000000000000 --- a/config/initializers/current_organization.rb +++ /dev/null @@ -1,5 +0,0 @@ -# frozen_string_literal: true - -Rails.application.configure do |config| - config.middleware.use(Gitlab::Middleware::Organizations::Current) -end diff --git a/ee/spec/requests/api/graphql/vulnerabilities/issue_links_spec.rb b/ee/spec/requests/api/graphql/vulnerabilities/issue_links_spec.rb index fcef65b569e536a9572c6dc64ecd24976e17175e..412783fbf7b46967106f7eb2184717c4834e8c83 100644 --- a/ee/spec/requests/api/graphql/vulnerabilities/issue_links_spec.rb +++ b/ee/spec/requests/api/graphql/vulnerabilities/issue_links_spec.rb @@ -80,31 +80,32 @@ # 3) Insert into personal access tokens # 4) Release savepoint # 5) Select personal access tokens - # 6) Select geo nodes - # 7) Update personal access tokens(last used at) - # 8) Select user - # 9) Authorization check - # 10) Select vulnerability_reads - # 11) Select vulnerabilities - # 12) Select project - # 13) Select route - # 14) Select vulnerability occurrences - # 15) Select vulnerability reads - # 16) Select vulnerability scanners - # 17) Select vulnerability identifiers join table - # 18) Select vulnerability identifiers - # 19) Select namespace - # 20) Select group links - # 21) Select project features - # 22) Authorization check - # 23) Select issue links - # 24) Select issues - # 25) Select issue project - # 26) Loading the project authorizations - # 27) Loading the namespace - # 28) Loading the user - # 29) Loading the organization - expect { query_issue_links }.not_to exceed_query_limit(29) + # 6) Select current organization + # 7) Select geo nodes + # 8) Update personal access tokens(last used at) + # 9) Select user + # 10) Authorization check + # 12) Select vulnerability_reads + # 13) Select vulnerabilities + # 13) Select project + # 14) Select route + # 15) Select vulnerability occurrences + # 16) Select vulnerability reads + # 17) Select vulnerability scanners + # 18) Select vulnerability identifiers join table + # 19) Select vulnerability identifiers + # 20) Select namespace + # 21) Select group links + # 22) Select project features + # 23) Authorization check + # 24) Select issue links + # 25) Select issues + # 26) Select issue project + # 27) Loading the project authorizations + # 28) Loading the namespace + # 29) Loading the user + # 30) Loading the organization + expect { query_issue_links }.not_to exceed_query_limit(30) end end diff --git a/lib/gitlab/current/organization.rb b/lib/gitlab/current/organization.rb new file mode 100644 index 0000000000000000000000000000000000000000..a2f2ed4d8b6178ded9fa44464f8c5a5b11a0de72 --- /dev/null +++ b/lib/gitlab/current/organization.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Gitlab + module Current + class Organization + attr_reader :params, :user + + def initialize(params: {}, user: nil) + @params = params + @user = user + end + + def organization + from_params || from_user || ::Organizations::Organization.default_organization + end + + def from_params + path = params[:namespace_id] || params[:group_id] + path ||= params[:id] if params[:controller] == 'groups' + + return if path.blank? + + ::Organizations::Organization.with_namespace_path(path).first + end + + def from_user + return unless user + + ::Organizations::Organization.with_user(user).first + end + end + end +end diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index cb8541658a711b0f19567718de7401f6db6af87c..4480b763f2108bc23976660601f7caa08a56ef38 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -30,12 +30,12 @@ def method_missing(name, *args, **kwargs, &block) return application_settings.send(name, *args, **kwargs, &block) if application_settings.respond_to?(name) - Organizations::OrganizationSetting.for(Current.organization_id).send(name, *args, **kwargs, &block) + Organizations::OrganizationSetting.for(::Current.organization_id).send(name, *args, **kwargs, &block) end # rubocop:enable GitlabSecurity/PublicSend def respond_to_missing?(name, include_private = false) - current_application_settings.respond_to?(name, include_private) || Organizations::OrganizationSetting.for(Current.organization_id).respond_to?(name, include_private) || super + current_application_settings.respond_to?(name, include_private) || Organizations::OrganizationSetting.for(::Current.organization_id).respond_to?(name, include_private) || super end end end diff --git a/lib/gitlab/github_gists_import/importer/gist_importer.rb b/lib/gitlab/github_gists_import/importer/gist_importer.rb index 2fa35f2bc28913b29eb71b333dbf506930583277..925a93992782aea9138bcc67d4a335c4996e6578 100644 --- a/lib/gitlab/github_gists_import/importer/gist_importer.rb +++ b/lib/gitlab/github_gists_import/importer/gist_importer.rb @@ -39,7 +39,7 @@ def execute # app/services/snippets/create_service.rb # Remove as part of refactor in: https://gitlab.com/gitlab-org/gitlab/-/issues/469564 def organization_id - Current.organization_id || Organizations::Organization::DEFAULT_ORGANIZATION_ID + ::Current.organization_id || Organizations::Organization::DEFAULT_ORGANIZATION_ID end def build_snippet diff --git a/lib/gitlab/middleware/organizations/current.rb b/lib/gitlab/middleware/organizations/current.rb deleted file mode 100644 index 1277459097e074755d5b936ff62daf9deab8ba6f..0000000000000000000000000000000000000000 --- a/lib/gitlab/middleware/organizations/current.rb +++ /dev/null @@ -1,41 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Middleware - module Organizations - # Logic of setting the Current.organization: - # - Request header value from injection on frontend - # - TODO: Request header from injection from routing layer - # see ideas in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/144811#note_1784126192 - class Current - def initialize(app) - @app = app - end - - def call(env) - @request = Rack::Request.new(env) - - ::Current.organization = calculated_organization - - @app.call(env) - end - - private - - POSITIVE_INTEGER_REGEX = %r{\A[1-9]\d*\z} - - def calculated_organization - find_from_header - end - - def find_from_header - header_organization_id = @request.get_header(::Organizations::ORGANIZATION_HTTP_HEADER) - - return unless header_organization_id.to_s.match?(POSITIVE_INTEGER_REGEX) # don't do unnecessary query - - ::Organizations::Organization.find_by_id(header_organization_id) - end - end - end - end -end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 7a5d12a21def5086c49bf0b04f2671a91dbf8755..6f595a6996d0956beb115cb986d5c4ffe3f0e5b7 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -56,6 +56,31 @@ end end + describe '#set_current_organization' do + let_it_be(:current_organization) { create(:organization, :default) } + + before do + sign_in user + end + + controller(described_class) do + def index; end + end + + it 'sets current organization' do + expect { get :index, format: :json }.to change { Current.organization }.from(nil).to(current_organization) + end + + context 'when multiple calls in one example are done' do + it 'does not update the organization' do + expect(Current).to receive(:organization=).once.and_call_original + + get :index, format: :json + get :index, format: :json + end + end + end + describe '#add_gon_variables' do before do Gon.clear diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb index 723b962737ac1793e337a4a6a9394dbd80a533a0..3163f47ff7dfc7bc59b2bab56f0a5bf100ecd64c 100644 --- a/spec/features/groups_spec.rb +++ b/spec/features/groups_spec.rb @@ -35,33 +35,6 @@ expect(page).to have_current_path(group_path(group), ignore_query: true) expect(page).to have_selector '.visibility-icon [data-testid="earth-icon"]' end - - context 'with current organization setting in middleware' do - let_it_be(:another_organization) { create(:organization, users: [user]) } - - before_all do - create(:organization, :default) - end - - context 'for setting from the header' do - it 'sets the organization to another organization', :feature do - fill_in 'Group name', with: 'test-group' - - inspect_requests( - inject_headers: { - ::Organizations::ORGANIZATION_HTTP_HEADER.sub(/^HTTP_/, '') => another_organization.id.to_s - } - ) do - click_button 'Create group' - end - - group = Group.find_by(name: 'test-group') - - expect(group.organization).to eq(another_organization) - expect(page).to have_current_path(group_path(group), ignore_query: true) - end - end - end end describe 'with expected fields' do @@ -307,8 +280,8 @@ end context 'when many parent groups are available' do - let_it_be(:group2) { create(:group, path: 'foo2') } - let_it_be(:group3) { create(:group, path: 'foo3') } + let_it_be(:group2) { create(:group, path: 'foo2', organization: group.organization) } + let_it_be(:group3) { create(:group, path: 'foo3', organization: group.organization) } before do group.add_owner(user) diff --git a/spec/lib/gitlab/current/organization_spec.rb b/spec/lib/gitlab/current/organization_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..41447daf8186d1e73d5cbb752bbb81c831f6ccb2 --- /dev/null +++ b/spec/lib/gitlab/current/organization_spec.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Current::Organization, feature_category: :cell do + let_it_be(:other_organization) { create(:organization) } + let_it_be(:organization) { create(:organization, name: 'Current Organization') } + let_it_be(:default_organization) { create(:organization, :default) } + let_it_be(:group) { create(:group, organization: organization) } + let_it_be(:user) { create(:user) } + + let(:group_path) { group.full_path } + let(:controller) { 'some_controller' } + let(:action) { 'some_action' } + let(:params) do + { + controller: controller, + action: action + } + end + + before do + default_organization.users.delete_all + default_organization.reload + + organization.users << user + end + + describe '.organization' do + subject(:current_organization) { described_class.new(params: params, user: user).organization } + + context 'when params result in an organization' do + let(:params) { super().merge(namespace_id: group_path) } + + it 'returns that organization' do + expect(current_organization).to eq(organization) + end + end + + context 'when only current user result in an organization' do + it 'returns that organization' do + expect(current_organization).to eq(organization) + end + end + + context 'when no organization can be derived' do + subject(:current_organization) { described_class.new(params: params, user: nil).organization } + + it 'falls back to default organization' do + expect(current_organization).to eq(default_organization) + end + end + end + + describe '.from_params' do + subject(:current_organization) { described_class.new(params: params).from_params } + + context 'when params contains namespace_id' do + let(:params) { super().merge(namespace_id: group_path) } + + context 'and namespace is found' do + it { is_expected.to eq(organization) } + end + + context 'and namespace is not found' do + let(:group_path) { 'not_found' } + + it { is_expected.to be(nil) } + end + + context 'and namespace_id is empty string' do + let(:params) { super().merge(namespace_id: '') } + + it { is_expected.to be(nil) } + + it 'does not execute query' do + expect { current_organization }.to match_query_count(0) + end + end + end + + context 'when params contains group_id' do + let(:params) { super().merge(group_id: group_path) } + + context 'and namespace is found' do + it { is_expected.to eq(organization) } + end + + context 'and namespace is not found' do + let(:group_path) { 'not_found' } + + it { is_expected.to be(nil) } + end + end + + context 'when params contains id' do + let(:params) { super().merge(id: group_path) } + + context 'and controller is groups' do + let(:controller) { 'groups' } + + context 'and namespace is found' do + it { is_expected.to eq(organization) } + end + + context 'and namespace is not found' do + let(:group_path) { non_existing_record_id } + + it { is_expected.to be(nil) } + end + end + + context 'and controller is not groups' do + it { is_expected.to be(nil) } + end + end + end + + describe '.from_user' do + let_it_be(:user) { create(:user) } + + subject(:current_organization) { described_class.new(user: user).from_user } + + it 'returns the organization the user is member of' do + expect(current_organization).to eq(organization) + end + end +end diff --git a/spec/lib/gitlab/github_gists_import/importer/gist_importer_spec.rb b/spec/lib/gitlab/github_gists_import/importer/gist_importer_spec.rb index 25fcb9c580ff71efeaad4781ec956139deff39ec..7d382630532134ca013e16582f9825c3ca2f35da 100644 --- a/spec/lib/gitlab/github_gists_import/importer/gist_importer_spec.rb +++ b/spec/lib/gitlab/github_gists_import/importer/gist_importer_spec.rb @@ -53,7 +53,7 @@ expect { subject.execute }.to change { user.snippets.count }.by(1) expect(user.snippets[0].attributes).to include expected_snippet_attrs - expect(user.snippets[0].organization_id).to eq(Current.organization_id) + expect(user.snippets[0].organization_id).to eq(::Current.organization_id) end end diff --git a/spec/lib/gitlab/middleware/organizations/current_spec.rb b/spec/lib/gitlab/middleware/organizations/current_spec.rb deleted file mode 100644 index 81703d58c6656237b43cd2bb84992f0c5f349c5d..0000000000000000000000000000000000000000 --- a/spec/lib/gitlab/middleware/organizations/current_spec.rb +++ /dev/null @@ -1,59 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Middleware::Organizations::Current, feature_category: :cell do - let(:headers) { {} } - let_it_be(:organization) { create(:organization) } - - subject(:perform_request) do - path = '/' - app = ->(env) { [200, env, 'app'] } - middleware = described_class.new(app) - Rack::MockRequest.new(middleware).get(path, headers) - end - - before_all do - create(:organization) # prove we are really being selective for the organization finder - end - - after do - Current.reset - end - - it 'does not set the organization' do - perform_request - - expect(Current.organization).to be_nil - end - - context 'when the organization header is set' do - let(:headers) { { ::Organizations::ORGANIZATION_HTTP_HEADER => organization.id } } - - it 'sets the organization' do - perform_request - - expect(Current.organization).to eq(organization) - end - - context 'when organization does not exist' do - let(:headers) { { ::Organizations::ORGANIZATION_HTTP_HEADER => non_existing_record_id } } - - it 'does not set the organization' do - perform_request - - expect(Current.organization).to be_nil - end - end - - context 'when organization has non-integer value' do - let(:headers) { { ::Organizations::ORGANIZATION_HTTP_HEADER => "#{organization.id}_some_words" } } - - it 'does not set the organization' do - perform_request - - expect(Current.organization).to be_nil - end - end - end -end diff --git a/spec/models/organizations/organization_spec.rb b/spec/models/organizations/organization_spec.rb index bd8c8d199f0cfb2c8c6962b6d182c8fe27021692..f99d96f247c0aa880f735d6e098226db6d9baa83 100644 --- a/spec/models/organizations/organization_spec.rb +++ b/spec/models/organizations/organization_spec.rb @@ -155,6 +155,40 @@ expect(described_class.without_default).to include(organization) end end + + describe '.with_namespace_path' do + let_it_be(:group) { create(:group, organization: organization) } + let(:path) { group.path } + + subject(:match) { described_class.with_namespace_path(path) } + + context 'when namespace path belongs to an organiation' do + it 'returns associated organization' do + expect(match).to contain_exactly(group.organization) + end + end + + context 'when namespace path does not have an organiation' do + let(:path) { non_existing_record_id } + + it 'returns nil' do + expect(match).to be_empty + end + end + end + + describe '.with_user' do + let_it_be(:user) { create(:user) } + let_it_be(:second_organization) { create(:organization, users: [user]) } + + subject(:organizations_for_user) { described_class.with_user(user) } + + before do + organization.users << user + end + + it { is_expected.to eq([default_organization, second_organization, organization]) } + end end describe '.default_organization' do diff --git a/spec/requests/user_avatar_spec.rb b/spec/requests/user_avatar_spec.rb index f84f59a63fb9057fc2965bea9a8e82bf71df6d92..fd91ce274b3d7c0c370be89acea966f0e2c806a2 100644 --- a/spec/requests/user_avatar_spec.rb +++ b/spec/requests/user_avatar_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' RSpec.describe 'Loading a user avatar', feature_category: :user_profile do - let(:user) { create(:user, :with_avatar) } + let_it_be(:organization) { create(:organization) } + let(:user) { create(:user, :with_avatar, organizations: [organization]) } context 'when logged in' do # The exact query count will vary depending on the 2FA settings of the @@ -12,11 +13,11 @@ before do stub_application_setting(require_two_factor_authentication: true) - login_as(create(:user, :two_factor)) + login_as(create(:user, :two_factor, organizations: [organization])) end - # One each for: current user, avatar user, and upload record - it 'only performs three SQL queries' do + # One each for: current user, current organization, avatar user, and upload record + it 'only performs four SQL queries' do get user.avatar_url # Skip queries on first application load expect(response).to have_gitlab_http_status(:ok) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b300a34f0ed746461ac2c0bccdac19e31e09f7c7..2086bf8ad9d7a7f17433a50c9cd5633e5bd7d724 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -462,6 +462,9 @@ # Re-enable query limiting in case it was disabled Gitlab::QueryLimiting.enable! + + # Reset ActiveSupport::CurrentAttributes models + ActiveSupport::CurrentAttributes.reset_all end config.before(:example, :mailer) do diff --git a/spec/support/shared_contexts/current_organization_context.rb b/spec/support/shared_contexts/current_organization_context.rb index 12a460706dc7c489fa9795b927f01a52dea3a980..50157abc5fa1dcf22ad38a5cdb69cd082afedc44 100644 --- a/spec/support/shared_contexts/current_organization_context.rb +++ b/spec/support/shared_contexts/current_organization_context.rb @@ -9,10 +9,6 @@ allow(Current).to receive(:organization).and_return(current_organization) allow(Current).to receive(:organization_id).and_return(current_organization.id) end - - after do - Current.reset - end end RSpec.configure do |rspec|