Merge branch 'saml-external-groups' into 'master'

Allow SAML to identify external users and set them as such

Related to #4009

Fixes #14577

This allows SAML to retrieve group information form the `SAML Response`
and match that to a setting that will flag all matching users as external.

See merge request !3530

8 files changed, 207 insertions, 102 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 92a0d8547e4..80fc2302b32 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -10,6 +10,7 @@ v 8.7.0 (unreleased)
   - Allow back dating on issues when created through the API
   - Fix Error 500 after renaming a project path (Stan Hu)
   - Fix avatar stretching by providing a cropping feature
+  - Allow SAML to handle external users based on user's information !3530
   - Add endpoints to archive or unarchive a project !3372
   - Add links to CI setup documentation from project settings and builds pages
   - Handle nil descriptions in Slack issue messages (Stan Hu)
diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index 21135f7d607..d28e96c3f18 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -55,7 +55,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
       end
     else
       saml_user = Gitlab::Saml::User.new(oauth)
-      saml_user.save
+      saml_user.save if saml_user.changed?
       @user = saml_user.gl_user
 
       continue_login_process
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index fb1c3476f65..ddee467b14b 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -345,6 +345,8 @@ production: &base
       #
       # - { name: 'saml',
       #     label: 'Our SAML Provider',
+      #     groups_attribute: 'Groups',
+      #     external_groups: ['Contractors', 'Freelancers'],
       #     args: {
       #             assertion_consumer_service_url: 'https://gitlab.example.com/users/auth/saml/callback',
       #             idp_cert_fingerprint: '43:51:43:a1:b5:fc:8b:b7:0a:3a:a9:b1:0f:66:73:a8',
@@ -352,6 +354,7 @@ production: &base
       #             issuer: 'https://gitlab.example.com',
       #             name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient'
       #           } }
+      #
       # - { name: 'crowd',
       #     args: {
       #       crowd_server_url: 'CROWD SERVER URL',
diff --git a/doc/integration/saml.md b/doc/integration/saml.md
index 1c3dc707f6d..8a7205caaa4 100644
--- a/doc/integration/saml.md
+++ b/doc/integration/saml.md
@@ -131,8 +131,75 @@ On the sign in page there should now be a SAML button below the regular sign in
 Click the icon to begin the authentication process. If everything goes well the user
 will be returned to GitLab and will be signed in.
 
+## External Groups
+
+>**Note:**
+This setting is only available on GitLab 8.7 and above.
+
+SAML login includes support for external groups. You can define in the SAML
+settings which groups, to which your users belong in your IdP, you wish to be
+marked as [external](../permissions/permissions.md).
+
+### Requirements
+
+First you need to tell GitLab where to look for group information. For this you
+need to make sure that your IdP server sends a specific `AttributeStament` along
+with the regular SAML response. Here is an example:
+
+```xml
+<saml:AttributeStatement>
+  <saml:Attribute Name="Groups">
+    <saml:AttributeValue xsi:type="xs:string">SecurityGroup</saml:AttributeValue>
+    <saml:AttributeValue xsi:type="xs:string">Developers</saml:AttributeValue>
+    <saml:AttributeValue xsi:type="xs:string">Designers</saml:AttributeValue>
+  </saml:Attribute>
+</saml:AttributeStatement>
+```
+
+The name of the attribute can be anything you like, but it must contain the groups
+to which a user belongs. In order to tell GitLab where to find these groups, you need
+to add a `groups_attribute:` element to your SAML settings. You will also need to
+tell GitLab which groups are external via the `external_groups:` element:
+
+```yaml
+{ name: 'saml',
+  label: 'Our SAML Provider',
+  groups_attribute: 'Groups',
+  external_groups: ['Freelancers', 'Interns'],
+  args: {
+          assertion_consumer_service_url: 'https://gitlab.example.com/users/auth/saml/callback',
+          idp_cert_fingerprint: '43:51:43:a1:b5:fc:8b:b7:0a:3a:a9:b1:0f:66:73:a8',
+          idp_sso_target_url: 'https://login.example.com/idp',
+          issuer: 'https://gitlab.example.com',
+          name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient'
+        } }
+```
+
 ## Customization
 
