Prevent users from disconnecting gitlab account from CAS

6 files changed, 75 insertions, 20 deletions

diff --git a/app/controllers/profiles/accounts_controller.rb b/app/controllers/profiles/accounts_controller.rb
index 69959fe3687..7d1aa8d1ce0 100644
--- a/app/controllers/profiles/accounts_controller.rb
+++ b/app/controllers/profiles/accounts_controller.rb
@@ -1,11 +1,22 @@
 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
diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb
index 1ee6c1d3afa..101fe579da2 100644
--- a/app/helpers/auth_helper.rb
+++ b/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
diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml
index 8a994f6d600..5ce2220c907 100644
--- a/app/views/profiles/accounts/show.html.haml
+++ b/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
diff --git a/changelogs/unreleased/25556-prevent-users-from-disconnecting-gitlab-account-from-cas.yml b/changelogs/unreleased/25556-prevent-users-from-disconnecting-gitlab-account-from-cas.yml
new file mode 100644
index 00000000000..17e38ba6243
--- /dev/null
+++ b/changelogs/unreleased/25556-prevent-users-from-disconnecting-gitlab-account-from-cas.yml
@@ -0,0 +1,4 @@
+---
+title: Prevent users from disconnecting GitLab account from CAS
+merge_request: 10282
+author:
diff --git a/spec/controllers/profiles/accounts_controller_spec.rb b/spec/controllers/profiles/accounts_controller_spec.rb
index 18148acde3e..2f9d18e3a0e 100644
--- a/spec/controllers/profiles/accounts_controller_spec.rb
+++ b/spec/controllers/profiles/accounts_controller_spec.rb
@@ -1,25 +1,47 @@
 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
+    before do
+      sign_in(user)
+    end
 
-  it 'does not allow to unlink SAML connected account' do
-    identity = user.identities.last
-    delete :unlink, provider: 'saml'
-    updated_user = User.find(user.id)
+    it 'renders 404 if someone tries to unlink a non existent provider' do
+      delete :unlink, provider: 'github'
 
-    expect(response).to have_http_status(302)
-    expect(updated_user.identities.size).to eq(1)
-    expect(updated_user.identities).to include(identity)
-  end
+      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: provider.to_s
+
+          expect(response).to have_http_status(302)
+          expect(user.reload.identities).to include(identity)
+        end
+      end
+    end
+
+    [: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) }
+
+        it 'allows to unlink connected account' do
+          identity = user.identities.last
 
-  it 'does allow to delete other linked accounts' do
-    user.identities.create(provider: 'twitter', extern_uid: 'twitter_123')
+          delete :unlink, provider: provider.to_s
 
-    expect { delete :unlink, provider: 'twitter' }.to change(Identity.all, :size).by(-1)
+          expect(response).to have_http_status(302)
+          expect(user.reload.identities).not_to include(identity)
+        end
+      end
+    end
   end
 end
diff --git a/spec/helpers/auth_helper_spec.rb b/spec/helpers/auth_helper_spec.rb
index cd3281d6f51..a0e1265efff 100644
--- a/spec/helpers/auth_helper_spec.rb
+++ b/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