Skip to content
Permalink
Browse files

Existing user session when requesting SSO session create endpoint wil…

…l fail device check because of missing fingerprint param (which is required as soon as a user/session is present).
  • Loading branch information...
thorsteneckel committed Sep 30, 2019
1 parent 817cc72 commit d1ed72a071765b8ec04323297e17af0a3ad28eb5
@@ -143,10 +143,10 @@ def authenticate_with_sso
User.lookup(login: login&.downcase)
end

raise Exceptions::NotAuthorized, 'no valid session' if !user
raise Exceptions::NotAuthorized, 'Missing SSO ENV REMOTE_USER' if !user

session.delete(:switched_from_user_id)
authentication_check_prerequesits(user, 'session', {})
authentication_check_prerequesits(user, 'SSO', {})
end

def authentication_check_prerequesits(user, auth_type, auth_param)
@@ -17,6 +17,7 @@ def user_device_log(user, type)
return true if switched_from_user_id
return true if !user
return true if !user.permissions?('user_preferences.device')
return true if type == 'SSO'

time_to_check = true
user_device_updated_at = session[:user_device_updated_at]
@@ -3,6 +3,7 @@
class SessionsController < ApplicationController
prepend_before_action :authentication_check, only: %i[switch_to_user list delete]
skip_before_action :verify_csrf_token, only: %i[show destroy create_omniauth failure_omniauth]
skip_before_action :user_device_check, only: %i[create_sso]

# "Create" a login, aka "log the user in"
def create
@@ -14,15 +15,21 @@ def create
json: SessionHelper.json_hash(user).merge(config: config_frontend)
end

def create_sso
authenticate_with_sso

redirect_to '/#'
end

def show
user = authentication_check_only || authenticate_with_sso
user = authentication_check_only
raise Exceptions::NotAuthorized, 'no valid session' if user.blank?

initiate_session_for(user)

# return current session
render json: SessionHelper.json_hash(user).merge(config: config_frontend)
rescue Exceptions::NotAuthorized => e
raise if e.message != 'no valid session'

