From 36940c60adbc1edbcc0c8d64e38feec0d33bd98c Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Thu, 7 Mar 2019 17:13:07 +0800
Subject: [PATCH] Group SAML prevents blank NameID on Identity

When SAML for GitLab.com is misconfigured the NameID can be missing.
Previously we would end up with broken identity links, which were hard
to debug and fix.

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/10119
---
 doc/user/group/saml_sso/index.md                |  6 ++++++
 ee/app/models/ee/identity.rb                    | 17 +++++++++++++++++
 .../jej-improve-group-saml-name-id.yml          |  5 +++++
 ee/spec/models/identity_spec.rb                 |  7 +++++++
 4 files changed, 35 insertions(+)
 create mode 100644 ee/changelogs/unreleased/jej-improve-group-saml-name-id.yml

diff --git a/doc/user/group/saml_sso/index.md b/doc/user/group/saml_sso/index.md
index cf57a707507..d1917894ee8 100644
--- a/doc/user/group/saml_sso/index.md
+++ b/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 |
diff --git a/ee/app/models/ee/identity.rb b/ee/app/models/ee/identity.rb
index f1ff740bbce..8eda5a95e98 100644
--- a/ee/app/models/ee/identity.rb
+++ b/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
diff --git a/ee/changelogs/unreleased/jej-improve-group-saml-name-id.yml b/ee/changelogs/unreleased/jej-improve-group-saml-name-id.yml
new file mode 100644
index 00000000000..5ffbc55656e
--- /dev/null
+++ b/ee/changelogs/unreleased/jej-improve-group-saml-name-id.yml
@@ -0,0 +1,5 @@
+---
+title: GroupSAML for GitLab.com prevents blank NameID
+merge_request: 9907
+author:
+type: fixed
diff --git a/ee/spec/models/identity_spec.rb b/ee/spec/models/identity_spec.rb
index 5c7e2d89ea1..17529529a86 100644
--- a/ee/spec/models/identity_spec.rb
+++ b/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
-- 
2.30.9