From b5878d16d98d2930fd9b36df9133dda1460ba94f Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Wed, 7 Nov 2018 18:07:00 +0000
Subject: [PATCH 1/3] Can unlink Group SAML from accounts page

Allows users to unlink their Group SAML accounts.
Needed so that users can decide when they no longer want to
allow an organization to log them into GitLab.com

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/5016
---
 app/assets/stylesheets/pages/profile.scss     |  2 +
 .../profiles/accounts_controller.rb           | 10 +++-
 app/helpers/auth_helper.rb                    |  4 ++
 app/views/profiles/accounts/show.html.haml    | 10 ++--
 .../ee/profiles/accounts_controller.rb        | 16 +++++++
 ee/app/controllers/groups/sso_controller.rb   | 15 +++++-
 ee/app/finders/group_saml_identity_finder.rb  | 19 ++++++++
 ee/app/helpers/ee/auth_helper.rb              |  9 ++++
 ee/app/models/ee/identity.rb                  |  6 +++
 .../group_saml/identity/destroy_service.rb    | 37 +++++++++++++++
 .../_group_saml_unlink_buttons.html.haml      |  7 +++
 .../jej-group-saml-unlink-from-account.yml    |  5 ++
 ee/config/routes/group.rb                     |  1 +
 ee/spec/features/profiles/account_spec.rb     | 46 +++++++++++++++++++
 .../group_saml_identity_finder_spec.rb        | 41 +++++++++++++++++
 .../identity/destroy_service_spec.rb          | 43 +++++++++++++++++
 locale/gitlab.pot                             |  3 ++
 spec/factories/identities.rb                  |  1 +
 18 files changed, 269 insertions(+), 6 deletions(-)
 create mode 100644 ee/app/controllers/ee/profiles/accounts_controller.rb
 create mode 100644 ee/app/finders/group_saml_identity_finder.rb
 create mode 100644 ee/app/services/group_saml/identity/destroy_service.rb
 create mode 100644 ee/app/views/profiles/accounts/_group_saml_unlink_buttons.html.haml
 create mode 100644 ee/changelogs/unreleased/jej-group-saml-unlink-from-account.yml
 create mode 100644 ee/spec/features/profiles/account_spec.rb
 create mode 100644 ee/spec/finders/group_saml_identity_finder_spec.rb
 create mode 100644 ee/spec/services/group_saml/identity/destroy_service_spec.rb

diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss
index 7fbec963ab924e..531f6a5ecbf37c 100644
--- a/app/assets/stylesheets/pages/profile.scss
+++ b/app/assets/stylesheets/pages/profile.scss
@@ -144,11 +144,13 @@
 .provider-btn-group {
   display: inline-block;
   margin-right: 10px;
+  margin-bottom: 10px;
   border: 1px solid $border-color;
   border-radius: 3px;
 
   &:last-child {
     margin-right: 0;
+    margin-bottom: 0;
   }
 }
 
diff --git a/app/controllers/profiles/accounts_controller.rb b/app/controllers/profiles/accounts_controller.rb
index cb3180f419676e..9e47627ba8b19b 100644
--- a/app/controllers/profiles/accounts_controller.rb
+++ b/app/controllers/profiles/accounts_controller.rb
@@ -4,7 +4,7 @@ class Profiles::AccountsController < Profiles::ApplicationController
   include AuthHelper
 
   def show
-    @user = current_user
+    render(locals: show_view_variables)
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
@@ -23,4 +23,12 @@ def unlink
     redirect_to profile_account_path
   end
   # rubocop: enable CodeReuse/ActiveRecord
+
+  private
+
+  def show_view_variables
+    { user: current_user }
+  end
 end
+
+Profiles::AccountsController.prepend(EE::Profiles::AccountsController)
diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb
index b038f6e612fcd4..0d9da67ec0f72a 100644
--- a/app/helpers/auth_helper.rb
+++ b/app/helpers/auth_helper.rb
@@ -57,6 +57,10 @@ def button_based_providers
     auth_providers.reject { |provider| form_based_provider?(provider) }
   end
 
+  def display_providers_on_profile?
+    button_based_providers.any?
+  end
+
   def providers_for_base_controller
     auth_providers.reject { |provider| LDAP_PROVIDER === provider }
   end
diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml
index 2220b4eee96e3f..fffdc305623bdf 100644
--- a/app/views/profiles/accounts/show.html.haml
+++ b/app/views/profiles/accounts/show.html.haml
@@ -1,5 +1,6 @@
 - page_title "Account"
 - @content_class = "limit-container-width" unless fluid_layout
+- user = local_assigns.fetch(:user)
 
 - if current_user.ldap_user?
   .alert.alert-info
@@ -21,7 +22,7 @@
         = link_to 'Enable two-factor authentication', profile_two_factor_auth_path, class: 'btn btn-success'
 
 %hr
-- if button_based_providers.any?
+- if display_providers_on_profile?
   .row.prepend-top-default
     .col-lg-4.profile-settings-sidebar
       %h4.prepend-top-0
@@ -46,6 +47,7 @@
           - else
             = link_to omniauth_authorize_path(:user, provider), method: :post, class: 'provider-btn not-active' do
               Connect
+      = render_if_exists 'profiles/accounts/group_saml_unlink_buttons', group_saml_identities: local_assigns[:group_saml_identities]
   %hr
 - if current_user.can_change_username?
   .row.prepend-top-default
@@ -66,7 +68,7 @@
     %h4.prepend-top-0.danger-title
       = s_('Profiles|Delete account')
   .col-lg-8
-    - if @user.can_be_removed? && can?(current_user, :destroy_user, @user)
+    - if user.can_be_removed? && can?(current_user, :destroy_user, user)
       %p
         = s_('Profiles|Deleting an account has the following effects:')
       = render 'users/deletion_guidance', user: current_user
@@ -79,10 +81,10 @@
         confirm_with_password: ('true' if current_user.confirm_deletion_with_password?),
         username: current_user.username } }
     - else
-      - if @user.solo_owned_groups.present?
+      - if user.solo_owned_groups.present?
         %p
           = s_('Profiles|Your account is currently an owner in these groups:')
-          %strong= @user.solo_owned_groups.map(&:name).join(', ')
+          %strong= user.solo_owned_groups.map(&:name).join(', ')
         %p
           = s_('Profiles|You must transfer ownership or delete these groups before you can delete your account.')
       - else
diff --git a/ee/app/controllers/ee/profiles/accounts_controller.rb b/ee/app/controllers/ee/profiles/accounts_controller.rb
new file mode 100644
index 00000000000000..79b556f4c4cbfa
--- /dev/null
+++ b/ee/app/controllers/ee/profiles/accounts_controller.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module EE
+  module Profiles::AccountsController
+    extend ::Gitlab::Utils::Override
+
+    private
+
+    override :show_view_variables
+    def show_view_variables
+      group_saml_identities = GroupSamlIdentityFinder.new(user: current_user).all
+
+      super.merge(group_saml_identities: group_saml_identities)
+    end
+  end
+end
diff --git a/ee/app/controllers/groups/sso_controller.rb b/ee/app/controllers/groups/sso_controller.rb
index d1c7abcf491a9f..a5ade772afc3c6 100644
--- a/ee/app/controllers/groups/sso_controller.rb
+++ b/ee/app/controllers/groups/sso_controller.rb
@@ -6,7 +6,8 @@ class Groups::SsoController < Groups::ApplicationController
   before_action :check_group_saml_configured
   before_action :check_group_saml_available!
   before_action :require_configured_provider
-  before_action :check_user_can_sign_in_with_provider
+  before_action :authenticate_user!, only: [:unlink]
+  before_action :check_user_can_sign_in_with_provider, only: [:saml]
   before_action :redirect_if_group_moved
 
   layout 'devise'
@@ -16,8 +17,20 @@ def saml
     @group_name = @unauthenticated_group.full_name
   end
 
+  def unlink
+    return route_not_found unless linked_identity
+
+    GroupSaml::Identity::DestroyService.new(linked_identity).execute
+
+    redirect_to profile_account_path
+  end
+
   private
 
+  def linked_identity
+    @linked_identity ||= GroupSamlIdentityFinder.new(user: current_user).find_linked(group: @unauthenticated_group)
+  end
+
   def check_group_saml_available!
     route_not_found unless @unauthenticated_group.feature_available?(:group_saml)
   end
