From b9adf92f3c96d5f24fa413944dff3b932740a613 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Tue, 28 Mar 2017 11:33:51 +0100 Subject: Prevent users from disconnecting gitlab account from CAS --- app/controllers/profiles/accounts_controller.rb | 13 +++++- app/helpers/auth_helper.rb | 4 ++ app/views/profiles/accounts/show.html.haml | 8 ++-- ...-from-disconnecting-gitlab-account-from-cas.yml | 4 ++ .../profiles/accounts_controller_spec.rb | 52 +++++++++++++++------- spec/helpers/auth_helper_spec.rb | 14 ++++++ 6 files changed, 75 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/25556-prevent-users-from-disconnecting-gitlab-account-from-cas.yml 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 -- cgit v1.2.1