From 3ff44757aa7a552517d2e9d0e91c7e8185489d13 Mon Sep 17 00:00:00 2001
From: Roger Meier <r.meier@siemens.com>
Date: Mon, 19 Feb 2024 08:12:21 +0000
Subject: [PATCH] CurrentUserMode with session, load known attributes within
ActiveSession
The original MR(Show admin mode within active sessions view) has
been reverted due to failures:
https://gitlab.com/gitlab-org/release/tasks/-/issues/8332#note_1748203585 on staging-canary.
See discussion https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142158#note_1748221369
It was agreed to implement it in two steps, and this is step one.
Within this MR CurrentUserMode is extend with session parameter and
within load_raw_session only known attributes of ActiveSession
are loaded. This fixes the root cause for the crash within
staging-canary.
In the future new attributes can be added without the risk of a crash
as only known attributes are passed to the constructor.
---
app/models/active_session.rb | 12 ++++-
lib/gitlab/auth/current_user_mode.rb | 13 +++--
.../user_settings/active_sessions_spec.rb | 16 ++++++
.../lib/gitlab/auth/current_user_mode_spec.rb | 49 +++++++++++++++++++
4 files changed, 83 insertions(+), 7 deletions(-)
diff --git a/app/models/active_session.rb b/app/models/active_session.rb
index 2eb9c9bca7f24..4ee04a3d62d2d 100644
--- a/app/models/active_session.rb
+++ b/app/models/active_session.rb
@@ -25,11 +25,17 @@ class ActiveSession
SESSION_BATCH_SIZE = 200
ALLOWED_NUMBER_OF_ACTIVE_SESSIONS = 100
- attr_accessor :ip_address, :browser, :os,
+ ATTR_ACCESSOR_LIST = [
+ :ip_address, :browser, :os,
:device_name, :device_type,
:is_impersonated, :session_id, :session_private_id
+ ].freeze
+ ATTR_READER_LIST = [
+ :created_at, :updated_at
+ ].freeze
- attr_reader :created_at, :updated_at
+ attr_accessor(*ATTR_ACCESSOR_LIST)
+ attr_reader(*ATTR_READER_LIST)
def created_at=(time)
@created_at = time.is_a?(String) ? Time.zone.parse(time) : time
@@ -240,6 +246,8 @@ def dump
if raw_session.start_with?('v2:')
session_data = Gitlab::Json.parse(raw_session[3..]).symbolize_keys
+ # load only known attributes
+ session_data.slice!(*ATTR_ACCESSOR_LIST.union(ATTR_READER_LIST))
new(**session_data)
else
# Deprecated legacy format. To be removed in 15.0
diff --git a/lib/gitlab/auth/current_user_mode.rb b/lib/gitlab/auth/current_user_mode.rb
index 9bd4711c4bbf1..4dd808182ec7e 100644
--- a/lib/gitlab/auth/current_user_mode.rb
+++ b/lib/gitlab/auth/current_user_mode.rb
@@ -8,6 +8,7 @@ module Auth
# an administrator must have explicitly enabled admin-mode
# e.g. on web access require re-authentication
class CurrentUserMode
+ include Gitlab::Utils::StrongMemoize
NotRequestedError = Class.new(StandardError)
# RequestStore entries
@@ -85,8 +86,9 @@ def current_admin
end
end
- def initialize(user)
+ def initialize(user, session = Gitlab::Session.current)
@user = user
+ @session = session
end
def admin_mode?
@@ -138,6 +140,11 @@ def request_admin_mode!
current_session_data[ADMIN_MODE_REQUESTED_TIME_KEY] = Time.now
end
+ def current_session_data
+ Gitlab::NamespacedSessionStore.new(SESSION_STORE_KEY, @session)
+ end
+ strong_memoize_attr :current_session_data
+
private
attr_reader :user
@@ -152,10 +159,6 @@ def admin_mode_requested_rs_key
@admin_mode_requested_rs_key ||= { res: :current_user_mode, user: user.id, method: :admin_mode_requested? }
end
- def current_session_data
- @current_session ||= Gitlab::NamespacedSessionStore.new(SESSION_STORE_KEY)
- end
-
def session_with_admin_mode?
return true if bypass_session?
diff --git a/spec/features/user_settings/active_sessions_spec.rb b/spec/features/user_settings/active_sessions_spec.rb
index bc0693d79e1f8..bb3ad718173ca 100644
--- a/spec/features/user_settings/active_sessions_spec.rb
+++ b/spec/features/user_settings/active_sessions_spec.rb
@@ -110,4 +110,20 @@
expect(page).to have_content('You need to sign in or sign up before continuing.')
end
end
+
+ it 'load_raw_session does load known attributes only' do
+ new_session = ActiveSession.send(:load_raw_session,
+ 'v2:{"ip_address": "127.0.0.1", "browser": "Firefox", "os": "Debian",' \
+ '"device_type": "desktop", "session_id": "8f62cc7383c",' \
+ '"new_attribute": "unknown attribute"}'
+ )
+
+ expect(new_session).to have_attributes(
+ ip_address: "127.0.0.1",
+ browser: "Firefox",
+ os: "Debian",
+ device_type: "desktop",
+ session_id: "8f62cc7383c"
+ )
+ end
end
diff --git a/spec/lib/gitlab/auth/current_user_mode_spec.rb b/spec/lib/gitlab/auth/current_user_mode_spec.rb
index 650af6af22961..014c812d9eea4 100644
--- a/spec/lib/gitlab/auth/current_user_mode_spec.rb
+++ b/spec/lib/gitlab/auth/current_user_mode_spec.rb
@@ -7,6 +7,55 @@
subject { described_class.new(user) }
+ describe '#initialize' do
+ context 'with user' do
+ around do |example|
+ Gitlab::Session.with_session(nil) do
+ example.run
+ end
+ end
+
+ it 'has no session' do
+ subject
+ expect(Gitlab::Session.current).to be_nil
+ end
+ end
+
+ context 'with user and session' do
+ include_context 'custom session'
+ let(:session) { { 'key' => "value" } }
+
+ it 'has a session' do
+ described_class.new(user, session)
+ expect(Gitlab::Session.current).to eq(session)
+ end
+ end
+ end
+
+ describe '#current_session_data' do
+ include_context 'custom session'
+ let(:session) { { 'key' => "value" } }
+
+ it 'without session' do
+ expect(Gitlab::Session.current).to eq(session)
+
+ expect(Gitlab::NamespacedSessionStore).to receive(:new).with(described_class::SESSION_STORE_KEY, session)
+
+ subject.current_session_data
+ expect(Gitlab::Session.current).to eq(session)
+ end
+
+ it 'with session' do
+ expect(Gitlab::Session.current).to eq(session)
+ subject = described_class.new(user, session)
+
+ expect(Gitlab::NamespacedSessionStore).to receive(:new).with(described_class::SESSION_STORE_KEY, session)
+
+ subject.current_session_data
+ expect(Gitlab::Session.current).to eq(session)
+ end
+ end
+
context 'when session is available' do
include_context 'custom session'
--
GitLab