fix(saml): fix review issues

* fix typos in config * fix defaults for auth0 -> add schema * change error message in saml certificate db entity creation * change redirectError to use StatusSeeOther (303) to switch from POST to GET request
teamhanko · Oct 9, 2023 · 06e1bca · 06e1bca
1 parent 4d285fa
commit 06e1bca
Show file tree

Hide file tree

Showing 9 changed files with 52 additions and 39 deletions.
diff --git a/backend/Dockerfile.debug b/backend/Dockerfile.debug
@@ -18,6 +18,7 @@ COPY server server/
 COPY handler handler/
 COPY crypto crypto/
 COPY dto dto/
+COPY ee/saml ee/saml/
 COPY session session/
 COPY mail mail/
 COPY audit_log audit_log/
@@ -27,6 +28,7 @@ COPY thirdparty thirdparty/
 COPY build_info build_info/
 COPY middleware middleware/
 COPY template template/
+COPY utils utils/
 
 # Build
 RUN go generate ./...

diff --git a/backend/ee/saml/config/saml.go b/backend/ee/saml/config/saml.go
@@ -34,7 +34,7 @@ type IdentityProvider struct {
 	Enabled               bool         `yaml:"enabled" json:"enabled,omitempty" koanf:"enabled" jsonschema:"default=false"`
 	Name                  string       `yaml:"name" json:"name,omitempty" koanf:"name"`
 	Domain                string       `yaml:"domain" json:"domain,omitempty" koanf:"domain"`
-	MetdadataUrl          string       `yaml:"metadata_url" json:"metadata_url,omitempty" koanf:"metadata_url"`
+	MetadataUrl           string       `yaml:"metadata_url" json:"metadata_url,omitempty" koanf:"metadata_url"`
 	SkipEmailVerification bool         `yaml:"skip_email_verification" json:"skip_email_verification,omitempty" koanf:"skip_email_verification"`
 	AttributeMap          AttributeMap `yaml:"attribute_map" json:"attribute_map,omitempty" koanf:"attribute_map"`
 }
