Merge branch '25556-prevent-users-from-disconnecting-gitlab-account-from-cas' into 'master'

Prevent users from disconnecting gitlab account from CAS

Closes #25556

See merge request !10282

app/controllers/profiles/accounts_controller.rb

 class Profiles::AccountsController < Profiles::ApplicationController
+  include AuthHelper
+
   def show
     @user = current_user
   end
 
   def unlink
     provider = params[:provider]
-    current_user.identities.find_by(provider: provider).destroy unless provider.to_s == 'saml'
+    identity = current_user.identities.find_by(provider: provider)
+
+    return render_404 unless identity
+
+    if unlink_allowed?(provider)
+      identity.destroy
+    else
+      flash[:alert] = "You are not allowed to unlink your primary login account"
+    end
+
     redirect_to profile_account_path
   end
 end

app/helpers/auth_helper.rb

@@ -76,5 +76,9 @@ module AuthHelper
       (current_user.otp_grace_period_started_at + current_application_settings.two_factor_grace_period.hours) < Time.current
   end
 
+  def unlink_allowed?(provider)
+    %w(saml cas3).exclude?(provider.to_s)
+  end
+
   extend self
 end

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

@@ -75,12 +75,12 @@
           .provider-btn-image
             = provider_image_tag(provider)
           - if auth_active?(provider)
-            - if provider.to_s == 'saml'
-              %a.provider-btn
-                Active
-            - else
+            - if unlink_allowed?(provider)
               = link_to unlink_profile_account_path(provider: provider), method: :delete, class: 'provider-btn' do
                 Disconnect
+            - else
+              %a.provider-btn
+                Active
           - else
             = link_to omniauth_authorize_path(:user, provider), method: :post, class: 'provider-btn not-active' do
               Connect

changelogs/unreleased/25556-prevent-users-from-disconnecting-gitlab-account-from-cas.yml

+---
+title: Prevent users from disconnecting GitLab account from CAS
+merge_request: 10282
+author:

spec/controllers/profiles/accounts_controller_spec.rb

 require 'spec_helper'
 
 describe Profiles::AccountsController do
-  let(:user) { create(:omniauth_user, provider: 'saml') }
+  describe 'DELETE unlink' do
+    let(:user) { create(:omniauth_user) }
 
     before do
       sign_in(user)
     end
 
-  it 'does not allow to unlink SAML connected account' do
+    it 'renders 404 if someone tries to unlink a non existent provider' do
+      delete :unlink, provider: 'github'
+
+      expect(response).to have_http_status(404)
+    end
+
+    [:saml, :cas3].each do |provider|
+      describe "#{provider} provider" do
+        let(:user) { create(:omniauth_user, provider: provider.to_s) }
+
+        it "does not allow to unlink connected account" do
           identity = user.identities.last
-    delete :unlink, provider: 'saml'
-    updated_user = User.find(user.id)
+
+          delete :unlink, provider: provider.to_s
 
           expect(response).to have_http_status(302)
-    expect(updated_user.identities.size).to eq(1)
-    expect(updated_user.identities).to include(identity)
+          expect(user.reload.identities).to include(identity)
+        end
+      end
     end
 
-  it 'does allow to delete other linked accounts' do
-    user.identities.create(provider: 'twitter', extern_uid: 'twitter_123')
+    [:twitter, :facebook, :google_oauth2, :gitlab, :github, :bitbucket, :crowd, :auth0].each do |provider|
+      describe "#{provider} provider" do
+        let(:user) { create(:omniauth_user, provider: provider.to_s) }
 
-    expect { delete :unlink, provider: 'twitter' }.to change(Identity.all, :size).by(-1)
+        it 'allows to unlink connected account' do
+          identity = user.identities.last
+
+          delete :unlink, provider: provider.to_s
+
+          expect(response).to have_http_status(302)
+          expect(user.reload.identities).not_to include(identity)
+        end
+      end
+    end
   end
 end

spec/helpers/auth_helper_spec.rb

@@ -62,4 +62,18 @@ describe AuthHelper do
       end
     end
   end
+
+  describe 'unlink_allowed?' do
+    [:saml, :cas3].each do |provider|
+      it "returns true if the provider is #{provider}" do
+        expect(helper.unlink_allowed?(provider)).to be false
+      end
+    end
+
+    [:twitter, :facebook, :google_oauth2, :gitlab, :github, :bitbucket, :crowd, :auth0].each do |provider|
+      it "returns false if the provider is #{provider}" do
+        expect(helper.unlink_allowed?(provider)).to be true
+      end
+    end
+  end
 end