From d1ed72a071765b8ec04323297e17af0a3ad28eb5 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Mon, 30 Sep 2019 19:34:13 +0200 Subject: [PATCH] Existing user session when requesting SSO session create endpoint will fail device check because of missing fingerprint param (which is required as soon as a user/session is present). --- .../application_controller/authenticates.rb | 4 +- .../application_controller/handles_devices.rb | 1 + app/controllers/sessions_controller.rb | 13 ++- config/routes/auth.rb | 3 + spec/requests/session_spec.rb | 95 +++++-------------- 5 files changed, 38 insertions(+), 78 deletions(-) diff --git a/app/controllers/application_controller/authenticates.rb b/app/controllers/application_controller/authenticates.rb index ef3ab3eff..07873bfd1 100644 --- a/app/controllers/application_controller/authenticates.rb +++ b/app/controllers/application_controller/authenticates.rb @@ -143,10 +143,10 @@ module ApplicationController::Authenticates 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) diff --git a/app/controllers/application_controller/handles_devices.rb b/app/controllers/application_controller/handles_devices.rb index 400565aae..27ed94720 100644 --- a/app/controllers/application_controller/handles_devices.rb +++ b/app/controllers/application_controller/handles_devices.rb @@ -17,6 +17,7 @@ module ApplicationController::HandlesDevices 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] diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index d242a4646..f01cc8a77 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -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 @@ class SessionsController < ApplicationController 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, diff --git a/config/routes/auth.rb b/config/routes/auth.rb index 701bc6c75..a63484bcc 100644 --- a/config/routes/auth.rb +++ b/config/routes/auth.rb @@ -5,6 +5,9 @@ Zammad::Application.routes.draw do 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] diff --git a/spec/requests/session_spec.rb b/spec/requests/session_spec.rb index 31edec401..c90981075 100644 --- a/spec/requests/session_spec.rb +++ b/spec/requests/session_spec.rb @@ -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 @@ RSpec.describe 'Sessions endpoints', type: :request do 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 @@ RSpec.describe 'Sessions endpoints', type: :request do 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 @@ RSpec.describe 'Sessions endpoints', type: :request do 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 @@ RSpec.describe 'Sessions endpoints', type: :request do 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