Commit 39cee0ce authored by Stan Hu's avatar Stan Hu

Fix GraphQlController not logging sessionless user

Most controllers call `ApplicationController#auth_user` first, which
ensures that the currently-logged in user will be memoized
properly. However, controllers such as `GraphQlController` allows
sessionless access via the `PRIVATE-TOKEN` header. They authenticate
access via `authenticate_sessionless_user!`.

Since `auth_user` is memoized before `authenticate_sessionless_user!`
gets to run, `ApplicationController#context_user` relies on a stale
`auth_user` value if a sessionless user is used. As a result,
`GraphQlController` erroneously logs that an anonymous user accessed
an endpoint when an actual user was responsible.

To fix this, we need to update `authenticate_sessionless_user!` so
that it flushes the memoization of `auth_user` if a sessionless user
has logged in. Note that we have to be careful not to call
`current_user` for anonymous users because each attempt will cause a
Warden reauthentication attempt.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/356213

Changelog: fixed
parent d9b30205
......@@ -196,6 +196,27 @@ class ApplicationController < ActionController::Base
end
end
# Devise defines current_user to be:
#
# def current_user
# @current_user ||= warden.authenticate(scope: mapping)
# end
#
# That means whenever current_user is called and `@current_user` is
# nil, Warden will attempt to authenticate a user. To avoid
# reauthenticating anonymous users, we may need to invalidate
# the user.
def reset_auth_user!
return if strong_memoized?(:auth_user) && auth_user
# Controllers usually call auth_user first, but for some controllers
# authenticate_sessionless_user! is called after that. If we relied
# on the memoized auth_user, the value would always be nil for
# sessionless users.
clear_memoization(:auth_user)
auth_user
end
def log_exception(exception)
# At this point, the controller already exits set_current_context around
# block. To maintain the context while handling error exception, we need to
......
......@@ -20,16 +20,21 @@ module SessionlessAuthentication
end
def sessionless_sign_in(user)
if user.can_log_in_with_non_expired_password?
# Notice we are passing store false, so the user is not
# actually stored in the session and a token is needed
# for every request. If you want the token to work as a
# sign in token, you can simply remove store: false.
sign_in(user, store: false, message: :sessionless_sign_in)
elsif request_authenticator.can_sign_in_bot?(user)
# we suppress callbacks to avoid redirecting the bot
sign_in(user, store: false, message: :sessionless_sign_in, run_callbacks: false)
end
signed_in_user =
if user.can_log_in_with_non_expired_password?
# Notice we are passing store false, so the user is not
# actually stored in the session and a token is needed
# for every request. If you want the token to work as a
# sign in token, you can simply remove store: false.
sign_in(user, store: false, message: :sessionless_sign_in)
elsif request_authenticator.can_sign_in_bot?(user)
# we suppress callbacks to avoid redirecting the bot
sign_in(user, store: false, message: :sessionless_sign_in, run_callbacks: false)
end
reset_auth_user! if respond_to?(:reset_auth_user!, true)
signed_in_user
end
def sessionless_bypass_admin_mode!(&block)
......
......@@ -134,6 +134,12 @@ RSpec.describe GraphqlController do
post :execute
end
it "assigns username in ApplicationContext" do
post :execute
expect(Gitlab::ApplicationContext.current).to include('meta.user' => user.username)
end
end
context 'when user uses an API token' do
......@@ -189,6 +195,12 @@ RSpec.describe GraphqlController do
expect(assigns(:context)[:is_sessionless_user]).to be true
end
it "assigns username in ApplicationContext" do
subject
expect(Gitlab::ApplicationContext.current).to include('meta.user' => user.username)
end
it 'calls the track api when trackable method' do
agent = 'vs-code-gitlab-workflow/3.11.1 VSCode/1.52.1 Node.js/12.14.1 (darwin; x64)'
request.env['HTTP_USER_AGENT'] = agent
......@@ -222,6 +234,12 @@ RSpec.describe GraphqlController do
expect(assigns(:context)[:is_sessionless_user]).to be false
end
it "does not assign a username in ApplicationContext" do
subject
expect(Gitlab::ApplicationContext.current.key?('meta.user')).to be false
end
end
it 'includes request object in context' do
......
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