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

@@ -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;
   }
 }
 

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 @@ class Profiles::AccountsController < Profiles::ApplicationController
     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)

app/helpers/auth_helper.rb

@@ -57,6 +57,10 @@ module AuthHelper
     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

app/views/profiles/accounts/show.html.haml

 - 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

ee/app/controllers/ee/profiles/accounts_controller.rb

+# 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

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 @@ class Groups::SsoController < Groups::ApplicationController
     @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

ee/app/finders/group_saml_identity_finder.rb

+# 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

ee/app/helpers/ee/auth_helper.rb

@@ -8,6 +8,11 @@ module EE
 
     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 @@ module EE
       ::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

ee/app/models/ee/identity.rb

@@ -18,5 +18,11 @@ module EE
         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

ee/app/services/group_saml/identity/destroy_service.rb

+# 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

ee/app/views/profiles/accounts/_group_saml_unlink_buttons.html.haml

+- 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

ee/changelogs/unreleased/jej-group-saml-unlink-from-account.yml

+---
+title: Users can unlink Group SAML from accounts page
+merge_request: 8682
+author:
+type: changed

ee/config/routes/group.rb

@@ -77,6 +77,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
     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'

ee/spec/features/profiles/account_spec.rb

+# 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

ee/spec/finders/group_saml_identity_finder_spec.rb

+# 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

ee/spec/services/group_saml/identity/destroy_service_spec.rb

+# 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

locale/gitlab.pot

@@ -7289,6 +7289,9 @@ msgstr ""
 msgid "SAML Single Sign On Settings"
 msgstr ""
 
+msgid "SAML for %{group_name}"
+msgstr ""
+
 msgid "SAST"
 msgstr ""
 

spec/factories/identities.rb

@@ -7,6 +7,7 @@ FactoryBot.define do
       provider 'group_saml'
       extern_uid { generate(:username) }
       saml_provider
+      user
     end
   end
 end