Merge branch 'create-identity-provider-policy' into 'master'

Move out link\unlink ability checks to a policy

See merge request gitlab-org/gitlab-ce!26278

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?

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"

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

app/policies/identity_provider_policy.rb

+# 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

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

+%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

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

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') }
 

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

spec/policies/identity_provider_policy_spec.rb

+# 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