Merge branch 'jej/group-saml-callback-flow' into 'master'

Add Group level SAML callback for GitLab.com SSO

Closes #4514

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

app/controllers/omniauth_callbacks_controller.rb

@@ -93,7 +93,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
 
       if identity_linker.changed?
         redirect_identity_linked
-      elsif identity_linker.error_message.present?
+      elsif identity_linker.failed?
         redirect_identity_link_failed(identity_linker.error_message)
       else
         redirect_identity_exists

config/routes/group.rb

@@ -68,7 +68,9 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
 
     resources :ldap_group_links, only: [:index, :create, :destroy]
 
+    ## EE-specific
     resource :saml_providers, path: 'saml', only: [:show, :create, :update] do
+      post :callback, to: 'omniauth_callbacks#group_saml'
       get :sso, to: 'sso#saml'
     end
 

ee/app/controllers/groups/omniauth_callbacks_controller.rb

+class Groups::OmniauthCallbacksController < OmniauthCallbacksController
+  extend ::Gitlab::Utils::Override
+
+  def group_saml
+    @unauthenticated_group = Group.find_by_full_path(params[:group_id])
+    saml_provider = @unauthenticated_group.saml_provider
+
+    identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(current_user, oauth, saml_provider)
+
+    omniauth_flow(Gitlab::Auth::GroupSaml, identity_linker: identity_linker)
+  end
+
+  private
+
+  override :redirect_identity_linked
+  def redirect_identity_linked
+    flash[:notice] = "SAML for #{@unauthenticated_group.name} was added to your connected accounts"
+
+    redirect_to after_sign_in_path_for(current_user)
+  end
+
+  override :redirect_identity_exists
+  def redirect_identity_exists
+    flash[:notice] = "Signed in with SAML for #{@unauthenticated_group.name}"
+
+    redirect_to after_sign_in_path_for(current_user)
+  end
+
+  override :after_sign_in_path_for
+  def after_sign_in_path_for(resource)
+    saml_redirect_path || super
+  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"
+
+    redirect_to new_user_session_path
+  end
+
+  def saml_redirect_path
+    params['RelayState'].presence if current_user
+  end
+end

ee/changelogs/unreleased/jej-group-saml-callback-flow.yml

+---
+title: Adds authentication flow for GitLab.com per group SAML beta
+merge_request: 5575
+author:
+type: changed

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

+module Gitlab
+  module Auth
+    module GroupSaml
+      class IdentityLinker < Gitlab::Auth::Saml::IdentityLinker
+        attr_reader :saml_provider
+
+        def initialize(current_user, oauth, saml_provider)
+          super(current_user, oauth)
+
+          @saml_provider = saml_provider
+        end
+
+        def link
+          super
+
+          update_group_membership unless failed?
+        end
+
+        protected
+
+        def identity
+          @identity ||= current_user.identities.where(provider: :group_saml,
+                                                      saml_provider: saml_provider,
+                                                      extern_uid: uid.to_s)
+                                    .first_or_initialize
+        end
+
+        def update_group_membership
+          MembershipUpdater.new(current_user, saml_provider).execute
+        end
+      end
+    end
+  end
+end

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

+module Gitlab
+  module Auth
+    module GroupSaml
+      class MembershipUpdater
+        attr_reader :user, :saml_provider
+
+        delegate :group, to: :saml_provider
+
+        def initialize(user, saml_provider)
+          @user = user
+          @saml_provider = saml_provider
+        end
+
+        def execute
+          return if group.member?(@user)
+
+          group.add_user(@user, default_membership_level)
+        end
+
+        private
+
+        def default_membership_level
+          :guest
+        end
+      end
+    end
+  end
+end

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

+module Gitlab
+  module Auth
+    module GroupSaml
+      class User
+        def initialize(auth_hash)
+          raise NotImplementedError
+        end
+      end
+    end
+  end
+end

ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb

