From ef405d89e69e008640f275bc37f8ec02ad32da40 Mon Sep 17 00:00:00 2001
From: Stojan Dimitrovski <sdimitrovski@gmail.com>
Date: Thu, 14 Mar 2024 04:15:22 +0100
Subject: [PATCH] feat: configurable NameID format for SAML provider (#1481)

Adds the ability to specify the [NameID
format](https://pkg.go.dev/github.com/crewjam/saml#NameIDFormat) for a
SAML SSO provider's authentication request, as some providers don't
appear to support accepting the persistent format.
---
 internal/api/sso.go                           |  8 +++-
 internal/api/ssoadmin.go                      | 39 ++++++++++++++++++-
 internal/models/sso.go                        |  2 +
 ...40314092811_add_saml_name_id_format.up.sql |  3 ++
 4 files changed, 48 insertions(+), 4 deletions(-)
 create mode 100644 migrations/20240314092811_add_saml_name_id_format.up.sql

diff --git a/internal/api/sso.go b/internal/api/sso.go
index e50d1a369..10034075c 100644
--- a/internal/api/sso.go
+++ b/internal/api/sso.go
@@ -91,8 +91,6 @@ func (a *API) SingleSignOn(w http.ResponseWriter, r *http.Request) error {
 		return internalServerError("Error parsing SAML Metadata for SAML provider").WithInternalError(err)
 	}
 
-	// TODO: fetch new metadata if validUntil < time.Now()
-
 	serviceProvider := a.getSAMLServiceProvider(entityDescriptor, false /* <- idpInitiated */)
 
 	authnRequest, err := serviceProvider.MakeAuthenticationRequest(
@@ -104,6 +102,12 @@ func (a *API) SingleSignOn(w http.ResponseWriter, r *http.Request) error {
 		return internalServerError("Error creating SAML Authentication Request").WithInternalError(err)
 	}
 
+	// Some IdPs do not support the use of the `persistent` NameID format,
+	// and require a different format to be sent to work.
+	if ssoProvider.SAMLProvider.NameIDFormat != nil {
+		authnRequest.NameIDPolicy.Format = ssoProvider.SAMLProvider.NameIDFormat
+	}
+
 	relayState := models.SAMLRelayState{
 		SSOProviderID: ssoProvider.ID,
 		RequestID:     authnRequest.ID,
diff --git a/internal/api/ssoadmin.go b/internal/api/ssoadmin.go
index 6e52fc8ff..fae328c4f 100644
--- a/internal/api/ssoadmin.go
+++ b/internal/api/ssoadmin.go
@@ -5,6 +5,7 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"strings"
 	"unicode/utf8"
 
 	"github.com/crewjam/saml"
@@ -74,6 +75,7 @@ type CreateSSOProviderParams struct {
 	MetadataXML      string                      `json:"metadata_xml"`
 	Domains          []string                    `json:"domains"`
 	AttributeMapping models.SAMLAttributeMapping `json:"attribute_mapping"`
+	NameIDFormat     string                      `json:"name_id_format"`
 }
 
 func (p *CreateSSOProviderParams) validate(forUpdate bool) error {
@@ -94,8 +96,22 @@ func (p *CreateSSOProviderParams) validate(forUpdate bool) error {
 		}
 	}
 
-	// TODO validate p.AttributeMapping
-	// TODO validate domains
+	switch p.NameIDFormat {
+	case "",
+		string(saml.PersistentNameIDFormat),
+		string(saml.EmailAddressNameIDFormat),
+		string(saml.TransientNameIDFormat),
+		string(saml.UnspecifiedNameIDFormat):
+		// it's valid
+
+	default:
+		return badRequestError(ErrorCodeValidationFailed, "name_id_format must be unspecified or one of %v", strings.Join([]string{
+			string(saml.PersistentNameIDFormat),
+			string(saml.EmailAddressNameIDFormat),
+			string(saml.TransientNameIDFormat),
+			string(saml.UnspecifiedNameIDFormat),
+		}, ", "))
+	}
 
 	return nil
 }
@@ -217,6 +233,10 @@ func (a *API) adminSSOProvidersCreate(w http.ResponseWriter, r *http.Request) er
 		provider.SAMLProvider.MetadataURL = &params.MetadataURL
 	}
 
+	if params.NameIDFormat != "" {
+		provider.SAMLProvider.NameIDFormat = &params.NameIDFormat
+	}
+
 	provider.SAMLProvider.AttributeMapping = params.AttributeMapping
 
 	for _, domain := range params.Domains {
@@ -335,6 +355,21 @@ func (a *API) adminSSOProvidersUpdate(w http.ResponseWriter, r *http.Request) er
 		provider.SAMLProvider.AttributeMapping = params.AttributeMapping
 	}
 
+	nameIDFormat := ""
+	if provider.SAMLProvider.NameIDFormat != nil {
+		nameIDFormat = *provider.SAMLProvider.NameIDFormat
+	}
+
+	if params.NameIDFormat != nameIDFormat {
+		modified = true
+
+		if params.NameIDFormat == "" {
+			provider.SAMLProvider.NameIDFormat = nil
+		} else {
+			provider.SAMLProvider.NameIDFormat = &params.NameIDFormat
+		}
+	}
+
 	if modified {
 		if err := db.Transaction(func(tx *storage.Connection) error {
 			if terr := tx.Eager().Update(provider); terr != nil {
diff --git a/internal/models/sso.go b/internal/models/sso.go
index d56c8962d..bbdd138ce 100644
--- a/internal/models/sso.go
+++ b/internal/models/sso.go
@@ -115,6 +115,8 @@ type SAMLProvider struct {
 
 	AttributeMapping SAMLAttributeMapping `db:"attribute_mapping" json:"attribute_mapping,omitempty"`
 
+	NameIDFormat *string `db:"name_id_format" json:"name_id_format,omitempty"`
+
 	CreatedAt time.Time `db:"created_at" json:"-"`
 	UpdatedAt time.Time `db:"updated_at" json:"-"`
 }
diff --git a/migrations/20240314092811_add_saml_name_id_format.up.sql b/migrations/20240314092811_add_saml_name_id_format.up.sql
new file mode 100644
index 000000000..0196250d2
--- /dev/null
+++ b/migrations/20240314092811_add_saml_name_id_format.up.sql
@@ -0,0 +1,3 @@
+do $$ begin
+alter table {{ index .Options "Namespace" }}.saml_providers add column if not exists name_id_format text null;
+end $$