render json: {
error: e.message,
config: config_frontend,
@@ -5,6 +5,9 @@
match '/auth/:provider/callback', to: 'sessions#create_omniauth', via: %i[post get puts delete]
match '/auth/failure', to: 'sessions#failure_omniauth', via: %i[post get]

# sso
match '/auth/sso', to: 'sessions#create_sso', via: %i[get post]

# sessions
match api_path + '/signin', to: 'sessions#create', via: :post
match api_path + '/signshow', to: 'sessions#show', via: %i[get post]
@@ -1,53 +1,38 @@
require 'rails_helper'

RSpec.describe 'Sessions endpoints', type: :request do
# The frontend sends a device fingerprint in the request parameters during authentication
# (as part of App.Auth.loginCheck() and App.WebSocket.auth()).
#
# Without this parameter, the controller will raise a 422 Unprocessable Entity error
# (in ApplicationController::HandlesDevices#user_device_log).
let(:fingerprint) { { fingerprint: 'foo' } }

describe 'GET /api/v1/signshow (single sign-on)' do

describe 'GET /auth/sso (single sign-on)' do
context 'with invalid user login' do
let(:login) { User.pluck(:login).max.next }

context 'in "REMOTE_USER" request env var' do
let(:env) { { 'REMOTE_USER' => login } }

it 'returns invalid session response' do
get '/api/v1/signshow', as: :json, env: env, params: fingerprint
it 'returns unauthorized response' do
get '/auth/sso', as: :json, env: env

expect(response).to have_http_status(:ok)
expect(json_response)
.to include('error' => 'no valid session')
.and not_include('session')
expect(response).to have_http_status(:unauthorized)
end
end

context 'in "HTTP_REMOTE_USER" request env var' do
let(:env) { { 'HTTP_REMOTE_USER' => login } }

it 'returns invalid session response' do
get '/api/v1/signshow', as: :json, env: env, params: fingerprint
it 'returns unauthorized response' do
get '/auth/sso', as: :json, env: env

expect(response).to have_http_status(:ok)
expect(json_response)
.to include('error' => 'no valid session')
.and not_include('session')
expect(response).to have_http_status(:unauthorized)
end
end

context 'in "X-Forwarded-User" request header' do
let(:headers) { { 'X-Forwarded-User' => login } }

it 'returns invalid session response' do
get '/api/v1/signshow', as: :json, headers: headers, params: fingerprint
it 'returns unauthorized response' do
get '/auth/sso', as: :json, headers: headers

expect(response).to have_http_status(:ok)
expect(json_response)
.to include('error' => 'no valid session')
.and not_include('session')
expect(response).to have_http_status(:unauthorized)
end
end
end
@@ -63,7 +48,7 @@
let(:env) { { 'REMOTE_USER' => login } }

it 'returns 401 unauthorized' do
get '/api/v1/signshow', as: :json, env: env, params: fingerprint
get '/auth/sso', as: :json, env: env

expect(response).to have_http_status(:unauthorized)
expect(json_response).to include('error' => 'Maintenance mode enabled!')
@@ -74,7 +59,7 @@
let(:env) { { 'HTTP_REMOTE_USER' => login } }

it 'returns 401 unauthorized' do
get '/api/v1/signshow', as: :json, env: env, params: fingerprint
get '/auth/sso', as: :json, env: env

expect(response).to have_http_status(:unauthorized)
expect(json_response).to include('error' => 'Maintenance mode enabled!')
@@ -85,7 +70,7 @@
let(:headers) { { 'X-Forwarded-User' => login } }

it 'returns 401 unauthorized' do
get '/api/v1/signshow', as: :json, headers: headers, params: fingerprint
get '/auth/sso', as: :json, headers: headers

expect(response).to have_http_status(:unauthorized)
expect(json_response).to include('error' => 'Maintenance mode enabled!')
@@ -97,81 +82,45 @@
let(:env) { { 'REMOTE_USER' => login } }

it 'returns a new user-session response' do
get '/api/v1/signshow', as: :json, env: env, params: fingerprint
get '/auth/sso', as: :json, env: env

expect(json_response)
.to include('session' => hash_including('login' => login))
.and not_include('error')
expect(response).to redirect_to('/#')
end

it 'sets the :user_id session parameter' do
expect { get '/api/v1/signshow', as: :json, env: env, params: fingerprint }
expect { get '/auth/sso', as: :json, env: env }
.to change { request&.session&.fetch(:user_id) }.to(user.id)
end

it 'sets the :persistent session parameter' do
expect { get '/api/v1/signshow', as: :json, env: env, params: fingerprint }
.to change { request&.session&.fetch(:persistent) }.to(true)
end

it 'adds an activity stream entry for the user’s session' do
expect { get '/api/v1/signshow', as: :json, env: env, params: fingerprint }
.to change(ActivityStream, :count).by(1)
end
end

context 'in "HTTP_REMOTE_USER" request env var' do
let(:env) { { 'HTTP_REMOTE_USER' => login } }

it 'returns a new user-session response' do
get '/api/v1/signshow', as: :json, env: env, params: fingerprint
get '/auth/sso', as: :json, env: env

expect(json_response)
.to include('session' => hash_including('login' => login))
.and not_include('error')
expect(response).to redirect_to('/#')
end

it 'sets the :user_id session parameter' do
expect { get '/api/v1/signshow', as: :json, env: env, params: fingerprint }
expect { get '/auth/sso', as: :json, env: env }
.to change { request&.session&.fetch(:user_id) }.to(user.id)
end

it 'sets the :persistent session parameter' do
expect { get '/api/v1/signshow', as: :json, env: env, params: fingerprint }
.to change { request&.session&.fetch(:persistent) }.to(true)
end

it 'adds an activity stream entry for the user’s session' do
expect { get '/api/v1/signshow', as: :json, env: env, params: fingerprint }
.to change(ActivityStream, :count).by(1)
end
end

context 'in "X-Forwarded-User" request header' do
let(:headers) { { 'X-Forwarded-User' => login } }

it 'returns a new user-session response' do
get '/api/v1/signshow', as: :json, headers: headers, params: fingerprint
get '/auth/sso', as: :json, headers: headers

expect(json_response)
.to include('session' => hash_including('login' => login))
.and not_include('error')
expect(response).to redirect_to('/#')
end

it 'sets the :user_id session parameter on the client' do
expect { get '/api/v1/signshow', as: :json, headers: headers, params: fingerprint }
expect { get '/auth/sso', as: :json, headers: headers }
.to change { request&.session&.fetch(:user_id) }.to(user.id)
end

it 'sets the :persistent session parameter' do
expect { get '/api/v1/signshow', as: :json, headers: headers, params: fingerprint }
.to change { request&.session&.fetch(:persistent) }.to(true)
end

it 'adds an activity stream entry for the user’s session' do
expect { get '/api/v1/signshow', as: :json, headers: headers, params: fingerprint }
.to change(ActivityStream, :count).by(1)
end
end
end
end

1 comment on commit d1ed72a

@thorsteneckel

This comment has been minimized.

Copy link
Collaborator Author

commented on d1ed72a Sep 30, 2019

GitLab squash error again. This is a follow up to #1192 - the previous implementation had an issue because SSO requires a redirect after successful session generation. Additionally no fingerprint parameter is provided when SSO is performed but the old implementation required it. This commit fixes both of the issues.

Please sign in to comment.
You can’t perform that action at this time.