Merge branch 'jej/improve-group-saml-name-id' into 'master'

Group SAML prevents blank NameID on Identity

Closes #10119

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

doc/user/group/saml_sso/index.md

@@ -16,6 +16,12 @@ NOTE: **Note:** SAML SSO for groups is used only as a convenient way to add user
 1. Find the SSO URL from your Identity Provider and enter it on GitLab.
 1. Find and enter the fingerprint for the SAML token signing certificate.
 
+## NameID
+
+GitLab.com uses the SAML NameID to identify users, so it must be present in the SAML response and unique to the user.
+
+The value should be something that will never change for that user, such as a unique ID or username. Email could also be used as the NameID, but only if it can be guaranteed to never change.
+
 ## Assertions
 
 | Field | Supported keys | Notes |

ee/app/models/ee/identity.rb

@@ -7,6 +7,8 @@ module EE
     prepended do
       belongs_to :saml_provider
 
+      validates :name_id, presence: true, if: :saml_provider
+
       validates :secondary_extern_uid,
         allow_blank: true,
         uniqueness: {
@@ -17,9 +19,24 @@ module EE
       scope :with_secondary_extern_uid, ->(provider, secondary_extern_uid) do
         iwhere(secondary_extern_uid: normalize_uid(provider, secondary_extern_uid)).with_provider(provider)
       end
+
+      def name_id
+        extern_uid
+      end
     end
 
     class_methods do
+      extend ::Gitlab::Utils::Override
+
+      override :human_attribute_name
+      def human_attribute_name(name, *args)
+        if name.to_sym == :name_id
+          "SAML NameID"
+        else
+          super
+        end
+      end
+
       def find_by_extern_uid(provider, extern_uid)
         with_extern_uid(provider, extern_uid).take
       end

ee/changelogs/unreleased/jej-improve-group-saml-name-id.yml

+---
+title: GroupSAML for GitLab.com prevents blank NameID
+merge_request: 9907
+author:
+type: fixed

ee/spec/models/identity_spec.rb

@@ -12,5 +12,12 @@ describe Identity do
 
       expect(identity_two).to be_valid
     end
+
+    it "doesn't allow NameID/extern_uid to be blank" do
+      identity = build(:identity, provider: 'group_saml', saml_provider: create(:saml_provider), extern_uid: "")
+
+      expect(identity).not_to be_valid
+      expect(identity.errors.full_messages.join).to include("NameID can't be blank")
+    end
   end
 end