diff --git a/ee/app/finders/group_saml_identity_finder.rb b/ee/app/finders/group_saml_identity_finder.rb
new file mode 100644
index 00000000000000..f9a415d1207f6e
--- /dev/null
+++ b/ee/app/finders/group_saml_identity_finder.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class GroupSamlIdentityFinder
+  attr_reader :user
+
+  def initialize(user:)
+    @user = user
+  end
+
+  def find_linked(group:)
+    return unless user
+
+    group&.saml_provider&.identities&.find_by(user: user)
+  end
+
+  def all
+    user.group_saml_identities.preload_saml_group
+  end
+end
diff --git a/ee/app/helpers/ee/auth_helper.rb b/ee/app/helpers/ee/auth_helper.rb
index b5154fc8ea5621..42933dbdd433cb 100644
--- a/ee/app/helpers/ee/auth_helper.rb
+++ b/ee/app/helpers/ee/auth_helper.rb
@@ -8,6 +8,11 @@ module AuthHelper
 
     delegate :slack_app_id, to: :'Gitlab::CurrentSettings.current_application_settings'
 
+    override :display_providers_on_profile?
+    def display_providers_on_profile?
+      super || group_saml_enabled?
+    end
+
     override :button_based_providers
     def button_based_providers
       super - GROUP_LEVEL_PROVIDERS
@@ -45,6 +50,10 @@ def smartcard_enabled?
       ::Gitlab::Auth::Smartcard.enabled?
     end
 
+    def group_saml_enabled?
+      auth_providers.include?(:group_saml)
+    end
+
     def slack_redirect_uri(project)
       slack_auth_project_settings_slack_url(project)
     end
diff --git a/ee/app/models/ee/identity.rb b/ee/app/models/ee/identity.rb
index 9da95000491f19..b145774ca1aab7 100644
--- a/ee/app/models/ee/identity.rb
+++ b/ee/app/models/ee/identity.rb
@@ -18,5 +18,11 @@ module Identity
         iwhere(secondary_extern_uid: normalize_uid(provider, secondary_extern_uid)).with_provider(provider)
       end
     end
+
+    class_methods do
+      def preload_saml_group
+        preload(saml_provider: { group: :route })
+      end
+    end
   end
 end
diff --git a/ee/app/services/group_saml/identity/destroy_service.rb b/ee/app/services/group_saml/identity/destroy_service.rb
new file mode 100644
index 00000000000000..c6e155382e3657
--- /dev/null
+++ b/ee/app/services/group_saml/identity/destroy_service.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+module GroupSaml
+  module Identity
+    class DestroyService
+      attr_reader :identity
+
+      delegate :user, to: :identity
+
+      def initialize(identity)
+        @identity = identity
+      end
+
+      def execute
+        identity.destroy!
+        remove_group_access
+      end
+
+      private
+
+      def remove_group_access
+        return unless group_membership
+        return if group.last_owner?(user)
+
+        Members::DestroyService.new(user).execute(group_membership)
+      end
+
+      def group
+        @group ||= identity.saml_provider.group
+      end
+
+      def group_membership
+        @group_membership ||= group.group_member(user)
+      end
+    end
+  end
+end
diff --git a/ee/app/views/profiles/accounts/_group_saml_unlink_buttons.html.haml b/ee/app/views/profiles/accounts/_group_saml_unlink_buttons.html.haml
new file mode 100644
index 00000000000000..bd54d52441e278
--- /dev/null
+++ b/ee/app/views/profiles/accounts/_group_saml_unlink_buttons.html.haml
@@ -0,0 +1,7 @@
+- group_saml_identities.each do |identity|
+  - group = identity.saml_provider.group
+  .provider-btn-group
+    .provider-btn-image
+      = _("SAML for %{group_name}") % { group_name: group.name }
+    = link_to unlink_group_saml_providers_path(group), method: :delete, class: 'provider-btn' do
+      Disconnect
diff --git a/ee/changelogs/unreleased/jej-group-saml-unlink-from-account.yml b/ee/changelogs/unreleased/jej-group-saml-unlink-from-account.yml
new file mode 100644
index 00000000000000..92182b383958fa
--- /dev/null
+++ b/ee/changelogs/unreleased/jej-group-saml-unlink-from-account.yml
@@ -0,0 +1,5 @@
+---
+title: Users can unlink Group SAML from accounts page
+merge_request: 8682
+author:
+type: changed
diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb
index 2f53e33759256d..2288aee1bf39c6 100644
--- a/ee/config/routes/group.rb
+++ b/ee/config/routes/group.rb
@@ -77,6 +77,7 @@
     resource :saml_providers, path: 'saml', only: [:show, :create, :update] do
       post :callback, to: 'omniauth_callbacks#group_saml'
       get :sso, to: 'sso#saml'
