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

Add support for Auditor Groups to SAML

Closes #7556

See merge request gitlab-org/gitlab-ee!7340

doc/integration/saml.md

@@ -236,6 +236,27 @@ considered `admin groups`.
         } }
 ```
 
+## Auditor Groups
+
+>**Note:**
+This setting is only available on GitLab 11.4 EE and above.
+
+This setting also follows the requirements documented for the `External Groups` setting.  GitLab uses the Group information provided by your IdP to determine if a user should be assigned the `auditor` role.
+
+```yaml
+{ name: 'saml',
+  label: 'Our SAML Provider',
+  groups_attribute: 'Groups',
+  auditor_groups: ['Auditors', 'Security'],
+  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'
+        } }
+```
+
 ## Bypass two factor authentication
 
 If you want some SAML authentication methods to count as 2FA on a per session basis, you can register them in the

ee/changelogs/unreleased/auditor-saml-groups.yml

+---
+title: Add `auditor_groups` configuration so Audit users can be specified using SAML groups
+merge_request: 7340
+author: St. John Johnson
+type: added

ee/lib/ee/gitlab/auth/saml/config.rb

@@ -6,6 +6,10 @@ module EE
           extend ActiveSupport::Concern
 
           class_methods do
+            def auditor_groups
+              options[:auditor_groups].is_a?(Array) ? options[:auditor_groups] : []
+            end
+
             def required_groups
               Array(options[:required_groups])
             end

ee/lib/ee/gitlab/auth/saml/user.rb

@@ -18,9 +18,10 @@ module EE
             end
 
             if user
-              # Check if there is overlap between the user's groups and the external groups
-              # setting then set user as external or internal.
+              # Check if there is overlap between the user's groups and the admin/auditor groups
+              # setting then set user as admin, auditor, or neither.
               user.admin = !(auth_hash.groups & saml_config.admin_groups).empty? if admin_groups_enabled?
+              user.auditor = !(auth_hash.groups & saml_config.auditor_groups).empty? if auditor_groups_enabled?
             end
 
             user
@@ -51,7 +52,11 @@ module EE
           end
 
           def admin_groups_enabled?
-            !saml_config.admin_groups.nil?
+            !saml_config.admin_groups.blank?
+          end
+
+          def auditor_groups_enabled?
+            !saml_config.auditor_groups.blank?
           end
         end
       end

ee/spec/lib/gitlab/auth/saml/user_spec.rb

@@ -31,8 +31,8 @@ describe Gitlab::Auth::Saml::User do
       allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', required_groups: groups, args: {} } })
     end
 
-    def stub_saml_admin_group_config(groups)
-      allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', admin_groups: groups, args: {} } })
+    def stub_saml_group_config(type, groups)
+      allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', "#{type}_groups": groups, args: {} } })
     end
 
     before do
@@ -44,37 +44,47 @@ describe Gitlab::Auth::Saml::User do
         stub_omniauth_config({ allow_single_sign_on: ['saml'], auto_link_saml_user: true })
       end
 
-      context 'admin groups' do
-        context 'are defined' do
-          it 'marks the user as admin' do
-            stub_saml_admin_group_config(%w(Developers))
+      context 'admin/auditor groups' do
+        %w(admin auditor).each do |group_type|
+          it "marks the user as #{group_type} when the user is in the configured group" do
+            stub_saml_group_config(group_type, %w(Developers))
             saml_user.save
 
             expect(gl_user).to be_valid
-            expect(gl_user.admin).to be_truthy
+            expect(gl_user.public_send(group_type)).to be_truthy
           end
-        end
 
-        before do
-          stub_saml_admin_group_config(%w(Admins))
-        end
+          it "does not mark the user as #{group_type} when the user is not in the configured group" do
+            stub_saml_group_config(group_type, %w(Admin))
+            saml_user.save
+
+            expect(gl_user).to be_valid
+            expect(gl_user.public_send(group_type)).to be_falsey
+          end
 
-        context 'are defined but the user does not belong there' do
-          it 'does not mark the user as admin' do
+          it "demotes from #{group_type} if not in the configured group" do
+            create(:user, email: 'john@mail.com', username: 'john').update_attribute(group_type, true)
+            stub_saml_group_config(group_type, %w(Admin))
             saml_user.save
 
             expect(gl_user).to be_valid
-            expect(gl_user.admin).to be_falsey
+            expect(gl_user.public_send(group_type)).to be_falsey
           end
-        end
 
-        context 'user was admin, now should not be' do
-          it 'makes user non admin' do
-            create(:user, email: 'john@mail.com', username: 'john').update_attribute('admin', true)
+          it "does not demote from #{group_type} if not configured" do
+            create(:user, email: 'john@mail.com', username: 'john').update_attribute(group_type, true)
+            stub_saml_group_config(group_type, [])
             saml_user.save
 
             expect(gl_user).to be_valid
-            expect(gl_user.admin).to be_falsey
+            expect(gl_user.public_send(group_type)).to be_truthy
+          end
+
+          it "skips #{group_type} if not configured" do
+            saml_user.save
+
+            expect(gl_user).to be_valid
+            expect(gl_user.public_send(group_type)).to be_falsey
           end
         end
       end
@@ -126,28 +136,6 @@ describe Gitlab::Auth::Saml::User do
           end
         end
       end
-
-      context 'admin groups' do
-        context 'are defined' do
-          it 'marks the user as admin' do
-            stub_saml_admin_group_config(%w(Developers))
-            saml_user.save
-
-            expect(gl_user).to be_valid
-            expect(gl_user.admin).to be_truthy
-          end
-        end
-
-        context 'are defined but the user does not belong there' do
-          it 'does not mark the user as admin' do
-            stub_saml_admin_group_config(%w(Admins))
-            saml_user.save
-
-            expect(gl_user).to be_valid
-            expect(gl_user.admin).to be_falsey
-          end
-        end
-      end
     end
   end
 end