From 06e1bca40e8f8bdb9b9a25c33a6d531fd192d886 Mon Sep 17 00:00:00 2001 From: Stefan Jacobi Date: Mon, 9 Oct 2023 11:24:03 +0200 Subject: [PATCH] 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 --- backend/Dockerfile.debug | 2 ++ backend/ee/saml/config/saml.go | 10 +++--- backend/ee/saml/config/saml_test.go | 36 +++++++++---------- backend/ee/saml/handler.go | 4 +-- backend/ee/saml/provider/auth0.go | 16 ++++++--- backend/ee/saml/provider/provider.go | 2 +- backend/ee/saml/provider/saml.go | 4 +-- .../persistence/saml_certificate_persister.go | 2 +- docs/docs/guides/ee/saml.mdx | 15 ++++---- 9 files changed, 52 insertions(+), 39 deletions(-) diff --git a/backend/Dockerfile.debug b/backend/Dockerfile.debug index c763c9d8d3..2e0a2a7da3 100644 --- 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 index fc414437b4..bb8706f3bf 100644 --- 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 index a97b484b9e..78070ffd73 100644 --- 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 index 6177497f0f..692277dab0 100644 --- 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 index 25a9d459f1..5726de8a33 100644 --- 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 index 2fbfeabaf1..adc3fec678 100644 --- 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 index d11cd0d022..2cbc5dcf43 100644 --- 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 index eb03ba25af..8340a9037a 100644 --- 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 index 39dac7e072..339caf29ce 100644 --- 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