Commit 8ee1927d authored by Pavel Shutsin's avatar Pavel Shutsin

Move out link\unlink ability checks to a policy

We can extend the policy in EE for additional behavior
parent a4b18040
......@@ -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?
......
......@@ -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"
......
......@@ -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
......
# 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
%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
......@@ -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
......
......@@ -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') }
......
......@@ -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
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
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
[: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
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
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment