Merge branch 'jej/saml-failure-messages' into 'master'

Display Group SAML failures in flash

Closes #5898

See merge request gitlab-org/gitlab-ee!5617

app/controllers/omniauth_callbacks_controller.rb

@@ -27,11 +27,11 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
 
   # Extend the standard message generation to accept our custom exception
   def failure_message
-    exception = env["omniauth.error"]
+    exception = request.env["omniauth.error"]
     error   = exception.error_reason if exception.respond_to?(:error_reason)
     error ||= exception.error        if exception.respond_to?(:error)
     error ||= exception.message      if exception.respond_to?(:message)
-    error ||= env["omniauth.error.type"].to_s
+    error ||= request.env["omniauth.error.type"].to_s
 
     error.to_s.humanize if error
   end

config/initializers/omniauth.rb

@@ -21,6 +21,10 @@ if Gitlab.config.omniauth.enabled
   provider_names = Gitlab.config.omniauth.providers.map(&:name)
   require 'omniauth-kerberos' if provider_names.include?('kerberos')
   require_dependency 'omni_auth/strategies/kerberos_spnego' if provider_names.include?('kerberos_spnego')
+
+  if provider_names.include?('group_saml')
+    OmniAuth.config.on_failure = ::Gitlab::Auth::GroupSaml::FailureHandler.new(OmniAuth.config.on_failure)
+  end
 end
 
 module OmniAuth

ee/app/controllers/groups/omniauth_callbacks_controller.rb

 class Groups::OmniauthCallbacksController < OmniauthCallbacksController
   extend ::Gitlab::Utils::Override
 
-  skip_before_filter :verify_authenticity_token, only: :group_saml
+  skip_before_filter :verify_authenticity_token, only: [:failure, :group_saml]
 
   def group_saml
     @unauthenticated_group = Group.find_by_full_path(params[:group_id])
@@ -46,4 +46,28 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
   def saml_redirect_path
     params['RelayState'].presence if current_user
   end
+
+  override :find_message
+  def find_message(kind, options = {})
+    _('Unable to sign you in to the group with SAML due to "%{reason}"') % options
+  end
+
+  override :after_omniauth_failure_path_for
+  def after_omniauth_failure_path_for(scope)
+    group_saml_failure_path(scope)
+  end
+
+  def group_saml_failure_path(scope)
+    group = Gitlab::Auth::GroupSaml::GroupLookup.new(request.env).group
+
+    unless can?(current_user, :sign_in_with_saml_provider, group&.saml_provider)
+      OmniAuth::Strategies::GroupSaml.invalid_group!(group&.path)
+    end
+
+    if can?(current_user, :admin_group_saml, group)
+      group_saml_providers_path(group)
+    else
+      sso_group_saml_providers_path(group)
+    end
+  end
 end

ee/lib/gitlab/auth/group_saml/failure_handler.rb

+module Gitlab
+  module Auth
+    module GroupSaml
+      class FailureHandler
+        def initialize(parent)
+          @parent = parent
+        end
+
+        def call(env)
+          if env['omniauth.error.strategy'].is_a?(OmniAuth::Strategies::GroupSaml)
+            ::Groups::OmniauthCallbacksController.action(:failure).call(env)
+          else
+            @parent.call(env)
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/omni_auth/strategies/group_saml.rb

@@ -11,7 +11,7 @@ module OmniAuth
         group_lookup = Gitlab::Auth::GroupSaml::GroupLookup.new(env)
 
         unless group_lookup.group_saml_enabled?
-          raise ActionController::RoutingError, group_lookup.path
+          self.class.invalid_group!(group_lookup.path)
         end
 
         saml_provider = group_lookup.saml_provider
@@ -27,6 +27,10 @@ module OmniAuth
         call_app!
       end
 
+      def self.invalid_group!(path)
+        raise ActionController::RoutingError, path
+      end
+
       def self.callback?(env)
         env['PATH_INFO'] =~ Gitlab::PathRegex.saml_callback_regex
       end

ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb

@@ -115,4 +115,74 @@ describe Groups::OmniauthCallbacksController do
       end
     end
   end