+### `auto_sign_in_with_provider`
+
+You can add this setting to your GitLab configuration to automatically redirect you
+to your SAML server for authentication, thus removing the need to click a button
+before actually signing in.
+
+For omnibus package:
+
+```ruby
+gitlab_rails['omniauth_auto_sign_in_with_provider'] = 'saml'
+```
+
+For installations from source:
+
+```yaml
+omniauth:
+  auto_sign_in_with_provider: saml
+```
+
+Please keep in mind that every sign in attempt will be redirected to the SAML server,
+so you will not be able to sign in using local credentials. Make sure that at least one
+of the SAML users has admin permissions.
+
 ### `attribute_statements`
 
 >**Note:**
@@ -205,6 +272,10 @@ To bypass this you can add `skip_before_action :verify_authenticity_token` to th
 where it can then be seen in the usual logs, or as a flash message in the login
 screen.
 
+That file is located at `/opt/gitlab/embedded/service/gitlab-rails/app/controllers`
+for Omnibus installations and by default on `/home/git/gitlab/app/controllers` for
+installations from source.
+
 ### Invalid audience
 
 This error means that the IdP doesn't recognize GitLab as a valid sender and
diff --git a/lib/gitlab/saml/auth_hash.rb b/lib/gitlab/saml/auth_hash.rb
new file mode 100644
index 00000000000..32c1c9ec5bb
--- /dev/null
+++ b/lib/gitlab/saml/auth_hash.rb
@@ -0,0 +1,19 @@
+module Gitlab
+  module Saml
+    class AuthHash < Gitlab::OAuth::AuthHash
+
+      def groups
+        get_raw(Gitlab::Saml::Config.groups)
+      end
+
+      private
+
+      def get_raw(key)
+        # Needs to call `all` because of https://git.io/vVo4u
+        # otherwise just the first value is returned
+        auth_hash.extra[:raw_info].all[key]
+      end
+
+    end
+  end
+end
diff --git a/lib/gitlab/saml/config.rb b/lib/gitlab/saml/config.rb
new file mode 100644
index 00000000000..0f40c00f547
--- /dev/null
+++ b/lib/gitlab/saml/config.rb
@@ -0,0 +1,21 @@
+module Gitlab
+  module Saml
+    class Config
+
+      class << self
+        def options
+          Gitlab.config.omniauth.providers.find { |provider| provider.name == 'saml' }
+        end
+
+        def groups
+          options[:groups_attribute]
+        end
+
+        def external_groups
+          options[:external_groups]
+        end
+      end
+
+    end
+  end
+end
diff --git a/lib/gitlab/saml/user.rb b/lib/gitlab/saml/user.rb
index b1e30110ef5..c1072452abe 100644
--- a/lib/gitlab/saml/user.rb
+++ b/lib/gitlab/saml/user.rb
@@ -18,7 +18,7 @@ module Gitlab
           @user ||= find_or_create_ldap_user
         end
 
-        if auto_link_saml_enabled?
+        if auto_link_saml_user?
           @user ||= find_by_email
         end
 
@@ -26,6 +26,16 @@ module Gitlab
           @user ||= build_new_user
         end
 
+        if external_users_enabled?
+          # Check if there is overlap between the user's groups and the external groups
+          # setting then set user as external or internal.
+          if (auth_hash.groups & Gitlab::Saml::Config.external_groups).empty?
+            @user.external = false
+          else
+            @user.external = true
+          end
+        end
+
         @user
       end
 
@@ -37,11 +47,23 @@ module Gitlab
         end
       end
 
+      def changed?
+        gl_user.changed? || gl_user.identities.any?(&:changed?)
+      end
+
       protected
 
-      def auto_link_saml_enabled?
+      def auto_link_saml_user?
         Gitlab.config.omniauth.auto_link_saml_user
       end
+
+      def external_users_enabled?
+        !Gitlab::Saml::Config.external_groups.nil?
+      end
+
+      def auth_hash=(auth_hash)
+        @auth_hash = Gitlab::Saml::AuthHash.new(auth_hash)
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/saml/user_spec.rb b/spec/lib/gitlab/saml/user_spec.rb
index de7cd99d49d..c2a51d9249c 100644
--- a/spec/lib/gitlab/saml/user_spec.rb
+++ b/spec/lib/gitlab/saml/user_spec.rb
@@ -5,7 +5,7 @@ describe Gitlab::Saml::User, lib: true do
   let(:gl_user) { saml_user.gl_user }
   let(:uid) { 'my-uid' }
   let(:provider) { 'saml' }