@@ -52,7 +52,7 @@ type AttributeMap struct {
 	Gender            string `yaml:"gender" json:"gender,omitempty" koanf:"gender"`
 	Birthdate         string `yaml:"birthdate" json:"birthdate,omitempty" koanf:"birthdate"`
 	ZoneInfo          string `yaml:"zone_info" json:"zone_info,omitempty" koanf:"zone_info"`
-	Locale            string `yaml:"local" json:"local,omitempty" koanf:"local"`
+	Locale            string `yaml:"locale" json:"locale,omitempty" koanf:"locale"`
 	UpdatedAt         string `yaml:"updated_at" json:"updated_at,omitempty" koanf:"updated_at"`
 	Email             string `yaml:"email" json:"email,omitempty" koanf:"email" jsonschema:"default=http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress"`
 	EmailVerified     string `yaml:"email_verified" json:"email_verified,omitempty" koanf:"email_verified"`
@@ -154,13 +154,13 @@ func (idp *IdentityProvider) Validate() error {
 		return fmt.Errorf(invalidUrlFormat, idp.Domain)
 	}
 
-	if strings.TrimSpace(idp.MetdadataUrl) == "" {
+	if strings.TrimSpace(idp.MetadataUrl) == "" {
 		return errors.New("identity provider metadata url must be set")
 	}
 
-	_, err = url.Parse(idp.MetdadataUrl)
+	_, err = url.Parse(idp.MetadataUrl)
 	if err != nil {
-		return fmt.Errorf(invalidUrlFormat, idp.MetdadataUrl)
+		return fmt.Errorf(invalidUrlFormat, idp.MetadataUrl)
 	}
 
 	return nil

diff --git a/backend/ee/saml/config/saml_test.go b/backend/ee/saml/config/saml_test.go
@@ -70,10 +70,10 @@ func TestSamlConfig_ValidateWithEnabledSaml(t *testing.T) {
 		AllowedRedirectURLS:   []string{},
 		AllowedRedirectURLMap: nil,
 		IdentityProviders: []IdentityProvider{{
-			Enabled:      true,
-			Name:         "Test Provider",
-			Domain:       "lorem.ipsum",
-			MetdadataUrl: "http://provider.lorem.ipsum/metadata",
+			Enabled:     true,
+			Name:        "Test Provider",
+			Domain:      "lorem.ipsum",
+			MetadataUrl: "http://provider.lorem.ipsum/metadata",
 		}},
 	}
 
@@ -172,10 +172,10 @@ func TestSamlConfig_ValidateUrlsWithWrongAllowedRedirectUrl(t *testing.T) {
 
 func TestSamlConfig_ValidateProvider(t *testing.T) {
 	cfg := &IdentityProvider{
-		Enabled:      true,
-		Name:         "Lorem",
-		Domain:       "lorem.ipsum",
-		MetdadataUrl: "http://provider.lorem.ipsum/metadata",
+		Enabled:     true,
+		Name:        "Lorem",
+		Domain:      "lorem.ipsum",
+		MetadataUrl: "http://provider.lorem.ipsum/metadata",
 	}
 
 	err := cfg.Validate()
@@ -232,9 +232,9 @@ func TestSamlConfig_ValidateProviderErrorWithInvalidDomain(t *testing.T) {
 
 func TestSamlConfig_ValidateProviderErrorWithEmptyMetadataUrl(t *testing.T) {
 	cfg := &IdentityProvider{
-		Domain:       "lorem.ipsum",
-		Name:         "Test",
-		MetdadataUrl: "",
+		Domain:      "lorem.ipsum",
+		Name:        "Test",
+		MetadataUrl: "",
 	}
 
 	err := cfg.Validate()
@@ -243,9 +243,9 @@ func TestSamlConfig_ValidateProviderErrorWithEmptyMetadataUrl(t *testing.T) {
 
 func TestSamlConfig_ValidateProviderErrorWithSpaceMetadataUrl(t *testing.T) {
 	cfg := &IdentityProvider{
-		Domain:       "lorem.ipsum",
-		Name:         "Test",
-		MetdadataUrl: "  ",
+		Domain:      "lorem.ipsum",
+		Name:        "Test",
+		MetadataUrl: "  ",
 	}
 
 	err := cfg.Validate()
@@ -254,11 +254,11 @@ func TestSamlConfig_ValidateProviderErrorWithSpaceMetadataUrl(t *testing.T) {
 
 func TestSamlConfig_ValidateProviderErrorWithInvalidMetadataUrl(t *testing.T) {
 	cfg := &IdentityProvider{
-		Domain:       "lorem.ipsum",
-		Name:         "Test",
-		MetdadataUrl: "http://lorem:8000.de/ipsum",
+		Domain:      "lorem.ipsum",
+		Name:        "Test",
+		MetadataUrl: "http://lorem:8000.de/ipsum",
 	}
 
 	err := cfg.Validate()
-	assert.Errorf(t, err, invalidUrlFormat, cfg.MetdadataUrl)
+	assert.Errorf(t, err, invalidUrlFormat, cfg.MetadataUrl)
 }
diff --git a/backend/ee/saml/handler.go b/backend/ee/saml/handler.go
@@ -34,7 +34,7 @@ func NewSamlHandler(cfg *config.Config, persister persistence.Persister, session
 	for _, idpConfig := range cfg.Saml.IdentityProviders {
 		if idpConfig.Enabled {
 			name := ""
-			name, err := parseProviderFromMetadataUrl(idpConfig.MetdadataUrl)
+			name, err := parseProviderFromMetadataUrl(idpConfig.MetadataUrl)
 			if err != nil {
 				panic(err)
 			}
@@ -291,7 +291,7 @@ func (handler *SamlHandler) redirectError(c echo.Context, error error, to string
 	}
 
 	redirectURL := thirdparty.GetErrorUrl(to, error)
-	return c.Redirect(http.StatusTemporaryRedirect, redirectURL)
+	return c.Redirect(http.StatusSeeOther, redirectURL)
 }
 
 func (handler *SamlHandler) auditError(c echo.Context, err error) error {

diff --git a/backend/ee/saml/provider/auth0.go b/backend/ee/saml/provider/auth0.go
@@ -27,19 +27,27 @@ func NewAuth0ServiceProvider(config *config.Config, idpConfig samlConfig.Identit
 func (sp *Auth0Provider) UseDefaultAttributesIfEmpty() {
 	attributeMap := &sp.Config.AttributeMap
 
+	if attributeMap.Name == "" {
+		attributeMap.Name = "http://schemas.auth0.com/name"
+	}
+
+	if attributeMap.Email == "" {
+		attributeMap.Name = "http://schemas.auth0.com/email"
+	}
+
 	if attributeMap.EmailVerified == "" {
-		attributeMap.EmailVerified = "email_verified"
+		attributeMap.EmailVerified = "http://schemas.auth0.com/email_verified"
 	}
 
 	if attributeMap.NickName == "" {
-		attributeMap.NickName = "nickname"
+		attributeMap.NickName = "http://schemas.auth0.com/nickname"
 	}
 
 	if attributeMap.Picture == "" {
-		attributeMap.Picture = "picture"
+		attributeMap.Picture = "http://schemas.auth0.com/picture"
 	}
 
 	if attributeMap.UpdatedAt == "" {
-		attributeMap.UpdatedAt = "updated_at"
+		attributeMap.UpdatedAt = "http://schemas.auth0.com/updated_at"
 	}
 }
diff --git a/backend/ee/saml/provider/provider.go b/backend/ee/saml/provider/provider.go
@@ -71,7 +71,7 @@ func loadCertificate(cfg *config.Config, persister persistence.SamlCertificatePe
 }
 
 func fetchIdpMetadata(idpConfig samlConfig.IdentityProvider) (*IdpMetadata, error) {
-	response, err := http.Get(idpConfig.MetdadataUrl)
+	response, err := http.Get(idpConfig.MetadataUrl)
 	if err != nil {
 		return nil, fmt.Errorf("unable to fetch metadata: %w", err)
 	}

diff --git a/backend/ee/saml/provider/saml.go b/backend/ee/saml/provider/saml.go
@@ -76,7 +76,7 @@ func (sp *BaseSamlProvider) GetUserData(assertionInfo *saml2.AssertionInfo) *thi
 
 	email := thirdparty.Email{
 		Email:    emailAddress,
-		Verified: false,
+		Verified: assertionValues.Get(attributeMap.EmailVerified) != "",
 		Primary:  true,
 	}
 
@@ -106,7 +106,7 @@ func (sp *BaseSamlProvider) GetUserData(assertionInfo *saml2.AssertionInfo) *thi
 		Locale:            assertionValues.Get(attributeMap.Locale),
 		UpdatedAt:         assertionValues.Get(attributeMap.UpdatedAt),
 		Email:             emailAddress,
-		EmailVerified:     assertionValues.Get(attributeMap.EmailVerified) != "" || sp.Config.SkipEmailVerification,
+		EmailVerified:     email.Verified || sp.Config.SkipEmailVerification,
 		Phone:             assertionValues.Get(attributeMap.Phone),
 		PhoneVerified:     assertionValues.Get(attributeMap.PhoneVerified) != "",
 		CustomClaims:      sp.mapCustomClaims(assertionInfo.Values, attributeMap),

diff --git a/backend/persistence/saml_certificate_persister.go b/backend/persistence/saml_certificate_persister.go
@@ -48,7 +48,7 @@ func (s samlCertificatePersister) Create(cert *models.SamlCertificate) error {
 	}
 
 	if validationError != nil && validationError.HasAny() {
-		return fmt.Errorf("token object validation failed: %w", validationError)
+		return fmt.Errorf("saml certificate validation failed: %w", validationError)
 	}
 
 	return nil

diff --git a/docs/docs/guides/ee/saml.mdx b/docs/docs/guides/ee/saml.mdx
@@ -113,12 +113,15 @@ For some providers we also provide some additional attributes. The provider will
 Currently, there the following extra defaults are provided for the following providers:
 
 #### Auth0
-| Field          | Default        |
-|----------------|----------------|
-| email_verified | email_verified |
-| nickname       | nickname       |
-| picture        | picture        |
-| updated_at     | updated_at     |
+| Field          | Default                                 |
+|----------------|-----------------------------------------|
+| email_verified | http://schemas.auth0.com/email_verified |
+| nickname       | http://schemas.auth0.com/nickname       |
+| picture        | http://schemas.auth0.com/picture        |
+| updated_at     | http://schemas.auth0.com/updated_at     |
+
+*Please be aware not to set `mapUnknownClaimsAsIs` to true in your auth0 IdP config.* If you set this attribute to true auth0
+will scratch the `http://schemas.auth0.com/auth0/` part, and you have to provide an `attribute_map`-Field.
 
 ## Configure Identity Provider