Merge branch 'jej/group-saml-unlink-from-account' into 'master'

Users can unlink Group SAML from accounts page

Closes #5016

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

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

@@ -21,7 +21,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 +46,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 +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
@@ -79,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

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/factories/group_saml_identities.rb

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

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(:group_saml_identity, 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/models/ee/members_preloader_spec.rb

@@ -14,12 +14,12 @@ describe EE::MembersPreloader do
 
     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

ee/spec/models/ee/user_spec.rb

@@ -257,7 +257,7 @@ describe EE::User do
     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 @@ describe EE::User do
         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)
 

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(:group_saml_identity) }
+
+  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

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

spec/factories/identities.rb

@@ -2,11 +2,5 @@ FactoryBot.define do
   factory :identity do
     provider 'ldapmain'
     extern_uid 'my-ldap-id'
-
-    trait :group_saml do
-      provider 'group_saml'
-      extern_uid { generate(:username) }
-      saml_provider
-    end
   end
 end