Merge branch 'jej/group-saml-sso-when-signed-out' into 'master'

Group SAML SSO when signed out

Closes #6261

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

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,19 +37,57 @@ 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.
 
-    flash[:notice] = "You must be signed in to use SAML with this group"
+    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)
 
-    redirect_to new_user_session_path
+    store_location_for(:redirect, after_gitlab_sign_in)
+    redirect_to new_user_session_path, notice: notice
   end
 
   def saml_redirect_path

ee/app/controllers/groups/sso_controller.rb

@@ -60,7 +60,12 @@ class Groups::SsoController < Groups::ApplicationController
   end
 
   def check_user_can_sign_in_with_provider
-    route_not_found unless can?(current_user, :sign_in_with_saml_provider, @unauthenticated_group.saml_provider)
+    actor = saml_discovery_token_actor || current_user
+    route_not_found unless can?(actor, :sign_in_with_saml_provider, @unauthenticated_group.saml_provider)
+  end
+
+  def saml_discovery_token_actor
+    Gitlab::Auth::GroupSaml::TokenActor.new(params[:token]) if params[:token]
   end
 
   def redirect_if_group_moved

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/policies/saml_provider_policy.rb

 # frozen_string_literal: true
 
 class SamlProviderPolicy < BasePolicy
-  rule { ~anonymous }.enable :sign_in_with_saml_provider
+  delegate { @subject.group }
+
+  def actor
+    @user
+  end
+
+  condition(:public_group, scope: :subject) { @subject.group.public? }
+
+  condition(:signed_in, scope: :user) { actor.is_a?(::User) }
+
+  condition(:token_grants_private_access) do
+    actor.is_a?(Gitlab::Auth::GroupSaml::TokenActor) && actor.valid_for?(@subject.group)
+  end
+
+  condition(:can_discover_group?) do
+    public_group? || token_grants_private_access? || signed_in?
+  end
+
+  rule { can_discover_group? }.enable :sign_in_with_saml_provider
 end

ee/app/views/groups/saml_providers/_info.html.haml

@@ -30,6 +30,6 @@
   - if @saml_provider.persisted?
     .well-segment.borderless
       %label= _("GitLab single sign on URL")
-      - user_login_url = sso_group_saml_providers_url(@group)
+      - user_login_url = sso_group_saml_providers_url(@group, token: @group.saml_discovery_token)
       %div= link_to user_login_url, user_login_url, class: "qa-user-login-url-link"
       .form-text.text-muted= _("Used by members to sign in to your group in GitLab")

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/token_actor.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module GroupSaml
+      class TokenActor
+        def initialize(token)
+          @token = token
+        end
+
+        def valid_for?(group)
+          group.saml_discovery_token.present? && group.saml_discovery_token == @token
+        end
+      end
+    end
+  end
+end

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,43 +38,45 @@ describe Groups::OmniauthCallbacksController do
       stub_omniauth_provider(provider, context: request)
     end
 
-    context "when signed in" do
-      before do
-        sign_in(user)
+    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' }
+
+        expect(response).to redirect_to('/explore')
       end
 
-      context "and identity already linked" do
-        let(:user) { create(:omniauth_user, extern_uid: uid, provider: provider, saml_provider: saml_provider) }
+      it "displays a flash message verifying group sign in" do
+        post provider, params: { group_id: group }
 
-        it "redirects to RelayState" do
-          post provider, params: { group_id: group, RelayState: '/explore' }
+        expect(flash[:notice]).to match(/Signed in with SAML/i)
+      end
 
-          expect(response).to redirect_to('/explore')
-        end
+      it 'uses existing linked identity' do
+        expect { post provider, params: { group_id: group } }.not_to change(linked_accounts, :count)
+      end
 
-        it "displays a flash message verifying group sign in" do
+      it 'skips authenticity token based forgery protection' do
+        with_forgery_protection do
           post provider, params: { group_id: group }
 
-          expect(flash[:notice]).to start_with "Signed in with SAML"
-        end
-
-        it 'uses existing linked identity' do
-          expect { post provider, params: { group_id: group } }.not_to change(linked_accounts, :count)
+          expect(response).not_to be_client_error
+          expect(response).not_to be_server_error
         end
+      end
+    end
 
