Avoid CSRF check on SAML failure endpoint

SAML and OAuth failures should cause a message to be presented, as well
as logging that an attempt was made. These were incorrectly prevented by
the CSRF check on POST endpoints such as SAML.

In addition we were using a NullSession forgery protection, which made
testing more difficult and could have allowed account linking to take
place if a CSRF was ever needed but not present.
This commit is contained in:
James Edwards-Jones 2019-01-19 20:41:39 +00:00
parent d4d4ebadfb
commit 6548e01f18
3 changed files with 29 additions and 1 deletions

View file

@ -4,7 +4,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
include AuthenticatesWithTwoFactor
include Devise::Controllers::Rememberable
protect_from_forgery except: [:kerberos, :saml, :cas3], prepend: true
protect_from_forgery except: [:kerberos, :saml, :cas3, :failure], with: :exception, prepend: true
def handle_omniauth
omniauth_flow(Gitlab::Auth::OAuth)

View file

@ -0,0 +1,5 @@
---
title: Display SAML failure messages instead of expecting CSRF token
merge_request: 24509
author:
type: fixed

View file

@ -45,6 +45,29 @@ describe OmniauthCallbacksController, type: :controller do
end
end
context 'when sign in fails' do
include RoutesHelpers
let(:extern_uid) { 'my-uid' }
let(:provider) { :saml }
def stub_route_as(path)
allow(@routes).to receive(:generate_extras) { [path, []] }
end
it 'it calls through to the failure handler' do
request.env['omniauth.error'] = OneLogin::RubySaml::ValidationError.new("Fingerprint mismatch")
request.env['omniauth.error.strategy'] = OmniAuth::Strategies::SAML.new(nil)
stub_route_as('/users/auth/saml/callback')
ForgeryProtection.with_forgery_protection do
post :failure
end
expect(flash[:alert]).to match(/Fingerprint mismatch/)
end
end
context 'when a redirect fragment is provided' do
let(:provider) { :jwt }
let(:extern_uid) { 'my-uid' }