+require 'spec_helper'
+
+describe Groups::OmniauthCallbacksController do
+  include LoginHelpers
+
+  let(:uid) { 'my-uid' }
+  let(:user) { create(:user) }
+  let(:provider) { :group_saml }
+  let(:group) { create(:group, :private) }
+  let!(:saml_provider) { create(:saml_provider, group: group) }
+
+  before do
+    stub_licensed_features(group_saml: true)
+  end
+
+  def linked_accounts
+    Identity.where(user: user, extern_uid: uid, provider: provider)
+  end
+
+  context "when request hasn't been validated by omniauth middleware" do
+    it "prevents authentication" do
+      sign_in(user)
+
+      expect do
+        post provider, group_id: group
+      end.to raise_error(AbstractController::ActionNotFound)
+    end
+  end
+
+  context "valid credentials" do
+    before do
+      mock_auth_hash(provider, uid, user.email)
+      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) }
+
+        it "redirects to RelayState" do
+          post provider, group_id: group, RelayState: '/explore'
+
+          expect(response).to redirect_to('/explore')
+        end
+
+        it "displays a flash message verifying group sign in" do
+          post provider, group_id: group
+
+          expect(flash[:notice]).to start_with "Signed in with SAML"
+        end
+
+        it 'uses existing linked identity' do
+          expect { post provider, group_id: group }.not_to change(linked_accounts, :count)
+        end
+      end
+
+      context 'oauth already linked to another account' do
+        before do
+          create(:omniauth_user, extern_uid: uid, provider: provider, saml_provider: saml_provider)
+        end
+
+        it 'displays warning to user' do
+          post provider, group_id: group
+
+          expect(flash[:notice]).to match(/has already been taken*/)
+        end
+      end
+
+      context "and identity hasn't been linked" do
+        it "links the identity" do
+          post provider, group_id: group
+
+          expect(group).to be_member(user)
+        end
+
+        it "redirects to RelayState" do
+          post provider, group_id: group, RelayState: '/explore'
+
+          expect(response).to redirect_to('/explore')
+        end
+
+        it "displays a flash indicating the account has been linked" do
+          post provider, group_id: group
+
+          expect(flash[:notice]).to match(/SAML for .* was added/)
+        end
+      end
+    end
+
+    context "when not signed in" do
+      it "redirects to sign in page" do
+        post provider, group_id: group
+
+        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, group_id: group
+
+        expect(flash[:notice]).to start_with("You must be signed in")
+      end
+    end
+  end
+end

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

+require 'spec_helper'
+
+describe Gitlab::Auth::GroupSaml::IdentityLinker do
+  let(:user) { create(:user) }
+  let(:provider) { 'group_saml' }
+  let(:uid) { user.email }
+  let(:oauth) { { 'provider' => provider, 'uid' => uid } }
+  let(:saml_provider) { create(:saml_provider) }
+
+  subject { described_class.new(user, oauth, saml_provider) }
+
+  context 'linked identity exists' do
+    let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid, saml_provider: saml_provider) }
+
+    it "doesn't create new identity" do
+      expect { subject.link }.not_to change { Identity.count }
+    end
+
+    it "sets #changed? to false" do
+      subject.link
+
+      expect(subject).not_to be_changed
+    end
+
+    it 'adds user to group' do
+      subject.link
+
+      expect(saml_provider.group.member?(user)).to eq(true)
+    end
+  end
+
+  context 'identity needs to be created' do
+    it 'creates linked identity' do
+      expect { subject.link }.to change { user.identities.count }
+    end
+
+    it 'sets identity provider' do
+      subject.link
+
+      expect(user.identities.last.provider).to eq provider
+    end
+
+    it 'sets saml provider' do
+      subject.link
+
+      expect(user.identities.last.saml_provider).to eq saml_provider
+    end
+
+    it 'sets identity extern_uid' do
+      subject.link
+
+      expect(user.identities.last.extern_uid).to eq uid
+    end
+
+    it 'sets #changed? to true' do
+      subject.link
+
+      expect(subject).to be_changed
+    end
+
+    it 'adds user to group' do
+      subject.link
+
+      expect(saml_provider.group.member?(user)).to eq(true)
+    end
+  end
+end

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

+require 'spec_helper'
+
+describe Gitlab::Auth::GroupSaml::MembershipUpdater do
+  let(:user) { create(:user) }
+  let(:saml_provider) { create(:saml_provider) }
+  let(:group) { saml_provider.group }
+
+  it "adds the user to the group" do
+    described_class.new(user, saml_provider).execute
+
+    expect(group.users).to include(user)
+  end
+
+  it "doesn't duplicate group membership" do
+    group.add_guest(user)
+
+    described_class.new(user, saml_provider).execute
+
+    expect(group.members.count).to eq 1
+  end
+
+  it "doesn't overwrite existing membership level" do
+    group.add_master(user)
+
+    described_class.new(user, saml_provider).execute
+
+    expect(group.members.pluck(:access_level)).to eq([Gitlab::Access::MASTER])
+  end
+end

lib/gitlab/auth/omniauth_identity_linker_base.rb

@@ -17,6 +17,10 @@ module Gitlab
         @changed
       end
 
+      def failed?
+        error_message.present?
+      end
+
       def error_message
         identity.validate
 

spec/factories/users.rb

@@ -70,13 +70,15 @@ FactoryBot.define do
       transient do
         extern_uid '123456'
         provider 'ldapmain'
+        saml_provider nil
       end
 
       after(:create) do |user, evaluator|
         user.identities << create(
           :identity,
           provider: evaluator.provider,
-          extern_uid: evaluator.extern_uid
+          extern_uid: evaluator.extern_uid,
+          saml_provider: evaluator.saml_provider
         )
       end
     end