Signed out user flow for Group SAML

This will allow Group SAML to be used for signing in to GitLab.com
Initially this will be behind the feature flag:
"group_saml_allows_sign_in_to_gitlab" so we can roll it out per group.

Implements Auth::GroupSaml::User to process callback when signed out.
Updates Groups::OmniauthCallbacksController to handle more complicated
redirect flows.

app/controllers/omniauth_callbacks_controller.rb

@@ -116,8 +116,12 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     session[:service_tickets][provider] = ticket
   end
 
+  def build_auth_user(auth_user_class)
+    auth_user_class.new(oauth)
+  end
+
   def sign_in_user_flow(auth_user_class)
-    auth_user = auth_user_class.new(oauth)
+    auth_user = build_auth_user(auth_user_class)
     user = auth_user.find_and_update!
 
     if auth_user.valid_sign_in?

ee/app/controllers/groups/omniauth_callbacks_controller.rb

@@ -7,9 +7,9 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
 
   def group_saml
     @unauthenticated_group = Group.find_by_full_path(params[:group_id])
-    saml_provider = @unauthenticated_group.saml_provider
+    @saml_provider = @unauthenticated_group.saml_provider
 
-    identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(current_user, oauth, saml_provider)
+    identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(current_user, oauth, @saml_provider)
 
     omniauth_flow(Gitlab::Auth::GroupSaml, identity_linker: identity_linker)
   end
@@ -25,7 +25,7 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
 
   override :redirect_identity_exists
   def redirect_identity_exists
-    flash[:notice] = "Signed in with SAML for #{@unauthenticated_group.name}"
+    flash[:notice] = "Already signed in with SAML for #{@unauthenticated_group.name}"
 
     redirect_to after_sign_in_path_for(current_user)
   end
@@ -37,20 +37,58 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
     redirect_to after_sign_in_path_for(current_user)
   end
 
+  override :sign_in_and_redirect
+  def sign_in_and_redirect(user, *args)
+    flash[:notice] = "Signed in with SAML for #{@unauthenticated_group.name}"
+
+    super
+  end
+
   override :after_sign_in_path_for
   def after_sign_in_path_for(resource)
     saml_redirect_path || super
   end
 
+  override :build_auth_user
+  def build_auth_user(auth_user_class)
+    Gitlab::Auth::GroupSaml::User.new(oauth, @saml_provider)
+  end
+
   override :sign_in_user_flow
   def sign_in_user_flow(auth_user_class)
     # User has successfully authenticated with the SAML provider for the group
     # but is not signed in to the GitLab instance.
 
+    if sign_in_to_gitlab_enabled?
+      super
+    else
       flash[:notice] = "You must be signed in to use SAML with this group"
 
       redirect_to new_user_session_path
     end
+  end
+
+  def sign_in_to_gitlab_enabled?
+    ::Feature.enabled?(:group_saml_allows_sign_in_to_gitlab, @unauthenticated_group)
+  end
+
+  override :fail_login
+  def fail_login(user)
+    if user
+      super
+    else
+      redirect_to_login_or_register
+    end
+  end
+
+  def redirect_to_login_or_register
+    notice = "Login to a GitLab account to link with your SAML identity"
+
+    after_gitlab_sign_in = sso_group_saml_providers_path(@unauthenticated_group)
+
+    store_location_for(:redirect, after_gitlab_sign_in)
+    redirect_to new_user_session_path, notice: notice
+  end
 
   def saml_redirect_path
     params['RelayState'].presence if current_user

ee/app/finders/auth/group_saml_identity_finder.rb

+# frozen_string_literal: true
+
+module Auth
+  class GroupSamlIdentityFinder
+    attr_reader :saml_provider, :auth_hash
+
+    def initialize(saml_provider, auth_hash)
+      @saml_provider = saml_provider
+      @auth_hash = auth_hash
+    end
+
+    def first
+      Identity.find_by_group_saml_uid(saml_provider, uid)
+    end
+
+    private
+
+    def uid
+      auth_hash.uid
+    end
+  end
+end

ee/app/models/ee/identity.rb

@@ -24,6 +24,12 @@ module EE
         with_extern_uid(provider, extern_uid).take
       end
 