+      delete :unlink, to: 'sso#unlink'
     end
 
     resource :roadmap, only: [:show], controller: 'roadmap'
diff --git a/ee/spec/features/profiles/account_spec.rb b/ee/spec/features/profiles/account_spec.rb
new file mode 100644
index 00000000000000..597a3df5b3c4cc
--- /dev/null
+++ b/ee/spec/features/profiles/account_spec.rb
@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe 'Profile > Account' do
+  let(:user) { create(:user) }
+
+  before do
+    sign_in(user)
+  end
+
+  describe "Disconnect Group SAML", :js do
+    let(:group) { create(:group, :private, name: 'Test Group') }
+    let(:saml_provider) { create(:saml_provider, group: group) }
+
+    def enable_group_saml
+      stub_licensed_features(group_saml: true)
+      allow(Devise).to receive(:omniauth_providers).and_return(%i(group_saml))
+    end
+
+    def create_linked_identity
+      oauth = { 'provider' => 'group_saml', 'uid' => '1' }
+      Gitlab::Auth::GroupSaml::IdentityLinker.new(user, oauth, saml_provider).link
+    end
+
+    before do
+      enable_group_saml
+      create_linked_identity
+    end
+
+    it 'unlinks account' do
+      visit profile_account_path
+
+      unlink_label = "SAML for Test Group"
+
+      expect(page).to have_content unlink_label
+      click_link "Disconnect"
+
+      expect(current_path).to eq profile_account_path
+      expect(page).not_to have_content(unlink_label)
+
+      visit group_path(group)
+      expect(page).to have_content('Page Not Found')
+    end
+  end
+end
diff --git a/ee/spec/finders/group_saml_identity_finder_spec.rb b/ee/spec/finders/group_saml_identity_finder_spec.rb
new file mode 100644
index 00000000000000..bdb116391669bf
--- /dev/null
+++ b/ee/spec/finders/group_saml_identity_finder_spec.rb
@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe GroupSamlIdentityFinder do
+  include Gitlab::Routing
+
+  let(:user) { create(:user) }
+  let!(:identity) { create(:identity, :group_saml, user: user) }
+  let(:group) { identity.saml_provider.group }
+
+  subject { described_class.new(user: user) }
+
+  describe "#find_linked" do
+    it "finds identity matching user and group" do
+      expect(subject.find_linked(group: group)).to eq(identity)
+    end
+
+    it "returns nil when no saml_provider exists" do
+      group.saml_provider.destroy!
+
+      expect(subject.find_linked(group: group)).to eq(nil)
+    end
+
+    it "returns nil when group is nil" do
+      expect(subject.find_linked(group: nil)).to eq(nil)
+    end
+  end
+
+  describe "#all" do
+    it "finds Group SAML identities for a user" do
+      expect(subject.all.first).to eq(identity)
+    end
+
+    it "avoids N+1 on access to provider and group path" do
+      identity = subject.all.first
+
+      expect { group_path(identity.saml_provider.group) }.not_to exceed_query_limit(0)
+    end
+  end
+end
diff --git a/ee/spec/services/group_saml/identity/destroy_service_spec.rb b/ee/spec/services/group_saml/identity/destroy_service_spec.rb
new file mode 100644
index 00000000000000..cd1e1ed54913aa
--- /dev/null
+++ b/ee/spec/services/group_saml/identity/destroy_service_spec.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe GroupSaml::Identity::DestroyService do
+  let(:identity) { create(:identity, :group_saml) }
+
+  subject { described_class.new(identity) }
+
+  before do
+    link_group_membership
+  end
+
+  def link_group_membership
+    Gitlab::Auth::GroupSaml::MembershipUpdater.new(identity.user, identity.saml_provider).execute
+  end
+
+  it "prevents future Group SAML logins" do
+    subject.execute
+
+    expect(identity).to be_destroyed
+  end
+
+  it "removes access to the group" do
+    expect do
+      subject.execute
+    end.to change(GroupMember, :count).by(-1)
+  end
+
+  it "doesn't remove the last group owner" do
+    identity.saml_provider.group.members.first.update!(access_level: Gitlab::Access::OWNER)
+
+    expect do
+      subject.execute
+    end.not_to change(GroupMember, :count)
+  end
+
+  it 'logs an audit event' do
+    expect do
+      subject.execute
+    end.to change { SecurityEvent.count }.by(1)
+  end
+end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 1ac5980efc1eb1..41567b8719fa75 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -7289,6 +7289,9 @@ msgstr ""
 msgid "SAML Single Sign On Settings"
 msgstr ""
 