-  let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid, provider: provider, info: info_hash) }
+  let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid, provider: provider, info: info_hash, extra: { raw_info: OneLogin::RubySaml::Attributes.new({ 'groups' => %w(Developers Freelancers Designers) }) }) }
   let(:info_hash) do
     {
       name: 'John',
@@ -23,10 +23,20 @@ describe Gitlab::Saml::User, lib: true do
       allow(Gitlab::LDAP::Config).to receive_messages(messages)
     end
 
+    def stub_basic_saml_config
+      allow(Gitlab::Saml::Config).to receive_messages({ options: { name: 'saml', args: {} } })
+    end
+
+    def stub_saml_group_config(groups)
+      allow(Gitlab::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', external_groups: groups, args: {} } })
+    end
+
+    before { stub_basic_saml_config }
+
     describe 'account exists on server' do
       before { stub_omniauth_config({ allow_single_sign_on: ['saml'], auto_link_saml_user: true }) }
+      let!(:existing_user) { create(:user, email: 'john@mail.com', username: 'john') }
       context 'and should bind with SAML' do
-        let!(:existing_user) { create(:user, email: 'john@mail.com', username: 'john') }
         it 'adds the SAML identity to the existing user' do
           saml_user.save
           expect(gl_user).to be_valid
@@ -36,6 +46,35 @@ describe Gitlab::Saml::User, lib: true do
           expect(identity.provider).to eql 'saml'
         end
       end
+
+      context 'external groups' do
+        context 'are defined' do
+          it 'marks the user as external' do
+            stub_saml_group_config(%w(Freelancers))
+            saml_user.save
+            expect(gl_user).to be_valid
+            expect(gl_user.external).to be_truthy
+          end
+        end
+
+        before { stub_saml_group_config(%w(Interns)) }
+        context 'are defined but the user does not belong there' do
+          it 'does not mark the user as external' do
+            saml_user.save
+            expect(gl_user).to be_valid
+            expect(gl_user.external).to be_falsey
+          end
+        end
+
+        context 'user was external, now should not be' do
+          it 'should make user internal' do
+            existing_user.update_attribute('external', true)
+            saml_user.save
+            expect(gl_user).to be_valid
+            expect(gl_user.external).to be_falsey
+          end
+        end
+      end
     end
 
     describe 'no account exists on server' do
@@ -68,6 +107,26 @@ describe Gitlab::Saml::User, lib: true do
         end
       end
 
+      context 'external groups' do
+        context 'are defined' do
+          it 'marks the user as external' do
+            stub_saml_group_config(%w(Freelancers))
+            saml_user.save
+            expect(gl_user).to be_valid
+            expect(gl_user.external).to be_truthy
+          end
+        end
+
+        context 'are defined but the user does not belong there' do
+          it 'does not mark the user as external' do
+            stub_saml_group_config(%w(Interns))
+            saml_user.save
+            expect(gl_user).to be_valid
+            expect(gl_user.external).to be_falsey
+          end
+        end
+      end
+
       context 'with auto_link_ldap_user disabled (default)' do
         before { stub_omniauth_config({ auto_link_ldap_user: false, auto_link_saml_user: false, allow_single_sign_on: ['saml'] }) }
         include_examples 'to verify compliance with allow_single_sign_on'
@@ -76,12 +135,6 @@ describe Gitlab::Saml::User, lib: true do
       context 'with auto_link_ldap_user enabled' do
         before { stub_omniauth_config({ auto_link_ldap_user: true, auto_link_saml_user: false }) }
 
-        context 'and no LDAP provider defined' do
-          before { stub_ldap_config(providers: []) }
-
-          include_examples 'to verify compliance with allow_single_sign_on'
-        end
-
         context 'and at least one LDAP provider is defined' do
           before { stub_ldap_config(providers: %w(ldapmain)) }
 
@@ -89,19 +142,18 @@ describe Gitlab::Saml::User, lib: true do
             before do
               allow(ldap_user).to receive(:uid) { uid }
               allow(ldap_user).to receive(:username) { uid }
-              allow(ldap_user).to receive(:email) { ['johndoe@example.com','john2@example.com'] }
+              allow(ldap_user).to receive(:email) { %w(john@mail.com john2@example.com) }
               allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
               allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
             end
 
             context 'and no account for the LDAP user' do
-
               it 'creates a user with dual LDAP and SAML identities' do
                 saml_user.save
 
                 expect(gl_user).to be_valid
                 expect(gl_user.username).to eql uid
-                expect(gl_user.email).to eql 'johndoe@example.com'
+                expect(gl_user.email).to eql 'john@mail.com'
                 expect(gl_user.identities.length).to eql 2
                 identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
                 expect(identities_as_hash).to match_array([ { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
@@ -111,13 +163,13 @@ describe Gitlab::Saml::User, lib: true do
             end
 
             context 'and LDAP user has an account already' do
-              let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'ldapmain', username: 'john') }
-              it "adds the omniauth identity to the LDAP account" do
+              let!(:existing_user) { create(:omniauth_user, email: 'john@mail.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'ldapmain', username: 'john') }
+              it 'adds the omniauth identity to the LDAP account' do
                 saml_user.save
 
                 expect(gl_user).to be_valid
                 expect(gl_user.username).to eql 'john'
-                expect(gl_user.email).to eql 'john@example.com'
+                expect(gl_user.email).to eql 'john@mail.com'
                 expect(gl_user.identities.length).to eql 2
                 identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
                 expect(identities_as_hash).to match_array([ { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
@@ -126,19 +178,13 @@ describe Gitlab::Saml::User, lib: true do
               end
             end
           end
-
-          context 'and no corresponding LDAP person' do
-            before { allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(nil) }
-
-            include_examples 'to verify compliance with allow_single_sign_on'
-          end
         end
       end
 
     end
 
     describe 'blocking' do
-      before { stub_omniauth_config({ allow_saml_sign_up: true, auto_link_saml_user: true }) }
+      before { stub_omniauth_config({ allow_single_sign_on: ['saml'], auto_link_saml_user: true }) }
 
       context 'signup with SAML only' do
         context 'dont block on create' do
@@ -162,64 +208,6 @@ describe Gitlab::Saml::User, lib: true do
         end
       end
 
-      context 'signup with linked omniauth and LDAP account' do
-        before do
-          stub_omniauth_config(auto_link_ldap_user: true)
-          allow(ldap_user).to receive(:uid) { uid }
-          allow(ldap_user).to receive(:username) { uid }
-          allow(ldap_user).to receive(:email) { ['johndoe@example.com','john2@example.com'] }
-          allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
-          allow(saml_user).to receive(:ldap_person).and_return(ldap_user)
-        end
-
-        context "and no account for the LDAP user" do
-          context 'dont block on create (LDAP)' do
-            before { allow_any_instance_of(Gitlab::LDAP::Config).to receive_messages(block_auto_created_users: false) }
-
-            it do
-              saml_user.save
-              expect(gl_user).to be_valid
-              expect(gl_user).not_to be_blocked
-            end
-          end
-
-          context 'block on create (LDAP)' do
-            before { allow_any_instance_of(Gitlab::LDAP::Config).to receive_messages(block_auto_created_users: true) }
-
-            it do
-              saml_user.save
-              expect(gl_user).to be_valid
-              expect(gl_user).to be_blocked
-            end
-          end
-        end
-
-        context 'and LDAP user has an account already' do
-          let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'ldapmain', username: 'john') }
-
-          context 'dont block on create (LDAP)' do
-            before { allow_any_instance_of(Gitlab::LDAP::Config).to receive_messages(block_auto_created_users: false) }
-
-            it do
-              saml_user.save
-              expect(gl_user).to be_valid
-              expect(gl_user).not_to be_blocked
-            end
-          end
-
-          context 'block on create (LDAP)' do
-            before { allow_any_instance_of(Gitlab::LDAP::Config).to receive_messages(block_auto_created_users: true) }
-
-            it do
-              saml_user.save
-              expect(gl_user).to be_valid
-              expect(gl_user).not_to be_blocked
-            end
-          end
-        end
-      end
-
-
       context 'sign-in' do
         before do
           saml_user.save
@@ -245,26 +233,6 @@ describe Gitlab::Saml::User, lib: true do
             expect(gl_user).not_to be_blocked
           end
         end
-
-        context 'dont block on create (LDAP)' do
-          before { allow_any_instance_of(Gitlab::LDAP::Config).to receive_messages(block_auto_created_users: false) }
-
-          it do
-            saml_user.save
-            expect(gl_user).to be_valid
-            expect(gl_user).not_to be_blocked
-          end
-        end
-
-        context 'block on create (LDAP)' do
-          before { allow_any_instance_of(Gitlab::LDAP::Config).to receive_messages(block_auto_created_users: true) }
-
-          it do
-            saml_user.save
-            expect(gl_user).to be_valid
-            expect(gl_user).not_to be_blocked
-          end
-        end
       end
     end
   end