+      def find_by_group_saml_uid(saml_provider, extern_uid)
+        where(provider: :group_saml,
+              saml_provider: saml_provider,
+              extern_uid: extern_uid).take
+      end
+
       def preload_saml_group
         preload(saml_provider: { group: :route })
       end

ee/app/views/groups/sso/saml.html.haml

@@ -3,14 +3,14 @@
 = render 'devise/shared/tab_single', tab_title: _('SAML SSO')
 .login-box
   .login-body
-    - if @group_saml_identity
+    - if @group_saml_identity || !user_signed_in?
       %h4= _('Sign in to "%{group_name}"') % { group_name: @group_name }
     - else
       %h4= _('Allow "%{group_name}" to sign you in') % { group_name: @group_name }
 
     %p= _('The "%{group_path}" group allows you to sign in with your Single Sign-On Account') % { group_path: @group_path }
 
-    - if @group_saml_identity
+    - if @group_saml_identity || !user_signed_in?
       %p= _("This will redirect you to an external sign in page.")
 
       = saml_link _('Sign in with Single Sign-On'), @group_path, html_class: 'btn btn-success btn-block qa-saml-sso-signin-button'

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

@@ -4,8 +4,41 @@ module Gitlab
   module Auth
     module GroupSaml
       class User
-        def initialize(auth_hash)
-          raise NotImplementedError
+        attr_reader :auth_hash, :saml_provider
+
+        def initialize(auth_hash, saml_provider)
+          @auth_hash = auth_hash
+          @saml_provider = saml_provider
+        end
+
+        def find_and_update!
+          update_group_membership
+
+          user_from_identity
+        end
+
+        def valid_sign_in?
+          user_from_identity.present?
+        end
+
+        def bypass_two_factor?
+          false
+        end
+
+        private
+
+        def identity
+          @identity ||= ::Auth::GroupSamlIdentityFinder.new(saml_provider, auth_hash).first
+        end
+
+        def user_from_identity
+          @user_from_identity ||= identity&.user
+        end
+
+        def update_group_membership
+          return unless user_from_identity
+
+          MembershipUpdater.new(user_from_identity, saml_provider).execute
         end
       end
     end

ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb

@@ -18,6 +18,10 @@ describe Groups::OmniauthCallbacksController do
     Identity.where(user: user, extern_uid: uid, provider: provider)
   end
 
+  def create_linked_user
+    create(:omniauth_user, extern_uid: uid, provider: provider, saml_provider: saml_provider)
+  end
+
   context "when request hasn't been validated by omniauth middleware" do
     it "prevents authentication" do
       sign_in(user)
@@ -34,13 +38,8 @@ describe Groups::OmniauthCallbacksController do
       stub_omniauth_provider(provider, context: request)
     end
 
-    context "when signed in" do
-      before do
-        sign_in(user)
-      end
-
-      context "and identity already linked" do
-        let(:user) { create(:omniauth_user, extern_uid: uid, provider: provider, saml_provider: saml_provider) }
+    shared_examples "and identity already linked" do
+      let!(:user) { create_linked_user }
 
       it "redirects to RelayState" do
         post provider, params: { group_id: group, RelayState: '/explore' }
@@ -51,7 +50,7 @@ describe Groups::OmniauthCallbacksController do
       it "displays a flash message verifying group sign in" do
         post provider, params: { group_id: group }
 
-          expect(flash[:notice]).to start_with "Signed in with SAML"
+        expect(flash[:notice]).to match(/Signed in with SAML/i)
       end
 
       it 'uses existing linked identity' do
@@ -68,9 +67,16 @@ describe Groups::OmniauthCallbacksController do
       end
     end
 
+    context "when signed in" do
+      before do
+        sign_in(user)
+      end
+
+      it_behaves_like "and identity already linked"
+
       context 'oauth already linked to another account' do
         before do
-          create(:omniauth_user, extern_uid: uid, provider: provider, saml_provider: saml_provider)
+          create_linked_user
         end
 
         it 'displays warning to user' do
@@ -102,6 +108,7 @@ describe Groups::OmniauthCallbacksController do
     end
 
     context "when not signed in" do