+msgid "SAML for %{group_name}"
+msgstr ""
+
 msgid "SAST"
 msgstr ""
 
diff --git a/spec/factories/identities.rb b/spec/factories/identities.rb
index 8584c8bd737557..89d0853fd43e18 100644
--- a/spec/factories/identities.rb
+++ b/spec/factories/identities.rb
@@ -7,6 +7,7 @@
       provider 'group_saml'
       extern_uid { generate(:username) }
       saml_provider
+      user
     end
   end
 end
-- 
GitLab


From d76f637ef6eefced04f24c537df3f2effcabbb70 Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Tue, 4 Dec 2018 10:17:21 +0000
Subject: [PATCH 2/3] Replace @user with current_user on Account page

Previously we used @user, but this changed to local_assigns[:user]
so we could pass a second variable to the view from an EE module

Since we were already using current_user librally there it makes sense
to replace the remaining usages.

See: https://docs.gitlab.com/ee/development/module_with_instance_variables.html
---
 app/controllers/profiles/accounts_controller.rb | 2 +-
 app/views/profiles/accounts/show.html.haml      | 7 +++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/app/controllers/profiles/accounts_controller.rb b/app/controllers/profiles/accounts_controller.rb
index 9e47627ba8b19b..8d60799390382f 100644
--- a/app/controllers/profiles/accounts_controller.rb
+++ b/app/controllers/profiles/accounts_controller.rb
@@ -27,7 +27,7 @@ def unlink
   private
 
   def show_view_variables
-    { user: current_user }
+    {}
   end
 end
 
diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml
index fffdc305623bdf..e167e094240fbe 100644
--- a/app/views/profiles/accounts/show.html.haml
+++ b/app/views/profiles/accounts/show.html.haml
@@ -1,6 +1,5 @@
 - page_title "Account"
 - @content_class = "limit-container-width" unless fluid_layout
-- user = local_assigns.fetch(:user)
 
 - if current_user.ldap_user?
   .alert.alert-info
@@ -68,7 +67,7 @@
     %h4.prepend-top-0.danger-title
       = s_('Profiles|Delete account')
   .col-lg-8
-    - if user.can_be_removed? && can?(current_user, :destroy_user, user)
+    - if current_user.can_be_removed? && can?(current_user, :destroy_user, current_user)
       %p
         = s_('Profiles|Deleting an account has the following effects:')
       = render 'users/deletion_guidance', user: current_user
@@ -81,10 +80,10 @@
         confirm_with_password: ('true' if current_user.confirm_deletion_with_password?),
         username: current_user.username } }
     - else
-      - if user.solo_owned_groups.present?
+      - if current_user.solo_owned_groups.present?
         %p
           = s_('Profiles|Your account is currently an owner in these groups:')
-          %strong= user.solo_owned_groups.map(&:name).join(', ')
+          %strong= current_user.solo_owned_groups.map(&:name).join(', ')
         %p
           = s_('Profiles|You must transfer ownership or delete these groups before you can delete your account.')
       - else
-- 
GitLab


From be318259a4da7c44844ae6b53d4009d7545d2f70 Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Tue, 4 Dec 2018 11:32:28 +0000
Subject: [PATCH 3/3] Add :group_saml_identity factory to avoid CE trait

Changes to the :group_saml trait were previously breaking
the ee-specific-lines check.
---
 ee/spec/factories/group_saml_identities.rb               | 9 +++++++++
 ee/spec/finders/group_saml_identity_finder_spec.rb       | 2 +-
 ee/spec/models/ee/members_preloader_spec.rb              | 6 +++---
 ee/spec/models/ee/user_spec.rb                           | 4 ++--
 .../services/group_saml/identity/destroy_service_spec.rb | 2 +-
 spec/factories/identities.rb                             | 7 -------
 6 files changed, 16 insertions(+), 14 deletions(-)
 create mode 100644 ee/spec/factories/group_saml_identities.rb

