Skip to content

G
gitlab-ce
Commit 6eba0dd7 authored by Douwe Maan's avatar Douwe Maan
Browse files
Options

Merge branch 'ee-46758-fallout-of-cacheable-attribute' into 'master' 

[EE] Ensure ApplicationSetting#performance_bar_allowed_group_id is properly set when retrieved from cache

Closes gitlab-ce#46758

See merge request gitlab-org/gitlab-ee!5869
parents 71d59fc7 0ae45b9e
Hide whitespace changes
Inline Side-by-side
Showing with 194 additions and 179 deletions
app/helpers/application_settings_helper.rb
View file @ 6eba0dd7
......@@ -205,7 +205,7 @@ module ApplicationSettingsHelper
:pages_domain_verification_enabled,
:password_authentication_enabled_for_web,
:password_authentication_enabled_for_git,
:performance_bar_allowed_group_id,
:performance_bar_allowed_group_path,
:performance_bar_enabled,
:plantuml_enabled,
:plantuml_url,
......
app/models/application_setting.rb
View file @ 6eba0dd7
......@@ -231,6 +231,7 @@ class ApplicationSetting < ActiveRecord::Base
after_commit do
reset_memoized_terms
end
after_commit :expire_performance_bar_allowed_user_ids_cache, if: -> { previous_changes.key?('performance_bar_allowed_group_id') }
def self.defaults
{
......@@ -387,31 +388,6 @@ class ApplicationSetting < ActiveRecord::Base
super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) })
end
def performance_bar_allowed_group_id=(group_full_path)
group_full_path = nil if group_full_path.blank?
if group_full_path.nil?
if group_full_path != performance_bar_allowed_group_id
super(group_full_path)
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
end
return
end
group = Group.find_by_full_path(group_full_path)
if group
if group.id != performance_bar_allowed_group_id
super(group.id)
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
end
else
super(nil)
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
end
end
def performance_bar_allowed_group
Group.find_by_id(performance_bar_allowed_group_id)
end
......@@ -421,15 +397,6 @@ class ApplicationSetting < ActiveRecord::Base
performance_bar_allowed_group_id.present?
end
# - If `enable` is true, we early return since the actual attribute that holds
# the enabling/disabling is `performance_bar_allowed_group_id`
# - If `enable` is false, we set `performance_bar_allowed_group_id` to `nil`
def performance_bar_enabled=(enable)
return if Gitlab::Utils.to_boolean(enable)
self.performance_bar_allowed_group_id = nil
end
# Choose one of the available repository storage options. Currently all have
# equal weighting.
def pick_repository_storage
......@@ -507,4 +474,8 @@ class ApplicationSetting < ActiveRecord::Base
errors.add(:terms, "You need to set terms to be enforced") unless terms.present?
end
def expire_performance_bar_allowed_user_ids_cache
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
end
end
app/services/application_settings/update_service.rb
View file @ 6eba0dd7
......@@ -7,6 +7,10 @@ module ApplicationSettings
update_terms(@params.delete(:terms))
if params.key?(:performance_bar_allowed_group_path)
params[:performance_bar_allowed_group_id] = performance_bar_allowed_group_id
end
@application_setting.update(@params)
end
......@@ -22,5 +26,13 @@ module ApplicationSettings
ApplicationSetting::Term.create(terms: terms)
@application_setting.reset_memoized_terms
end
def performance_bar_allowed_group_id
performance_bar_enabled = !params.key?(:performance_bar_enabled) || params.delete(:performance_bar_enabled)
group_full_path = params.delete(:performance_bar_allowed_group_path)
return nil unless Gitlab::Utils.to_boolean(performance_bar_enabled)
Group.find_by_full_path(group_full_path)&.id if group_full_path.present?
end
end
end
app/views/admin/application_settings/_performance_bar.html.haml
View file @ 6eba0dd7
......@@ -9,8 +9,8 @@
= f.check_box :performance_bar_enabled
Enable the Performance Bar
.form-group.row
= f.label :performance_bar_allowed_group_id, 'Allowed group', class: 'col-form-label col-sm-2'
= f.label :performance_bar_allowed_group_path, 'Allowed group', class: 'col-form-label col-sm-2'
.col-sm-10
= f.text_field :performance_bar_allowed_group_id, class: 'form-control', placeholder: 'my-org/my-group', value: @application_setting.performance_bar_allowed_group&.full_path
= f.text_field :performance_bar_allowed_group_path, class: 'form-control', placeholder: 'my-org/my-group', value: @application_setting.performance_bar_allowed_group&.full_path
= f.submit 'Save changes', class: "btn btn-success"
doc/api/settings.md
View file @ 6eba0dd7
......@@ -55,6 +55,7 @@ Example response:
"ed25519_key_restriction": 0,
"enforce_terms": true,
"terms": "Hello world!",
"performance_bar_allowed_group_id": 42
}
```
......@@ -133,8 +134,9 @@ PUT /application/settings
| `metrics_timeout` | integer | yes (if `metrics_enabled` is `true`) | The amount of seconds after which InfluxDB will time out. |
| `password_authentication_enabled_for_web` | boolean | no | Enable authentication for the web interface via a GitLab account password. Default is `true`. |
| `password_authentication_enabled_for_git` | boolean | no | Enable authentication for Git over HTTP(S) via a GitLab account password. Default is `true`. |
| `performance_bar_allowed_group_id` | string | no | The group that is allowed to enable the performance bar |
| `performance_bar_enabled` | boolean | no | Allow enabling the performance bar |
| `performance_bar_allowed_group_path` | string | no | Path of the group that is allowed to toggle the performance bar |
| `performance_bar_allowed_group_id` | string | no | Deprecated: Use `performance_bar_allowed_group_path` instead. Path of the group that is allowed to toggle the performance bar |
| `performance_bar_enabled` | boolean | no | Deprecated: Pass `performance_bar_allowed_group_path: nil` instead. Allow enabling the performance bar |
| `plantuml_enabled` | boolean | no | Enable PlantUML integration. Default is `false`. |
| `plantuml_url` | string | yes (if `plantuml_enabled` is `true`) | The PlantUML instance URL for integration. |
| `polling_interval_multiplier` | decimal | no | Interval multiplier used by endpoints that perform polling. Set to 0 to disable polling. |
......@@ -228,5 +230,6 @@ Example response:
"ed25519_key_restriction": 0,
"enforce_terms": true,
"terms": "Hello world!",
"performance_bar_allowed_group_id": 42
}
```
lib/api/entities.rb
View file @ 6eba0dd7
......@@ -961,8 +961,16 @@ module API
end
class ApplicationSetting < Grape::Entity
expose :id
expose(*::ApplicationSettingsHelper.visible_attributes)
def self.exposed_attributes
attributes = ::ApplicationSettingsHelper.visible_attributes
attributes.delete(:performance_bar_allowed_group_path)
attributes.delete(:performance_bar_enabled)
attributes
end
expose :id, :performance_bar_allowed_group_id
expose(*exposed_attributes)
expose(:restricted_visibility_levels) do |setting, _options|
setting.restricted_visibility_levels.map { |level| Gitlab::VisibilityLevel.string_level(level) }
end
......
lib/api/settings.rb
View file @ 6eba0dd7
......@@ -49,6 +49,9 @@ module API
optional :signin_enabled, type: Boolean, desc: 'Flag indicating if password authentication is enabled for the web interface' # support legacy names, can be removed in v5
mutually_exclusive :password_authentication_enabled_for_web, :password_authentication_enabled, :signin_enabled
optional :password_authentication_enabled_for_git, type: Boolean, desc: 'Flag indicating if password authentication is enabled for Git over HTTP(S)'
optional :performance_bar_allowed_group_path, type: String, desc: 'Path of the group that is allowed to toggle the performance bar.'
optional :performance_bar_allowed_group_id, type: String, desc: 'Depreated: Use :performance_bar_allowed_group_path instead. Path of the group that is allowed to toggle the performance bar.' # support legacy names, can be removed in v6
optional :performance_bar_enabled, type: String, desc: 'Deprecated: Pass `performance_bar_allowed_group_path: nil` instead. Allow enabling the performance.' # support legacy names, can be removed in v6
optional :require_two_factor_authentication, type: Boolean, desc: 'Require all users to setup Two-factor authentication'
given require_two_factor_authentication: ->(val) { val } do
requires :two_factor_grace_period, type: Integer, desc: 'Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication'
......@@ -151,18 +154,37 @@ module API
optional :repository_storages, type: Array[String], desc: 'A list of names of enabled storage paths, taken from `gitlab.yml`. New projects will be created in one of these stores, chosen at random.'
optional :repository_size_limit, type: Integer, desc: 'Size limit per repository (MB)'
all_attributes = ApplicationSettingsHelper.visible_attributes
optional_attributes = ::ApplicationSettingsHelper.visible_attributes << :performance_bar_allowed_group_id
## EE-only
all_attributes += EE::ApplicationSettingsHelper.possible_licensed_attributes
## EE-only START
optional_attributes += EE::ApplicationSettingsHelper.possible_licensed_attributes
## EE-only END
optional(*all_attributes)
at_least_one_of(*all_attributes)
optional(*optional_attributes)
at_least_one_of(*optional_attributes)
end
put "application/settings" do
attrs = declared_params(include_missing: false)
## EE-only: Remove unlicensed attributes
# support legacy names, can be removed in v6
if attrs.has_key?(:performance_bar_allowed_group_id)
attrs[:performance_bar_allowed_group_path] = attrs.delete(:performance_bar_allowed_group_id)
end
# support legacy names, can be removed in v6
if attrs.has_key?(:performance_bar_enabled)
performance_bar_enabled = attrs.delete(:performance_bar_allowed_group_id)
attrs[:performance_bar_allowed_group_path] = nil unless performance_bar_enabled
end
# support legacy names, can be removed in v5
if attrs.has_key?(:signin_enabled)
attrs[:password_authentication_enabled_for_web] = attrs.delete(:signin_enabled)
elsif attrs.has_key?(:password_authentication_enabled)
attrs[:password_authentication_enabled_for_web] = attrs.delete(:password_authentication_enabled)
end
## EE-only START: Remove unlicensed attributes
unless ::License.feature_available?(:repository_mirrors)
attrs = attrs.except(*::EE::ApplicationSettingsHelper.repository_mirror_attributes)
end
......@@ -174,13 +196,7 @@ module API
unless ::License.feature_available?(:email_additional_text)
attrs = attrs.except(:email_additional_text)
end
# support legacy names, can be removed in v5
if attrs.has_key?(:signin_enabled)
attrs[:password_authentication_enabled_for_web] = attrs.delete(:signin_enabled)
elsif attrs.has_key?(:password_authentication_enabled)
attrs[:password_authentication_enabled_for_web] = attrs.delete(:password_authentication_enabled)
end
## EE-only END: Remove unlicensed attributes
if ApplicationSettings::UpdateService.new(current_settings, current_user, attrs).execute
present current_settings, with: Entities::ApplicationSetting
......
rubocop/cop/line_break_around_conditional_block.rb
View file @ 6eba0dd7
......@@ -95,7 +95,7 @@ module RuboCop
end
def end_clause_line?(line)
line =~ /^\s*(rescue|else|elsif|when)/
line =~ /^\s*(#|rescue|else|elsif|when)/
end
def begin_line?(line)
......
spec/factories/application_settings.rb
View file @ 6eba0dd7
FactoryBot.define do
factory :application_setting do
default_projects_limit 42
end
end
spec/models/application_setting_spec.rb
View file @ 6eba0dd7
......@@ -391,68 +391,6 @@ describe ApplicationSetting do
end
describe 'performance bar settings' do
describe 'performance_bar_allowed_group_id=' do
context 'with a blank path' do
before do
setting.performance_bar_allowed_group_id = create(:group).full_path
end
it 'persists nil for a "" path and clears allowed user IDs cache' do
expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_allowed_group_id = ''
expect(setting.performance_bar_allowed_group_id).to be_nil
end
end
context 'with an invalid path' do
it 'does not persist an invalid group path' do
setting.performance_bar_allowed_group_id = 'foo'
expect(setting.performance_bar_allowed_group_id).to be_nil
end
end
context 'with a path to an existing group' do
let(:group) { create(:group) }
it 'persists a valid group path and clears allowed user IDs cache' do
expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_allowed_group_id = group.full_path
expect(setting.performance_bar_allowed_group_id).to eq(group.id)
end
context 'when the given path is the same' do
context 'with a blank path' do
before do
setting.performance_bar_allowed_group_id = nil
end
it 'clears the cached allowed user IDs' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_allowed_group_id = ''
end
end
context 'with a valid path' do
before do
setting.performance_bar_allowed_group_id = group.full_path
end
it 'clears the cached allowed user IDs' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_allowed_group_id = group.full_path
end
end
end
end
end
describe 'performance_bar_allowed_group' do
context 'with no performance_bar_allowed_group_id saved' do
it 'returns nil' do
......@@ -464,11 +402,11 @@ describe ApplicationSetting do
let(:group) { create(:group) }
before do
setting.performance_bar_allowed_group_id = group.full_path
setting.update!(performance_bar_allowed_group_id: group.id)
end
it 'returns the group' do
expect(setting.performance_bar_allowed_group).to eq(group)
expect(setting.reload.performance_bar_allowed_group).to eq(group)
end
end
end
......@@ -478,67 +416,11 @@ describe ApplicationSetting do
let(:group) { create(:group) }
before do
setting.performance_bar_allowed_group_id = group.full_path
setting.update!(performance_bar_allowed_group_id: group.id)
end
it 'returns true' do
expect(setting.performance_bar_enabled).to be_truthy
end
end
end
describe 'performance_bar_enabled=' do
context 'when the performance bar is enabled' do
let(:group) { create(:group) }
before do
setting.performance_bar_allowed_group_id = group.full_path
end
context 'when passing true' do
it 'does not clear allowed user IDs cache' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_enabled = true
expect(setting.performance_bar_allowed_group_id).to eq(group.id)
expect(setting.performance_bar_enabled).to be_truthy
end
end
context 'when passing false' do
it 'disables the performance bar and clears allowed user IDs cache' do
expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_enabled = false
expect(setting.performance_bar_allowed_group_id).to be_nil
expect(setting.performance_bar_enabled).to be_falsey
end
end
end
context 'when the performance bar is disabled' do
context 'when passing true' do
it 'does nothing and does not clear allowed user IDs cache' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_enabled = true
expect(setting.performance_bar_allowed_group_id).to be_nil
expect(setting.performance_bar_enabled).to be_falsey
end
end
context 'when passing false' do
it 'does nothing and does not clear allowed user IDs cache' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_enabled = false
expect(setting.performance_bar_allowed_group_id).to be_nil
expect(setting.performance_bar_enabled).to be_falsey
end
expect(setting.reload.performance_bar_enabled).to be_truthy
end
end
end
......
spec/requests/api/settings_spec.rb
View file @ 6eba0dd7
......@@ -26,10 +26,15 @@ describe API::Settings, 'Settings' do
expect(json_response['ecdsa_key_restriction']).to eq(0)
expect(json_response['ed25519_key_restriction']).to eq(0)
expect(json_response['circuitbreaker_failure_count_threshold']).not_to be_nil
expect(json_response['performance_bar_allowed_group_id']).to be_nil
expect(json_response).not_to have_key('performance_bar_allowed_group_path')
expect(json_response).not_to have_key('performance_bar_enabled')
end
end
describe "PUT /application/settings" do
let(:group) { create(:group) }
context "custom repository storage type set in the config" do
before do
storages = { 'custom' => 'tmp/tests/custom_repositories' }
......@@ -58,7 +63,8 @@ describe API::Settings, 'Settings' do
ed25519_key_restriction: 256,
circuitbreaker_check_interval: 2,
enforce_terms: true,
terms: 'Hello world!'
terms: 'Hello world!',
performance_bar_allowed_group_path: group.full_path
expect(response).to have_gitlab_http_status(200)
expect(json_response['default_projects_limit']).to eq(3)
......@@ -82,9 +88,27 @@ describe API::Settings, 'Settings' do
expect(json_response['circuitbreaker_check_interval']).to eq(2)
expect(json_response['enforce_terms']).to be(true)
expect(json_response['terms']).to eq('Hello world!')
expect(json_response['performance_bar_allowed_group_id']).to eq(group.id)
end
end
it "supports legacy performance_bar_allowed_group_id" do
put api("/application/settings", admin),
performance_bar_allowed_group_id: group.full_path
expect(response).to have_gitlab_http_status(200)
expect(json_response['performance_bar_allowed_group_id']).to eq(group.id)
end
it "supports legacy performance_bar_enabled" do
put api("/application/settings", admin),
performance_bar_enabled: false,
performance_bar_allowed_group_id: group.full_path
expect(response).to have_gitlab_http_status(200)
expect(json_response['performance_bar_allowed_group_id']).to be_nil
end
context "missing koding_url value when koding_enabled is true" do
it "returns a blank parameter error message" do
put api("/application/settings", admin), koding_enabled: true
......
spec/rubocop/cop/line_break_around_conditional_block_spec.rb
View file @ 6eba0dd7
......@@ -256,6 +256,18 @@ describe RuboCop::Cop::LineBreakAroundConditionalBlock do
expect(cop.offenses).to be_empty
end
it "doesn't flag violation for #{conditional} followed by a comment" do
source = <<~RUBY
#{conditional} condition
do_something
end
# a short comment
RUBY
inspect_source(source)
expect(cop.offenses).to be_empty
end
it "doesn't flag violation for #{conditional} followed by an end" do
source = <<~RUBY
class Foo
......
spec/services/application_settings/update_service_spec.rb
View file @ 6eba0dd7
require 'spec_helper'
describe ApplicationSettings::UpdateService do
let(:application_settings) { Gitlab::CurrentSettings.current_application_settings }
let(:application_settings) { create(:application_setting) }
let(:admin) { create(:user, :admin) }
let(:params) { {} }
......@@ -54,4 +54,90 @@ describe ApplicationSettings::UpdateService do
end
end
end
describe 'performance bar settings' do
using RSpec::Parameterized::TableSyntax
where(:params_performance_bar_enabled,
:params_performance_bar_allowed_group_path,
:previous_performance_bar_allowed_group_id,
:expected_performance_bar_allowed_group_id) do
true | '' | nil | nil
true | '' | 42_000_000 | nil
true | nil | nil | nil
true | nil | 42_000_000 | nil
true | 'foo' | nil | nil
true | 'foo' | 42_000_000 | nil
true | 'group_a' | nil | 42_000_000
true | 'group_b' | 42_000_000 | 43_000_000
true | 'group_a' | 42_000_000 | 42_000_000
false | '' | nil | nil
false | '' | 42_000_000 | nil
false | nil | nil | nil
false | nil | 42_000_000 | nil
false | 'foo' | nil | nil
false | 'foo' | 42_000_000 | nil
false | 'group_a' | nil | nil
false | 'group_b' | 42_000_000 | nil
false | 'group_a' | 42_000_000 | nil
end
with_them do
let(:params) do
{
performance_bar_enabled: params_performance_bar_enabled,
performance_bar_allowed_group_path: params_performance_bar_allowed_group_path
}
end
before do
if previous_performance_bar_allowed_group_id == 42_000_000 || params_performance_bar_allowed_group_path == 'group_a'
create(:group, id: 42_000_000, path: 'group_a')
end
if expected_performance_bar_allowed_group_id == 43_000_000 || params_performance_bar_allowed_group_path == 'group_b'
create(:group, id: 43_000_000, path: 'group_b')
end
application_settings.update!(performance_bar_allowed_group_id: previous_performance_bar_allowed_group_id)
end
it 'sets performance_bar_allowed_group_id when present and performance_bar_enabled == true' do
expect(application_settings.performance_bar_allowed_group_id).to eq(previous_performance_bar_allowed_group_id)
if previous_performance_bar_allowed_group_id != expected_performance_bar_allowed_group_id
expect { subject.execute }
.to change(application_settings, :performance_bar_allowed_group_id)
.from(previous_performance_bar_allowed_group_id).to(expected_performance_bar_allowed_group_id)
else
expect { subject.execute }
.not_to change(application_settings, :performance_bar_allowed_group_id)
end
end
end
context 'when :performance_bar_allowed_group_path is not present' do
let(:group) { create(:group) }
before do
application_settings.update!(performance_bar_allowed_group_id: group.id)
end
it 'does not change the performance bar settings' do
expect { subject.execute }
.not_to change(application_settings, :performance_bar_allowed_group_id)
end
end
context 'when :performance_bar_enabled is not present' do
let(:group) { create(:group) }
let(:params) { { performance_bar_allowed_group_path: group.full_path } }
it 'implicitely defaults to true' do
expect { subject.execute }
.to change(application_settings, :performance_bar_allowed_group_id)
.from(nil).to(group.id)
end
end
end
end
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7