+
+  describe "#failure" do
+    include RoutesHelpers
+
+    def fake_error_callback_route
+      fake_routes do
+        post '/groups/:group_id/-/saml/callback', to: 'groups/omniauth_callbacks#failure'
+      end
+    end
+
+    def stub_certificate_error
+      strategy = OmniAuth::Strategies::GroupSaml.new(nil)
+      exception = OneLogin::RubySaml::ValidationError.new("Fingerprint mismatch")
+      stub_omniauth_failure(strategy, :invalid_ticket, exception)
+    end
+
+    before do
+      fake_error_callback_route
+      stub_certificate_error
+      set_devise_mapping(context: @request)
+    end
+
+    context "not signed in" do
+      it "doesn't disclose group existence" do
+        expect do
+          post :failure, group_id: group
+        end.to raise_error(ActionController::RoutingError)
+      end
+
+      context "group doesn't exist" do
+        it "doesn't disclose group non-existence" do
+          expect do
+            post :failure, group_id: 'not-a-group'
+          end.to raise_error(ActionController::RoutingError)
+        end
+      end
+    end
+
+    context "with access" do
+      before do
+        sign_in(user)
+      end
+
+      it "has descriptive error flash" do
+        post :failure, group_id: group
+
+        expect(flash[:alert]).to start_with("Unable to sign you in to the group with SAML due to")
+        expect(flash[:alert]).to include("Fingerprint mismatch")
+      end
+
+      it "redirects back go the SSO page" do
+        post :failure, group_id: group
+
+        expect(response).to redirect_to(sso_group_saml_providers_path)
+      end
+    end
+
+    context "with access to SAML settings for the group" do
+      before do
+        group.add_owner(user)
+        sign_in(user)
+      end
+
+      it "redirects to the settings page" do
+        post :failure, group_id: group
+
+        expect(response).to redirect_to(group_saml_providers_path)
+      end
+    end
+  end
 end

ee/spec/lib/gitlab/auth/group_saml/failure_handler_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Auth::GroupSaml::FailureHandler do
+  include Gitlab::Routing
+
+  let(:parent_handler) { double }
+  let(:user) { create(:user) }
+  let(:saml_provider) { create(:saml_provider) }
+  let(:group) { saml_provider.group }
+  let(:warden) { Warden::Proxy.new({}, Warden::Manager.new(nil)) }
+
+  subject { described_class.new(parent_handler) }
+
+  def failure_env(path, strategy)
+    params = {
+      'omniauth.error.strategy' => strategy,
+      'devise.mapping' => Devise.mappings[:user],
+      'warden' => warden
+    }
+    Rack::MockRequest.env_for(path, params)
+  end
+
+  def sign_in
+    warden.set_user(user, scope: :user)
+  end
+
+  before do
+    sign_in
+  end
+
+  it 'calls Groups::OmniauthCallbacksController#failure for GroupSaml' do
+    strategy = OmniAuth::Strategies::GroupSaml.new({})
+    callback_path = callback_group_saml_providers_path(group)
+    env = failure_env(callback_path, strategy)
+
+    expect_any_instance_of(Groups::OmniauthCallbacksController).to receive(:failure).and_call_original
+
+    subject.call(env)
+  end
+
+  it 'falls back to parent on_failure handler' do
+    env = failure_env('/', double)
+
+    expect(parent_handler).to receive(:call).with(env)
+
+    subject.call(env)
+  end
+end

spec/support/helpers/login_helpers.rb

@@ -132,6 +132,14 @@ module LoginHelpers
     env['omniauth.auth'] = OmniAuth.config.mock_auth[provider.to_sym]
   end
 
+  def stub_omniauth_failure(strategy, message_key, exception = nil)
+    env = @request.env
+
+    env['omniauth.error'] = exception
+    env['omniauth.error.type'] = message_key.to_sym
+    env['omniauth.error.strategy'] = strategy
+  end
+
   def stub_omniauth_saml_config(messages)
     set_devise_mapping(context: Rails.application)
     Rails.application.routes.disable_clear_and_finalize = true

spec/support/helpers/routes_helpers.rb

+module RoutesHelpers
+  def fake_routes(&block)
+    @routes = @routes.dup
+    @routes.formatter.clear
+    ActionDispatch::Routing::Mapper.new(@routes).instance_exec(&block)
+  end
+end