-        it 'skips authenticity token based forgery protection' do
-          with_forgery_protection do
-            post provider, params: { group_id: group }
-
-            expect(response).not_to be_client_error
-            expect(response).not_to be_server_error
-          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,17 +108,35 @@ describe Groups::OmniauthCallbacksController do
     end
 
     context "when not signed in" do
-      it "redirects to sign in page" do
-        post provider, params: { group_id: group }
+      context "and identity hasn't been linked" do
+        it "redirects to sign in page" do
+          post provider, params: { group_id: group }
 
-        expect(response).to redirect_to(new_user_session_path)
+          expect(response).to redirect_to(new_user_session_path)
+        end
+
+        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("Login to a GitLab account to link with your SAML identity")
+        end
       end
 
-      it "informs users that they need to sign in to the GitLab instance first" do
-        post provider, params: { group_id: group }
+      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
 
-        expect(flash[:notice]).to start_with("You must be signed in")
+        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

@@ -92,7 +92,8 @@ describe 'SAML provider settings' do
 
         login_url = find('label', text: 'GitLab single sign on URL').find('~* a').text
 
-        expect(login_url).to end_with "/groups/#{group.full_path}/-/saml/sso"
+        expect(login_url).to include "/groups/#{group.full_path}/-/saml/sso"
+        expect(login_url).to end_with "?token=#{group.reload.saml_discovery_token}"
       end
 
       context 'enforced sso enabled' do
@@ -167,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
 
@@ -219,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/token_actor_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Gitlab::Auth::GroupSaml::TokenActor do
+  let(:saml_provider) { create(:saml_provider) }
+  let(:group) { saml_provider.group }
+
+  subject { described_class.new(token) }
+
+  context 'valid token' do
+    let(:token) { group.saml_discovery_token }
+
+    it 'is valid for the group' do
+      expect(subject).to be_valid_for(group)
+    end
+  end
+
+  context 'invalid token' do
+    let(:token) { 'abcdef' }
+
+    it 'is invalid for the group' do
+      expect(subject).not_to be_valid_for(group)
+    end
+  end
+
+  context 'missing token' do
+    let(:token) { nil }
+
+    it 'is invalid for the group' do
+      expect(subject).not_to be_valid_for(group)
+    end
+  end
+
+  context 'when geo prevents saml_provider from having a token' do
+    let(:token) { nil }
+    let(:group) { double(:group, saml_discovery_token: nil) }
+
+    it 'prevents nil token from allowing access' do
+      expect(subject).not_to be_valid_for(group)
+    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

ee/spec/policies/saml_provider_policy_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe SamlProviderPolicy do
+  let(:group_visibility) { :public }
+  let(:group) { create(:group, group_visibility) }
+  let(:saml_provider) { create(:saml_provider, group: group) }
+
+  context 'with a user' do
+    let(:user) { create(:user) }
+
+    subject { described_class.new(user, saml_provider) }
+
+    it 'allows access to public groups' do
+      is_expected.to be_allowed(:sign_in_with_saml_provider)
+    end
+
+    it 'allows access to private groups' do
+      group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+
+      is_expected.to be_allowed(:sign_in_with_saml_provider)
+    end
+  end
+
+  context 'with a token actor' do
+    subject { described_class.new(token_actor, saml_provider) }
+
+    context 'valid token' do
+      let(:token_actor) { Gitlab::Auth::GroupSaml::TokenActor.new(group.saml_discovery_token) }
+
+      it 'allows access to public groups' do
+        is_expected.to be_allowed(:sign_in_with_saml_provider)
+      end
+
+      it 'allows access to private groups' do
+        group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+
+        is_expected.to be_allowed(:sign_in_with_saml_provider)
+      end
+    end
+
+    context 'invalid or missing token' do
+      let(:token_actor) { Gitlab::Auth::GroupSaml::TokenActor.new("xyz") }
+
+      it 'allows anonymous access to public groups' do
+        is_expected.to be_allowed(:sign_in_with_saml_provider)
+      end
+
+      it 'prevents access to private groups' do
+        group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+
+        is_expected.not_to be_allowed(:sign_in_with_saml_provider)
+      end
+    end
+  end
+
+  context 'without a user or actor' do
+    subject { described_class.new(nil, saml_provider) }
+
+    it 'allows access to public groups' do
+      is_expected.to be_allowed(:sign_in_with_saml_provider)
+    end
+
+    it 'prevents access to private groups' do
+      group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+
+      is_expected.not_to be_allowed(:sign_in_with_saml_provider)
+    end
+  end
+end