diff --git a/ee/spec/factories/group_saml_identities.rb b/ee/spec/factories/group_saml_identities.rb
new file mode 100644
index 00000000000000..6d425da743c422
--- /dev/null
+++ b/ee/spec/factories/group_saml_identities.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+FactoryBot.define do
+  factory :group_saml_identity, class: Identity, parent: :identity do
+    provider 'group_saml'
+    extern_uid { generate(:username) }
+    saml_provider
+    user
+  end
+end
diff --git a/ee/spec/finders/group_saml_identity_finder_spec.rb b/ee/spec/finders/group_saml_identity_finder_spec.rb
index bdb116391669bf..db096089accf8f 100644
--- a/ee/spec/finders/group_saml_identity_finder_spec.rb
+++ b/ee/spec/finders/group_saml_identity_finder_spec.rb
@@ -6,7 +6,7 @@
   include Gitlab::Routing
 
   let(:user) { create(:user) }
-  let!(:identity) { create(:identity, :group_saml, user: user) }
+  let!(:identity) { create(:group_saml_identity, user: user) }
   let(:group) { identity.saml_provider.group }
 
   subject { described_class.new(user: user) }
diff --git a/ee/spec/models/ee/members_preloader_spec.rb b/ee/spec/models/ee/members_preloader_spec.rb
index fe01212cb021c8..7dec082e054283 100644
--- a/ee/spec/models/ee/members_preloader_spec.rb
+++ b/ee/spec/models/ee/members_preloader_spec.rb
@@ -14,12 +14,12 @@ def group_sso_with_preload(members)
 
     it 'preloads SAML identities to avoid N+1 queries in MembersPresenter' do
       member = create(:group_member, group: group)
-      create(:identity, :group_saml, user: member.user, saml_provider: saml_provider)
+      create(:group_saml_identity, user: member.user, saml_provider: saml_provider)
       control = ActiveRecord::QueryRecorder.new { group_sso_with_preload([member]) }
 
       members = create_list(:group_member, 3, group: group)
-      create(:identity, :group_saml, user: members.first.user, saml_provider: saml_provider)
-      create(:identity, :group_saml, user: members.last.user, saml_provider: saml_provider)
+      create(:group_saml_identity, user: members.first.user, saml_provider: saml_provider)
+      create(:group_saml_identity, user: members.last.user, saml_provider: saml_provider)
 
       expect { group_sso_with_preload(members) }.not_to exceed_query_limit(control)
     end
diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb
index 86865058684cb7..aba3df29912754 100644
--- a/ee/spec/models/ee/user_spec.rb
+++ b/ee/spec/models/ee/user_spec.rb
@@ -257,7 +257,7 @@
     end
 
     context 'with linked identity' do
-      let!(:identity) { create(:identity, :group_saml, user: user) }
+      let!(:identity) { create(:group_saml_identity, user: user) }
       let(:saml_provider) { identity.saml_provider }
       let(:group) { saml_provider.group }
 
@@ -267,7 +267,7 @@
         end
 
         it 'does not cause ActiveRecord to loop through identites' do
-          create(:identity, :group_saml, user: user)
+          create(:group_saml_identity, user: user)
 
           expect(Identity).not_to receive(:instantiate)
 
diff --git a/ee/spec/services/group_saml/identity/destroy_service_spec.rb b/ee/spec/services/group_saml/identity/destroy_service_spec.rb
index cd1e1ed54913aa..48ebb065b47168 100644
--- a/ee/spec/services/group_saml/identity/destroy_service_spec.rb
+++ b/ee/spec/services/group_saml/identity/destroy_service_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe GroupSaml::Identity::DestroyService do
-  let(:identity) { create(:identity, :group_saml) }
+  let(:identity) { create(:group_saml_identity) }
 
   subject { described_class.new(identity) }
 
diff --git a/spec/factories/identities.rb b/spec/factories/identities.rb
index 89d0853fd43e18..122d0d4293801e 100644
--- a/spec/factories/identities.rb
+++ b/spec/factories/identities.rb
@@ -2,12 +2,5 @@
   factory :identity do
     provider 'ldapmain'
     extern_uid 'my-ldap-id'
-
-    trait :group_saml do
-      provider 'group_saml'
-      extern_uid { generate(:username) }
-      saml_provider
-      user
-    end
   end
 end
-- 
GitLab