Commit 2b8eb727 authored by Rémy Coutable's avatar Rémy Coutable

Ensure ApplicationSetting#performance_bar_allowed_group_id is properly set...

Ensure ApplicationSetting#performance_bar_allowed_group_id is properly set when retrieved from cache
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent c5adf04c
...@@ -204,7 +204,7 @@ module ApplicationSettingsHelper ...@@ -204,7 +204,7 @@ module ApplicationSettingsHelper
:pages_domain_verification_enabled, :pages_domain_verification_enabled,
:password_authentication_enabled_for_web, :password_authentication_enabled_for_web,
:password_authentication_enabled_for_git, :password_authentication_enabled_for_git,
:performance_bar_allowed_group_id, :performance_bar_allowed_group_path,
:performance_bar_enabled, :performance_bar_enabled,
:plantuml_enabled, :plantuml_enabled,
:plantuml_url, :plantuml_url,
......
...@@ -230,6 +230,7 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -230,6 +230,7 @@ class ApplicationSetting < ActiveRecord::Base
after_commit do after_commit do
reset_memoized_terms reset_memoized_terms
end end
after_commit :expire_performance_bar_allowed_user_ids_cache, if: -> { previous_changes.key?('performance_bar_allowed_group_id') }
def self.defaults def self.defaults
{ {
...@@ -386,31 +387,6 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -386,31 +387,6 @@ class ApplicationSetting < ActiveRecord::Base
super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) }) super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) })
end 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 def performance_bar_allowed_group
Group.find_by_id(performance_bar_allowed_group_id) Group.find_by_id(performance_bar_allowed_group_id)
end end
...@@ -420,15 +396,6 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -420,15 +396,6 @@ class ApplicationSetting < ActiveRecord::Base
performance_bar_allowed_group_id.present? performance_bar_allowed_group_id.present?
end 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 # Choose one of the available repository storage options. Currently all have
# equal weighting. # equal weighting.
def pick_repository_storage def pick_repository_storage
...@@ -506,4 +473,8 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -506,4 +473,8 @@ class ApplicationSetting < ActiveRecord::Base
errors.add(:terms, "You need to set terms to be enforced") unless terms.present? errors.add(:terms, "You need to set terms to be enforced") unless terms.present?
end end
def expire_performance_bar_allowed_user_ids_cache
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
end
end end
...@@ -3,6 +3,10 @@ module ApplicationSettings ...@@ -3,6 +3,10 @@ module ApplicationSettings
def execute def execute
update_terms(@params.delete(:terms)) 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) @application_setting.update(@params)
end end
...@@ -18,5 +22,13 @@ module ApplicationSettings ...@@ -18,5 +22,13 @@ module ApplicationSettings
ApplicationSetting::Term.create(terms: terms) ApplicationSetting::Term.create(terms: terms)
@application_setting.reset_memoized_terms @application_setting.reset_memoized_terms
end 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
end end
...@@ -9,8 +9,8 @@ ...@@ -9,8 +9,8 @@
= f.check_box :performance_bar_enabled = f.check_box :performance_bar_enabled
Enable the Performance Bar Enable the Performance Bar
.form-group.row .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 .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" = f.submit 'Save changes', class: "btn btn-success"
...@@ -55,6 +55,7 @@ Example response: ...@@ -55,6 +55,7 @@ Example response:
"ed25519_key_restriction": 0, "ed25519_key_restriction": 0,
"enforce_terms": true, "enforce_terms": true,
"terms": "Hello world!", "terms": "Hello world!",
"performance_bar_allowed_group_id": 42
} }
``` ```
...@@ -120,8 +121,9 @@ PUT /application/settings ...@@ -120,8 +121,9 @@ PUT /application/settings
| `metrics_timeout` | integer | yes (if `metrics_enabled` is `true`) | The amount of seconds after which InfluxDB will time out. | | `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_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`. | | `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_allowed_group_path` | string | no | Path of the group that is allowed to toggle the performance bar |
| `performance_bar_enabled` | boolean | no | Allow enabling 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_enabled` | boolean | no | Enable PlantUML integration. Default is `false`. |
| `plantuml_url` | string | yes (if `plantuml_enabled` is `true`) | The PlantUML instance URL for integration. | | `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. | | `polling_interval_multiplier` | decimal | no | Interval multiplier used by endpoints that perform polling. Set to 0 to disable polling. |
...@@ -201,5 +203,6 @@ Example response: ...@@ -201,5 +203,6 @@ Example response:
"ed25519_key_restriction": 0, "ed25519_key_restriction": 0,
"enforce_terms": true, "enforce_terms": true,
"terms": "Hello world!", "terms": "Hello world!",
"performance_bar_allowed_group_id": 42
} }
``` ```
...@@ -933,8 +933,16 @@ module API ...@@ -933,8 +933,16 @@ module API
end end
class ApplicationSetting < Grape::Entity class ApplicationSetting < Grape::Entity
expose :id def self.exposed_attributes
expose(*::ApplicationSettingsHelper.visible_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| expose(:restricted_visibility_levels) do |setting, _options|
setting.restricted_visibility_levels.map { |level| Gitlab::VisibilityLevel.string_level(level) } setting.restricted_visibility_levels.map { |level| Gitlab::VisibilityLevel.string_level(level) }
end end
......
...@@ -49,6 +49,9 @@ module API ...@@ -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 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 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 :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' optional :require_two_factor_authentication, type: Boolean, desc: 'Require all users to setup Two-factor authentication'
given require_two_factor_authentication: ->(val) { val } do 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' 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'
...@@ -134,12 +137,25 @@ module API ...@@ -134,12 +137,25 @@ module API
desc: "Restrictions on the complexity of uploaded #{type.upcase} keys. A value of #{ApplicationSetting::FORBIDDEN_KEY_VALUE} disables all #{type.upcase} keys." desc: "Restrictions on the complexity of uploaded #{type.upcase} keys. A value of #{ApplicationSetting::FORBIDDEN_KEY_VALUE} disables all #{type.upcase} keys."
end end
optional(*::ApplicationSettingsHelper.visible_attributes) optional_attributes = ::ApplicationSettingsHelper.visible_attributes << :performance_bar_allowed_group_id
at_least_one_of(*::ApplicationSettingsHelper.visible_attributes)
optional(*optional_attributes)
at_least_one_of(*optional_attributes)
end end
put "application/settings" do put "application/settings" do
attrs = declared_params(include_missing: false) attrs = declared_params(include_missing: false)
# 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 # support legacy names, can be removed in v5
if attrs.has_key?(:signin_enabled) if attrs.has_key?(:signin_enabled)
attrs[:password_authentication_enabled_for_web] = attrs.delete(:signin_enabled) attrs[:password_authentication_enabled_for_web] = attrs.delete(:signin_enabled)
......
FactoryBot.define do FactoryBot.define do
factory :application_setting do factory :application_setting do
default_projects_limit 42
end end
end end
...@@ -391,68 +391,6 @@ describe ApplicationSetting do ...@@ -391,68 +391,6 @@ describe ApplicationSetting do
end end
describe 'performance bar settings' do 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 describe 'performance_bar_allowed_group' do
context 'with no performance_bar_allowed_group_id saved' do context 'with no performance_bar_allowed_group_id saved' do
it 'returns nil' do it 'returns nil' do
...@@ -464,11 +402,11 @@ describe ApplicationSetting do ...@@ -464,11 +402,11 @@ describe ApplicationSetting do
let(:group) { create(:group) } let(:group) { create(:group) }
before do before do
setting.performance_bar_allowed_group_id = group.full_path setting.update!(performance_bar_allowed_group_id: group.id)
end end
it 'returns the group' do 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 end
end end
...@@ -478,67 +416,11 @@ describe ApplicationSetting do ...@@ -478,67 +416,11 @@ describe ApplicationSetting do
let(:group) { create(:group) } let(:group) { create(:group) }
before do before do
setting.performance_bar_allowed_group_id = group.full_path setting.update!(performance_bar_allowed_group_id: group.id)
end end
it 'returns true' do it 'returns true' do
expect(setting.performance_bar_enabled).to be_truthy expect(setting.reload.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
end end
end end
end end
......
...@@ -24,10 +24,15 @@ describe API::Settings, 'Settings' do ...@@ -24,10 +24,15 @@ describe API::Settings, 'Settings' do
expect(json_response['ecdsa_key_restriction']).to eq(0) expect(json_response['ecdsa_key_restriction']).to eq(0)
expect(json_response['ed25519_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['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
end end
describe "PUT /application/settings" do describe "PUT /application/settings" do
let(:group) { create(:group) }
context "custom repository storage type set in the config" do context "custom repository storage type set in the config" do
before do before do
storages = { 'custom' => 'tmp/tests/custom_repositories' } storages = { 'custom' => 'tmp/tests/custom_repositories' }
...@@ -56,7 +61,8 @@ describe API::Settings, 'Settings' do ...@@ -56,7 +61,8 @@ describe API::Settings, 'Settings' do
ed25519_key_restriction: 256, ed25519_key_restriction: 256,
circuitbreaker_check_interval: 2, circuitbreaker_check_interval: 2,
enforce_terms: true, 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(response).to have_gitlab_http_status(200)
expect(json_response['default_projects_limit']).to eq(3) expect(json_response['default_projects_limit']).to eq(3)
...@@ -80,9 +86,27 @@ describe API::Settings, 'Settings' do ...@@ -80,9 +86,27 @@ describe API::Settings, 'Settings' do
expect(json_response['circuitbreaker_check_interval']).to eq(2) expect(json_response['circuitbreaker_check_interval']).to eq(2)
expect(json_response['enforce_terms']).to be(true) expect(json_response['enforce_terms']).to be(true)
expect(json_response['terms']).to eq('Hello world!') expect(json_response['terms']).to eq('Hello world!')
expect(json_response['performance_bar_allowed_group_id']).to eq(group.id)
end end
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 context "missing koding_url value when koding_enabled is true" do
it "returns a blank parameter error message" do it "returns a blank parameter error message" do
put api("/application/settings", admin), koding_enabled: true put api("/application/settings", admin), koding_enabled: true
......
require 'spec_helper' require 'spec_helper'
describe ApplicationSettings::UpdateService do describe ApplicationSettings::UpdateService do
let(:application_settings) { Gitlab::CurrentSettings.current_application_settings } let(:application_settings) { create(:application_setting) }
let(:admin) { create(:user, :admin) } let(:admin) { create(:user, :admin) }
let(:params) { {} } let(:params) { {} }
...@@ -54,4 +54,90 @@ describe ApplicationSettings::UpdateService do ...@@ -54,4 +54,90 @@ describe ApplicationSettings::UpdateService do
end end
end 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 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