+      context "and identity hasn't been linked" do
         it "redirects to sign in page" do
           post provider, params: { group_id: group }
 
@@ -111,8 +118,25 @@ describe Groups::OmniauthCallbacksController do
         it "informs users that they need to sign in to the GitLab instance first" do
           post provider, params: { group_id: group }
 
-        expect(flash[:notice]).to start_with("You must be signed in")
+          expect(flash[:notice]).to start_with("Login to a GitLab account to link with your SAML identity")
+        end
+      end
+
+      context 'identity linked but sign in flow disabled' do
+        before do
+          create_linked_user
+          stub_feature_flags(group_saml_allows_sign_in_to_gitlab: false)
         end
+
+        it 'prevents sign in' do
+          post provider, params: { group_id: group }
+
+          expect(flash[:notice]).to start_with('You must be signed in')
+          expect(response).to redirect_to('/users/sign_in')
+        end
+      end
+
+      it_behaves_like "and identity already linked"
     end
   end
 

ee/spec/features/groups/saml_providers_spec.rb

@@ -168,10 +168,12 @@ describe 'SAML provider settings' do
       end
 
       context 'when not signed in' do
-        it "doesn't show sso page" do
+        it "shows the sso page so user can sign in" do
           visit sso_group_saml_providers_path(group)
 
-          expect(current_path).to eq(new_user_session_path)
+          expect(page).to have_content('SAML SSO')
+          expect(page).to have_content("Sign in to \"#{group.full_name}\"")
+          expect(page).to have_content('Sign in with Single Sign-On')
         end
       end
 
@@ -220,6 +222,12 @@ describe 'SAML provider settings' do
 
             expect(current_path).to eq(new_user_session_path)
           end
+
+          it "shows the sso page if the token is given" do
+            visit sso_group_saml_providers_path(group, token: group.saml_discovery_token)
+
+            expect(current_path).to eq sso_group_saml_providers_path(group)
+          end
         end
 
         context 'when signed in' do

ee/spec/finders/auth/group_saml_identity_finder_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Auth::GroupSamlIdentityFinder do
+  let(:uid) { 1234 }
+  let!(:identity) { create(:group_saml_identity, extern_uid: uid) }
+  let(:saml_provider) { identity.saml_provider }
+  let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid) }
+
+  subject { described_class.new(saml_provider, auth_hash) }
+
+  describe '#first' do
+    it 'looks up identity by saml_provider and uid' do
+      expect(subject.first).to eq identity
+    end
+  end
+end

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

+# frozen_string_literal: true
+require 'spec_helper'
+
+require 'spec_helper'
+
+describe Gitlab::Auth::GroupSaml::User do
+  let(:uid) { 1234 }
+  let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid) }
+  let(:saml_provider) { create(:saml_provider) }
+  let(:group) { saml_provider.group }
+
+  subject { described_class.new(auth_hash, saml_provider) }
+
+  def create_existing_identity
+    create(:group_saml_identity, extern_uid: uid, saml_provider: saml_provider)
+  end
+
+  describe '#valid_sign_in?' do
+    context 'with matching user for that group and uid' do
+      let!(:identity) { create_existing_identity }
+
+      it 'returns true' do
+        is_expected.to be_valid_sign_in
+      end
+    end
+
+    context 'with no matching user identity' do
+      it 'returns false' do
+        is_expected.not_to be_valid_sign_in
+      end
+    end
+  end
+
+  describe '#find_and_update!' do
+    context 'with matching user for that group and uid' do
+      let!(:identity) { create_existing_identity }
+
+      it 'updates group membership' do
+        expect do
+          subject.find_and_update!
+        end.to change { group.members.count }.by(1)
+      end
+
+      it 'returns the user' do
+        expect(subject.find_and_update!).to eq identity.user
+      end
+    end
+
+    context 'with no matching user identity' do
+      it 'does nothing' do
+        expect(subject.find_and_update!).to eq nil
+        expect(group.members.count).to eq 0
+      end
+    end
+  end
+
+  describe '#bypass_two_factor?' do
+    it 'is false' do
+      expect(subject.bypass_two_factor?).to eq false
+    end
+  end
+end