From c970511c82ad7d3c2046b7e4f9a0b37ed1059991 Mon Sep 17 00:00:00 2001 From: Michael Mayer Date: Sat, 6 Jul 2024 11:15:23 +0200 Subject: [PATCH] OIDC: Upgrade "zitadel/oidc" from v1 to v2 #782 Signed-off-by: Michael Mayer --- compose.yaml | 1 + go.mod | 2 + go.sum | 4 + internal/api/oidc_login.go | 7 +- internal/api/oidc_redirect.go | 134 +++++++++++++------------ internal/auth/oidc/client.go | 109 +++++++++++--------- internal/auth/oidc/client_test.go | 45 +++++++++ internal/auth/oidc/helper.go | 63 ------------ internal/auth/oidc/helper_test.go | 95 ------------------ internal/auth/oidc/http_client.go | 17 ++-- internal/auth/oidc/http_client_test.go | 2 +- internal/auth/oidc/redirect_url.go | 26 +++++ internal/auth/oidc/username.go | 56 +++++++++++ internal/auth/oidc/username_test.go | 69 +++++++++++++ internal/entity/auth_user.go | 59 ++--------- internal/entity/auth_user_test.go | 101 ++++++++----------- internal/photoprism/get/get.go | 4 + internal/photoprism/get/oidc.go | 2 +- internal/photoprism/get/services.go | 4 +- pkg/authn/const.go | 1 + pkg/clean/scope.go | 9 ++ pkg/clean/scope_test.go | 15 +++ pkg/list/attributes.go | 7 +- pkg/rnd/uuid.go | 5 + 24 files changed, 440 insertions(+), 397 deletions(-) create mode 100644 internal/auth/oidc/client_test.go delete mode 100644 internal/auth/oidc/helper.go delete mode 100644 internal/auth/oidc/helper_test.go create mode 100644 internal/auth/oidc/redirect_url.go create mode 100644 internal/auth/oidc/username.go create mode 100644 internal/auth/oidc/username_test.go diff --git a/compose.yaml b/compose.yaml index 776811cac..1bdcc2858 100644 --- a/compose.yaml +++ b/compose.yaml @@ -9,6 +9,7 @@ services: depends_on: - mariadb - dummy-webdav + - dummy-oidc stop_grace_period: 10s security_opt: - seccomp:unconfined diff --git a/go.mod b/go.mod index 0203b11f4..48a6c4243 100644 --- a/go.mod +++ b/go.mod @@ -118,6 +118,7 @@ require ( github.com/mattn/go-isatty v0.0.20 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.2 // indirect + github.com/muhlemmer/gu v0.3.1 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/client_model v0.6.1 // indirect @@ -128,6 +129,7 @@ require ( github.com/twitchyliquid64/golang-asm v0.15.1 // indirect github.com/ugorji/go/codec v1.2.12 // indirect github.com/zitadel/logging v0.5.0 // indirect + github.com/zitadel/oidc/v2 v2.12.0 // indirect golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8 // indirect golang.org/x/oauth2 v0.21.0 // indirect golang.org/x/sys v0.21.0 // indirect diff --git a/go.sum b/go.sum index fa3863468..8c88b0691 100644 --- a/go.sum +++ b/go.sum @@ -294,6 +294,8 @@ github.com/modern-go/reflect2 v1.0.2 h1:xBagoLtFs94CBntxluKeaWgTMpvLxC4ur3nMaC9G github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/montanaflynn/stats v0.7.1 h1:etflOAAHORrCC44V+aR6Ftzort912ZU+YLiSTuV8eaE= github.com/montanaflynn/stats v0.7.1/go.mod h1:etXPPgVO6n31NxCd9KQUMvCM+ve0ruNzt6R8Bnaayow= +github.com/muhlemmer/gu v0.3.1 h1:7EAqmFrW7n3hETvuAdmFmn4hS8W+z3LgKtrnow+YzNM= +github.com/muhlemmer/gu v0.3.1/go.mod h1:YHtHR+gxM+bKEIIs7Hmi9sPT3ZDUvTN/i88wQpZkrdM= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= @@ -381,6 +383,8 @@ github.com/zitadel/logging v0.5.0 h1:Kunouvqse/efXy4UDvFw5s3vP+Z4AlHo3y8wF7stXHA github.com/zitadel/logging v0.5.0/go.mod h1:IzP5fzwFhzzyxHkSmfF8dsyqFsQRJLLcQmwhIBzlGsE= github.com/zitadel/oidc v1.13.5 h1:7jhh68NGZitLqwLiVU9Dtwa4IraJPFF1vS+4UupO93U= github.com/zitadel/oidc v1.13.5/go.mod h1:rHs1DhU3Sv3tnI6bQRVlFa3u0lCwtR7S21WHY+yXgPA= +github.com/zitadel/oidc/v2 v2.12.0 h1:4aMTAy99/4pqNwrawEyJqhRb3yY3PtcDxnoDSryhpn4= +github.com/zitadel/oidc/v2 v2.12.0/go.mod h1:LrRav74IiThHGapQgCHZOUNtnqJG0tcZKHro/91rtLw= go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU= go.opencensus.io v0.22.0/go.mod h1:+kGneAE2xo2IficOXnaByMWTGM9T73dGwxeWcUqIpI8= go.opencensus.io v0.22.2/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw= diff --git a/internal/api/oidc_login.go b/internal/api/oidc_login.go index c6b094774..fccec6bca 100644 --- a/internal/api/oidc_login.go +++ b/internal/api/oidc_login.go @@ -29,18 +29,17 @@ func OIDCLogin(router *gin.RouterGroup) { // Get client IP address for logs and rate limiting checks. clientIp := ClientIP(c) - action := "sign in" // Get global config. conf := get.Config() // Abort in public mode and if OIDC is disabled. if get.Config().Public() { - event.AuditErr([]string{clientIp, "oidc", action, authn.ErrDisabledInPublicMode.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", authn.ErrDisabledInPublicMode.Error()}) c.Redirect(http.StatusTemporaryRedirect, conf.LoginUri()) return } else if !conf.OIDCEnabled() { - event.AuditErr([]string{clientIp, "oidc", action, authn.ErrAuthenticationDisabled.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", authn.ErrAuthenticationDisabled.Error()}) c.Redirect(http.StatusTemporaryRedirect, conf.LoginUri()) return } @@ -59,7 +58,7 @@ func OIDCLogin(router *gin.RouterGroup) { provider := get.OIDC() if provider == nil { - event.AuditErr([]string{clientIp, "oidc", action, authn.ErrInvalidProviderConfiguration.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", authn.ErrInvalidProviderConfiguration.Error()}) c.HTML(http.StatusInternalServerError, "auth.gohtml", CreateSessionError(http.StatusInternalServerError, i18n.Error(i18n.ErrConnectionFailed))) return } diff --git a/internal/api/oidc_redirect.go b/internal/api/oidc_redirect.go index c7c82cabe..8b51596ff 100644 --- a/internal/api/oidc_redirect.go +++ b/internal/api/oidc_redirect.go @@ -8,6 +8,7 @@ import ( "github.com/gin-gonic/gin" + "github.com/photoprism/photoprism/internal/auth/oidc" "github.com/photoprism/photoprism/internal/entity" "github.com/photoprism/photoprism/internal/event" "github.com/photoprism/photoprism/internal/photoprism/get" @@ -22,7 +23,7 @@ import ( "github.com/photoprism/photoprism/pkg/txt" ) -// OIDCRedirect creates a new access token when a user has been successfully authenticated, +// OIDCRedirect creates a new API access token when a user has been successfully authenticated via OIDC, // and then redirects the browser back to the app. // // GET /api/v1/oidc/redirect @@ -41,18 +42,17 @@ func OIDCRedirect(router *gin.RouterGroup) { clientIp := ClientIP(c) userAgent := UserAgent(c) userName := "unknown user" - action := "sign in" // Get global config. conf := get.Config() // Abort in public mode and if OIDC is disabled. if get.Config().Public() { - event.AuditErr([]string{clientIp, "oidc", action, authn.ErrDisabledInPublicMode.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", authn.ErrDisabledInPublicMode.Error()}) c.Redirect(http.StatusTemporaryRedirect, conf.LoginUri()) return } else if !conf.OIDCEnabled() { - event.AuditErr([]string{clientIp, "oidc", action, authn.ErrAuthenticationDisabled.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", authn.ErrAuthenticationDisabled.Error()}) c.Redirect(http.StatusTemporaryRedirect, conf.LoginUri()) return } @@ -69,7 +69,7 @@ func OIDCRedirect(router *gin.RouterGroup) { // Check if the required request parameters are present. if c.Query("state") == "" || c.Query("code") == "" { - event.AuditErr([]string{clientIp, "oidc", action, authn.ErrAuthCodeRequired.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", authn.ErrAuthCodeRequired.Error()}) c.Redirect(http.StatusTemporaryRedirect, conf.LoginUri()) return } @@ -78,15 +78,16 @@ func OIDCRedirect(router *gin.RouterGroup) { provider := get.OIDC() if provider == nil { - event.AuditErr([]string{clientIp, "oidc", action, authn.ErrAuthenticationDisabled.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", authn.ErrInvalidProviderConfiguration.Error()}) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return } + // Check the auth request and, if successful, get user information and tokens. userInfo, tokens, claimErr := provider.CodeExchangeUserInfo(c) if claimErr != nil { - event.AuditErr([]string{clientIp, "oidc", action, claimErr.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", claimErr.Error()}) return } @@ -94,53 +95,55 @@ func OIDCRedirect(router *gin.RouterGroup) { var user *entity.User var err error - userEmail := clean.Email(userInfo.GetEmail()) + userEmail := clean.Email(userInfo.Email) // Optionally check if the email domain matches. if domain := conf.OIDCDomain(); domain == "" { // Do nothing. - } else if _, emailDomain, _ := strings.Cut(userEmail, "@"); emailDomain == "" || !userInfo.IsEmailVerified() { - event.AuditErr([]string{clientIp, "oidc", action, authn.ErrVerifiedEmailRequired.Error()}) + } else if _, emailDomain, _ := strings.Cut(userEmail, "@"); emailDomain == "" || !userInfo.EmailVerified { + event.AuditErr([]string{clientIp, "create session", "oidc", authn.ErrVerifiedEmailRequired.Error()}) event.LoginError(clientIp, "oidc", userEmail, userAgent, authn.ErrVerifiedEmailRequired.Error()) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrForbidden))) return } else if !strings.HasSuffix("."+emailDomain, "."+domain) { message := fmt.Sprintf("domain must match '%s'", domain) - event.AuditErr([]string{clientIp, "oidc", action, userEmail, message}) + event.AuditErr([]string{clientIp, "create session", "oidc", userEmail, message}) event.LoginError(clientIp, "oidc", userEmail, userAgent, message) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrForbidden))) return } // Find existing user record and update it, if necessary. - if oidcUser := entity.OidcUser(userInfo, conf.OIDCUsername()); authn.ProviderOIDC.NotEqual(oidcUser.AuthProvider) { - event.AuditErr([]string{clientIp, "oidc", action, authn.ErrAuthProviderIsNotOIDC.Error()}) + if oidcUser := entity.OidcUser(userInfo, oidc.Username(userInfo, conf.OIDCUsername())); authn.ProviderOIDC.NotEqual(oidcUser.AuthProvider) { + event.AuditErr([]string{clientIp, "create session", "oidc", authn.ErrAuthProviderIsNotOIDC.Error()}) event.LoginError(clientIp, "oidc", oidcUser.UserName, userAgent, authn.ErrAuthProviderIsNotOIDC.Error()) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return } else if oidcUser.UserName == "" { - event.AuditErr([]string{clientIp, "oidc", action, authn.ErrUsernameRequiredToRegister.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", authn.ErrUsernameRequiredToRegister.Error()}) event.LoginError(clientIp, "oidc", oidcUser.UserName, userAgent, authn.ErrUsernameRequiredToRegister.Error()) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return } else if user = entity.FindUser(oidcUser); user != nil { - // Check if username and subject UID match. + // Ensure user has a username. if user.Username() == "" { - event.AuditErr([]string{clientIp, "oidc", action, oidcUser.UserName, authn.ErrUsernameRequired.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", oidcUser.UserName, authn.ErrUsernameRequired.Error()}) event.LoginError(clientIp, "oidc", oidcUser.UserName, userAgent, authn.ErrUsernameRequired.Error()) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return } userName = user.Username() + event.AuditInfo([]string{clientIp, "create session", "oidc", "found user", userName}) + // Check if OIDC subject identifier matches. if authn.ProviderOIDC.NotEqual(user.AuthProvider) { - event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrAuthProviderIsNotOIDC.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", userName, authn.ErrAuthProviderIsNotOIDC.Error()}) event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrAuthProviderIsNotOIDC.Error()) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return } else if user.AuthID == "" || oidcUser.AuthID == "" || user.AuthID != oidcUser.AuthID { - event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrInvalidAuthID.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", userName, authn.ErrInvalidAuthID.Error()}) event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrInvalidAuthID.Error()) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return @@ -151,43 +154,43 @@ func OIDCRedirect(router *gin.RouterGroup) { // Update user display name. if entity.SrcPriority[details.NameSrc] <= entity.SrcPriority[entity.SrcOIDC] { - user.SetDisplayName(userInfo.GetName(), entity.SrcOIDC) - user.SetGivenName(userInfo.GetGivenName()) - user.SetFamilyName(userInfo.GetFamilyName()) - details.UserGender = clean.Name(string(userInfo.GetGender())) + user.SetDisplayName(userInfo.Name, entity.SrcOIDC) + user.SetGivenName(userInfo.GivenName) + user.SetFamilyName(userInfo.FamilyName) + details.UserGender = clean.Name(string(userInfo.Gender)) } // Update nickname. - if name := clean.Name(userInfo.GetNickname()); name != "" { - details.NickName = clean.Name(userInfo.GetNickname()) + if name := clean.Name(userInfo.Nickname); name != "" { + details.NickName = clean.Name(userInfo.Nickname) } // Update profile URL. - if u := clean.Uri(userInfo.GetProfile()); u != "" { + if u := clean.Uri(userInfo.Profile); u != "" { details.ProfileURL = u } // Update website URL. - if u := clean.Uri(userInfo.GetWebsite()); u != "" { + if u := clean.Uri(userInfo.Website); u != "" { details.SiteURL = u } // Update UI locale. - user.Settings().UILanguage = clean.Locale(userInfo.GetLocale().String(), user.Settings().UILanguage) + user.Settings().UILanguage = clean.Locale(userInfo.Locale.String(), user.Settings().UILanguage) // Update UI timezone. - if tz := userInfo.GetZoneinfo(); tz != "" && tz != time.UTC.String() { + if tz := userInfo.Zoneinfo; tz != "" && tz != time.UTC.String() { user.Settings().UITimeZone = tz } // Update user location, if available. if addr := userInfo.GetAddress(); addr != nil { - user.Details().UserLocation = clean.Name(addr.GetLocality()) - user.Details().UserCountry = clean.TypeLowerUnderscore(addr.GetCountry()) + user.Details().UserLocation = clean.Name(addr.Locality) + user.Details().UserCountry = clean.TypeLowerUnderscore(addr.Country) } // Update birthday, if available. - if birthDate := txt.ParseTime(userInfo.GetBirthdate(), userInfo.GetZoneinfo()); !birthDate.IsZero() { + if birthDate := txt.ParseTime(userInfo.Birthdate, userInfo.Zoneinfo); !birthDate.IsZero() { user.BornAt = &birthDate user.Details().BirthDay = birthDate.Day() user.Details().BirthMonth = int(birthDate.Month()) @@ -195,28 +198,26 @@ func OIDCRedirect(router *gin.RouterGroup) { } // Update email, if verified. - if userInfo.IsEmailVerified() { - user.UserEmail = clean.Email(userInfo.GetEmail()) + if userInfo.EmailVerified { + user.UserEmail = clean.Email(userInfo.Email) user.VerifiedAt = entity.TimeStamp() } // Update existing user account. if err = user.Save(); err != nil { - event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrAccountUpdateFailed.Error(), err.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", userName, authn.ErrAccountUpdateFailed.Error(), err.Error()}) event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrAccountUpdateFailed.Error()+": "+err.Error()) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return } // Set user avatar image? - if avatarUrl := userInfo.GetPicture(); avatarUrl == "" || user.HasAvatar() { + if avatarUrl := userInfo.Picture; avatarUrl == "" || user.HasAvatar() { // Do nothing. } else if err = avatar.SetUserImageURL(user, avatarUrl, entity.SrcOIDC); err != nil { - event.AuditWarn([]string{clientIp, "oidc", action, userName, "failed to set avatar image", err.Error()}) + event.AuditWarn([]string{clientIp, "create session", "oidc", userName, "failed to set avatar image", err.Error()}) } } else if conf.OIDCRegister() { - action = "register" - // Create new user record. user = &oidcUser @@ -227,35 +228,37 @@ func OIDCRedirect(router *gin.RouterGroup) { userName = userName + rnd.Base10(6) } + event.AuditInfo([]string{clientIp, "create session", "oidc", "create user", userName}) + user.UserName = userName // Set user profile information. - user.SetDisplayName(userInfo.GetName(), entity.SrcOIDC) - user.SetGivenName(userInfo.GetGivenName()) - user.SetFamilyName(userInfo.GetFamilyName()) - user.Details().UserGender = clean.Name(string(userInfo.GetGender())) - user.Details().NickName = clean.Name(userInfo.GetNickname()) + user.SetDisplayName(userInfo.Name, entity.SrcOIDC) + user.SetGivenName(userInfo.GivenName) + user.SetFamilyName(userInfo.FamilyName) + user.Details().UserGender = clean.Name(string(userInfo.Gender)) + user.Details().NickName = clean.Name(userInfo.Nickname) // Set user profile URL. - user.Details().ProfileURL = clean.Uri(userInfo.GetProfile()) + user.Details().ProfileURL = clean.Uri(userInfo.Profile) // Set user site URL. - user.Details().SiteURL = clean.Uri(userInfo.GetWebsite()) + user.Details().SiteURL = clean.Uri(userInfo.Website) // Set UI locale. - user.Settings().UILanguage = clean.Locale(userInfo.GetLocale().String(), "") + user.Settings().UILanguage = clean.Locale(userInfo.Locale.String(), "") // Set UI timezone. - user.Settings().UITimeZone = userInfo.GetZoneinfo() + user.Settings().UITimeZone = userInfo.Zoneinfo // Set user location, if available. if addr := userInfo.GetAddress(); addr != nil { - user.Details().UserLocation = clean.Name(addr.GetLocality()) - user.Details().UserCountry = clean.TypeLowerUnderscore(addr.GetCountry()) + user.Details().UserLocation = clean.Name(addr.Locality) + user.Details().UserCountry = clean.TypeLowerUnderscore(addr.Country) } // Set birthday, if available. - if birthDate := txt.ParseTime(userInfo.GetBirthdate(), userInfo.GetZoneinfo()); !birthDate.IsZero() { + if birthDate := txt.ParseTime(userInfo.Birthdate, userInfo.Zoneinfo); !birthDate.IsZero() { user.BornAt = &birthDate user.Details().BirthDay = birthDate.Day() user.Details().BirthMonth = int(birthDate.Month()) @@ -263,8 +266,8 @@ func OIDCRedirect(router *gin.RouterGroup) { } // Set email, if verified. - if userInfo.IsEmailVerified() { - user.UserEmail = clean.Email(userInfo.GetEmail()) + if userInfo.EmailVerified { + user.UserEmail = clean.Email(userInfo.Email) user.VerifiedAt = entity.TimeStamp() } @@ -275,20 +278,20 @@ func OIDCRedirect(router *gin.RouterGroup) { // Create new user account. if err = user.Create(); err != nil { - event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrAccountCreateFailed.Error(), err.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", userName, authn.ErrAccountCreateFailed.Error(), err.Error()}) event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrAccountCreateFailed.Error()+": "+err.Error()) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return } // Set user avatar image. - if avatarUrl := userInfo.GetPicture(); avatarUrl == "" { - event.AuditDebug([]string{clientIp, "oidc", action, userName, "no avatar image provided"}) + if avatarUrl := userInfo.Picture; avatarUrl == "" { + event.AuditDebug([]string{clientIp, "create session", "oidc", userName, "no avatar image provided"}) } else if err = avatar.SetUserImageURL(user, avatarUrl, entity.SrcOIDC); err != nil { - event.AuditWarn([]string{clientIp, "oidc", action, userName, "failed to set avatar image", err.Error()}) + event.AuditWarn([]string{clientIp, "create session", "oidc", userName, "failed to set avatar image", err.Error()}) } } else { - event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrRegistrationDisabled.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", userName, authn.ErrRegistrationDisabled.Error()}) event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrRegistrationDisabled.Error()) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return @@ -296,7 +299,7 @@ func OIDCRedirect(router *gin.RouterGroup) { // Login allowed? if !user.CanLogIn() { - event.AuditErr([]string{clientIp, "oidc", action, userName, authn.ErrAccountDisabled.Error()}) + event.AuditErr([]string{clientIp, "create session", "oidc", userName, authn.ErrAccountDisabled.Error()}) event.LoginError(clientIp, "oidc", userName, userAgent, authn.ErrAccountDisabled.Error()) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return @@ -320,11 +323,11 @@ func OIDCRedirect(router *gin.RouterGroup) { // Save session after successful authentication. if sess, err = get.Session().Save(sess); err != nil { - event.AuditErr([]string{clientIp, "oidc", action, userName, "%s"}, err) + event.AuditErr([]string{clientIp, "create session", "oidc", userName, "%s"}, err) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrInvalidCredentials))) return } else if sess == nil { - event.AuditErr([]string{clientIp, "oidc", action, userName, "session is nil"}) + event.AuditErr([]string{clientIp, "create session", "oidc", userName, authn.Failed}) c.HTML(http.StatusUnauthorized, "auth.gohtml", CreateSessionError(http.StatusUnauthorized, i18n.Error(i18n.ErrUnexpected))) return } @@ -335,8 +338,15 @@ func OIDCRedirect(router *gin.RouterGroup) { // Response includes user data, session data, and client config values. response := CreateSessionResponse(sess.AuthToken(), sess, conf.ClientSession(sess)) - // Log success. - event.AuditInfo([]string{clientIp, "oidc", action, userName, authn.Succeeded}) + // Log session created event. + event.AuditInfo([]string{clientIp, "session %s", "oidc", userName, authn.Created}, sess.RefID) + + // Log session expiration time. + if expires := sess.ExpiresAt(); !expires.IsZero() { + event.AuditDebug([]string{clientIp, "session %s", "oidc", userName, "expires at %s"}, sess.RefID, txt.DateTime(&expires)) + } + + // Log successful login. event.LoginInfo(clientIp, "oidc", userName, userAgent) // Update login timestamp. diff --git a/internal/auth/oidc/client.go b/internal/auth/oidc/client.go index b466f0b93..0fdc31367 100644 --- a/internal/auth/oidc/client.go +++ b/internal/auth/oidc/client.go @@ -5,56 +5,66 @@ import ( "fmt" "net/http" "net/url" - "path" - "strings" "time" "github.com/gin-gonic/gin" - "github.com/zitadel/oidc/pkg/client" - "github.com/zitadel/oidc/pkg/client/rp" - utils "github.com/zitadel/oidc/pkg/http" - "github.com/zitadel/oidc/pkg/oidc" + "github.com/zitadel/oidc/v2/pkg/client" + "github.com/zitadel/oidc/v2/pkg/client/rp" + utils "github.com/zitadel/oidc/v2/pkg/http" + "github.com/zitadel/oidc/v2/pkg/oidc" - "github.com/photoprism/photoprism/internal/config" + "github.com/photoprism/photoprism/internal/event" + "github.com/photoprism/photoprism/pkg/clean" "github.com/photoprism/photoprism/pkg/rnd" ) -const ( - RoleClaim = "photoprism_role" - AdminRole = "photoprism_admin" -) - +// Client represents an OpenID Connect (OIDC) Relying Party Client. type Client struct { rp.RelyingParty - debug bool + insecure bool } -func NewClient(oidcUri *url.URL, oidcClient, oidcSecret, oidcScopes, siteUrl string, debug bool) (result *Client, err error) { - u, err := url.Parse(siteUrl) - - if err != nil { - log.Debug(err) +// NewClient creates and returns a new OpenID Connect (OIDC) Relying Party Client based on the specified parameters. +func NewClient(issuerUri *url.URL, oidcClient, oidcSecret, oidcScopes, siteUrl string, insecure bool) (result *Client, err error) { + if issuerUri == nil { + err = errors.New("issuer uri required") + event.AuditErr([]string{"oidc", "provider", "%s"}, err) + return nil, errors.New("issuer uri required") + } else if insecure == false && issuerUri.Scheme != "https" { + err = errors.New("issuer uri must use https") + event.AuditErr([]string{"oidc", "provider", "%s"}, err) return nil, err } - u.Path = path.Join(u.Path, config.OidcRedirectUri) + // Get redirect URL based on site URL. + redirectUrl, urlErr := RedirectURL(siteUrl) + if urlErr != nil { + event.AuditErr([]string{"oidc", "redirect url", "%s"}, err) + return nil, err + } + + // Generate cryptographic keys. var hashKey, encryptKey []byte if hashKey, err = rnd.RandomBytes(16); err != nil { - log.Debugf("oidc: %q (create hash key)", err) + event.AuditErr([]string{"oidc", "hash key", "%s"}, err) return nil, err } if encryptKey, err = rnd.RandomBytes(16); err != nil { - log.Debugf("oidc: %q (create encrypt key)", err) + event.AuditErr([]string{"oidc", "encrypt key", "%s"}, err) return nil, err } + // Create cookie handler. cookieHandler := utils.NewCookieHandler(hashKey, encryptKey, utils.WithUnsecure()) - httpClient := HttpClient(debug) + // Create HTTP client. + httpClient := HttpClient(insecure) + + // Set OIDC Relying Party client options. clientOpt := []rp.Option{ rp.WithHTTPClient(httpClient), rp.WithCookieHandler(cookieHandler), @@ -62,77 +72,80 @@ func NewClient(oidcUri *url.URL, oidcClient, oidcSecret, oidcScopes, siteUrl str rp.WithIssuedAtOffset(5 * time.Second), ), rp.WithErrorHandler(func(w http.ResponseWriter, r *http.Request, errorType string, errorDesc string, state string) { - log.Debugf("oidc: %s: %s (state: %s)", errorType, errorDesc, state) + event.AuditErr([]string{"oidc", "%s", "%s (state %s)"}, errorType, errorDesc, state) w.WriteHeader(http.StatusInternalServerError) w.Header().Add("oidc_error", fmt.Sprintf("oidc: %s", errorDesc)) }), } - discover, err := client.Discover(oidcUri.String(), httpClient) + // Perform service discovery through the standardized /.well-known/openid-configuration endpoint. + discover, err := client.Discover(issuerUri.String(), httpClient) if err != nil { - log.Debugf("oidc: %q (discover)", err) + event.AuditErr([]string{"oidc", "provider", "service discovery", "%s"}, err) return nil, err } + // If possible, use Proof of Key Code Exchange (PKCE). for _, v := range discover.CodeChallengeMethodsSupported { if v == oidc.CodeChallengeMethodS256 { clientOpt = append(clientOpt, rp.WithPKCE(cookieHandler)) } } + // Set default scopes if no scopes were specified. if oidcScopes == "" { oidcScopes = "openid email profile" } - scopes := strings.Split(strings.TrimSpace(oidcScopes), " ") + event.AuditDebug([]string{"oidc", "provider", "scopes", oidcScopes}) - provider, err := rp.NewRelyingPartyOIDC(oidcUri.String(), oidcClient, oidcSecret, u.String(), scopes, clientOpt...) + // Parse scopes into string slice. + scopes := clean.Scopes(oidcScopes) + + // Create RelyingParty provider. + provider, err := rp.NewRelyingPartyOIDC(issuerUri.String(), oidcClient, oidcSecret, redirectUrl, scopes, clientOpt...) if err != nil { - log.Debugf("oidc: %s (issuer)", err) + event.AuditErr([]string{"oidc", "provider", "%s"}, err) return nil, err } - log.Tracef("oidc: pkce enabled %v", provider.IsPKCE()) + if provider.IsPKCE() { + event.AuditDebug([]string{"oidc", "provider", "pkce", "enabled"}) + } else { + event.AuditDebug([]string{"oidc", "provider", "pkce", "disabled"}) + } + // Return OIDC Client with RelyingParty provider. return &Client{ provider, - debug, + insecure, }, nil } -func state() string { - return rnd.UUID() -} - +// AuthCodeUrlHandler redirects a browser to the login page of the configured OIDC identity provider. func (c *Client) AuthCodeUrlHandler(ctx *gin.Context) { - handle := rp.AuthURLHandler(state, c) + handle := rp.AuthURLHandler(rnd.State, c) handle(ctx.Writer, ctx.Request) } -func (c *Client) CodeExchangeUserInfo(ctx *gin.Context) (userInfo oidc.UserInfo, tokens *oidc.Tokens, err error) { - userinfoClosure := func(w http.ResponseWriter, r *http.Request, t *oidc.Tokens, state string, rp rp.RelyingParty, i oidc.UserInfo) { +// CodeExchangeUserInfo verifies a redirect auth request and returns the user information and tokens if successful. +func (c *Client) CodeExchangeUserInfo(ctx *gin.Context) (userInfo *oidc.UserInfo, tokens *oidc.Tokens[*oidc.IDTokenClaims], err error) { + getInfo := func(w http.ResponseWriter, r *http.Request, t *oidc.Tokens[*oidc.IDTokenClaims], state string, rp rp.RelyingParty, i *oidc.UserInfo) { userInfo = i tokens = t } - /* - You could also just take the access_token and id_token without calling the userinfo endpoint, e.g.: - - tokeninfoClosure := func(w http.ResponseWriter, r *http.Request, tokens *oidc.Tokens, state string, rp rp.RelyingParty) { - log.Infof("IDTOKEN: %q\n\n" , tokens.IDToken) - log.Infof("ACCESSTOKEN: %q\n\n" , tokens.AccessToken) - log.Infof("REFRESHTOKEN: %q\n\n" , tokens.RefreshToken) - */ - - handle := rp.CodeExchangeHandler(rp.UserinfoCallback(userinfoClosure), c) + // It would also be possible to directly get the user info from the oidc.IDTokenClaims + // without performing a request to the userinfo endpoint of the OIDC identity provider. + handle := rp.CodeExchangeHandler(rp.UserinfoCallback(getInfo), c) handle(ctx.Writer, ctx.Request) if sc := ctx.Writer.Status(); sc != 0 && sc != http.StatusOK { if oidcErr := ctx.Writer.Header().Get("oidc_error"); oidcErr == "" { - return userInfo, tokens, errors.New("tailed to exchange the authentication code and retrieve the user information") + return userInfo, tokens, errors.New("failed to exchange token for user info") } else { return userInfo, tokens, errors.New(oidcErr) } diff --git a/internal/auth/oidc/client_test.go b/internal/auth/oidc/client_test.go new file mode 100644 index 000000000..4fcfd7e9b --- /dev/null +++ b/internal/auth/oidc/client_test.go @@ -0,0 +1,45 @@ +package oidc + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNewClient(t *testing.T) { + t.Run("Prod", func(t *testing.T) { + uri, err := url.Parse("http://dummy-oidc:9998") + + assert.NoError(t, err) + + client, err := NewClient( + uri, + "csg6yqvykh0780f9", + "nd09wkee0ElsMvzLGkgWS9wJAttHwF2h", + "openid email profile", + "https://app.localssl.dev/", + false, + ) + + assert.Error(t, err) + assert.Nil(t, client) + }) + t.Run("Debug", func(t *testing.T) { + uri, err := url.Parse("http://dummy-oidc:9998") + + assert.NoError(t, err) + + client, err := NewClient( + uri, + "csg6yqvykh0780f9", + "nd09wkee0ElsMvzLGkgWS9wJAttHwF2h", + "openid email profile", + "https://app.localssl.dev/", + true, + ) + + assert.NoError(t, err) + assert.IsType(t, &Client{}, client) + }) +} diff --git a/internal/auth/oidc/helper.go b/internal/auth/oidc/helper.go deleted file mode 100644 index 00f3da953..000000000 --- a/internal/auth/oidc/helper.go +++ /dev/null @@ -1,63 +0,0 @@ -package oidc - -import ( - "errors" - "strings" -) - -type UserInfo interface { - GetPreferredUsername() string - GetNickname() string - GetName() string - GetEmail() string - GetClaim(key string) interface{} -} - -func UsernameFromUserInfo(userinfo UserInfo) (username string) { - if len(userinfo.GetPreferredUsername()) >= 4 { - username = userinfo.GetPreferredUsername() - } else if len(userinfo.GetNickname()) >= 4 { - username = userinfo.GetNickname() - } else if len(userinfo.GetName()) >= 4 { - username = strings.ReplaceAll(strings.ToLower(userinfo.GetName()), " ", "-") - } else if len(userinfo.GetEmail()) >= 4 { - username = userinfo.GetEmail() - } else { - log.Debug("oidc: no username found") - } - return username -} - -// HasRoleAdmin searches UserInfo claims for admin role. -// Returns true if role is present or false if claim was found but no role in there. -// Error will be returned if the role claim is not delivered at all. -func HasRoleAdmin(userinfo UserInfo) (bool, error) { - claim := userinfo.GetClaim(RoleClaim) - return claimContainsProp(claim, AdminRole) -} - -func claimContainsProp(claim interface{}, property string) (bool, error) { - switch t := claim.(type) { - case nil: - return false, errors.New("oidc: claim not found") - case []interface{}: - for _, value := range t { - res, err := claimContainsProp(value, property) - if err != nil { - return false, err - } - if res { - return res, nil - } - } - return false, nil - case interface{}: - if value, ok := t.(string); ok { - return value == property, nil - } else { - return false, errors.New("oidc: unexpected type") - } - default: - return false, errors.New("oidc: unexpected type") - } -} diff --git a/internal/auth/oidc/helper_test.go b/internal/auth/oidc/helper_test.go deleted file mode 100644 index b3128facc..000000000 --- a/internal/auth/oidc/helper_test.go +++ /dev/null @@ -1,95 +0,0 @@ -package oidc - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestUsernameFromUserInfo(t *testing.T) { - t.Run("PreferredUsername", func(t *testing.T) { - u := &userinfo{PreferredUsername: "testfest"} - assert.Equal(t, "testfest", UsernameFromUserInfo(u)) - }) - t.Run("PreferredUsername too short", func(t *testing.T) { - u := &userinfo{PreferredUsername: "tes"} - assert.Equal(t, "", UsernameFromUserInfo(u)) - }) - t.Run("EMail", func(t *testing.T) { - u := &userinfo{Nickname: "tes", Email: "hello@world.com"} - assert.Equal(t, "hello@world.com", UsernameFromUserInfo(u)) - }) - t.Run("Nickname", func(t *testing.T) { - u := &userinfo{Nickname: "testofesto", Email: "hel"} - assert.Equal(t, "testofesto", UsernameFromUserInfo(u)) - }) - t.Run("Name", func(t *testing.T) { - u := &userinfo{Name: "Jane Doe", Email: "hello@world.com"} - assert.Equal(t, "jane-doe", UsernameFromUserInfo(u)) - }) -} - -func TestHasRoleAdmin(t *testing.T) { - t.Run("true case", func(t *testing.T) { - u := &userinfo{Claim: []interface{}{ - "admin", - "photoprism_admin", - "photoprism", - "random", - }} - hasRoleAdmin, err := HasRoleAdmin(u) - assert.True(t, hasRoleAdmin) - assert.Nil(t, err) - }) - t.Run("false case", func(t *testing.T) { - u := &userinfo{Claim: []interface{}{ - "admin", - "photoprismo_admin", - "photoprism", - "random", - }} - hasRoleAdmin, err := HasRoleAdmin(u) - assert.False(t, hasRoleAdmin) - assert.Nil(t, err) - }) - t.Run("false case 2", func(t *testing.T) { - u := &userinfo{Claim: []interface{}{}} - hasRoleAdmin, err := HasRoleAdmin(u) - assert.False(t, hasRoleAdmin) - assert.Nil(t, err) - }) - t.Run("error case", func(t *testing.T) { - u := &userinfo{Claim: nil} - hasRoleAdmin, err := HasRoleAdmin(u) - assert.False(t, hasRoleAdmin) - assert.Error(t, err) - }) -} - -type userinfo struct { - PreferredUsername string - Nickname string - Name string - Email string - Claim interface{} -} - -func (u *userinfo) GetPreferredUsername() string { - return u.PreferredUsername -} - -func (u *userinfo) GetNickname() string { - return u.Nickname -} - -func (u *userinfo) GetName() string { - return u.Name -} - -func (u *userinfo) GetEmail() string { - return u.Email -} - -func (u *userinfo) GetClaim(key string) interface{} { - return u.Claim -} diff --git a/internal/auth/oidc/http_client.go b/internal/auth/oidc/http_client.go index 1e93a8683..b18491dc8 100644 --- a/internal/auth/oidc/http_client.go +++ b/internal/auth/oidc/http_client.go @@ -3,6 +3,8 @@ package oidc import ( "net/http" "time" + + "github.com/photoprism/photoprism/internal/event" ) // HttpClient represents a client that makes HTTP requests. @@ -16,11 +18,11 @@ func HttpClient(debug bool) *http.Client { if debug { return &http.Client{ Transport: LoggingRoundTripper{http.DefaultTransport}, - Timeout: time.Second * 20, + Timeout: time.Second * 30, } } - return &http.Client{Timeout: 20 * time.Second} + return &http.Client{Timeout: 30 * time.Second} } // LoggingRoundTripper specifies the http.RoundTripper interface. @@ -28,15 +30,16 @@ type LoggingRoundTripper struct { proxy http.RoundTripper } +// RoundTrip logs the request method, URL and error, if any. func (lrt LoggingRoundTripper) RoundTrip(req *http.Request) (res *http.Response, err error) { - log.Tracef("oidc: %s %s", req.Method, req.URL.String()) - - // Send request. + // Perform HTTP request. res, err = lrt.proxy.RoundTrip(req) - // Log error, if any. + // Log the request method, URL and error, if any. if err != nil { - log.Debugf("oidc: request to %s has failed (%s)", req.URL.String(), err) + event.AuditErr([]string{"oidc", "provider", "request", "%s %s", "%s"}, req.Method, req.URL.String(), err) + } else { + event.AuditDebug([]string{"oidc", "provider", "request", "%s %s", "%s"}, req.Method, req.URL.String(), res.Status) } return res, err diff --git a/internal/auth/oidc/http_client_test.go b/internal/auth/oidc/http_client_test.go index b0ef91109..d19b7549a 100644 --- a/internal/auth/oidc/http_client_test.go +++ b/internal/auth/oidc/http_client_test.go @@ -20,7 +20,7 @@ func TestHttpClient(t *testing.T) { assert.IsType(t, LoggingRoundTripper{}, client.Transport) }) t.Run("GetRequest", func(t *testing.T) { - req, err := http.NewRequest("GET", "https://www.photoprism.app/", nil) + req, err := http.NewRequest("GET", "https://accounts.google.com/.well-known/openid-configuration", nil) assert.Nil(t, err) rt := LoggingRoundTripper{http.DefaultTransport} _, err = rt.RoundTrip(req) diff --git a/internal/auth/oidc/redirect_url.go b/internal/auth/oidc/redirect_url.go new file mode 100644 index 000000000..5e327546a --- /dev/null +++ b/internal/auth/oidc/redirect_url.go @@ -0,0 +1,26 @@ +package oidc + +import ( + "errors" + "net/url" + "path" + + "github.com/photoprism/photoprism/internal/config" +) + +// RedirectURL returns the redirect URL for authentication via OIDC based on the specified site URL. +func RedirectURL(siteUrl string) (string, error) { + if siteUrl == "" { + return "", errors.New("site url required") + } + + u, err := url.Parse(siteUrl) + + if err != nil { + return "", err + } + + u.Path = path.Join(u.Path, config.OidcRedirectUri) + + return u.String(), nil +} diff --git a/internal/auth/oidc/username.go b/internal/auth/oidc/username.go new file mode 100644 index 000000000..4e059390e --- /dev/null +++ b/internal/auth/oidc/username.go @@ -0,0 +1,56 @@ +package oidc + +import ( + "github.com/zitadel/oidc/v2/pkg/oidc" + + "github.com/photoprism/photoprism/pkg/authn" + "github.com/photoprism/photoprism/pkg/clean" +) + +// Username returns the preferred username based on the userinfo and the preferred username OIDC claim. +func Username(userInfo *oidc.UserInfo, preferredClaim string) (userName string) { + switch preferredClaim { + case authn.ClaimName: + if name := clean.Handle(userInfo.Name); len(name) > 0 { + userName = name + } else if name = clean.Handle(userInfo.PreferredUsername); len(name) > 0 { + userName = name + } else if name = clean.Handle(userInfo.Nickname); len(name) > 0 { + userName = name + } else if name = clean.Email(userInfo.Email); userInfo.EmailVerified && len(name) > 4 { + userName = name + } + case authn.ClaimNickname: + if name := clean.Handle(userInfo.Nickname); len(name) > 0 { + userName = name + } else if name = clean.Handle(userInfo.PreferredUsername); len(name) > 0 { + userName = name + } else if name = clean.Handle(userInfo.Name); len(name) > 0 { + userName = name + } else if name = clean.Email(userInfo.Email); userInfo.EmailVerified && len(name) > 4 { + userName = name + } + case authn.ClaimEmail: + if name := clean.Email(userInfo.Email); userInfo.EmailVerified && len(name) > 4 { + userName = name + } else if name = clean.Handle(userInfo.PreferredUsername); len(name) > 0 { + userName = name + } else if name = clean.Handle(userInfo.Name); len(name) > 0 { + userName = name + } else if name = clean.Handle(userInfo.Nickname); len(name) > 0 { + userName = name + } + default: + if name := clean.Handle(userInfo.PreferredUsername); len(name) > 0 { + userName = name + } else if name = clean.Handle(userInfo.Name); len(name) > 0 { + userName = name + } else if name = clean.Handle(userInfo.Nickname); len(name) > 0 { + userName = name + } else if name = clean.Email(userInfo.Email); userInfo.EmailVerified && len(name) > 4 { + userName = name + } + } + + return userName +} diff --git a/internal/auth/oidc/username_test.go b/internal/auth/oidc/username_test.go new file mode 100644 index 000000000..0935d76a8 --- /dev/null +++ b/internal/auth/oidc/username_test.go @@ -0,0 +1,69 @@ +package oidc + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/zitadel/oidc/v2/pkg/oidc" + + "github.com/photoprism/photoprism/pkg/authn" +) + +func TestUsername(t *testing.T) { + t.Run("ClaimPreferredUsername", func(t *testing.T) { + info := &oidc.UserInfo{} + info.Name = "Jane Doe" + info.GivenName = "Jane" + info.FamilyName = "Doe" + info.Email = "jane@doe.com" + info.EmailVerified = true + info.Subject = "e3a9f4a6-9d60-47cb-9bf5-02bd15b0c68d" + info.PreferredUsername = "Jane Doe" + result := Username(info, authn.ClaimPreferredUsername) + assert.Equal(t, "jane.doe", result) + }) + t.Run("ClaimPreferredUsernameMissing", func(t *testing.T) { + info := &oidc.UserInfo{} + info.Name = "Jane Doe" + info.FamilyName = "Doe" + info.Email = "jane@doe.com" + info.EmailVerified = true + result := Username(info, authn.ClaimPreferredUsername) + assert.Equal(t, "jane.doe", result) + }) + t.Run("ClaimName", func(t *testing.T) { + info := &oidc.UserInfo{} + info.Name = "Jane Doe" + info.GivenName = "Jane" + info.FamilyName = "Doe" + info.Nickname = "Jens Mander" + info.Email = "jane@doe.com" + info.EmailVerified = true + info.Subject = "abcd123" + result := Username(info, authn.ClaimName) + assert.Equal(t, "jane.doe", result) + }) + t.Run("ClaimNickname", func(t *testing.T) { + info := &oidc.UserInfo{} + info.Name = "Jane Doe" + info.GivenName = "Jane" + info.FamilyName = "Doe" + info.Nickname = "Jens Mander" + info.Email = "jane@doe.com" + info.EmailVerified = true + info.Subject = "abcd123" + result := Username(info, authn.ClaimNickname) + assert.Equal(t, "jens.mander", result) + }) + t.Run("ClaimEmail", func(t *testing.T) { + info := &oidc.UserInfo{} + info.Name = "Jane Doe" + info.GivenName = "Jane" + info.FamilyName = "Doe" + info.Email = "jane@doe.com" + info.EmailVerified = true + info.Subject = "abcd123" + result := Username(info, authn.ClaimEmail) + assert.Equal(t, "jane@doe.com", result) + }) +} diff --git a/internal/entity/auth_user.go b/internal/entity/auth_user.go index 8801aea55..ac8fdfff8 100644 --- a/internal/entity/auth_user.go +++ b/internal/entity/auth_user.go @@ -10,7 +10,7 @@ import ( "github.com/jinzhu/gorm" "github.com/ulule/deepcopier" - "github.com/zitadel/oidc/pkg/oidc" + "github.com/zitadel/oidc/v2/pkg/oidc" "github.com/photoprism/photoprism/internal/auth/acl" "github.com/photoprism/photoprism/internal/event" @@ -105,64 +105,17 @@ func NewUser() (m *User) { } // OidcUser creates a new OIDC user entity. -func OidcUser(userInfo oidc.UserInfo, usernameClaim string) User { - var userName, userEmail string +func OidcUser(userInfo *oidc.UserInfo, userName string) User { + authId := clean.Auth(userInfo.Subject) - switch usernameClaim { - case authn.ClaimName: - if name := clean.Handle(userInfo.GetName()); len(name) > 0 { - userName = name - } else if name = clean.Handle(userInfo.GetPreferredUsername()); len(name) > 0 { - userName = name - } else if name = clean.Handle(userInfo.GetNickname()); len(name) > 0 { - userName = name - } else if name = clean.Email(userInfo.GetEmail()); userInfo.IsEmailVerified() && len(name) > 4 { - userName = name - } - case authn.ClaimNickname: - if name := clean.Handle(userInfo.GetNickname()); len(name) > 0 { - userName = name - } else if name = clean.Handle(userInfo.GetPreferredUsername()); len(name) > 0 { - userName = name - } else if name = clean.Handle(userInfo.GetName()); len(name) > 0 { - userName = name - } else if name = clean.Email(userInfo.GetEmail()); userInfo.IsEmailVerified() && len(name) > 4 { - userName = name - } - case authn.ClaimEmail: - if name := clean.Email(userInfo.GetEmail()); userInfo.IsEmailVerified() && len(name) > 4 { - userName = name - } else if name = clean.Handle(userInfo.GetPreferredUsername()); len(name) > 0 { - userName = name - } else if name = clean.Handle(userInfo.GetName()); len(name) > 0 { - userName = name - } else if name = clean.Handle(userInfo.GetNickname()); len(name) > 0 { - userName = name - } - default: - if name := clean.Handle(userInfo.GetPreferredUsername()); len(name) > 0 { - userName = name - } else if name = clean.Handle(userInfo.GetName()); len(name) > 0 { - userName = name - } else if name = clean.Handle(userInfo.GetNickname()); len(name) > 0 { - userName = name - } else if name = clean.Email(userInfo.GetEmail()); userInfo.IsEmailVerified() && len(name) > 4 { - userName = name - } - } - - userEmail = clean.Email(userInfo.GetEmail()) - - authId := clean.Auth(userInfo.GetSubject()) - - if userName == "" || authId == "" { + if authId == "" { return User{} } return User{ - DisplayName: userInfo.GetName(), UserName: userName, - UserEmail: userEmail, + DisplayName: userInfo.Name, + UserEmail: clean.Email(userInfo.Email), AuthID: authId, AuthProvider: authn.ProviderOIDC.String(), } diff --git a/internal/entity/auth_user_test.go b/internal/entity/auth_user_test.go index fc6c5ac64..a44d2e22e 100644 --- a/internal/entity/auth_user_test.go +++ b/internal/entity/auth_user_test.go @@ -4,9 +4,8 @@ import ( "testing" "time" - "github.com/zitadel/oidc/pkg/oidc" - "github.com/stretchr/testify/assert" + "github.com/zitadel/oidc/v2/pkg/oidc" "github.com/photoprism/photoprism/internal/auth/acl" "github.com/photoprism/photoprism/internal/form" @@ -22,77 +21,59 @@ func TestNewUser(t *testing.T) { } func TestOidcUser(t *testing.T) { - t.Run("ClaimPreferredUsername", func(t *testing.T) { - info := oidc.NewUserInfo() - info.SetName("Jane Doe") - info.SetGivenName("Jane") - info.SetFamilyName("Doe") - info.SetEmail("jane@doe.com", true) - info.SetSubject("abcd123") - info.SetPreferredUsername("Jane Doe") - m := OidcUser(info, authn.ClaimPreferredUsername) + t.Run("Success", func(t *testing.T) { + info := &oidc.UserInfo{} + info.Name = "Jane Doe" + info.GivenName = "Jane" + info.FamilyName = "Doe" + info.Email = "jane@doe.com" + info.EmailVerified = true + info.Subject = "e3a9f4a6-9d60-47cb-9bf5-02bd15b0c68d" + info.PreferredUsername = "Jane Doe" + + m := OidcUser(info, "jane.doe") assert.Equal(t, "oidc", m.AuthProvider) - assert.Equal(t, "abcd123", m.AuthID) + assert.Equal(t, "e3a9f4a6-9d60-47cb-9bf5-02bd15b0c68d", m.AuthID) assert.Equal(t, "jane@doe.com", m.UserEmail) assert.Equal(t, "jane.doe", m.UserName) assert.Equal(t, "Jane Doe", m.DisplayName) }) - t.Run("ClaimName", func(t *testing.T) { - info := oidc.NewUserInfo() - info.SetName("Jane Doe") - info.SetGivenName("Jane") - info.SetFamilyName("Doe") - info.SetNickname("Jens Mander") - info.SetEmail("jane@doe.com", true) - info.SetSubject("abcd123") - m := OidcUser(info, authn.ClaimName) + t.Run("NoUsername", func(t *testing.T) { + info := &oidc.UserInfo{} + info.Name = "Jane Doe" + info.GivenName = "Jane" + info.FamilyName = "Doe" + info.Email = "jane@doe.com" + info.EmailVerified = true + info.Subject = "e3a9f4a6-9d60-47cb-9bf5-02bd15b0c68d" + info.PreferredUsername = "Jane Doe" + + m := OidcUser(info, "") assert.Equal(t, "oidc", m.AuthProvider) - assert.Equal(t, "abcd123", m.AuthID) + assert.Equal(t, "e3a9f4a6-9d60-47cb-9bf5-02bd15b0c68d", m.AuthID) assert.Equal(t, "jane@doe.com", m.UserEmail) - assert.Equal(t, "jane.doe", m.UserName) + assert.Equal(t, "", m.UserName) assert.Equal(t, "Jane Doe", m.DisplayName) }) - t.Run("ClaimNickname", func(t *testing.T) { - info := oidc.NewUserInfo() - info.SetName("Jane Doe") - info.SetGivenName("Jane") - info.SetFamilyName("Doe") - info.SetNickname("Jens Mander") - info.SetEmail("jane@doe.com", true) - info.SetSubject("abcd123") - m := OidcUser(info, authn.ClaimNickname) + t.Run("NoSubject", func(t *testing.T) { + info := &oidc.UserInfo{} + info.Name = "Jane Doe" + info.GivenName = "Jane" + info.FamilyName = "Doe" + info.Nickname = "Jens Mander" + info.Email = "jane@doe.com" + info.EmailVerified = true + info.Subject = "" - assert.Equal(t, "oidc", m.AuthProvider) - assert.Equal(t, "abcd123", m.AuthID) - assert.Equal(t, "jane@doe.com", m.UserEmail) - assert.Equal(t, "jens.mander", m.UserName) - assert.Equal(t, "Jane Doe", m.DisplayName) - }) - t.Run("ClaimEmail", func(t *testing.T) { - info := oidc.NewUserInfo() - info.SetName("Jane Doe") - info.SetGivenName("Jane") - info.SetFamilyName("Doe") - info.SetEmail("jane@doe.com", true) - info.SetSubject("abcd123") - m := OidcUser(info, authn.ClaimEmail) + m := OidcUser(info, "jane.doe") - assert.Equal(t, "oidc", m.AuthProvider) - assert.Equal(t, "abcd123", m.AuthID) - assert.Equal(t, "jane@doe.com", m.UserEmail) - assert.Equal(t, "jane@doe.com", m.UserName) - assert.Equal(t, "Jane Doe", m.DisplayName) - }) - t.Run("EmptyAuthId", func(t *testing.T) { - info := oidc.NewUserInfo() - info.SetName("Jane") - info.SetFamilyName("Doe") - info.SetEmail("jane@doe.com", true) - m := OidcUser(info, authn.ClaimPreferredUsername) - - assert.Empty(t, m) + assert.Equal(t, "", m.AuthProvider) + assert.Equal(t, "", m.AuthID) + assert.Equal(t, "", m.UserEmail) + assert.Equal(t, "", m.UserName) + assert.Equal(t, "", m.DisplayName) }) } diff --git a/internal/photoprism/get/get.go b/internal/photoprism/get/get.go index 566c62205..d0a65e45b 100644 --- a/internal/photoprism/get/get.go +++ b/internal/photoprism/get/get.go @@ -23,3 +23,7 @@ Additional information can be found in our Developer Guide: */ package get + +import "github.com/photoprism/photoprism/internal/event" + +var log = event.Log diff --git a/internal/photoprism/get/oidc.go b/internal/photoprism/get/oidc.go index 670bca2a5..103d4aa62 100644 --- a/internal/photoprism/get/oidc.go +++ b/internal/photoprism/get/oidc.go @@ -15,7 +15,7 @@ func initOidc() { Config().OIDCSecret(), Config().OIDCScopes(), Config().SiteUrl(), - Config().Debug(), + false, ) } diff --git a/internal/photoprism/get/services.go b/internal/photoprism/get/services.go index 6ac6ba1b5..d70e826cb 100644 --- a/internal/photoprism/get/services.go +++ b/internal/photoprism/get/services.go @@ -64,7 +64,7 @@ var services struct { func SetConfig(c *config.Config) { if c == nil { - panic("config is nil") + log.Panic("panic: argument is nil in get.SetConfig(c *config.Config)") } conf = c @@ -74,7 +74,7 @@ func SetConfig(c *config.Config) { func Config() *config.Config { if conf == nil { - panic("config is nil") + log.Panic("panic: conf is nil in get.Config()") } return conf diff --git a/pkg/authn/const.go b/pkg/authn/const.go index 06713cf75..e4c698527 100644 --- a/pkg/authn/const.go +++ b/pkg/authn/const.go @@ -2,6 +2,7 @@ package authn // Generic status messages for authentication and authorization: const ( + Failed = "failed" Denied = "denied" Granted = "granted" Created = "created" diff --git a/pkg/clean/scope.go b/pkg/clean/scope.go index 81d92466f..a42a1c033 100644 --- a/pkg/clean/scope.go +++ b/pkg/clean/scope.go @@ -14,3 +14,12 @@ func Scope(s string) string { return list.ParseAttr(strings.ToLower(s)).String() } + +// Scopes sanitizes authentication scope identifiers and returns them as string slice. +func Scopes(s string) []string { + if s == "" { + return []string{} + } + + return list.ParseAttr(strings.ToLower(s)).Strings() +} diff --git a/pkg/clean/scope_test.go b/pkg/clean/scope_test.go index 36c122c53..f4f100b1f 100644 --- a/pkg/clean/scope_test.go +++ b/pkg/clean/scope_test.go @@ -20,3 +20,18 @@ func TestScope(t *testing.T) { assert.Equal(t, "*", q) }) } + +func TestScopes(t *testing.T) { + t.Run("Empty", func(t *testing.T) { + q := Scopes("") + assert.Equal(t, []string{}, q) + }) + t.Run("Sanitized", func(t *testing.T) { + q := Scopes(" foo:BAR webdav openid metrics !") + assert.Equal(t, []string{"foo:bar", "metrics", "openid", "webdav"}, q) + }) + t.Run("All", func(t *testing.T) { + q := Scopes("*") + assert.Equal(t, []string{"*"}, q) + }) +} diff --git a/pkg/list/attributes.go b/pkg/list/attributes.go index f0b46a443..77b0b9648 100644 --- a/pkg/list/attributes.go +++ b/pkg/list/attributes.go @@ -26,6 +26,11 @@ func ParseAttr(s string) Attr { // String returns the attributes as string. func (list Attr) String() string { + return strings.Join(list.Strings(), " ") +} + +// Strings returns the attributes as string slice. +func (list Attr) Strings() []string { result := make([]string, 0, len(list)) list.Sort() @@ -55,7 +60,7 @@ func (list Attr) String() string { i++ } - return strings.Join(result, " ") + return result } // Sort sorts the attributes by key. diff --git a/pkg/rnd/uuid.go b/pkg/rnd/uuid.go index 8db66d4b6..f31a100ef 100644 --- a/pkg/rnd/uuid.go +++ b/pkg/rnd/uuid.go @@ -10,3 +10,8 @@ import ( func UUID() string { return uuid.NewString() } + +// State is an alias for UUID for use in the context of OpenID Connect (OIDC). +func State() string { + return UUID() +}