summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/controllers/omniauth_callbacks_controller.rb4
-rw-r--r--app/controllers/profiles/accounts_controller.rb2
-rw-r--r--app/helpers/auth_helper.rb8
-rw-r--r--app/policies/identity_provider_policy.rb15
-rw-r--r--app/views/profiles/accounts/_providers.html.haml21
-rw-r--r--app/views/profiles/accounts/show.html.haml19
-rw-r--r--spec/controllers/omniauth_callbacks_controller_spec.rb27
-rw-r--r--spec/helpers/auth_helper_spec.rb38
-rw-r--r--spec/policies/identity_provider_policy_spec.rb30
9 files changed, 133 insertions, 31 deletions
diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index cc2bb99f55b..e90e8278c13 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -3,6 +3,7 @@
class OmniauthCallbacksController < Devise::OmniauthCallbacksController
include AuthenticatesWithTwoFactor
include Devise::Controllers::Rememberable
+ include AuthHelper
protect_from_forgery except: [:kerberos, :saml, :cas3, :failure], with: :exception, prepend: true
@@ -80,10 +81,11 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
end
if current_user
+ return render_403 unless link_provider_allowed?(oauth['provider'])
+
log_audit_event(current_user, with: oauth['provider'])
identity_linker ||= auth_module::IdentityLinker.new(current_user, oauth)
-
identity_linker.link
if identity_linker.changed?
diff --git a/app/controllers/profiles/accounts_controller.rb b/app/controllers/profiles/accounts_controller.rb
index b0d65f284af..0d2a6145d0e 100644
--- a/app/controllers/profiles/accounts_controller.rb
+++ b/app/controllers/profiles/accounts_controller.rb
@@ -14,7 +14,7 @@ class Profiles::AccountsController < Profiles::ApplicationController
return render_404 unless identity
- if unlink_allowed?(provider)
+ if unlink_provider_allowed?(provider)
identity.destroy
else
flash[:alert] = "You are not allowed to unlink your primary login account"
diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb
index 2b1d6f49878..b4ee648361c 100644
--- a/app/helpers/auth_helper.rb
+++ b/app/helpers/auth_helper.rb
@@ -100,8 +100,12 @@ module AuthHelper
end
# rubocop: enable CodeReuse/ActiveRecord
- def unlink_allowed?(provider)
- %w(saml cas3).exclude?(provider.to_s)
+ def unlink_provider_allowed?(provider)
+ IdentityProviderPolicy.new(current_user, provider).can?(:unlink)
+ end
+
+ def link_provider_allowed?(provider)
+ IdentityProviderPolicy.new(current_user, provider).can?(:link)
end
extend self
diff --git a/app/policies/identity_provider_policy.rb b/app/policies/identity_provider_policy.rb
new file mode 100644
index 00000000000..d34cdd5bdd4
--- /dev/null
+++ b/app/policies/identity_provider_policy.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class IdentityProviderPolicy < BasePolicy
+ desc "Provider is SAML or CAS3"
+ condition(:protected_provider, scope: :subject, score: 0) { %w(saml cas3).include?(@subject.to_s) }
+
+ rule { anonymous }.prevent_all
+
+ rule { default }.policy do
+ enable :unlink
+ enable :link
+ end
+
+ rule { protected_provider }.prevent(:unlink)
+end
diff --git a/app/views/profiles/accounts/_providers.html.haml b/app/views/profiles/accounts/_providers.html.haml
new file mode 100644
index 00000000000..068f9cc70f7
--- /dev/null
+++ b/app/views/profiles/accounts/_providers.html.haml
@@ -0,0 +1,21 @@
+%label.label-bold
+ = s_('Profiles|Connected Accounts')
+ %p= s_('Profiles|Click on icon to activate signin with one of the following services')
+ - providers.each do |provider|
+ - unlink_allowed = unlink_provider_allowed?(provider)
+ - link_allowed = link_provider_allowed?(provider)
+ - if unlink_allowed || link_allowed
+ .provider-btn-group
+ .provider-btn-image
+ = provider_image_tag(provider)
+ - if auth_active?(provider)
+ - if unlink_allowed
+ = link_to unlink_profile_account_path(provider: provider), method: :delete, class: 'provider-btn' do
+ = s_('Profiles|Disconnect')
+ - else
+ %a.provider-btn
+ = s_('Profiles|Active')
+ - elsif link_allowed
+ = link_to omniauth_authorize_path(:user, provider), method: :post, class: 'provider-btn not-active' do
+ = s_('Profiles|Connect')
+ = render_if_exists 'profiles/accounts/group_saml_unlink_buttons', group_saml_identities: group_saml_identities
diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml
index ee2c5a13b8a..e6380817c8f 100644
--- a/app/views/profiles/accounts/show.html.haml
+++ b/app/views/profiles/accounts/show.html.haml
@@ -29,24 +29,7 @@
%p
= s_('Profiles|Activate signin with one of the following services')
.col-lg-8
- %label.label-bold
- = s_('Profiles|Connected Accounts')
- %p= s_('Profiles|Click on icon to activate signin with one of the following services')
- - button_based_providers.each do |provider|
- .provider-btn-group
- .provider-btn-image
- = provider_image_tag(provider)
- - if auth_active?(provider)
- - if unlink_allowed?(provider)
- = link_to unlink_profile_account_path(provider: provider), method: :delete, class: 'provider-btn' do
- = s_('Profiles|Disconnect')
- - else
- %a.provider-btn
- = s_('Profiles|Active')
- - else
- = link_to omniauth_authorize_path(:user, provider), method: :post, class: 'provider-btn not-active' do
- = s_('Profiles|Connect')
- = render_if_exists 'profiles/accounts/group_saml_unlink_buttons', group_saml_identities: local_assigns[:group_saml_identities]
+ = render 'providers', providers: button_based_providers, group_saml_identities: local_assigns[:group_saml_identities]
%hr
- if current_user.can_change_username?
.row.prepend-top-default
diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb
index e0da23ca0b8..06c6f49f7cc 100644
--- a/spec/controllers/omniauth_callbacks_controller_spec.rb
+++ b/spec/controllers/omniauth_callbacks_controller_spec.rb
@@ -113,6 +113,33 @@ describe OmniauthCallbacksController, type: :controller do
expect(request.env['warden']).to be_authenticated
end
+ context 'when user has no linked provider' do
+ let(:user) { create(:user) }
+
+ before do
+ sign_in user
+ end
+
+ it 'links identity' do
+ expect do
+ post provider
+ user.reload
+ end.to change { user.identities.count }.by(1)
+ end
+
+ context 'and is not allowed to link the provider' do
+ before do
+ allow_any_instance_of(IdentityProviderPolicy).to receive(:can?).with(:link).and_return(false)
+ end
+
+ it 'returns 403' do
+ post provider
+
+ expect(response).to have_gitlab_http_status(403)
+ end
+ end
+ end
+
shared_context 'sign_up' do
let(:user) { double(email: 'new@example.com') }
diff --git a/spec/helpers/auth_helper_spec.rb b/spec/helpers/auth_helper_spec.rb
index f0c2e4768ec..2ba8b3dbf22 100644
--- a/spec/helpers/auth_helper_spec.rb
+++ b/spec/helpers/auth_helper_spec.rb
@@ -97,17 +97,37 @@ describe AuthHelper do
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
+ describe '#link_provider_allowed?' do
+ let(:policy) { instance_double('IdentityProviderPolicy') }
+ let(:current_user) { instance_double('User') }
+ let(:provider) { double }
+
+ before do
+ allow(helper).to receive(:current_user).and_return(current_user)
+ allow(IdentityProviderPolicy).to receive(:new).with(current_user, provider).and_return(policy)
end
- [:twitter, :facebook, :google_oauth2, :gitlab, :github, :bitbucket, :crowd, :auth0, :authentiq].each do |provider|
- it "returns false if the provider is #{provider}" do
- expect(helper.unlink_allowed?(provider)).to be true
- end
+ it 'delegates to identity provider policy' do
+ allow(policy).to receive(:can?).with(:link).and_return('policy_link_result')
+
+ expect(helper.link_provider_allowed?(provider)).to eq 'policy_link_result'
+ end
+ end
+
+ describe '#unlink_provider_allowed?' do
+ let(:policy) { instance_double('IdentityProviderPolicy') }
+ let(:current_user) { instance_double('User') }
+ let(:provider) { double }
+
+ before do
+ allow(helper).to receive(:current_user).and_return(current_user)
+ allow(IdentityProviderPolicy).to receive(:new).with(current_user, provider).and_return(policy)
+ end
+
+ it 'delegates to identity provider policy' do
+ allow(policy).to receive(:can?).with(:unlink).and_return('policy_unlink_result')
+
+ expect(helper.unlink_provider_allowed?(provider)).to eq 'policy_unlink_result'
end
end
end
diff --git a/spec/policies/identity_provider_policy_spec.rb b/spec/policies/identity_provider_policy_spec.rb
new file mode 100644
index 00000000000..2520469d4e7
--- /dev/null
+++ b/spec/policies/identity_provider_policy_spec.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe IdentityProviderPolicy do
+ subject(:policy) { described_class.new(user, provider) }
+ let(:user) { User.new }
+ let(:provider) { :a_provider }
+
+ describe '#rules' do
+ it { is_expected.to be_allowed(:link) }
+ it { is_expected.to be_allowed(:unlink) }
+
+ context 'when user is anonymous' do
+ let(:user) { nil }
+
+ it { is_expected.not_to be_allowed(:link) }
+ it { is_expected.not_to be_allowed(:unlink) }
+ end
+
+ %w[saml cas3].each do |provider_name|
+ context "when provider is #{provider_name}" do
+ let(:provider) { provider_name }
+
+ it { is_expected.to be_allowed(:link) }
+ it { is_expected.not_to be_allowed(:unlink) }
+ end
+ end
+ end
+end