Commit d3b84962 authored by Heinrich Lee Yu's avatar Heinrich Lee Yu

Improve performance bar permission checks

Checks the cookie first because it is a cheaper check. This also renames
enabled_for_user? to allowed_for_user? since that is more accurate.
parent f4337bc9
...@@ -20,12 +20,12 @@ module WithPerformanceBar ...@@ -20,12 +20,12 @@ module WithPerformanceBar
end end
def cookie_or_default_value def cookie_or_default_value
return false unless Gitlab::PerformanceBar.enabled_for_user?(current_user) cookie_enabled = if cookies[:perf_bar_enabled].present?
if cookies[:perf_bar_enabled].present?
cookies[:perf_bar_enabled] == 'true' cookies[:perf_bar_enabled] == 'true'
else else
cookies[:perf_bar_enabled] = 'true' if Rails.env.development? cookies[:perf_bar_enabled] = 'true' if Rails.env.development?
end end
cookie_enabled && Gitlab::PerformanceBar.allowed_for_user?(current_user)
end end
end end
...@@ -4,17 +4,17 @@ module API ...@@ -4,17 +4,17 @@ module API
module Helpers module Helpers
module PerformanceBarHelpers module PerformanceBarHelpers
def set_peek_enabled_for_current_request def set_peek_enabled_for_current_request
Gitlab::SafeRequestStore.fetch(:peek_enabled) { perf_bar_cookie_enabled? && perf_bar_enabled_for_user? } Gitlab::SafeRequestStore.fetch(:peek_enabled) { perf_bar_cookie_enabled? && perf_bar_allowed_for_user? }
end end
def perf_bar_cookie_enabled? def perf_bar_cookie_enabled?
cookies[:perf_bar_enabled] == 'true' cookies[:perf_bar_enabled] == 'true'
end end
def perf_bar_enabled_for_user? def perf_bar_allowed_for_user?
# We cannot use `current_user` here because that method raises an exception when the user # We cannot use `current_user` here because that method raises an exception when the user
# is unauthorized and some API endpoints require that `current_user` is not called. # is unauthorized and some API endpoints require that `current_user` is not called.
Gitlab::PerformanceBar.enabled_for_user?(find_user_from_sources) Gitlab::PerformanceBar.allowed_for_user?(find_user_from_sources)
end end
end end
end end
......
...@@ -10,7 +10,7 @@ module Gitlab ...@@ -10,7 +10,7 @@ module Gitlab
Gitlab::SafeRequestStore[:peek_enabled] Gitlab::SafeRequestStore[:peek_enabled]
end end
def self.enabled_for_user?(user = nil) def self.allowed_for_user?(user = nil)
return true if Rails.env.development? return true if Rails.env.development?
return true if user&.admin? return true if user&.admin?
return false unless user && allowed_group_id return false unless user && allowed_group_id
......
...@@ -6,7 +6,7 @@ RSpec.describe Gitlab::PerformanceBar do ...@@ -6,7 +6,7 @@ RSpec.describe Gitlab::PerformanceBar do
it { expect(described_class.l1_cache_backend).to eq(Gitlab::ProcessMemoryCache.cache_backend) } it { expect(described_class.l1_cache_backend).to eq(Gitlab::ProcessMemoryCache.cache_backend) }
it { expect(described_class.l2_cache_backend).to eq(Rails.cache) } it { expect(described_class.l2_cache_backend).to eq(Rails.cache) }
describe '.enabled_for_user?' do describe '.allowed_for_user?' do
let(:user) { create(:user) } let(:user) { create(:user) }
before do before do
...@@ -14,24 +14,24 @@ RSpec.describe Gitlab::PerformanceBar do ...@@ -14,24 +14,24 @@ RSpec.describe Gitlab::PerformanceBar do
end end
it 'returns false when given user is nil' do it 'returns false when given user is nil' do
expect(described_class.enabled_for_user?(nil)).to be_falsy expect(described_class.allowed_for_user?(nil)).to be_falsy
end end
it 'returns true when given user is an admin' do it 'returns true when given user is an admin' do
user = build_stubbed(:user, :admin) user = build_stubbed(:user, :admin)
expect(described_class.enabled_for_user?(user)).to be_truthy expect(described_class.allowed_for_user?(user)).to be_truthy
end end
it 'returns false when allowed_group_id is nil' do it 'returns false when allowed_group_id is nil' do
expect(described_class).to receive(:allowed_group_id).and_return(nil) expect(described_class).to receive(:allowed_group_id).and_return(nil)
expect(described_class.enabled_for_user?(user)).to be_falsy expect(described_class.allowed_for_user?(user)).to be_falsy
end end
context 'when allowed group ID does not exist' do context 'when allowed group ID does not exist' do
it 'returns false' do it 'returns false' do
expect(described_class.enabled_for_user?(user)).to be_falsy expect(described_class.allowed_for_user?(user)).to be_falsy
end end
end end
...@@ -44,15 +44,15 @@ RSpec.describe Gitlab::PerformanceBar do ...@@ -44,15 +44,15 @@ RSpec.describe Gitlab::PerformanceBar do
context 'when user is not a member of the allowed group' do context 'when user is not a member of the allowed group' do
it 'returns false' do it 'returns false' do
expect(described_class.enabled_for_user?(user)).to be_falsy expect(described_class.allowed_for_user?(user)).to be_falsy
end end
context 'caching of allowed user IDs' do context 'caching of allowed user IDs' do
subject { described_class.enabled_for_user?(user) } subject { described_class.allowed_for_user?(user) }
before do before do
# Warm the caches # Warm the caches
described_class.enabled_for_user?(user) described_class.allowed_for_user?(user)
end end
it_behaves_like 'allowed user IDs are cached' it_behaves_like 'allowed user IDs are cached'
...@@ -65,15 +65,15 @@ RSpec.describe Gitlab::PerformanceBar do ...@@ -65,15 +65,15 @@ RSpec.describe Gitlab::PerformanceBar do
end end
it 'returns true' do it 'returns true' do
expect(described_class.enabled_for_user?(user)).to be_truthy expect(described_class.allowed_for_user?(user)).to be_truthy
end end
context 'caching of allowed user IDs' do context 'caching of allowed user IDs' do
subject { described_class.enabled_for_user?(user) } subject { described_class.allowed_for_user?(user) }
before do before do
# Warm the caches # Warm the caches
described_class.enabled_for_user?(user) described_class.allowed_for_user?(user)
end end
it_behaves_like 'allowed user IDs are cached' it_behaves_like 'allowed user IDs are cached'
...@@ -91,7 +91,7 @@ RSpec.describe Gitlab::PerformanceBar do ...@@ -91,7 +91,7 @@ RSpec.describe Gitlab::PerformanceBar do
end end
it 'returns the nested group' do it 'returns the nested group' do
expect(described_class.enabled_for_user?(user)).to be_truthy expect(described_class.allowed_for_user?(user)).to be_truthy
end end
end end
...@@ -101,7 +101,7 @@ RSpec.describe Gitlab::PerformanceBar do ...@@ -101,7 +101,7 @@ RSpec.describe Gitlab::PerformanceBar do
end end
it 'returns false' do it 'returns false' do
expect(described_class.enabled_for_user?(user)).to be_falsy expect(described_class.allowed_for_user?(user)).to be_falsy
end 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