diff --git a/backend/internal/appconfig/login_notification.go b/backend/internal/appconfig/legacy.go similarity index 52% rename from backend/internal/appconfig/login_notification.go rename to backend/internal/appconfig/legacy.go index 62b2d9e8..a6d45059 100644 --- a/backend/internal/appconfig/login_notification.go +++ b/backend/internal/appconfig/legacy.go @@ -2,19 +2,13 @@ package appconfig import "encoding/json" -const ( - LoginNotificationDisabled = "disabled" - LoginNotificationAlways = "always" - LoginNotificationIPAndUserAgent = "ipAndUserAgent" - LoginNotificationBrowserRecognition = "browserRecognition" -) - -// UnmarshalJSON preserves the notification policy when loading state written before notification modes existed +// UnmarshalJSON preserves settings stored in actor state before boolean toggles were replaced by modes func (m *AppConfigModel) UnmarshalJSON(data []byte) error { type config AppConfigModel value := struct { *config - LegacyNotificationEnabled AppConfigValue `json:"emailLoginNotificationEnabled"` + LegacyNotificationEnabled AppConfigValue `json:"emailLoginNotificationEnabled"` + LegacyEmailVerificationEnabled AppConfigValue `json:"emailVerificationEnabled"` }{config: (*config)(m)} if err := json.Unmarshal(data, &value); err != nil { return err @@ -22,6 +16,9 @@ func (m *AppConfigModel) UnmarshalJSON(data []byte) error { if m.EmailLoginNotificationMode == "" && value.LegacyNotificationEnabled != "" { m.EmailLoginNotificationMode = legacyLoginNotificationMode(value.LegacyNotificationEnabled) } + if m.EmailVerification == "" && value.LegacyEmailVerificationEnabled != "" { + m.EmailVerification = legacyEmailVerification(value.LegacyEmailVerificationEnabled) + } return nil } @@ -31,3 +28,10 @@ func legacyLoginNotificationMode(enabled AppConfigValue) AppConfigValue { } return LoginNotificationDisabled } + +func legacyEmailVerification(enabled AppConfigValue) AppConfigValue { + if enabled.IsTrue() { + return EmailVerificationOptional + } + return EmailVerificationDisabled +} diff --git a/backend/internal/appconfig/migration.go b/backend/internal/appconfig/migration.go index 126ea0da..38119c21 100644 --- a/backend/internal/appconfig/migration.go +++ b/backend/internal/appconfig/migration.go @@ -62,6 +62,9 @@ func fromLegacyConfig(legacyCfg map[string]string) (*AppConfigModel, error) { if legacyCfg["emailLoginNotificationMode"] == "" { dest.EmailLoginNotificationMode = legacyLoginNotificationMode(AppConfigValue(legacyCfg["emailLoginNotificationEnabled"])) } + if legacyCfg["emailVerification"] == "" { + dest.EmailVerification = legacyEmailVerification(AppConfigValue(legacyCfg["emailVerificationEnabled"])) + } rt := reflect.ValueOf(dest).Elem().Type() rv := reflect.ValueOf(dest).Elem() diff --git a/backend/internal/appconfig/model.go b/backend/internal/appconfig/model.go index bdb88793..d765ae0b 100644 --- a/backend/internal/appconfig/model.go +++ b/backend/internal/appconfig/model.go @@ -13,6 +13,19 @@ import ( "github.com/pocket-id/pocket-id/backend/internal/dto" ) +const ( + LoginNotificationDisabled = "disabled" + LoginNotificationAlways = "always" + LoginNotificationIPAndUserAgent = "ipAndUserAgent" + LoginNotificationBrowserRecognition = "browserRecognition" +) + +const ( + EmailVerificationDisabled = "disabled" + EmailVerificationOptional = "optional" + EmailVerificationRequired = "required" +) + type AppConfigModel struct { // General AppName AppConfigValue `json:"appName" env:"APP_NAME" public:"true"` @@ -39,7 +52,7 @@ type AppConfigModel struct { EmailOneTimeAccessAsUnauthenticatedEnabled AppConfigValue `json:"emailOneTimeAccessAsUnauthenticatedEnabled" env:"EMAIL_ONE_TIME_ACCESS_AS_UNAUTHENTICATED_ENABLED" type:"bool" public:"true"` EmailOneTimeAccessAsAdminEnabled AppConfigValue `json:"emailOneTimeAccessAsAdminEnabled" env:"EMAIL_ONE_TIME_ACCESS_AS_ADMIN_ENABLED" type:"bool" public:"true"` EmailApiKeyExpirationEnabled AppConfigValue `json:"emailApiKeyExpirationEnabled" env:"EMAIL_API_KEY_EXPIRATION_ENABLED" type:"bool"` - EmailVerificationEnabled AppConfigValue `json:"emailVerificationEnabled" env:"EMAIL_VERIFICATION_ENABLED" type:"bool" public:"true"` + EmailVerification AppConfigValue `json:"emailVerification" env:"EMAIL_VERIFICATION" public:"true"` // LDAP LdapEnabled AppConfigValue `json:"ldapEnabled" env:"LDAP_ENABLED" type:"bool" public:"true"` LdapUrl AppConfigValue `json:"ldapUrl" env:"LDAP_URL"` @@ -145,7 +158,7 @@ func getDefaultConfig() *AppConfigModel { EmailOneTimeAccessAsUnauthenticatedEnabled: "false", EmailOneTimeAccessAsAdminEnabled: "false", EmailApiKeyExpirationEnabled: "false", - EmailVerificationEnabled: "false", + EmailVerification: EmailVerificationDisabled, // LDAP LdapEnabled: "false", LdapUrl: "", diff --git a/backend/internal/appconfig/model_test.go b/backend/internal/appconfig/model_test.go index 19d3b9a3..d766773e 100644 --- a/backend/internal/appconfig/model_test.go +++ b/backend/internal/appconfig/model_test.go @@ -1,6 +1,7 @@ package appconfig import ( + "encoding/json" "errors" "reflect" "strings" @@ -300,3 +301,27 @@ func TestAppConfigModel_Update(t *testing.T) { assert.Equal(t, "thisKeyDoesNotExist", notFound.field) }) } + +func TestAppConfigModel_UnmarshalJSON(t *testing.T) { + t.Run("maps the legacy email verification toggle to a mode", func(t *testing.T) { + tests := map[string]AppConfigValue{ + "true": EmailVerificationOptional, + "false": EmailVerificationDisabled, + } + for enabled, expected := range tests { + var m AppConfigModel + err := json.Unmarshal([]byte(`{"emailVerificationEnabled":"`+enabled+`"}`), &m) + require.NoError(t, err) + + assert.Equal(t, expected, m.EmailVerification) + } + }) + + t.Run("an explicit email verification mode wins over the legacy toggle", func(t *testing.T) { + var m AppConfigModel + err := json.Unmarshal([]byte(`{"emailVerification":"required","emailVerificationEnabled":"false"}`), &m) + require.NoError(t, err) + + assert.Equal(t, AppConfigValue(EmailVerificationRequired), m.EmailVerification) + }) +} diff --git a/backend/internal/appconfig/service.go b/backend/internal/appconfig/service.go index b4507e83..f9411421 100644 --- a/backend/internal/appconfig/service.go +++ b/backend/internal/appconfig/service.go @@ -247,6 +247,17 @@ func (s *AppConfigService) loadDbConfigFromEnv() (*AppConfigModel, error) { } } + // TODO: Remove in next major version (v3) + // Preserve the old environment setting unless an explicit email verification mode replaces it + if _, ok := os.LookupEnv("EMAIL_VERIFICATION"); !ok { + if enabled, exists := os.LookupEnv("EMAIL_VERIFICATION_ENABLED"); exists { + if enabled != "true" && enabled != "false" { + return nil, errors.New("EMAIL_VERIFICATION_ENABLED must be true or false") + } + dest.EmailVerification = legacyEmailVerification(AppConfigValue(enabled)) + } + } + // Validate the resolved configuration before exposing values to the rest of the application err := validateEnvConfig(dest) if err != nil { diff --git a/backend/internal/appconfig/service_test.go b/backend/internal/appconfig/service_test.go index 0cc6838c..0c1cc173 100644 --- a/backend/internal/appconfig/service_test.go +++ b/backend/internal/appconfig/service_test.go @@ -462,6 +462,37 @@ func TestService_CIMDURLAllowlist(t *testing.T) { } func TestService_LoadDbConfigFromEnv(t *testing.T) { + t.Run("required verification rejects conflicting settings", func(t *testing.T) { + for _, setting := range []struct{ name, value string }{ + {"EMAILS_VERIFIED", "true"}, + {"REQUIRE_USER_EMAIL", "false"}, + {"SMTP_HOST", ""}, + {"SMTP_FROM", ""}, + } { + t.Run(setting.name, func(t *testing.T) { + t.Setenv("EMAIL_VERIFICATION", "required") + t.Setenv("EMAILS_VERIFIED", "false") + t.Setenv("REQUIRE_USER_EMAIL", "true") + t.Setenv("SMTP_HOST", "smtp.example.com") + t.Setenv("SMTP_FROM", "pocket-id@example.com") + t.Setenv(setting.name, setting.value) + + _, err := (&AppConfigService{}).loadDbConfigFromEnv() + require.ErrorContains(t, err, "EMAIL_VERIFICATION requires user emails, an SMTP host and sender address, and forbids emails verified by default") + }) + } + }) + + t.Run("required verification accepts consistent settings", func(t *testing.T) { + t.Setenv("EMAIL_VERIFICATION", "required") + t.Setenv("SMTP_HOST", "smtp.example.com") + t.Setenv("SMTP_FROM", "pocket-id@example.com") + + cfg, err := (&AppConfigService{}).loadDbConfigFromEnv() + require.NoError(t, err) + assert.Equal(t, AppConfigValue(EmailVerificationRequired), cfg.EmailVerification) + }) + // writeSecretFile writes content to a file in a temporary directory that is removed when the test ends, returning its path writeSecretFile := func(t *testing.T, content string) string { t.Helper() diff --git a/backend/internal/appconfig/testing_unit.go b/backend/internal/appconfig/testing_unit.go index 3df1fa6c..3c94bee3 100644 --- a/backend/internal/appconfig/testing_unit.go +++ b/backend/internal/appconfig/testing_unit.go @@ -3,6 +3,8 @@ // This file contains utils for unit tests and it's only built when the "unit" tag is set package appconfig +import "context" + // NewTestAppConfigService is a function used by tests to create AppConfigService objects with pre-defined configuration values func NewTestAppConfigService(config *AppConfigModel) *AppConfigService { if config == nil { @@ -25,3 +27,12 @@ func NewTestConfig(config *AppConfigModel) *AppConfigModel { return config } + +// StaticResolver is an AppConfigResolver for tests that always returns the same configuration, or the default one when none is set +type StaticResolver struct { + Config *AppConfigModel +} + +func (r StaticResolver) GetConfig(context.Context) (*AppConfigModel, error) { + return NewTestConfig(r.Config), nil +} diff --git a/backend/internal/apperror/constructors.go b/backend/internal/apperror/constructors.go index c621265f..ee9a92af 100644 --- a/backend/internal/apperror/constructors.go +++ b/backend/internal/apperror/constructors.go @@ -308,3 +308,11 @@ func OidcPARRequired() *Error { func InvalidEmailVerificationToken() *Error { return New(CodeEmailVerificationTokenInvalid, http.StatusBadRequest, "Email verification token is invalid") } + +func EmailVerificationRequired() *Error { + return New(CodeEmailVerificationRequired, http.StatusForbidden, "You must verify your email address before you can access this service.") +} + +func EmailChangeNotStarted(cause error) *Error { + return Wrap(cause, CodeEmailChangeNotStarted, http.StatusInternalServerError, "Your other changes were saved, but the email to confirm your new email address could not be sent") +} diff --git a/backend/internal/apperror/error.go b/backend/internal/apperror/error.go index 26ea71ac..75c027f3 100644 --- a/backend/internal/apperror/error.go +++ b/backend/internal/apperror/error.go @@ -35,6 +35,8 @@ const ( CodeDeviceLoginExpired Code = "device_login_expired" CodeReauthenticationRequired Code = "reauthentication_required" CodeEmailVerificationTokenInvalid Code = "invalid_email_verification_token" + CodeEmailVerificationRequired Code = "email_verification_required" + CodeEmailChangeNotStarted Code = "email_change_not_started" CodeSetupNotAvailable Code = "setup_not_available" CodeSetupAlreadyCompleted Code = "setup_already_completed" CodeTokenInvalidOrExpired Code = "token_invalid_or_expired" diff --git a/backend/internal/authz/middleware.go b/backend/internal/authz/middleware.go index 6bcacc13..12e91c69 100644 --- a/backend/internal/authz/middleware.go +++ b/backend/internal/authz/middleware.go @@ -85,6 +85,11 @@ func (m *Middleware) require(scope Scope, optional bool) gin.HandlerFunc { // A valid credential without the scope is forbidden even on optional routes, so a caller is never silently downgraded to anonymous if !principal.Scopes.Has(scope) { c.Abort() + // A scope that is only withheld until the email is verified tells the user how to regain it + if principal.WithheldUntilVerified.Has(scope) { + _ = c.Error(apperror.EmailVerificationRequired()) + return + } _ = c.Error(apperror.MissingScope(string(scope))) return } diff --git a/backend/internal/authz/middleware_test.go b/backend/internal/authz/middleware_test.go index 92655328..221a0d32 100644 --- a/backend/internal/authz/middleware_test.go +++ b/backend/internal/authz/middleware_test.go @@ -15,12 +15,13 @@ import ( // fakeAuthenticator accepts any request carrying its header and resolves to the configured outcome type fakeAuthenticator struct { - kind PrincipalKind - header string - user string - admin bool - err error - calls int + kind PrincipalKind + header string + user string + admin bool + unverified bool + err error + calls int } func (a *fakeAuthenticator) Kind() PrincipalKind { @@ -38,6 +39,9 @@ func (a *fakeAuthenticator) Authenticate(*gin.Context) (*Principal, error) { } principal := &Principal{Kind: a.kind, UserID: a.user, Scopes: UserScopes(a.admin, a.kind)} + if a.unverified { + principal.RestrictUnverified() + } if a.kind == KindSession { principal.AuthenticationMethod = "passkey" principal.AuthenticationTime = time.Unix(1700000000, 0) @@ -136,6 +140,31 @@ func TestMiddlewareAuthorizes(t *testing.T) { }) } +func TestMiddlewareExplainsScopesWithheldUntilVerified(t *testing.T) { + session := &fakeAuthenticator{kind: KindSession, header: "X-Session", user: "session-user", unverified: true} + m := NewMiddleware(session) + + t.Run("allows a scope needed to verify the email", func(t *testing.T) { + result := serve(t, m, AccountWrite, false, map[string]string{"X-Session": "1"}) + + require.Equal(t, http.StatusNoContent, result.status) + }) + + t.Run("asks to verify the email for a withheld scope, even on optional routes", func(t *testing.T) { + for _, optional := range []bool{false, true} { + result := serve(t, m, AccountAuthorize, optional, map[string]string{"X-Session": "1"}) + + require.True(t, apperror.IsCode(result.err, apperror.CodeEmailVerificationRequired), "optional=%v", optional) + } + }) + + t.Run("reports a scope the role never grants as missing", func(t *testing.T) { + result := serve(t, m, UsersRead, false, map[string]string{"X-Session": "1"}) + + require.True(t, apperror.IsCode(result.err, apperror.CodeForbidden)) + }) +} + func TestMiddlewareFallsThroughInvalidCredentials(t *testing.T) { invalidSession := &fakeAuthenticator{kind: KindSession, header: "X-Session", err: apperror.NotSignedIn()} apiKey := &fakeAuthenticator{kind: KindAPIKey, header: "X-Key", user: "key-user"} diff --git a/backend/internal/authz/principal.go b/backend/internal/authz/principal.go index 28dd2529..4b49857b 100644 --- a/backend/internal/authz/principal.go +++ b/backend/internal/authz/principal.go @@ -14,11 +14,29 @@ type Principal struct { UserID string Scopes ScopeSet + // WithheldUntilVerified are the scopes the user's role grants but that are withheld until the user verifies their email + WithheldUntilVerified ScopeSet + // AuthenticationMethod and AuthenticationTime describe how the session was established and are only set for KindSession AuthenticationMethod string AuthenticationTime time.Time } +// RestrictUnverified withholds every scope except those a user needs to verify their email, such as signing out or correcting the address +func (p *Principal) RestrictUnverified() { + kept := make(ScopeSet, len(unverifiedScopes)) + withheld := make(ScopeSet, len(p.Scopes)) + for scope := range p.Scopes { + if unverifiedScopes.Has(scope) { + kept[scope] = struct{}{} + } else { + withheld[scope] = struct{}{} + } + } + p.Scopes = kept + p.WithheldUntilVerified = withheld +} + // PrincipalFrom returns the principal the authorization middleware attached to the request // Anonymous requests on optional and public routes get the zero Principal, whose UserID is empty func PrincipalFrom(c *gin.Context) Principal { diff --git a/backend/internal/authz/router.go b/backend/internal/authz/router.go index 0442e677..a535b5e2 100644 --- a/backend/internal/authz/router.go +++ b/backend/internal/authz/router.go @@ -22,6 +22,11 @@ func (r *Router) Group(relativePath string) *Router { return &Router{group: r.group.Group(relativePath), auth: r.auth, optional: r.optional} } +// Use returns a router whose routes run the handlers before authorization, for example to render its errors in a way the route's clients understand +func (r *Router) Use(handlers ...gin.HandlerFunc) *Router { + return &Router{group: r.group.Group("", handlers...), auth: r.auth, optional: r.optional} +} + // Optional returns a router whose routes let requests without a usable credential through as anonymous // Handlers on these routes must check PrincipalFrom before relying on a signed-in user func (r *Router) Optional() *Router { diff --git a/backend/internal/authz/router_test.go b/backend/internal/authz/router_test.go index 1c2afae3..0326ee2a 100644 --- a/backend/internal/authz/router_test.go +++ b/backend/internal/authz/router_test.go @@ -2,10 +2,13 @@ package authz import ( "net/http" + "net/http/httptest" "testing" "github.com/gin-gonic/gin" "github.com/stretchr/testify/require" + + "github.com/pocket-id/pocket-id/backend/internal/apperror" ) func noop(c *gin.Context) { @@ -25,6 +28,7 @@ func TestIsDeclared(t *testing.T) { api.Group("/nested").Group("/deeper").DELETE("/:id", UsersWrite, noop) api.Optional().GET("/optional", AccountSession, noop) api.Public().POST("/signup", noop) + api.Use(noop).GET("/with-middleware", AccountRead, noop) m.Router(engine.Group("/")).Optional().GET("/authorize", AccountSession, noop) // Register routes past the routers @@ -41,12 +45,33 @@ func TestIsDeclared(t *testing.T) { "DELETE /api/nested/deeper/:id": true, "GET /api/optional": true, "POST /api/signup": true, + "GET /api/with-middleware": true, "GET /authorize": true, "GET /api/forgotten": false, "POST /api/users": false, }, declared) } +func TestRouterUseWrapsAuthorization(t *testing.T) { + gin.SetMode(gin.TestMode) + engine := gin.New() + + // The middleware added with Use runs around authorization, so it sees the error of a refused request and can render it + var seen error + r := NewMiddleware().Router(engine.Group("/api")).Use(func(c *gin.Context) { + c.Next() + if len(c.Errors) > 0 { + seen = c.Errors.Last().Err + } + }) + r.GET("/account", AccountRead, noop) + + recorder := httptest.NewRecorder() + engine.ServeHTTP(recorder, httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/api/account", nil)) + + require.True(t, apperror.IsCode(seen, apperror.CodeNotSignedIn)) +} + func TestRouterRejectsUnknownScopes(t *testing.T) { gin.SetMode(gin.TestMode) r := NewMiddleware().Router(gin.New().Group("/api")) diff --git a/backend/internal/authz/scope.go b/backend/internal/authz/scope.go index edb39cd5..24ccbe54 100644 --- a/backend/internal/authz/scope.go +++ b/backend/internal/authz/scope.go @@ -13,6 +13,7 @@ const ( AccountApps Scope = "account:apps" AccountAuditLogs Scope = "account:audit-logs" AccountSession Scope = "account:session" + AccountAuthorize Scope = "account:authorize" AccountPasskeysEnroll Scope = "account:passkeys:enroll" AccountAPIKeysCreate Scope = "account:api-keys:create" ) @@ -77,6 +78,7 @@ var catalog = []definition{ {AccountApps, CategoryAccount, delegated}, {AccountAuditLogs, CategoryAccount, delegated}, {AccountSession, CategoryAccount, KindSession}, + {AccountAuthorize, CategoryAccount, KindSession}, {AccountPasskeysEnroll, CategoryAccount, KindSession}, {AccountAPIKeysCreate, CategoryAccount, KindSession}, @@ -132,6 +134,9 @@ func (s ScopeSet) Has(scope Scope) bool { return ok } +// unverifiedScopes are the scopes a user keeps while their email still has to be verified +var unverifiedScopes = NewScopeSet(AccountRead, AccountWrite, AccountPasskeys, AccountPasskeysEnroll, AccountSession) + // UserScopes returns the scopes a user holds when authenticated with a credential of the given kind // The admin flag stands in for roles: admins hold every scope and other users hold the account scopes func UserScopes(isAdmin bool, kind PrincipalKind) ScopeSet { diff --git a/backend/internal/authz/scope_test.go b/backend/internal/authz/scope_test.go index 341b0bd0..1530e5eb 100644 --- a/backend/internal/authz/scope_test.go +++ b/backend/internal/authz/scope_test.go @@ -62,6 +62,36 @@ func TestUserScopes(t *testing.T) { }) } +func TestPrincipalRestrictUnverified(t *testing.T) { + t.Run("keeps only the scopes needed to verify the email and withholds the rest", func(t *testing.T) { + principal := &Principal{Kind: KindSession, Scopes: UserScopes(false, KindSession)} + principal.RestrictUnverified() + + require.Equal(t, NewScopeSet(AccountRead, AccountWrite, AccountPasskeys, AccountPasskeysEnroll, AccountSession), principal.Scopes) + require.Equal(t, NewScopeSet(AccountAPIKeys, AccountApps, AccountAuditLogs, AccountAuthorize, AccountAPIKeysCreate), principal.WithheldUntilVerified) + }) + + t.Run("never adds scopes the credential kind cannot hold", func(t *testing.T) { + principal := &Principal{Kind: KindAPIKey, Scopes: UserScopes(false, KindAPIKey)} + principal.RestrictUnverified() + + require.Equal(t, NewScopeSet(AccountRead, AccountWrite, AccountPasskeys), principal.Scopes) + require.False(t, principal.WithheldUntilVerified.Has(AccountAuthorize)) + }) + + t.Run("withholds admin scopes", func(t *testing.T) { + principal := &Principal{Kind: KindSession, Scopes: UserScopes(true, KindSession)} + principal.RestrictUnverified() + + for _, entry := range catalog { + if entry.category == CategoryAdmin { + require.False(t, principal.Scopes.Has(entry.scope), entry.scope) + require.True(t, principal.WithheldUntilVerified.Has(entry.scope), entry.scope) + } + } + }) +} + func TestUnknownScope(t *testing.T) { unknown := Scope("unknown:scope") require.False(t, unknown.Known()) diff --git a/backend/internal/bootstrap/route_coverage_test.go b/backend/internal/bootstrap/route_coverage_test.go index 7b2062c6..57e58b6c 100644 --- a/backend/internal/bootstrap/route_coverage_test.go +++ b/backend/internal/bootstrap/route_coverage_test.go @@ -45,7 +45,7 @@ func TestEveryAPIRouteDeclaresItsAccess(t *testing.T) { } engine := gin.New() - auth := middleware.NewAuthorization(nil, nil, nil) + auth := middleware.NewAuthorization(nil, nil, nil, nil) require.NoError(t, registerRoutes(engine, nil, svc, auth, nil)) // Collect every API route that bypassed the authz routers diff --git a/backend/internal/bootstrap/router_bootstrap.go b/backend/internal/bootstrap/router_bootstrap.go index e5fb8df0..6b3d326a 100644 --- a/backend/internal/bootstrap/router_bootstrap.go +++ b/backend/internal/bootstrap/router_bootstrap.go @@ -40,7 +40,7 @@ func initRouter(db *gorm.DB, svc *services, rateLimitServices map[string]*rateli if err != nil { return nil, err } - auth := middleware.NewAuthorization(svc.apiKeyModule, svc.userService, svc.jwtService) + auth := middleware.NewAuthorization(svc.apiKeyModule, svc.userService, svc.jwtService, svc.appConfigService) err = registerRoutes(r, db, svc, auth, rateLimitServices) if err != nil { return nil, err @@ -171,7 +171,7 @@ func registerRoutes(r *gin.Engine, db *gorm.DB, svc *services, auth *authz.Middl rateLimitMiddleware.Add(middleware.RateLimitDeviceLoginVerification), ) controller.NewOidcController(apiRouter, fileSizeLimitMiddleware, svc.oidcService, svc.appConfigService) - controller.NewUserController(apiRouter, fileSizeLimitMiddleware, svc.appConfigService, svc.userService, svc.webauthnModule, rateLimitMiddleware.Add(middleware.RateLimitUpdateOwnAccount)) + controller.NewUserController(apiRouter, fileSizeLimitMiddleware, svc.appConfigService, svc.userService, svc.webauthnModule, rateLimitMiddleware.Add(middleware.RateLimitSendEmailVerification)) controller.NewAppConfigController(apiRouter, svc.appConfigService, svc.emailModule) svc.ldapSyncModule.RegisterRoutes(apiRouter) controller.NewAppImagesController(apiRouter, fileSizeLimitMiddleware, svc.appImagesService) diff --git a/backend/internal/bootstrap/services_bootstrap.go b/backend/internal/bootstrap/services_bootstrap.go index 4312141a..0a09dd12 100644 --- a/backend/internal/bootstrap/services_bootstrap.go +++ b/backend/internal/bootstrap/services_bootstrap.go @@ -175,6 +175,7 @@ func initServices( Reauth: svc.webauthnModule, AuditLog: svc.auditLogsModule, APIAccess: svc.apiModule, + AppConfig: svc.appConfigService, // Disable in test environment CleanupDisabled: common.EnvConfig.AppEnv.IsTest(), }) @@ -193,7 +194,20 @@ func initServices( } svc.userGroupService = service.NewUserGroupService(db, svc.scimSyncModule, backchannelLogoutService) - svc.userService = service.NewUserService(db, svc.jwtService, svc.customClaimService, svc.appImagesService, svc.scimSyncModule, backchannelLogoutService, fileStorage) + // The user service sends verification links through this module, so it's created first and never depends on the user service itself + svc.emailVerificationModule, err = emailverification.New(emailverification.Dependencies{ + DB: db, + Actors: actors, + EmailSender: svc.emailModule, + ScimSync: svc.scimSyncModule, + AppConfig: svc.appConfigService, + AppURL: common.EnvConfig.AppURL, + }) + if err != nil { + return nil, fmt.Errorf("failed to create email verification module: %w", err) + } + + svc.userService = service.NewUserService(db, svc.jwtService, svc.customClaimService, svc.appImagesService, svc.scimSyncModule, backchannelLogoutService, fileStorage, svc.emailVerificationModule) svc.ldapSyncModule, err = ldapsync.New(ldapsync.Dependencies{ DB: db, @@ -225,13 +239,17 @@ func initServices( } svc.userSignUpModule, err = usersignup.New(usersignup.Dependencies{ - DB: db, - Actors: actors, - Signer: svc.jwtService, - AuditLog: svc.auditLogsModule, - UserCreator: svc.userService, - AppConfig: svc.appConfigService, - ScimSync: svc.scimSyncModule, + DB: db, + Actors: actors, + Signer: svc.jwtService, + AuditLog: svc.auditLogsModule, + UserCreator: svc.userService, + AppConfig: svc.appConfigService, + ScimSync: svc.scimSyncModule, + UserDeleter: svc.userService, + EmailVerification: svc.emailVerificationModule, + // Disable in test environment + CleanupDisabled: common.EnvConfig.AppEnv.IsTest(), }) if err != nil { return nil, fmt.Errorf("failed to create user signup module: %w", err) @@ -250,18 +268,6 @@ func initServices( return nil, fmt.Errorf("failed to create one-time access module: %w", err) } - svc.emailVerificationModule, err = emailverification.New(emailverification.Dependencies{ - DB: db, - Actors: actors, - Users: svc.userService, - EmailSender: svc.emailModule, - AppConfig: svc.appConfigService, - AppURL: common.EnvConfig.AppURL, - }) - if err != nil { - return nil, fmt.Errorf("failed to create email verification module: %w", err) - } - svc.environmentModule = environment.New(environment.Dependencies{ HTTPClient: httpClient, SQLiteOnNetworkedFilesystem: sqliteOnNetworkedFilesystem, diff --git a/backend/internal/controller/upload_size_limit_test.go b/backend/internal/controller/upload_size_limit_test.go index 1a29d1a5..4e1e4b6b 100644 --- a/backend/internal/controller/upload_size_limit_test.go +++ b/backend/internal/controller/upload_size_limit_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "github.com/pocket-id/pocket-id/backend/internal/apikey" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/apperror" "github.com/pocket-id/pocket-id/backend/internal/common" "github.com/pocket-id/pocket-id/backend/internal/instanceid" @@ -39,11 +40,11 @@ func TestImageUploadRoutesLimitRequestSize(t *testing.T) { jwtService, err := service.NewJwtService(t.Context(), db, instanceID) require.NoError(t, err) - userService := service.NewUserService(db, jwtService, nil, nil, nil, nil, nil) + userService := service.NewUserService(db, jwtService, nil, nil, nil, nil, nil, nil) apiKeyModule, err := apikey.New(t.Context(), apikey.Dependencies{DB: db, CleanupDisabled: true}) require.NoError(t, err) - auth := middleware.NewAuthorization(apiKeyModule, userService, jwtService) + auth := middleware.NewAuthorization(apiKeyModule, userService, jwtService, appconfig.StaticResolver{}) fileSizeLimitMiddleware := middleware.NewFileSizeLimitMiddleware() user := model.User{Username: "upload-admin", IsAdmin: true} diff --git a/backend/internal/dto/app_config_dto.go b/backend/internal/dto/app_config_dto.go index a4b678f3..cb2f0499 100644 --- a/backend/internal/dto/app_config_dto.go +++ b/backend/internal/dto/app_config_dto.go @@ -59,7 +59,7 @@ type AppConfigUpdateDto struct { EmailOneTimeAccessAsUnauthenticatedEnabled string `json:"emailOneTimeAccessAsUnauthenticatedEnabled" binding:"required,boolean_string"` EmailLoginNotificationMode string `json:"emailLoginNotificationMode" binding:"required,oneof=disabled always ipAndUserAgent browserRecognition"` EmailApiKeyExpirationEnabled string `json:"emailApiKeyExpirationEnabled" binding:"required,boolean_string"` - EmailVerificationEnabled string `json:"emailVerificationEnabled" binding:"required,boolean_string"` + EmailVerification string `json:"emailVerification" binding:"required,oneof=disabled optional required,email_verification"` CIMDURLAllowlist string `json:"cimdUrlAllowlist" binding:"omitempty,cimd_url_allowlist"` AutoCreateOIDCClientSecret string `json:"autoCreateOidcClientSecret" binding:"required,boolean_string"` } diff --git a/backend/internal/dto/user_dto.go b/backend/internal/dto/user_dto.go index 545e4ccc..fc957012 100644 --- a/backend/internal/dto/user_dto.go +++ b/backend/internal/dto/user_dto.go @@ -4,6 +4,8 @@ import ( "errors" "github.com/gin-gonic/gin/binding" + + datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" ) type UserDto struct { @@ -20,6 +22,8 @@ type UserDto struct { UserGroups []UserGroupMinimalDto `json:"userGroups"` LdapID *string `json:"ldapId"` Disabled bool `json:"disabled"` + + EmailVerificationDeadline *datatype.DateTime `json:"emailVerificationDeadline,omitempty"` } type UserCreateDto struct { diff --git a/backend/internal/dto/validations.go b/backend/internal/dto/validations.go index 4af67d44..8a8a69cd 100644 --- a/backend/internal/dto/validations.go +++ b/backend/internal/dto/validations.go @@ -92,6 +92,15 @@ func init() { "integer_string": func(fl validator.FieldLevel) bool { return validateIntegerString(fl.Field().String()) }, + "email_verification": func(fl validator.FieldLevel) bool { + // Required verification needs emails that aren't trusted by default and the SMTP host and sender address without which no verification link can be sent + parent := fl.Parent() + return fl.Field().String() != "required" || + (parent.FieldByName("RequireUserEmail").String() == "true" && + parent.FieldByName("EmailsVerified").String() == "false" && + parent.FieldByName("SmtpHost").String() != "" && + parent.FieldByName("SmtpFrom").String() != "") + }, } for k, v := range validators { err := engine.RegisterValidation(k, v) @@ -182,6 +191,8 @@ func ValidationErrorDetails(validationError validator.FieldError) (string, strin return "invalid_format", "must be either true or false" case "integer_string": return "invalid_format", "must be an integer" + case "email_verification": + return "invalid_configuration", "requires user emails, an SMTP host and sender address, and forbids emails verified by default" default: return validationError.Tag(), "is invalid" } diff --git a/backend/internal/emailverification/actor.go b/backend/internal/emailverification/actor.go index e3dfe17b..d9c8d65a 100644 --- a/backend/internal/emailverification/actor.go +++ b/backend/internal/emailverification/actor.go @@ -17,10 +17,12 @@ const ActorType = "EmailVerification" const ( // MethodIssue replaces the outstanding verification state for a user - MethodIssue = "issue" - methodConsume = "consume" - methodDiscard = "discard" - methodRestore = "restore" + MethodIssue = "issue" + methodConsume = "consume" + methodDiscard = "discard" + methodRestore = "restore" + methodCancelChange = "cancelChange" + methodPendingChange = "pendingChange" ) type consumeStatus string @@ -32,7 +34,14 @@ const ( // State is the persisted verification state for one user type State struct { - TokenHash string + TokenHash string + Email string + ReplacesEmail string + ExpiresAt time.Time +} + +// EmailChange describes an email change that is waiting for the user to confirm it +type EmailChange struct { Email string ExpiresAt time.Time } @@ -68,11 +77,35 @@ func (a *emailVerificationActor) Invoke(ctx context.Context, method string, data return nil, a.discard(ctx, data) case methodRestore: return nil, a.restore(ctx, data) + case methodCancelChange: + return nil, a.cancelChange(ctx) default: return nil, common.ErrUnsupportedActorMethod{Method: method} } } +// Peek implements actor.ActorPeek +func (a *emailVerificationActor) Peek(ctx context.Context, method string, _ actor.Envelope) (any, error) { + if method != methodPendingChange { + return nil, common.ErrUnsupportedActorMethod{Method: method} + } + + state, err := a.client.GetState(ctx) + if err != nil { + return nil, fmt.Errorf("error retrieving actor state: %w", err) + } + + // Only an unexpired email change is pending, a token for the current email is not + if state.ReplacesEmail == "" || state.ExpiresAt.Before(time.Now()) { + return EmailChange{}, nil + } + + return EmailChange{ + Email: state.Email, + ExpiresAt: state.ExpiresAt, + }, nil +} + func (a *emailVerificationActor) issue(ctx context.Context, data actor.Envelope) error { state, err := decodeState(data, MethodIssue) if err != nil { @@ -134,6 +167,23 @@ func (a *emailVerificationActor) discard(ctx context.Context, data actor.Envelop return nil } +func (a *emailVerificationActor) cancelChange(ctx context.Context) error { + state, err := a.client.GetState(ctx) + if err != nil { + return fmt.Errorf("error retrieving actor state: %w", err) + } + if state.ReplacesEmail == "" { + return nil + } + + err = a.client.DeleteState(ctx) + if err != nil && !errors.Is(err, actor.ErrStateNotFound) { + return fmt.Errorf("error deleting actor state: %w", err) + } + + return nil +} + func (a *emailVerificationActor) restore(ctx context.Context, data actor.Envelope) error { state, err := decodeState(data, methodRestore) if err != nil { diff --git a/backend/internal/emailverification/handler.go b/backend/internal/emailverification/handler.go index c1406312..31e6aa35 100644 --- a/backend/internal/emailverification/handler.go +++ b/backend/internal/emailverification/handler.go @@ -10,8 +10,15 @@ import ( "github.com/pocket-id/pocket-id/backend/internal/authz" "github.com/pocket-id/pocket-id/backend/internal/dto" "github.com/pocket-id/pocket-id/backend/internal/httpserver" + datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" ) +// emailChangeDto describes an email change that waits for the user to confirm the new address +type emailChangeDto struct { + Email string `json:"email"` + ExpiresAt datatype.DateTime `json:"expiresAt"` +} + type handler struct { service *Service appConfig appconfig.AppConfigResolver @@ -64,3 +71,44 @@ func (h *handler) verify(c *gin.Context) error { c.Status(http.StatusNoContent) return nil } + +// pendingEmailChange godoc +// @Summary Get pending email change +// @Description Get the email change of the currently authenticated user that still has to be confirmed through the emailed link +// @Tags Users +// @Produce json +// @Success 200 {object} emailChangeDto +// @Success 204 "No pending email change" +// @Router /api/users/me/email-change [get] +func (h *handler) pendingEmailChange(c *gin.Context) error { + pending, err := h.service.PendingEmailChange(c.Request.Context(), authz.PrincipalFrom(c).UserID) + if err != nil { + return err + } + if pending == nil { + c.Status(http.StatusNoContent) + return nil + } + + c.JSON(http.StatusOK, emailChangeDto{ + Email: pending.Email, + ExpiresAt: datatype.DateTime(pending.ExpiresAt), + }) + return nil +} + +// cancelEmailChange godoc +// @Summary Cancel pending email change +// @Description Cancel the pending email change of the currently authenticated user, so its verification link stops working +// @Tags Users +// @Success 204 "No Content" +// @Router /api/users/me/email-change [delete] +func (h *handler) cancelEmailChange(c *gin.Context) error { + err := h.service.CancelEmailChange(c.Request.Context(), authz.PrincipalFrom(c).UserID) + if err != nil { + return err + } + + c.Status(http.StatusNoContent) + return nil +} diff --git a/backend/internal/emailverification/module.go b/backend/internal/emailverification/module.go index 74caf2d5..cc6aa9b4 100644 --- a/backend/internal/emailverification/module.go +++ b/backend/internal/emailverification/module.go @@ -1,6 +1,7 @@ package emailverification import ( + "context" "fmt" "github.com/gin-gonic/gin" @@ -16,8 +17,8 @@ type Dependencies struct { DB *gorm.DB Actors francishost.Host - Users UserProvider EmailSender EmailSender + ScimSync ScimSyncScheduler AppConfig appconfig.AppConfigResolver AppURL string } @@ -33,15 +34,27 @@ func New(deps Dependencies) (*Module, error) { return nil, fmt.Errorf("error registering the %s actor: %w", ActorType, err) } - service := newService(deps.DB, deps.Actors.Service(), deps.Users, deps.EmailSender, deps.AppURL) + service := newService(deps.DB, deps.Actors.Service(), deps.EmailSender, deps.ScimSync, deps.AppURL) return &Module{ service: service, handler: newHandler(service, deps.AppConfig), }, nil } +// Send issues a new verification token for the user's current email and emails the verification link +func (m *Module) Send(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID string) error { + return m.service.Send(ctx, dbConfig, userID) +} + +// RequestEmailChange emails a verification link to a new address, which replaces the user's current email once the link is used +func (m *Module) RequestEmailChange(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID, newEmail string) error { + return m.service.RequestEmailChange(ctx, dbConfig, userID, newEmail) +} + // RegisterRoutes mounts the email verification endpoints func (m *Module) RegisterRoutes(r *authz.Router, sendRateLimit, verifyRateLimit gin.HandlerFunc) { r.POST("/users/me/send-email-verification", authz.AccountWrite, sendRateLimit, httpserver.Handle(m.handler.send)) r.POST("/users/me/verify-email", authz.AccountWrite, verifyRateLimit, httpserver.Handle(m.handler.verify)) + r.GET("/users/me/email-change", authz.AccountRead, httpserver.Handle(m.handler.pendingEmailChange)) + r.DELETE("/users/me/email-change", authz.AccountWrite, httpserver.Handle(m.handler.cancelEmailChange)) } diff --git a/backend/internal/emailverification/pending.go b/backend/internal/emailverification/pending.go new file mode 100644 index 00000000..ff8a3ed4 --- /dev/null +++ b/backend/internal/emailverification/pending.go @@ -0,0 +1,21 @@ +package emailverification + +import ( + "github.com/pocket-id/pocket-id/backend/internal/appconfig" + "github.com/pocket-id/pocket-id/backend/internal/model" +) + +// IsPending reports whether the user must verify their email before they can use their account +func IsPending(cfg *appconfig.AppConfigModel, user *model.User) bool { + return cfg.EmailVerification.String() == appconfig.EmailVerificationRequired && CanBePending(user) +} + +// CanBePending reports whether the user would have to verify their email if verification were required +func CanBePending(user *model.User) bool { + return !user.IsAdmin && !user.EmailVerified +} + +// DefersEmailChange reports whether the user's own email change has to be confirmed before it replaces their current email +func DefersEmailChange(cfg *appconfig.AppConfigModel, user *model.User) bool { + return cfg.EmailVerification.String() == appconfig.EmailVerificationRequired && !user.IsAdmin && user.EmailVerified && user.Email != nil +} diff --git a/backend/internal/emailverification/service.go b/backend/internal/emailverification/service.go index b89de110..dd01b774 100644 --- a/backend/internal/emailverification/service.go +++ b/backend/internal/emailverification/service.go @@ -17,36 +17,52 @@ import ( "github.com/pocket-id/pocket-id/backend/internal/utils" ) -const tokenLifetime = 24 * time.Hour - -type UserProvider interface { - GetUser(ctx context.Context, userID string) (model.User, error) -} +// TokenLifetime is how long a verification link stays valid +const TokenLifetime = 24 * time.Hour type EmailSender interface { SendEmailVerification(ctx context.Context, dbConfig *appconfig.AppConfigModel, userFullName, userEmail, verificationLink string) error } +// ScimSyncScheduler schedules a SCIM sync after a confirmed email change replaced a user's address +type ScimSyncScheduler interface { + ScheduleSync(ctx context.Context) +} + type Service struct { db *gorm.DB actors *actor.Service - users UserProvider emailSender EmailSender + scimSync ScimSyncScheduler appURL string } -func newService(db *gorm.DB, actors *actor.Service, users UserProvider, emailSender EmailSender, appURL string) *Service { +func newService(db *gorm.DB, actors *actor.Service, emailSender EmailSender, scimSync ScimSyncScheduler, appURL string) *Service { return &Service{ db: db, actors: actors, - users: users, emailSender: emailSender, + scimSync: scimSync, appURL: appURL, } } +func (s *Service) loadUser(ctx context.Context, userID string) (model.User, error) { + var user model.User + err := s.db. + WithContext(ctx). + Where("id = ?", userID). + First(&user). + Error + if errors.Is(err, gorm.ErrRecordNotFound) { + return model.User{}, apperror.UserNotFound() + } + return user, err +} + +// Send emails a verification link for the user's current email func (s *Service) Send(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID string) error { - user, err := s.users.GetUser(ctx, userID) + user, err := s.loadUser(ctx, userID) if err != nil { return err } @@ -54,28 +70,76 @@ func (s *Service) Send(ctx context.Context, dbConfig *appconfig.AppConfigModel, return apperror.UserEmailNotSet() } + return s.issue(ctx, dbConfig, user, State{Email: *user.Email}) +} + +// RequestEmailChange emails a verification link to a new address, which replaces the user's current email once the link is used +// Until then the current email stays in effect, so a mistyped address never restricts the account +func (s *Service) RequestEmailChange(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID, newEmail string) error { + user, err := s.loadUser(ctx, userID) + if err != nil { + return err + } + if user.Email == nil { + return apperror.UserEmailNotSet() + } + + return s.issue(ctx, dbConfig, user, State{Email: newEmail, ReplacesEmail: *user.Email}) +} + +// PendingEmailChange returns the email change the user still has to confirm, or nil if there is none +func (s *Service) PendingEmailChange(ctx context.Context, userID string) (*EmailChange, error) { + response, err := s.actors.Peek(ctx, ActorType, userID, methodPendingChange, nil) + if err != nil { + return nil, fmt.Errorf("error reading pending email change: %w", err) + } + if response == nil { + return nil, errors.New("email verification actor returned an empty response") + } + + var pending EmailChange + err = response.Decode(&pending) + if err != nil { + return nil, fmt.Errorf("error decoding pending email change: %w", err) + } + if pending.Email == "" { + return nil, nil + } + + return &pending, nil +} + +// CancelEmailChange drops a pending email change, so its verification link stops working +func (s *Service) CancelEmailChange(ctx context.Context, userID string) error { + _, err := s.actors.Invoke(ctx, ActorType, userID, methodCancelChange, nil) + if err != nil { + return fmt.Errorf("error canceling email change: %w", err) + } + return nil +} + +// issue stores a new verification token for the state's email and emails the verification link to that address +// Issuing replaces any outstanding token, so only the most recent link works +func (s *Service) issue(ctx context.Context, dbConfig *appconfig.AppConfigModel, user model.User, state State) error { token, err := utils.GenerateRandomAlphanumericString(32) if err != nil { return err } // Persist the token hash in the email verification actor for this user - state := State{ - TokenHash: utils.CreateSha256Hash(token), - Email: *user.Email, - ExpiresAt: time.Now().Add(tokenLifetime), - } + state.TokenHash = utils.CreateSha256Hash(token) + state.ExpiresAt = time.Now().Add(TokenLifetime) _, err = s.actors.Invoke(ctx, ActorType, user.ID, MethodIssue, state) if err != nil { return fmt.Errorf("error issuing email verification token: %w", err) } - // Send the email verification message to the user + // Send the email verification message to the address being verified err = s.emailSender.SendEmailVerification( ctx, dbConfig, user.FullName(), - *user.Email, + state.Email, s.appURL+"/verify-email?token="+token, ) if err != nil { @@ -108,16 +172,33 @@ func (s *Service) Verify(ctx context.Context, userID, token string) error { return apperror.InvalidEmailVerificationToken() } - // Update the user's email_verified field in the database - // We are querying by both user ID and email to ensure that the email has not changed since the token was issued - update := s.db. + // Mark the email as verified + // A verified account is no longer deleted for missing the signup verification deadline + updates := map[string]any{ + "email_verified": true, + "email_verification_deadline": nil, + "updated_at": new(datatype.DateTime(time.Now())), + } + query := s.db. WithContext(ctx). Model(&model.User{}). - Where("id = ? AND email = ?", userID, result.State.Email). - Updates(map[string]any{ - "email_verified": true, - "updated_at": new(datatype.DateTime(time.Now())), - }) + Where("id = ?", userID) + + // A token for an email change also moves the new address into place, unless the email was changed another way in the meantime + // A token for the current email only applies while the email is still the one the token was issued for + isEmailChange := result.State.ReplacesEmail != "" + if isEmailChange { + updates["email"] = result.State.Email + query = query.Where("email = ?", result.State.ReplacesEmail) + } else { + query = query.Where("email = ?", result.State.Email) + } + + update := query.Updates(updates) + if errors.Is(update.Error, gorm.ErrDuplicatedKey) { + // Another account took the new address while the change was waiting for confirmation + return apperror.AlreadyInUse("email") + } if update.Error != nil { // If the database update fails, restore the token in the actor to allow the user to retry verification s.restoreAfterDatabaseFailure(ctx, userID, result.State) @@ -127,6 +208,11 @@ func (s *Service) Verify(ctx context.Context, userID, token string) error { return apperror.InvalidEmailVerificationToken() } + // Service providers have to learn about the new address + if isEmailChange && s.scimSync != nil { + s.scimSync.ScheduleSync(ctx) + } + return nil } diff --git a/backend/internal/emailverification/service_test.go b/backend/internal/emailverification/service_test.go index 132f5371..6fd2d398 100644 --- a/backend/internal/emailverification/service_test.go +++ b/backend/internal/emailverification/service_test.go @@ -15,20 +15,11 @@ import ( "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/apperror" "github.com/pocket-id/pocket-id/backend/internal/model" + datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" "github.com/pocket-id/pocket-id/backend/internal/utils" testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" ) -type testUserProvider struct { - db *gorm.DB -} - -func (p testUserProvider) GetUser(ctx context.Context, userID string) (model.User, error) { - var user model.User - err := p.db.WithContext(ctx).Where("id = ?", userID).First(&user).Error - return user, err -} - type testEmailSender struct { err error sent []sentVerificationEmail @@ -61,7 +52,7 @@ func newServiceForTest(t *testing.T, emailSender *testEmailSender) (*Service, *l var service *Service host := testutils.NewActorHostForTest(t, func(t *testing.T, host *local.Host) { require.NoError(t, host.RegisterActor(ActorType, NewActor)) - service = newService(db, host.Service(), testUserProvider{db: db}, emailSender, "https://id.example.test") + service = newService(db, host.Service(), emailSender, nil, "https://id.example.test") }) require.NotNil(t, service) @@ -129,6 +120,22 @@ func TestVerifyConsumesTokenAndMarksBoundAddressVerified(t *testing.T) { require.ErrorIs(t, host.GetState(t.Context(), ActorType, user.ID, &state), actor.ErrStateNotFound) } +func TestVerifyClearsSignupDeadline(t *testing.T) { + emailSender := &testEmailSender{} + service, _, db := newServiceForTest(t, emailSender) + user := createTestUser(t, db, "user-deadline", "user@example.test") + require.NoError(t, db.Model(&user).Update("email_verification_deadline", datatype.DateTime(time.Now().Add(time.Hour))).Error) + + require.NoError(t, service.Send(t.Context(), &appconfig.AppConfigModel{}, user.ID)) + token := verificationTokenFromEmail(t, emailSender.sent[0]) + require.NoError(t, service.Verify(t.Context(), user.ID, token)) + + var updated model.User + require.NoError(t, db.Where("id = ?", user.ID).First(&updated).Error) + require.True(t, updated.EmailVerified) + require.Nil(t, updated.EmailVerificationDeadline) +} + func TestVerifyRejectsTokenAfterAddressChanges(t *testing.T) { emailSender := &testEmailSender{} service, host, db := newServiceForTest(t, emailSender) @@ -255,3 +262,107 @@ func TestSendDiscardsTokenWhenEmailDeliveryFails(t *testing.T) { var state State require.ErrorIs(t, host.GetState(t.Context(), ActorType, user.ID, &state), actor.ErrStateNotFound) } + +func TestRequestEmailChangeSendsLinkToNewAddressAndKeepsCurrentEmail(t *testing.T) { + emailSender := &testEmailSender{} + service, _, db := newServiceForTest(t, emailSender) + user := createTestUser(t, db, "user-change", "old@example.test") + require.NoError(t, db.Model(&user).Update("email_verified", true).Error) + + require.NoError(t, service.RequestEmailChange(t.Context(), &appconfig.AppConfigModel{}, user.ID, "new@example.test")) + require.Len(t, emailSender.sent, 1) + require.Equal(t, "new@example.test", emailSender.sent[0].userEmail) + + pending, err := service.PendingEmailChange(t.Context(), user.ID) + require.NoError(t, err) + require.NotNil(t, pending) + require.Equal(t, "new@example.test", pending.Email) + + var unchanged model.User + require.NoError(t, db.Where("id = ?", user.ID).First(&unchanged).Error) + require.Equal(t, "old@example.test", *unchanged.Email) + require.True(t, unchanged.EmailVerified) +} + +func TestVerifyEmailChangeReplacesEmail(t *testing.T) { + emailSender := &testEmailSender{} + service, _, db := newServiceForTest(t, emailSender) + user := createTestUser(t, db, "user-confirm", "old@example.test") + + require.NoError(t, service.RequestEmailChange(t.Context(), &appconfig.AppConfigModel{}, user.ID, "new@example.test")) + token := verificationTokenFromEmail(t, emailSender.sent[0]) + require.NoError(t, service.Verify(t.Context(), user.ID, token)) + + var updated model.User + require.NoError(t, db.Where("id = ?", user.ID).First(&updated).Error) + require.Equal(t, "new@example.test", *updated.Email) + require.True(t, updated.EmailVerified) + + pending, err := service.PendingEmailChange(t.Context(), user.ID) + require.NoError(t, err) + require.Nil(t, pending) +} + +func TestVerifyEmailChangeRejectsTokenAfterEmailChangedElsewhere(t *testing.T) { + emailSender := &testEmailSender{} + service, _, db := newServiceForTest(t, emailSender) + user := createTestUser(t, db, "user-admin-changed", "old@example.test") + + require.NoError(t, service.RequestEmailChange(t.Context(), &appconfig.AppConfigModel{}, user.ID, "new@example.test")) + token := verificationTokenFromEmail(t, emailSender.sent[0]) + + // An admin sets a different email before the user confirms their change, and that one must win + require.NoError(t, db.Model(&user).Update("email", "admin-set@example.test").Error) + + err := service.Verify(t.Context(), user.ID, token) + require.True(t, apperror.IsCode(err, apperror.CodeEmailVerificationTokenInvalid)) + + var updated model.User + require.NoError(t, db.Where("id = ?", user.ID).First(&updated).Error) + require.Equal(t, "admin-set@example.test", *updated.Email) +} + +func TestVerifyEmailChangeRejectsAddressTakenInTheMeantime(t *testing.T) { + emailSender := &testEmailSender{} + service, _, db := newServiceForTest(t, emailSender) + user := createTestUser(t, db, "user-race", "old@example.test") + + require.NoError(t, service.RequestEmailChange(t.Context(), &appconfig.AppConfigModel{}, user.ID, "new@example.test")) + token := verificationTokenFromEmail(t, emailSender.sent[0]) + createTestUser(t, db, "user-taker", "new@example.test") + + err := service.Verify(t.Context(), user.ID, token) + require.True(t, apperror.IsCode(err, apperror.CodeAlreadyInUse)) + + var unchanged model.User + require.NoError(t, db.Where("id = ?", user.ID).First(&unchanged).Error) + require.Equal(t, "old@example.test", *unchanged.Email) +} + +func TestCancelEmailChangeOnlyDropsPendingChange(t *testing.T) { + emailSender := &testEmailSender{} + service, host, db := newServiceForTest(t, emailSender) + user := createTestUser(t, db, "user-cancel", "old@example.test") + + t.Run("cancels a pending change and invalidates its link", func(t *testing.T) { + require.NoError(t, service.RequestEmailChange(t.Context(), &appconfig.AppConfigModel{}, user.ID, "new@example.test")) + token := verificationTokenFromEmail(t, emailSender.sent[len(emailSender.sent)-1]) + + require.NoError(t, service.CancelEmailChange(t.Context(), user.ID)) + + pending, err := service.PendingEmailChange(t.Context(), user.ID) + require.NoError(t, err) + require.Nil(t, pending) + require.True(t, apperror.IsCode(service.Verify(t.Context(), user.ID, token), apperror.CodeEmailVerificationTokenInvalid)) + }) + + t.Run("keeps a verification of the current email", func(t *testing.T) { + require.NoError(t, service.Send(t.Context(), &appconfig.AppConfigModel{}, user.ID)) + + require.NoError(t, service.CancelEmailChange(t.Context(), user.ID)) + + var state State + require.NoError(t, host.GetState(t.Context(), ActorType, user.ID, &state)) + require.Equal(t, "old@example.test", state.Email) + }) +} diff --git a/backend/internal/ldapsync/service_test.go b/backend/internal/ldapsync/service_test.go index bb415be1..9c375870 100644 --- a/backend/internal/ldapsync/service_test.go +++ b/backend/internal/ldapsync/service_test.go @@ -586,6 +586,7 @@ func newTestLdapService(t *testing.T, client ldapClient) (*Service, *gorm.DB) { nil, nil, fileStorage, + nil, ) svc := newService(Dependencies{ diff --git a/backend/internal/middleware/authenticator.go b/backend/internal/middleware/authenticator.go index 74683096..95fc2b4a 100644 --- a/backend/internal/middleware/authenticator.go +++ b/backend/internal/middleware/authenticator.go @@ -1,13 +1,17 @@ package middleware import ( + "context" "strings" "github.com/gin-gonic/gin" "github.com/pocket-id/pocket-id/backend/internal/apikey" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/apperror" "github.com/pocket-id/pocket-id/backend/internal/authz" + "github.com/pocket-id/pocket-id/backend/internal/emailverification" + "github.com/pocket-id/pocket-id/backend/internal/model" "github.com/pocket-id/pocket-id/backend/internal/service" "github.com/pocket-id/pocket-id/backend/internal/utils/cookie" ) @@ -17,21 +21,39 @@ const apiKeyHeader = "X-API-Key" // NewAuthorization creates the authorization middleware with every credential Pocket ID accepts // A browser session is tried before an API key, so a signed-in browser is never mistaken for an API client -func NewAuthorization(apiKeyModule *apikey.Module, userService *service.UserService, jwtService *service.JwtService) *authz.Middleware { +func NewAuthorization(apiKeyModule *apikey.Module, userService *service.UserService, jwtService *service.JwtService, appConfig appconfig.AppConfigResolver) *authz.Middleware { return authz.NewMiddleware( - NewSessionAuthenticator(jwtService, userService), - NewAPIKeyAuthenticator(apiKeyModule), + NewSessionAuthenticator(jwtService, userService, appConfig), + NewAPIKeyAuthenticator(apiKeyModule, appConfig), ) } +// restrictUnverified withholds the scopes a user who still has to verify their email may not use yet +func restrictUnverified(ctx context.Context, appConfig appconfig.AppConfigResolver, user *model.User, principal *authz.Principal) error { + if !emailverification.CanBePending(user) { + return nil + } + + cfg, err := appConfig.GetConfig(ctx) + if err != nil { + return err + } + if emailverification.IsPending(cfg, user) { + principal.RestrictUnverified() + } + + return nil +} + // SessionAuthenticator authenticates the session access token Pocket ID issues after sign-in type SessionAuthenticator struct { jwtService *service.JwtService userService *service.UserService + appConfig appconfig.AppConfigResolver } -func NewSessionAuthenticator(jwtService *service.JwtService, userService *service.UserService) *SessionAuthenticator { - return &SessionAuthenticator{jwtService: jwtService, userService: userService} +func NewSessionAuthenticator(jwtService *service.JwtService, userService *service.UserService, appConfig appconfig.AppConfigResolver) *SessionAuthenticator { + return &SessionAuthenticator{jwtService: jwtService, userService: userService, appConfig: appConfig} } func (a *SessionAuthenticator) Kind() authz.PrincipalKind { @@ -59,7 +81,7 @@ func (a *SessionAuthenticator) Authenticate(c *gin.Context) (*authz.Principal, e return nil, apperror.TokenInvalid() } - // Load the user so disabling an account or changing its admin flag takes effect before the token expires + // Load the user so disabling an account, changing its admin flag or its email verification takes effect before the token expires user, err := a.userService.GetUser(c, subject) if err != nil { return nil, apperror.NotSignedIn() @@ -68,13 +90,19 @@ func (a *SessionAuthenticator) Authenticate(c *gin.Context) (*authz.Principal, e return nil, apperror.UserDisabled() } - return &authz.Principal{ + principal := &authz.Principal{ Kind: authz.KindSession, UserID: user.ID, Scopes: authz.UserScopes(user.IsAdmin, authz.KindSession), AuthenticationMethod: authenticationMethod, AuthenticationTime: authenticationTime, - }, nil + } + err = restrictUnverified(c.Request.Context(), a.appConfig, &user, principal) + if err != nil { + return nil, err + } + + return principal, nil } // sessionToken reads the session access token from its cookie, or from the Authorization header when the cookie is absent @@ -91,10 +119,11 @@ func sessionToken(c *gin.Context) string { // APIKeyAuthenticator authenticates a personal API key sent in the X-API-Key header type APIKeyAuthenticator struct { apiKeyModule *apikey.Module + appConfig appconfig.AppConfigResolver } -func NewAPIKeyAuthenticator(apiKeyModule *apikey.Module) *APIKeyAuthenticator { - return &APIKeyAuthenticator{apiKeyModule: apiKeyModule} +func NewAPIKeyAuthenticator(apiKeyModule *apikey.Module, appConfig appconfig.AppConfigResolver) *APIKeyAuthenticator { + return &APIKeyAuthenticator{apiKeyModule: apiKeyModule, appConfig: appConfig} } func (a *APIKeyAuthenticator) Kind() authz.PrincipalKind { @@ -114,9 +143,15 @@ func (a *APIKeyAuthenticator) Authenticate(c *gin.Context) (*authz.Principal, er return nil, apperror.UserDisabled() } - return &authz.Principal{ + principal := &authz.Principal{ Kind: authz.KindAPIKey, UserID: user.ID, Scopes: authz.UserScopes(user.IsAdmin, authz.KindAPIKey), - }, nil + } + err = restrictUnverified(c.Request.Context(), a.appConfig, &user, principal) + if err != nil { + return nil, err + } + + return principal, nil } diff --git a/backend/internal/middleware/authenticator_test.go b/backend/internal/middleware/authenticator_test.go index 919739a0..0a242a69 100644 --- a/backend/internal/middleware/authenticator_test.go +++ b/backend/internal/middleware/authenticator_test.go @@ -12,6 +12,7 @@ import ( "gorm.io/gorm" "github.com/pocket-id/pocket-id/backend/internal/apikey" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/authz" "github.com/pocket-id/pocket-id/backend/internal/common" "github.com/pocket-id/pocket-id/backend/internal/instanceid" @@ -41,30 +42,42 @@ func TestAuthorizationWithRealCredentials(t *testing.T) { jwtService, err := service.NewJwtService(t.Context(), db, instanceID) require.NoError(t, err) - userService := service.NewUserService(db, jwtService, nil, nil, nil, nil, nil) + userService := service.NewUserService(db, jwtService, nil, nil, nil, nil, nil, nil) apiKeyModule, err := apikey.New(t.Context(), apikey.Dependencies{DB: db, CleanupDisabled: true}) require.NoError(t, err) // Create one credential of each kind for a regular user, an admin and a disabled user - user := createUserForAuthorizationTest(t, db, "auth-user", false, false) - admin := createUserForAuthorizationTest(t, db, "auth-admin", true, false) - disabled := createUserForAuthorizationTest(t, db, "auth-disabled", true, true) + // The regular users and the admin have a verified email, the others still have to verify theirs + user := createUserForAuthorizationTest(t, db, "auth-user", false, false, true) + admin := createUserForAuthorizationTest(t, db, "auth-admin", true, false, true) + disabled := createUserForAuthorizationTest(t, db, "auth-disabled", true, true, true) + unverified := createUserForAuthorizationTest(t, db, "auth-unverified", false, false, false) + unverifiedAdmin := createUserForAuthorizationTest(t, db, "auth-unverified-admin", true, false, false) userSession, err := jwtService.GenerateAccessToken(user, "", time.Hour) require.NoError(t, err) + unverifiedSession, err := jwtService.GenerateAccessToken(unverified, "", time.Hour) + require.NoError(t, err) userAPIKey := createAPIKeyForAuthorizationTest(t, db, user, "user-raw-api-key") adminAPIKey := createAPIKeyForAuthorizationTest(t, db, admin, "admin-raw-api-key") disabledAPIKey := createAPIKeyForAuthorizationTest(t, db, disabled, "disabled-raw-api-key") + unverifiedAPIKey := createAPIKeyForAuthorizationTest(t, db, unverified, "unverified-raw-api-key") + unverifiedAdminAPIKey := createAPIKeyForAuthorizationTest(t, db, unverifiedAdmin, "unverified-admin-raw-api-key") // Mount one route per kind of requirement + // Email verification is required, so unverified regular users only keep the scopes needed to verify their email + cfg := appconfig.NewTestConfig(nil) + cfg.EmailVerification = appconfig.EmailVerificationRequired router := gin.New() router.Use(NewErrorHandlerMiddleware().Add()) - apiRouter := NewAuthorization(apiKeyModule, userService, jwtService).Router(router.Group("/api")) + apiRouter := NewAuthorization(apiKeyModule, userService, jwtService, appconfig.StaticResolver{Config: cfg}).Router(router.Group("/api")) ok := func(c *gin.Context) { c.String(http.StatusOK, authz.PrincipalFrom(c).UserID) } apiRouter.GET("/session-only", authz.AccountSession, ok) apiRouter.GET("/account", authz.AccountRead, ok) + apiRouter.GET("/apps", authz.AccountApps, ok) + apiRouter.GET("/authorize", authz.AccountAuthorize, ok) apiRouter.GET("/admin", authz.UsersRead, ok) tests := []struct { @@ -85,6 +98,15 @@ func TestAuthorizationWithRealCredentials(t *testing.T) { {name: "admin API key on admin route", path: "/api/admin", apiKey: adminAPIKey, status: http.StatusOK, userID: admin.ID}, {name: "invalid session cookie falls back to API key", path: "/api/admin", sessionCookie: "not-a-jwt", apiKey: adminAPIKey, status: http.StatusOK, userID: admin.ID}, {name: "disabled user's API key", path: "/api/account", apiKey: disabledAPIKey, status: http.StatusForbidden, errorCode: "user_disabled"}, + {name: "verified user session on apps route", path: "/api/apps", authorization: "Bearer " + userSession, status: http.StatusOK, userID: user.ID}, + {name: "unverified user session on session-only route", path: "/api/session-only", authorization: "Bearer " + unverifiedSession, status: http.StatusOK, userID: unverified.ID}, + {name: "unverified user session on account route", path: "/api/account", authorization: "Bearer " + unverifiedSession, status: http.StatusOK, userID: unverified.ID}, + {name: "unverified user session on apps route", path: "/api/apps", authorization: "Bearer " + unverifiedSession, status: http.StatusForbidden, errorCode: "email_verification_required"}, + {name: "unverified user session on authorize route", path: "/api/authorize", authorization: "Bearer " + unverifiedSession, status: http.StatusForbidden, errorCode: "email_verification_required"}, + {name: "verified user session on authorize route", path: "/api/authorize", authorization: "Bearer " + userSession, status: http.StatusOK, userID: user.ID}, + {name: "unverified user API key on account route", path: "/api/account", apiKey: unverifiedAPIKey, status: http.StatusOK, userID: unverified.ID}, + {name: "unverified user API key on apps route", path: "/api/apps", apiKey: unverifiedAPIKey, status: http.StatusForbidden, errorCode: "email_verification_required"}, + {name: "unverified admin API key on admin route", path: "/api/admin", apiKey: unverifiedAdminAPIKey, status: http.StatusOK, userID: unverifiedAdmin.ID}, {name: "unknown API key", path: "/api/account", apiKey: "unknown", status: http.StatusUnauthorized, errorCode: "not_signed_in"}, {name: "no credentials", path: "/api/account", status: http.StatusUnauthorized, errorCode: "not_signed_in"}, } @@ -118,17 +140,18 @@ func TestAuthorizationWithRealCredentials(t *testing.T) { } } -func createUserForAuthorizationTest(t *testing.T, db *gorm.DB, username string, isAdmin, disabled bool) model.User { +func createUserForAuthorizationTest(t *testing.T, db *gorm.DB, username string, isAdmin, disabled, emailVerified bool) model.User { t.Helper() user := model.User{ - Username: username, - Email: new(username + "@example.com"), - FirstName: "Auth", - LastName: "User", - DisplayName: "Auth User", - IsAdmin: isAdmin, - Disabled: disabled, + Username: username, + Email: new(username + "@example.com"), + EmailVerified: emailVerified, + FirstName: "Auth", + LastName: "User", + DisplayName: "Auth User", + IsAdmin: isAdmin, + Disabled: disabled, } err := db.Create(&user).Error diff --git a/backend/internal/middleware/error_handler_test.go b/backend/internal/middleware/error_handler_test.go index dd103831..a2b127d6 100644 --- a/backend/internal/middleware/error_handler_test.go +++ b/backend/internal/middleware/error_handler_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/apperror" "github.com/pocket-id/pocket-id/backend/internal/dto" "github.com/pocket-id/pocket-id/backend/internal/httpserver" @@ -228,7 +229,7 @@ func TestValidationResponseUsesAppConfigTypeMessages(t *testing.T) { EmailOneTimeAccessAsUnauthenticatedEnabled: "false", EmailLoginNotificationMode: "disabled", EmailApiKeyExpirationEnabled: "false", - EmailVerificationEnabled: "false", + EmailVerification: "disabled", AutoCreateOIDCClientSecret: "true", } payload, err := json.Marshal(input) @@ -269,3 +270,57 @@ func TestClassifyDeadlineAsRequestTimeout(t *testing.T) { require.Equal(t, http.StatusGatewayTimeout, classified.status) require.Equal(t, "Request timed out", classified.message) } + +func TestAppConfigBindingValidatesRequiredEmailVerification(t *testing.T) { + gin.SetMode(gin.TestMode) + router := gin.New() + router.Use(NewErrorHandlerMiddleware().Add()) + router.PUT("/config", httpserver.Handle(func(c *gin.Context) error { + var input dto.AppConfigUpdateDto + return httpserver.BindJSON(c, &input) + })) + + const ( + smtpHost = "smtp.example.com" + smtpFrom = "pocket-id@example.com" + ) + for _, tt := range []struct { + name, mode, required, verified, host, from string + status int + }{ + {"required with trusted emails", "required", "true", "true", smtpHost, smtpFrom, http.StatusBadRequest}, + {"required without email", "required", "false", "false", smtpHost, smtpFrom, http.StatusBadRequest}, + {"required without SMTP host", "required", "true", "false", "", smtpFrom, http.StatusBadRequest}, + {"required without sender address", "required", "true", "false", smtpHost, "", http.StatusBadRequest}, + {"valid required", "required", "true", "false", smtpHost, smtpFrom, http.StatusOK}, + {"optional with trusted emails", "optional", "false", "true", "", "", http.StatusOK}, + {"disabled with trusted emails", "disabled", "false", "true", "", "", http.StatusOK}, + } { + t.Run(tt.name, func(t *testing.T) { + var input dto.AppConfigUpdateDto + require.NoError(t, dto.MapStruct(appconfig.NewTestConfig(nil), &input)) + input.EmailVerification = tt.mode + input.RequireUserEmail = tt.required + input.EmailsVerified = tt.verified + input.SmtpHost = tt.host + input.SmtpFrom = tt.from + payload, err := json.Marshal(input) + require.NoError(t, err) + request := httptest.NewRequestWithContext(t.Context(), http.MethodPut, "/config", strings.NewReader(string(payload))) + request.Header.Set("Content-Type", "application/json") + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, request) + + require.Equal(t, tt.status, recorder.Code, recorder.Body.String()) + if tt.status == http.StatusBadRequest { + var body struct { + Details struct { + Fields []apperror.FieldError `json:"fields"` + } `json:"details"` + } + require.NoError(t, json.Unmarshal(recorder.Body.Bytes(), &body)) + require.Equal(t, []apperror.FieldError{{Field: "emailVerification", Code: "invalid_configuration", Message: "requires user emails, an SMTP host and sender address, and forbids emails verified by default"}}, body.Details.Fields) + } + }) + } +} diff --git a/backend/internal/middleware/rate_limit.go b/backend/internal/middleware/rate_limit.go index 8acab376..6a0e7d96 100644 --- a/backend/internal/middleware/rate_limit.go +++ b/backend/internal/middleware/rate_limit.go @@ -19,7 +19,6 @@ import ( const ( RateLimitAPI = "api" RateLimitSignup = "signup" - RateLimitUpdateOwnAccount = "update-own-account" RateLimitWebauthnLogin = "webauthn-login" RateLimitWebauthnReauthenticate = "webauthn-reauthenticate" RateLimitOneTimeAccessToken = "one-time-access-token" @@ -33,7 +32,7 @@ const ( ) // RateLimitPolicy is the configuration for a single rate-limit actor -// Each policy maps to one Francis rate-limit actor type and requests are keyed by client IP, so every IP is limited independently and per-route limits stay isolated from each other +// Each policy maps to one Francis rate-limit actor type and requests are keyed by client IP, so every IP is limited independently and policies can be shared across routes type RateLimitPolicy struct { // Name must be unique across policies and must not contain '/' Name string @@ -51,7 +50,6 @@ func RateLimitPolicies() []RateLimitPolicy { return []RateLimitPolicy{ {Name: RateLimitAPI, Rate: 100, Per: time.Second, Burst: 300}, {Name: RateLimitSignup, Rate: 2, Per: time.Minute, Burst: 10}, - {Name: RateLimitUpdateOwnAccount, Rate: 2, Per: time.Minute, Burst: 10}, {Name: RateLimitWebauthnLogin, Rate: 1, Per: 5 * time.Second, Burst: 10}, {Name: RateLimitWebauthnReauthenticate, Rate: 1, Per: 10 * time.Second, Burst: 5}, {Name: RateLimitOneTimeAccessToken, Rate: 1, Per: 10 * time.Second, Burst: 5}, diff --git a/backend/internal/model/user.go b/backend/internal/model/user.go index ee05ceab..29dc3e38 100644 --- a/backend/internal/model/user.go +++ b/backend/internal/model/user.go @@ -13,17 +13,18 @@ import ( type User struct { Base - Username string `sortable:"case-insensitive"` - Email *string `sortable:"case-insensitive"` - EmailVerified bool `sortable:"true" filterable:"true"` - FirstName string `sortable:"case-insensitive"` - LastName string `sortable:"case-insensitive"` - DisplayName string `sortable:"case-insensitive"` - IsAdmin bool `sortable:"true" filterable:"true"` - Locale *string - LdapID *string - Disabled bool `sortable:"true" filterable:"true"` - UpdatedAt *datatype.DateTime + Username string `sortable:"case-insensitive"` + Email *string `sortable:"case-insensitive"` + EmailVerified bool `sortable:"true" filterable:"true"` + EmailVerificationDeadline *datatype.DateTime + FirstName string `sortable:"case-insensitive"` + LastName string `sortable:"case-insensitive"` + DisplayName string `sortable:"case-insensitive"` + IsAdmin bool `sortable:"true" filterable:"true"` + Locale *string + LdapID *string + Disabled bool `sortable:"true" filterable:"true"` + UpdatedAt *datatype.DateTime CustomClaims []CustomClaim UserGroups []UserGroup `gorm:"many2many:user_groups_users;"` diff --git a/backend/internal/oidc/authorization_handler.go b/backend/internal/oidc/authorization_handler.go index 554fe81b..9085a2cd 100644 --- a/backend/internal/oidc/authorization_handler.go +++ b/backend/internal/oidc/authorization_handler.go @@ -10,6 +10,7 @@ import ( "github.com/gin-gonic/gin" "github.com/pocket-id/fosite" + "github.com/pocket-id/pocket-id/backend/internal/apperror" "github.com/pocket-id/pocket-id/backend/internal/authz" "github.com/pocket-id/pocket-id/backend/internal/httpserver" "github.com/pocket-id/pocket-id/backend/internal/utils" @@ -136,6 +137,17 @@ func (h *authorizationHandler) completeInteraction(c *gin.Context) { c.JSON(http.StatusOK, response) } +// redirectAuthorizationErrors shows the errors of the authorization middleware, such as a disabled account or an unverified email, on the interaction error page +// The middleware refuses these requests before the authorize handler runs, so its error is still unwritten when the chain returns here +func (h *authorizationHandler) redirectAuthorizationErrors(c *gin.Context) { + c.Next() + + if len(c.Errors) == 0 || c.Writer.Written() { + return + } + h.redirectToInteractionError(c, c.Errors.Last().Err) +} + func (h *authorizationHandler) writeAuthorizeError(ctx context.Context, c *gin.Context, ar fosite.AuthorizeRequester, err error) { if rfcErr, ok := errors.AsType[*fosite.RFC6749Error](err); ok { attrs := []slog.Attr{ @@ -170,12 +182,14 @@ func (h *authorizationHandler) writeAuthorizeError(ctx context.Context, c *gin.C func (h *authorizationHandler) redirectToInteractionError(c *gin.Context, err error) { errorMessage := "An unknown error occurred during the authorization request." - if err, ok := errors.AsType[*fosite.RFC6749Error](err); ok { - if err.HintField != "" { - errorMessage = err.HintField - } else if err.DescriptionField != "" { - errorMessage = err.DescriptionField + if rfcErr, ok := errors.AsType[*fosite.RFC6749Error](err); ok { + if rfcErr.HintField != "" { + errorMessage = rfcErr.HintField + } else if rfcErr.DescriptionField != "" { + errorMessage = rfcErr.DescriptionField } + } else if appErr, ok := errors.AsType[*apperror.Error](err); ok { + errorMessage = appErr.ClientMessage() } query := url.Values{} diff --git a/backend/internal/oidc/authorization_handler_test.go b/backend/internal/oidc/authorization_handler_test.go index 3fed7dbc..9dbeb3df 100644 --- a/backend/internal/oidc/authorization_handler_test.go +++ b/backend/internal/oidc/authorization_handler_test.go @@ -15,6 +15,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/authz" "github.com/pocket-id/pocket-id/backend/internal/model" datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" @@ -34,6 +35,58 @@ type authorizationPARTestCase struct { malformed bool } +// fakeSessionAuthenticator signs every request in as a regular user, optionally one who still has to verify their email +type fakeSessionAuthenticator struct { + unverified bool +} + +func (fakeSessionAuthenticator) Kind() authz.PrincipalKind { return authz.KindSession } + +func (fakeSessionAuthenticator) Present(*gin.Context) bool { return true } + +func (a fakeSessionAuthenticator) Authenticate(*gin.Context) (*authz.Principal, error) { + principal := &authz.Principal{Kind: authz.KindSession, UserID: "test-user", Scopes: authz.UserScopes(false, authz.KindSession)} + if a.unverified { + principal.RestrictUnverified() + } + return principal, nil +} + +// A browser that the authorization middleware refuses at the authorize endpoint must land on the interaction error page instead of a JSON error +func TestAuthorizationHandlerRedirectsAuthorizationErrors(t *testing.T) { + gin.SetMode(gin.TestMode) + handler := &authorizationHandler{} + + serve := func(t *testing.T, authenticator authz.Authenticator) *httptest.ResponseRecorder { + t.Helper() + + router := gin.New() + authz.NewMiddleware(authenticator).Router(router.Group("/")).Use(handler.redirectAuthorizationErrors).Optional(). + GET("/authorize", authz.AccountAuthorize, func(c *gin.Context) { c.Status(http.StatusNoContent) }) + + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/authorize", nil)) + return recorder + } + + t.Run("redirects a user who still has to verify their email", func(t *testing.T) { + recorder := serve(t, fakeSessionAuthenticator{unverified: true}) + + require.Equal(t, http.StatusFound, recorder.Code) + location, err := url.Parse(recorder.Header().Get("Location")) + require.NoError(t, err) + require.Equal(t, "/interaction/error", location.Path) + require.Equal(t, "You must verify your email address before you can access this service.", location.Query().Get("error")) + }) + + t.Run("leaves an authorized request to the authorize handler", func(t *testing.T) { + recorder := serve(t, fakeSessionAuthenticator{}) + + require.Equal(t, http.StatusNoContent, recorder.Code) + require.Empty(t, recorder.Header().Get("Location")) + }) +} + func TestAuthorizationHandlerRequiresPAR(t *testing.T) { gin.SetMode(gin.TestMode) const fakeURI = parRequestURIPrefix + "missing" @@ -91,7 +144,7 @@ func testAuthorizationHandlerPAR(t *testing.T, clientType string, tt authorizati BaseURL: baseURL, TokenBaseURL: baseURL, Secret: []byte("test-secret"), }, nil) require.NoError(t, err) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, baseURL, nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), nil, nil, nil) handler := newAuthorizationHandler(provider, service) params := url.Values{ "client_id": {clientID}, diff --git a/backend/internal/oidc/authorization_service_test.go b/backend/internal/oidc/authorization_service_test.go index d3d10a55..73188d6f 100644 --- a/backend/internal/oidc/authorization_service_test.go +++ b/backend/internal/oidc/authorization_service_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" "gorm.io/gorm" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/apperror" "github.com/pocket-id/pocket-id/backend/internal/auditlogs" "github.com/pocket-id/pocket-id/backend/internal/model" @@ -31,7 +32,7 @@ func (f *fakeAuditLogger) Create(_ context.Context, event auditlogs.Event, _, _, func TestAuthorizationServiceAuthorizeLogsClientAuthorization(t *testing.T) { db := testutils.NewDatabaseForTest(t) auditLogger := &fakeAuditLogger{} - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, auditLogger, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, auditLogger, nil) const ( userID = "test-user" @@ -66,7 +67,7 @@ func TestAuthorizationServiceAuthorizeLogsClientAuthorization(t *testing.T) { func TestAuthorizationServiceRejectsCustomScopeWithoutResource(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -106,7 +107,7 @@ func TestAuthorizationServiceCollapsesResourceErrorsBeforeAuthentication(t *test t.Helper() db := testutils.NewDatabaseForTest(t) require.NoError(t, db.Create(&model.OidcClient{Base: model.Base{ID: clientID}, Name: "Test Client"}).Error) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, apiAccess) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, apiAccess) requester := newTestAuthorizeRequesterWithForm("resource-probe", clientID, url.Values{"resource": {resource}}) _, err := service.authorize(t.Context(), authorizeInput{ @@ -140,7 +141,7 @@ func TestAuthorizationServiceCollapsesResourceErrorsBeforeAuthentication(t *test func TestAuthorizationServiceConsentStepLogsNewClientAuthorization(t *testing.T) { db := testutils.NewDatabaseForTest(t) auditLogger := &fakeAuditLogger{} - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, auditLogger, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, auditLogger, nil) const ( userID = "test-user" @@ -183,7 +184,7 @@ func TestAuthorizationServiceConsentMergesAudienceQualifiedScopeKeys(t *testing. apiB = "https://api-b.example.com" ) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, auditLogger, userAccess(map[string][]string{ + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, auditLogger, userAccess(map[string][]string{ apiA: {"read"}, apiB: {"read"}, })) @@ -250,7 +251,7 @@ func TestAuthorizationServiceRequiresConsentForScopelessAPIAccess(t *testing.T) ) // The client may reach the API but was granted none of its permissions, which is the MCP-style scopeless grant - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, userAccess(map[string][]string{ + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, userAccess(map[string][]string{ api: {}, })) @@ -292,7 +293,7 @@ func TestAuthorizationServiceRequiresConsentForScopelessAPIAccess(t *testing.T) func TestAuthorizationServiceAuthorizeConsumesInteractionSession(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -376,7 +377,7 @@ func TestInteractionSessionServiceGetRejectsExpiredSession(t *testing.T) { func TestInteractionAPIClassifiesMissingSession(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) _, err := service.getInteractionSession(t.Context(), "missing-interaction") require.True(t, apperror.IsCode(err, apperror.CodeNotFound)) @@ -387,7 +388,7 @@ func TestInteractionAPIClassifiesMissingSession(t *testing.T) { func TestAuthorizationServiceAuthorizeBindsScopesToInteractionSession(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -442,7 +443,7 @@ func TestAuthorizationServiceAuthorizeBindsScopesToInteractionSession(t *testing func TestAuthorizationServiceAuthorizeRejectsInteractionSessionOfOtherClient(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -481,7 +482,7 @@ func TestAuthorizationServiceAuthorizeRejectsInteractionSessionOfOtherClient(t * func TestAuthorizationServiceAuthorizeSwitchesUserAndResetsRequirements(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -541,7 +542,7 @@ func TestAuthorizationServiceAuthorizeSwitchesUserAndResetsRequirements(t *testi func TestAuthorizationServiceAuthorizeRequiresLoginForUserBoundInteraction(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -574,7 +575,7 @@ func TestAuthorizationServiceAuthorizeRequiresLoginForUserBoundInteraction(t *te func TestAuthorizationServiceCompleteInteractionBindsUserToSession(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -610,7 +611,7 @@ func TestAuthorizationServiceCompleteInteractionBindsUserToSession(t *testing.T) func TestAuthorizationServiceCompleteInteractionSwitchesUserAndResetsRequirements(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -663,7 +664,7 @@ func TestAuthorizationServiceCompleteInteractionSwitchesUserAndResetsRequirement // still be required for that user rather than being inherited from the initiator. func TestAuthorizationServiceSelectAccountRecomputesConsentForSelectedUser(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( initiatorID = "initiator-user" @@ -725,7 +726,7 @@ func TestValidateClientPKCERequirement(t *testing.T) { // flag is honored for confidential clients (fosite only enforces PKCE for public clients). func TestAuthorizationServiceAuthorizeEnforcesPerClientPKCE(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -761,7 +762,7 @@ func TestAuthorizationServiceAuthorizeEnforcesPerClientPKCE(t *testing.T) { func TestAuthorizationServiceAuthorizePARRequiredClient(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -826,7 +827,7 @@ func TestAuthorizationServiceAuthorizePARRequiredClient(t *testing.T) { func TestAuthorizationServiceInteractionRequestQuery(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( clientID = "test-client" @@ -866,7 +867,7 @@ func TestAuthorizationServiceInteractionRequestQuery(t *testing.T) { func TestAuthorizationServiceAuthorizeUsesLoginAuthenticationTime(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -908,7 +909,7 @@ func TestAuthorizationServiceAuthorizeUsesLoginAuthenticationTime(t *testing.T) func TestAuthorizationServiceAuthorizeRequiresReauthenticationWhenMaxAgeExceeded(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -945,7 +946,7 @@ func TestAuthorizationServiceAuthorizeRequiresReauthenticationWhenMaxAgeExceeded func TestAuthorizationServiceAuthorizeUsesCompletedReauthenticationTime(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -992,7 +993,7 @@ func TestAuthorizationServiceAuthorizeUsesCompletedReauthenticationTime(t *testi func TestAuthorizationServiceAuthorizeUsesOriginalInteractionRequestTime(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" @@ -1143,7 +1144,7 @@ func TestConsentRequired(t *testing.T) { func TestAuthorizationServiceSkipConsentGrantsWithoutInteraction(t *testing.T) { db := testutils.NewDatabaseForTest(t) auditLogger := &fakeAuditLogger{} - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, auditLogger, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, auditLogger, nil) const ( userID = "test-user" @@ -1176,7 +1177,7 @@ func TestAuthorizationServiceSkipConsentGrantsWithoutInteraction(t *testing.T) { // A client with SkipConsent must still show the consent screen when the request explicitly asks for it with prompt=consent func TestAuthorizationServiceSkipConsentHonorsPromptConsent(t *testing.T) { db := testutils.NewDatabaseForTest(t) - service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil) + service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil) const ( userID = "test-user" diff --git a/backend/internal/oidc/claims_service.go b/backend/internal/oidc/claims_service.go index b7b0073e..1b78574e 100644 --- a/backend/internal/oidc/claims_service.go +++ b/backend/internal/oidc/claims_service.go @@ -9,7 +9,9 @@ import ( "strings" "github.com/pocket-id/fosite" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/common" + "github.com/pocket-id/pocket-id/backend/internal/emailverification" "github.com/pocket-id/pocket-id/backend/internal/model" "gorm.io/gorm" ) @@ -18,22 +20,40 @@ const ( idTokenType = "id-token" ) +const emailVerificationRequiredHint = "You must verify your email address before you can access this service." + type ClaimsService struct { db *gorm.DB customClaims CustomClaimSource baseURL string signer TokenSigner + appConfig appconfig.AppConfigResolver } -func newClaimsService(db *gorm.DB, customClaims CustomClaimSource, baseURL string, signer TokenSigner) *ClaimsService { +func newClaimsService(db *gorm.DB, customClaims CustomClaimSource, baseURL string, signer TokenSigner, appConfig appconfig.AppConfigResolver) *ClaimsService { return &ClaimsService{ db: db, customClaims: customClaims, baseURL: baseURL, signer: signer, + appConfig: appConfig, } } +// isEmailVerificationPending reports whether the user still has to verify their email before tokens can be issued for them +func (s *ClaimsService) isEmailVerificationPending(ctx context.Context, user *model.User) (bool, error) { + if !emailverification.CanBePending(user) { + return false, nil + } + + cfg, err := s.appConfig.GetConfig(ctx) + if err != nil { + return false, err + } + + return emailverification.IsPending(cfg, user), nil +} + // ValidateUserAccess re-checks, at token-issuance time, that the user behind a grant is // still allowed to obtain tokens for the client. func (s *ClaimsService) ValidateUserAccess(ctx context.Context, userID string, client Client) error { @@ -59,6 +79,15 @@ func (s *ClaimsService) ValidateUserAccess(ctx context.Context, userID string, c return fosite.ErrInvalidGrant.WithHint("The user account is disabled.") } + // The authorization middleware already stops these users at the authorize step, but grants issued before the email became unverified, such as refresh tokens, are only checked here + pending, err := s.isEmailVerificationPending(ctx, &user) + if err != nil { + return err + } + if pending { + return fosite.ErrInvalidGrant.WithHint(emailVerificationRequiredHint) + } + if !IsUserGroupAllowedToAuthorize(user, client.OidcClient) { return fosite.ErrAccessDenied.WithHint("You are not allowed to access this service.") } diff --git a/backend/internal/oidc/claims_service_test.go b/backend/internal/oidc/claims_service_test.go index ea44492e..7d1bc655 100644 --- a/backend/internal/oidc/claims_service_test.go +++ b/backend/internal/oidc/claims_service_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "gorm.io/gorm" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/model" testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" ) @@ -20,7 +21,7 @@ import ( // refresh token. func TestClaimsServiceValidateUserAccess(t *testing.T) { db := testutils.NewDatabaseForTest(t) - claimsService := newClaimsService(db, nil, "", nil) + claimsService := newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}) group := model.UserGroup{Base: model.Base{ID: "group-allowed"}, Name: "allowed", FriendlyName: "Allowed"} require.NoError(t, db.Create(&group).Error) @@ -71,6 +72,49 @@ func TestClaimsServiceValidateUserAccess(t *testing.T) { }) } +// requiredEmailVerification returns a configuration resolver that requires users to verify their email +func requiredEmailVerification() appconfig.StaticResolver { + cfg := appconfig.NewTestConfig(nil) + cfg.EmailVerification = appconfig.EmailVerificationRequired + return appconfig.StaticResolver{Config: cfg} +} + +// TestClaimsServiceValidateUserAccessEmailVerification makes sure users who still have to verify their email cannot redeem codes or refresh tokens when verification is required +func TestClaimsServiceValidateUserAccessEmailVerification(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + claimsService := newClaimsService(db, nil, "", nil, requiredEmailVerification()) + + unverifiedUser := model.User{Base: model.Base{ID: "user-unverified"}, Username: "unverified"} + require.NoError(t, db.Create(&unverifiedUser).Error) + verifiedUser := model.User{Base: model.Base{ID: "user-verified"}, Username: "verified", EmailVerified: true} + require.NoError(t, db.Create(&verifiedUser).Error) + unverifiedAdmin := model.User{Base: model.Base{ID: "admin-unverified"}, Username: "admin", IsAdmin: true} + require.NoError(t, db.Create(&unverifiedAdmin).Error) + + client := Client{OidcClient: model.OidcClient{Base: model.Base{ID: "client-open"}, Name: "Open"}} + + t.Run("unverified user is rejected with invalid_grant", func(t *testing.T) { + err := claimsService.ValidateUserAccess(t.Context(), unverifiedUser.ID, client) + require.ErrorIs(t, err, fosite.ErrInvalidGrant) + require.Contains(t, fosite.ErrorToRFC6749Error(err).GetDescription(), emailVerificationRequiredHint) + }) + + t.Run("verified user is allowed", func(t *testing.T) { + require.NoError(t, claimsService.ValidateUserAccess(t.Context(), verifiedUser.ID, client)) + }) + + t.Run("unverified admin is allowed", func(t *testing.T) { + require.NoError(t, claimsService.ValidateUserAccess(t.Context(), unverifiedAdmin.ID, client)) + }) + + t.Run("unverified user is allowed when verification is optional", func(t *testing.T) { + cfg := appconfig.NewTestConfig(nil) + cfg.EmailVerification = appconfig.EmailVerificationOptional + optional := newClaimsService(db, nil, "", nil, appconfig.StaticResolver{Config: cfg}) + require.NoError(t, optional.ValidateUserAccess(t.Context(), unverifiedUser.ID, client)) + }) +} + type fakeCustomClaimSource struct { claims []model.CustomClaim } @@ -94,7 +138,7 @@ func TestClaimsServiceGetUserClaims(t *testing.T) { {Key: "department", Value: "engineering"}, // plain string {Key: "roles", Value: `["admin","dev"]`}, // JSON document }} - service := newClaimsService(db, customClaims, baseURL, nil) + service := newClaimsService(db, customClaims, baseURL, nil, appconfig.StaticResolver{}) group := model.UserGroup{Base: model.Base{ID: "group-1"}, Name: "developers", FriendlyName: "Developers"} require.NoError(t, db.Create(&group).Error) @@ -164,7 +208,7 @@ func TestClaimsServiceGetUserClaimsKeepsIntegerCustomClaims(t *testing.T) { {Key: "nested", Value: `{"ids":[1234567890123456789]}`}, {Key: "not_json", Value: "1 2"}, }} - service := newClaimsService(db, customClaims, "", nil) + service := newClaimsService(db, customClaims, "", nil, appconfig.StaticResolver{}) claims, err := service.GetUserClaims(t.Context(), "user-1", []string{"profile"}) require.NoError(t, err) @@ -184,7 +228,7 @@ func TestClaimsServiceAppliesSigningAlgToIDTokenHeader(t *testing.T) { for _, alg := range []jwa.SignatureAlgorithm{jwa.RS256(), jwa.RS384(), jwa.ES512()} { t.Run(alg.String(), func(t *testing.T) { - service := newClaimsService(db, nil, "", algTestSigner{alg: alg}) + service := newClaimsService(db, nil, "", algTestSigner{alg: alg}, appconfig.StaticResolver{}) session := NewEmptySession() session.Subject = "alg-user" diff --git a/backend/internal/oidc/device_service_test.go b/backend/internal/oidc/device_service_test.go index a4279c7a..78229cbc 100644 --- a/backend/internal/oidc/device_service_test.go +++ b/backend/internal/oidc/device_service_test.go @@ -13,6 +13,7 @@ import ( "time" "github.com/pocket-id/fosite" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/apperror" "github.com/pocket-id/pocket-id/backend/internal/model" testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" @@ -164,7 +165,7 @@ func newTestDeviceService(t *testing.T, clientID, userID string, requiresReauthe }, nil) require.NoError(t, err) - claimsService := newClaimsService(db, nil, "", nil) + claimsService := newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}) authorizationService := newAuthorizationService(db, newInteractionSessionService(db), claimsService, reauth, &fakeAuditLogger{}, apiAccess) service := newDeviceService(provider, store, provider.deviceStrategy, authorizationService, claimsService, &fakeAuditLogger{}, db) diff --git a/backend/internal/oidc/module.go b/backend/internal/oidc/module.go index 9ce6f646..9e0a3c29 100644 --- a/backend/internal/oidc/module.go +++ b/backend/internal/oidc/module.go @@ -12,6 +12,7 @@ import ( "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" "gorm.io/gorm" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/auditlogs" "github.com/pocket-id/pocket-id/backend/internal/authz" "github.com/pocket-id/pocket-id/backend/internal/model" @@ -56,6 +57,7 @@ type Dependencies struct { Reauth ReauthenticationTokenConsumer AuditLog AuditLogger APIAccess APIAccessProvider + AppConfig appconfig.AppConfigResolver // CleanupDisabled skips registering the cron jobs that delete expired rows from the database, for example in tests CleanupDisabled bool @@ -97,7 +99,7 @@ func New(ctx context.Context, deps Dependencies) (*Module, error) { return nil, fmt.Errorf("failed to create OAuth2 provider: %w", err) } - claimsService := newClaimsService(deps.DB, deps.CustomClaims, deps.Config.BaseURL, deps.Signer) + claimsService := newClaimsService(deps.DB, deps.CustomClaims, deps.Config.BaseURL, deps.Signer, deps.AppConfig) previewBuilder := newClientPreviewBuilder(claimsService, provider.tokenStrategies) interactionSessionService := newInteractionSessionService(deps.DB) authorizationService := newAuthorizationService(deps.DB, interactionSessionService, claimsService, deps.Reauth, deps.AuditLog, deps.APIAccess) @@ -146,11 +148,13 @@ func (m *Module) RefreshClientMetadata(ctx context.Context, clientID string) (mo } func (m *Module) RegisterRoutes(root, api *authz.Router) { - root.Optional().GET("/authorize", authz.AccountSession, m.authorizationHandler.authorize) - root.Optional().POST("/authorize", authz.AccountSession, m.authorizationHandler.authorize) + // Browsers reach the authorize endpoint through a redirect, so authorization errors are shown on the interaction error page instead of as JSON + authorize := root.Use(m.authorizationHandler.redirectAuthorizationErrors).Optional() + authorize.GET("/authorize", authz.AccountAuthorize, m.authorizationHandler.authorize) + authorize.POST("/authorize", authz.AccountAuthorize, m.authorizationHandler.authorize) api.Public().GET("/oidc/interactions/:id", m.authorizationHandler.getInteractionSession) - api.POST("/oidc/interactions/:id/complete", authz.AccountSession, m.authorizationHandler.completeInteraction) + api.POST("/oidc/interactions/:id/complete", authz.AccountAuthorize, m.authorizationHandler.completeInteraction) api.Public().POST("/oidc/par", m.parHandler.pushedAuthorizationRequest) @@ -165,6 +169,6 @@ func (m *Module) RegisterRoutes(root, api *authz.Router) { api.Optional().POST("/oidc/end-session", authz.AccountSession, m.endSessionHandler.endSession) api.Public().POST("/oidc/device/authorize", m.deviceHandler.authorizeDevice) - api.POST("/oidc/device/verify", authz.AccountSession, m.deviceHandler.verifyDeviceCode) - api.GET("/oidc/device/info", authz.AccountSession, m.deviceHandler.deviceCodeInfo) + api.POST("/oidc/device/verify", authz.AccountAuthorize, m.deviceHandler.verifyDeviceCode) + api.GET("/oidc/device/info", authz.AccountAuthorize, m.deviceHandler.deviceCodeInfo) } diff --git a/backend/internal/oidc/preview_test.go b/backend/internal/oidc/preview_test.go index 58bdaab2..820b07c1 100644 --- a/backend/internal/oidc/preview_test.go +++ b/backend/internal/oidc/preview_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/model" testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" ) @@ -26,7 +27,7 @@ func TestClientPreviewBuilderUsesFositeTokenStrategies(t *testing.T) { }, nil) require.NoError(t, err) - builder := newClientPreviewBuilder(newClaimsService(db, nil, "https://issuer.example.com", nil), provider.tokenStrategies) + builder := newClientPreviewBuilder(newClaimsService(db, nil, "https://issuer.example.com", nil, appconfig.StaticResolver{}), provider.tokenStrategies) const ( userID = "test-user" @@ -89,7 +90,7 @@ func TestClientPreviewBuilderIgnoresUnknownScopes(t *testing.T) { // The preview mirrors the authorize endpoint: unknown scopes are dropped // from the previewed tokens instead of failing the preview. - builder := newClientPreviewBuilder(newClaimsService(db, nil, "https://issuer.example.com", nil), provider.tokenStrategies) + builder := newClientPreviewBuilder(newClaimsService(db, nil, "https://issuer.example.com", nil, appconfig.StaticResolver{}), provider.tokenStrategies) preview, err := builder.BuildClientPreview(t.Context(), model.OidcClient{ Base: model.Base{ID: "test-client"}, Name: "Test Client", diff --git a/backend/internal/oidc/refresh_token_test.go b/backend/internal/oidc/refresh_token_test.go index 03a2f478..4fabbf04 100644 --- a/backend/internal/oidc/refresh_token_test.go +++ b/backend/internal/oidc/refresh_token_test.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/require" "gorm.io/gorm" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/model" datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" @@ -42,7 +43,7 @@ func TestRefreshTokenRotationGrace(t *testing.T) { require.NoError(t, db.Create(&model.User{Base: model.Base{ID: userID}, Username: "grace"}).Error) provider, err := newProvider(store, nil, testTokenSigner{key: key}, Config{BaseURL: issuer, TokenBaseURL: issuer, Secret: []byte(secret)}, nil) require.NoError(t, err) - handler := newTokenHandler(provider, newClaimsService(db, nil, issuer, nil), nil) + handler := newTokenHandler(provider, newClaimsService(db, nil, issuer, nil, appconfig.StaticResolver{}), nil) // Seed an old grant so the grace period must start at rotation rather than token issuance request := newTestRequester(requestID, clientID, userID, "grace-jti") diff --git a/backend/internal/oidc/token_handler_test.go b/backend/internal/oidc/token_handler_test.go index afdf96de..127f5dd8 100644 --- a/backend/internal/oidc/token_handler_test.go +++ b/backend/internal/oidc/token_handler_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/require" "gorm.io/gorm" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/model" datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" @@ -63,7 +64,7 @@ func TestTokenHandlerClientCredentialsGrant(t *testing.T) { Secret: []byte(secret), }, nil) require.NoError(t, err) - handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), nil) + handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), nil) form := url.Values{"grant_type": {"client_credentials"}} req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/oidc/token", strings.NewReader(form.Encode())) @@ -122,7 +123,7 @@ func TestTokenHandlerClientCredentialsDropsIdentityScopes(t *testing.T) { Secret: []byte(secret), }, nil) require.NoError(t, err) - handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), nil) + handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), nil) form := url.Values{"grant_type": {"client_credentials"}, "scope": {"openid"}} req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/oidc/token", strings.NewReader(form.Encode())) @@ -182,7 +183,7 @@ func TestTokenHandlerClientCredentialsUsesClientSubjectGrants(t *testing.T) { Secret: []byte(secret), }, nil) require.NoError(t, err) - handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), apiAccess) + handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), apiAccess) requestToken := func(t *testing.T, scope string) map[string]any { t.Helper() @@ -253,7 +254,7 @@ func TestTokenHandlerClientCredentialsDefaultsResourceScopes(t *testing.T) { Secret: []byte(secret), }, nil) require.NoError(t, err) - handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), apiAccess) + handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), apiAccess) requestToken := func(t *testing.T, target string, form url.Values) map[string]any { t.Helper() @@ -408,7 +409,7 @@ func TestTokenHandlerRefreshGrantRevalidatesUser(t *testing.T) { Secret: []byte(secret), }, nil) require.NoError(t, err) - handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), nil) + handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), nil) form := url.Values{ "grant_type": {"refresh_token"}, @@ -592,7 +593,7 @@ func TestTokenHandlerRefreshGrantPreservesAudienceAndScope(t *testing.T) { Secret: []byte(secret), }, nil) require.NoError(t, err) - handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), apiAccess) + handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), apiAccess) form := url.Values{ "grant_type": {"refresh_token"}, @@ -759,7 +760,7 @@ func TestTokenHandlerClientSecretRotation(t *testing.T) { Secret: []byte(secret), }, nil) require.NoError(t, err) - handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), nil) + handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), nil) requestToken := func(t *testing.T, clientSecret string) map[string]any { t.Helper() @@ -827,7 +828,7 @@ func TestTokenHandlerClientWithoutSecrets(t *testing.T) { Secret: []byte(secret), }, nil) require.NoError(t, err) - handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), nil) + handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), nil) form := url.Values{"grant_type": {"client_credentials"}} req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/oidc/token", strings.NewReader(form.Encode())) diff --git a/backend/internal/oidc/userinfo_handler_test.go b/backend/internal/oidc/userinfo_handler_test.go index 51c036fd..3327e6a1 100644 --- a/backend/internal/oidc/userinfo_handler_test.go +++ b/backend/internal/oidc/userinfo_handler_test.go @@ -14,6 +14,7 @@ import ( "github.com/pocket-id/fosite" "github.com/stretchr/testify/require" + "github.com/pocket-id/pocket-id/backend/internal/appconfig" "github.com/pocket-id/pocket-id/backend/internal/model" testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" ) @@ -54,7 +55,7 @@ func TestUserInfoHandler(t *testing.T) { }, nil) require.NoError(t, err) - handler := newUserInfoHandler(provider, newClaimsService(db, nil, baseURL, nil), baseURL) + handler := newUserInfoHandler(provider, newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), baseURL) issueAccessToken := func(t *testing.T, requestID, subject string, scopes ...string) string { t.Helper() diff --git a/backend/internal/service/user_service.go b/backend/internal/service/user_service.go index 6222f2e6..df80fa9c 100644 --- a/backend/internal/service/user_service.go +++ b/backend/internal/service/user_service.go @@ -21,6 +21,7 @@ import ( "github.com/pocket-id/pocket-id/backend/internal/apperror" "github.com/pocket-id/pocket-id/backend/internal/backchannellogout" "github.com/pocket-id/pocket-id/backend/internal/dto" + "github.com/pocket-id/pocket-id/backend/internal/emailverification" "github.com/pocket-id/pocket-id/backend/internal/model" datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" "github.com/pocket-id/pocket-id/backend/internal/storage" @@ -36,9 +37,24 @@ type UserService struct { scimSyncScheduler ScimSyncScheduler backchannelLogout *backchannellogout.Service fileStorage storage.FileStorage + emailVerification EmailVerificationSender } -func NewUserService(db *gorm.DB, jwtService *JwtService, customClaimService *CustomClaimService, appImagesService *AppImagesService, scimSyncScheduler ScimSyncScheduler, backchannelLogout *backchannellogout.Service, fileStorage storage.FileStorage) *UserService { +// EmailVerificationSender emails verification links for a user's current email or for an email they want to change to +type EmailVerificationSender interface { + Send(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID string) error + RequestEmailChange(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID, newEmail string) error +} + +// ownEmailChange records how a user's change of their own email is handled once the update is committed +type ownEmailChange struct { + // applied is set when the new email replaced the current one right away + applied bool + // deferred holds the new email when it only replaces the current one after the user confirms it + deferred *string +} + +func NewUserService(db *gorm.DB, jwtService *JwtService, customClaimService *CustomClaimService, appImagesService *AppImagesService, scimSyncScheduler ScimSyncScheduler, backchannelLogout *backchannellogout.Service, fileStorage storage.FileStorage, emailVerification EmailVerificationSender) *UserService { return &UserService{ db: db, jwtService: jwtService, @@ -47,6 +63,7 @@ func NewUserService(db *gorm.DB, jwtService *JwtService, customClaimService *Cus scimSyncScheduler: scimSyncScheduler, backchannelLogout: backchannelLogout, fileStorage: fileStorage, + emailVerification: emailVerification, } } @@ -498,6 +515,16 @@ func (s *UserService) UpdateUser(ctx context.Context, cfg *appconfig.AppConfigMo } } + // A user's own email change may have to be confirmed first, so that's decided before the update is applied + var change ownEmailChange + if updateOwnUser { + var err error + change, err = s.prepareOwnEmailChange(ctx, cfg, userID, &updatedUser, isLdapSync, tx) + if err != nil { + return model.User{}, err + } + } + user, err := s.UpdateUserInternal(ctx, cfg, userID, updatedUser, updateOwnUser, isLdapSync, tx) if err != nil { return model.User{}, err @@ -516,9 +543,85 @@ func (s *UserService) UpdateUser(ctx context.Context, cfg *appconfig.AppConfigMo s.backchannelLogout.NotifyUser(ctx, userID) } + // The verified email stays in effect until the user confirms the new address through the emailed link + // The other changes are already saved, so the error tells the user that only the email change could not be started + if change.deferred != nil { + err = s.emailVerification.RequestEmailChange(ctx, cfg, userID, *change.deferred) + if err != nil { + return user, apperror.EmailChangeNotStarted(err) + } + } + + // A user whose new email still restricts the account needs the verification link right away + if change.applied && emailverification.IsPending(cfg, &user) { + err = s.emailVerification.Send(ctx, cfg, userID) + if err != nil { + slog.WarnContext(ctx, "Failed to send the email verification message after an email change", slog.String("userID", userID), slog.Any("error", err)) + } + } + return user, nil } +// prepareOwnEmailChange decides how a user's change of their own email is handled +// A deferred change keeps the current email in the update, so the new address only takes effect once the user confirms it +func (s *UserService) prepareOwnEmailChange(ctx context.Context, cfg *appconfig.AppConfigModel, userID string, updatedUser *dto.UserCreateDto, isLdapSync bool, tx *gorm.DB) (ownEmailChange, error) { + var change ownEmailChange + if s.emailVerification == nil || updatedUser.Email == nil { + return change, nil + } + + // Lock the user, so the decision below still holds when the update is applied + var user model.User + err := tx. + WithContext(ctx). + Where("id = ?", userID). + Clauses(clause.Locking{Strength: "UPDATE"}). + First(&user). + Error + if errors.Is(err, gorm.ErrRecordNotFound) { + return change, apperror.UserNotFound() + } + if err != nil { + return change, err + } + + // The update ignores an email the user may not edit, so there's nothing to verify then + emailChanged := user.Email == nil || *user.Email != *updatedUser.Email + if !emailChanged || !canEditProfile(cfg, &user, true, isLdapSync) { + return change, nil + } + + if !emailverification.DefersEmailChange(cfg, &user) { + change.applied = true + return change, nil + } + + // The address is checked now, so the user doesn't wait for a link to find out it's taken + taken, err := isEmailTaken(ctx, tx, user.ID, *updatedUser.Email) + if err != nil { + return change, err + } + if taken { + return change, apperror.AlreadyInUse("email") + } + + change.deferred = updatedUser.Email + updatedUser.Email = user.Email + return change, nil +} + +// canEditProfile reports whether an update may change the user's personal fields, such as their name and email +// LDAP users are managed by the directory and users may be barred from editing their own account, while the LDAP sync itself can always update them +func canEditProfile(cfg *appconfig.AppConfigModel, user *model.User, updateOwnUser bool, isLdapSync bool) bool { + if isLdapSync { + return true + } + + isLdapUser := user.LdapID != nil && cfg.LdapEnabled.IsTrue() + return !isLdapUser && (!updateOwnUser || cfg.AllowOwnAccountEdit.IsTrue()) +} + // UpdateUserInternal updates a user within an existing transaction // It's exported for the LDAP sync, which reconciles users and groups in a single transaction of its own func (s *UserService) UpdateUserInternal(ctx context.Context, cfg *appconfig.AppConfigModel, userID string, updatedUser dto.UserCreateDto, updateOwnUser bool, isLdapSync bool, tx *gorm.DB) (model.User, error) { @@ -540,15 +643,10 @@ func (s *UserService) UpdateUserInternal(ctx context.Context, cfg *appconfig.App return model.User{}, err } - // Check if this is an LDAP user and LDAP is enabled - isLdapUser := user.LdapID != nil && cfg.LdapEnabled.IsTrue() - allowOwnAccountEdit := cfg.AllowOwnAccountEdit.IsTrue() - - if !isLdapSync && (isLdapUser || (!allowOwnAccountEdit && updateOwnUser)) { + if !canEditProfile(cfg, &user, updateOwnUser, isLdapSync) { // Restricted update: Only locale can be changed when: // - User is from LDAP, OR // - User is editing their own account but global setting disallows self-editing - // (Exception: LDAP sync operations can update everything) user.Locale = updatedUser.Locale } else { // Full update: Allow updating all personal fields @@ -573,6 +671,11 @@ func (s *UserService) UpdateUserInternal(ctx context.Context, cfg *appconfig.App } } + // A verified account is no longer deleted for missing the signup verification deadline + if user.EmailVerified { + user.EmailVerificationDeadline = nil + } + user.UpdatedAt = new(datatype.DateTime(time.Now())) err = tx. @@ -669,22 +772,20 @@ func (s *UserService) UpdateUserGroups(ctx context.Context, id string, userGroup } func (s *UserService) checkDuplicatedFields(ctx context.Context, user model.User, tx *gorm.DB) error { + if user.Email != nil { + taken, err := isEmailTaken(ctx, tx, user.ID, *user.Email) + if err != nil { + return err + } + if taken { + return apperror.AlreadyInUse("email") + } + } + var result struct { Found bool } err := tx. - WithContext(ctx). - Raw(`SELECT EXISTS(SELECT 1 FROM users WHERE id != ? AND email = ?) AS found`, user.ID, user.Email). - First(&result). - Error - if err != nil { - return err - } - if result.Found { - return apperror.AlreadyInUse("email") - } - - err = tx. WithContext(ctx). Raw(`SELECT EXISTS(SELECT 1 FROM users WHERE id != ? AND username = ?) AS found`, user.ID, user.Username). First(&result). @@ -740,3 +841,16 @@ func (s *UserService) DisableUserInternal(ctx context.Context, tx *gorm.DB, user return nil } + +// isEmailTaken reports whether a user other than the given one already uses the email +func isEmailTaken(ctx context.Context, tx *gorm.DB, userID, email string) (bool, error) { + var result struct { + Found bool + } + err := tx. + WithContext(ctx). + Raw(`SELECT EXISTS(SELECT 1 FROM users WHERE id != ? AND email = ?) AS found`, userID, email). + First(&result). + Error + return result.Found, err +} diff --git a/backend/internal/service/user_service_test.go b/backend/internal/service/user_service_test.go index ae1910ed..bc67ba49 100644 --- a/backend/internal/service/user_service_test.go +++ b/backend/internal/service/user_service_test.go @@ -1,7 +1,9 @@ package service import ( + "context" "encoding/json" + "errors" "strings" "testing" "time" @@ -34,6 +36,7 @@ func newTestUserService(t *testing.T) (*UserService, *UserGroupService) { nil, nil, fileStorage, + nil, ) groupService := NewUserGroupService(db, nil, nil) @@ -204,3 +207,174 @@ func TestDeleteUserBumpsGroupUpdatedAt(t *testing.T) { require.Empty(t, updated.Users) require.True(t, updated.LastModified().After(past), "deleting a member must bump the group's UpdatedAt") } + +type recordingEmailVerificationSender struct { + sent []string + requested []string + requestErr error +} + +func (r *recordingEmailVerificationSender) Send(_ context.Context, _ *appconfig.AppConfigModel, userID string) error { + r.sent = append(r.sent, userID) + return nil +} + +func (r *recordingEmailVerificationSender) RequestEmailChange(_ context.Context, _ *appconfig.AppConfigModel, _, newEmail string) error { + r.requested = append(r.requested, newEmail) + return r.requestErr +} + +func TestUpdateOwnEmailWithVerification(t *testing.T) { + // setup creates a user with the given verification state and an update that changes only their email + setup := func(t *testing.T, mode string, emailVerified bool) (*UserService, *recordingEmailVerificationSender, *appconfig.AppConfigModel, model.User, dto.UserCreateDto) { + t.Helper() + + config := appconfig.NewTestConfig(nil) + config.EmailVerification = appconfig.AppConfigValue(mode) + userService, _ := newTestUserService(t) + sender := &recordingEmailVerificationSender{} + userService.emailVerification = sender + + user, err := userService.CreateUser(t.Context(), config, dto.UserCreateDto{ + Username: "changer", + Email: new("old@example.com"), + EmailVerified: emailVerified, + FirstName: "Email", + LastName: "Changer", + }) + require.NoError(t, err) + + update := dto.UserCreateDto{ + Username: user.Username, + Email: new("new@example.com"), + FirstName: user.FirstName, + LastName: user.LastName, + } + return userService, sender, config, user, update + } + + t.Run("a verified email stays in effect until the new address is confirmed", func(t *testing.T) { + userService, sender, config, user, update := setup(t, appconfig.EmailVerificationRequired, true) + + updated, err := userService.UpdateUser(t.Context(), config, user.ID, update, true, false) + require.NoError(t, err) + + require.Equal(t, "old@example.com", *updated.Email) + require.True(t, updated.EmailVerified) + require.Equal(t, []string{"new@example.com"}, sender.requested) + require.Empty(t, sender.sent) + }) + + t.Run("a deferred change to an address another user has is rejected right away", func(t *testing.T) { + userService, sender, config, user, update := setup(t, appconfig.EmailVerificationRequired, true) + _, err := userService.CreateUser(t.Context(), config, dto.UserCreateDto{Username: "other", Email: new("new@example.com"), FirstName: "Other"}) + require.NoError(t, err) + + _, err = userService.UpdateUser(t.Context(), config, user.ID, update, true, false) + require.True(t, apperror.IsCode(err, apperror.CodeAlreadyInUse)) + require.Empty(t, sender.requested) + }) + + t.Run("an unverified email is replaced right away and the new one has to be verified", func(t *testing.T) { + userService, sender, config, user, update := setup(t, appconfig.EmailVerificationRequired, false) + + updated, err := userService.UpdateUser(t.Context(), config, user.ID, update, true, false) + require.NoError(t, err) + + require.Equal(t, "new@example.com", *updated.Email) + require.False(t, updated.EmailVerified) + require.Equal(t, []string{user.ID}, sender.sent) + require.Empty(t, sender.requested) + }) + + t.Run("the other fields are saved while the email change waits for confirmation", func(t *testing.T) { + userService, _, config, user, update := setup(t, appconfig.EmailVerificationRequired, true) + update.FirstName = "Renamed" + + updated, err := userService.UpdateUser(t.Context(), config, user.ID, update, true, false) + require.NoError(t, err) + + require.Equal(t, "Renamed", updated.FirstName) + require.Equal(t, "old@example.com", *updated.Email) + }) + + t.Run("a change link that can't be sent is reported apart from the saved fields", func(t *testing.T) { + userService, sender, config, user, update := setup(t, appconfig.EmailVerificationRequired, true) + sender.requestErr = errors.New("smtp unavailable") + update.FirstName = "Renamed" + + updated, err := userService.UpdateUser(t.Context(), config, user.ID, update, true, false) + require.True(t, apperror.IsCode(err, apperror.CodeEmailChangeNotStarted)) + require.Equal(t, "Renamed", updated.FirstName) + + stored, err := userService.GetUser(t.Context(), user.ID) + require.NoError(t, err) + require.Equal(t, "Renamed", stored.FirstName) + require.Equal(t, "old@example.com", *stored.Email) + }) + + t.Run("no change link is sent when the user may not edit their own account", func(t *testing.T) { + userService, sender, config, user, update := setup(t, appconfig.EmailVerificationRequired, true) + config.AllowOwnAccountEdit = "false" + + updated, err := userService.UpdateUser(t.Context(), config, user.ID, update, true, false) + require.NoError(t, err) + + require.Equal(t, "old@example.com", *updated.Email) + require.Empty(t, sender.requested) + require.Empty(t, sender.sent) + }) + + t.Run("an admin changing another user's email replaces it right away", func(t *testing.T) { + userService, sender, config, user, update := setup(t, appconfig.EmailVerificationRequired, true) + update.EmailVerified = true + + updated, err := userService.UpdateUser(t.Context(), config, user.ID, update, false, false) + require.NoError(t, err) + + require.Equal(t, "new@example.com", *updated.Email) + require.Empty(t, sender.requested) + require.Empty(t, sender.sent) + }) + + t.Run("the email is replaced right away when verification is optional", func(t *testing.T) { + userService, sender, config, user, update := setup(t, appconfig.EmailVerificationOptional, true) + + updated, err := userService.UpdateUser(t.Context(), config, user.ID, update, true, false) + require.NoError(t, err) + + require.Equal(t, "new@example.com", *updated.Email) + require.False(t, updated.EmailVerified) + require.Empty(t, sender.sent) + require.Empty(t, sender.requested) + }) +} + +func TestMarkingEmailVerifiedClearsSignupDeadline(t *testing.T) { + config := appconfig.NewTestConfig(nil) + userService, _ := newTestUserService(t) + + user, err := userService.CreateUser(t.Context(), config, dto.UserCreateDto{ + Username: "pending", + Email: new("pending@example.com"), + FirstName: "Pending", + LastName: "User", + }) + require.NoError(t, err) + deadline := datatype.DateTime(time.Now().Add(time.Hour)) + require.NoError(t, userService.db.Model(&model.User{}).Where("id = ?", user.ID).Update("email_verification_deadline", deadline).Error) + + updated, err := userService.UpdateUser(t.Context(), config, user.ID, dto.UserCreateDto{ + Username: user.Username, + Email: user.Email, + EmailVerified: true, + FirstName: user.FirstName, + LastName: user.LastName, + }, false, false) + require.NoError(t, err) + require.True(t, updated.EmailVerified) + + stored, err := userService.GetUser(t.Context(), user.ID) + require.NoError(t, err) + require.Nil(t, stored.EmailVerificationDeadline) +} diff --git a/backend/internal/usersignup/cleanup_job.go b/backend/internal/usersignup/cleanup_job.go new file mode 100644 index 00000000..4d88a4a2 --- /dev/null +++ b/backend/internal/usersignup/cleanup_job.go @@ -0,0 +1,113 @@ +package usersignup + +import ( + "context" + "fmt" + "log/slog" + "time" + + "github.com/italypaleale/francis/builtin/cronjob" + "gorm.io/gorm" + + "github.com/pocket-id/pocket-id/backend/internal/appconfig" + "github.com/pocket-id/pocket-id/backend/internal/model" + datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" +) + +const ( + // cleanupJobInterval is how often overdue signups are deleted, which bounds how long an account can outlive its deadline + cleanupJobInterval = time.Hour + // cleanupJobJitter spreads each occurrence around its scheduled time, so it doesn't coincide with the other cleanup jobs + cleanupJobJitter = 5 * time.Minute +) + +// UserDeleter deletes a user together with everything that references it +type UserDeleter interface { + DeleteUser(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID string, allowLdapDelete bool) error +} + +type cleanupJob struct { + db *gorm.DB + appConfig appconfig.AppConfigResolver + users UserDeleter +} + +func newCleanupJob(db *gorm.DB, appConfig appconfig.AppConfigResolver, users UserDeleter) (*cronjob.CronJob, error) { + job := &cleanupJob{ + db: db, + appConfig: appConfig, + users: users, + } + + cronActor, err := cronjob.New( + "DeleteUnverifiedSignups", + cronjob.WithJob(job.deleteUnverifiedSignups), + cronjob.WithInterval(cleanupJobInterval), + cronjob.WithJitter(cleanupJobJitter), + // Also run right after the job is first registered, so accounts that missed their deadline while Pocket ID wasn't running are removed at startup + cronjob.WithImmediate(), + cronjob.WithLogger(slog.Default()), + ) + if err != nil { + return nil, fmt.Errorf("error creating unverified signup cleanup cron job: %w", err) + } + + return cronActor, nil +} + +// deleteUnverifiedSignups deletes self-signed-up accounts that did not verify their email before their deadline +func (j *cleanupJob) deleteUnverifiedSignups(ctx context.Context) error { + dbConfig, err := j.appConfig.GetConfig(ctx) + if err != nil { + return fmt.Errorf("error loading app config: %w", err) + } + + // Deadlines only apply while verification is required + // Clearing them otherwise keeps accounts that were used in the meantime from being deleted once verification is required again + if dbConfig.EmailVerification.String() != appconfig.EmailVerificationRequired { + st := j.db. + WithContext(ctx). + Model(&model.User{}). + Where("email_verification_deadline IS NOT NULL"). + Update("email_verification_deadline", nil) + if st.Error != nil { + return fmt.Errorf("failed to clear email verification deadlines: %w", st.Error) + } + if st.RowsAffected > 0 { + slog.InfoContext(ctx, "Cleared email verification deadlines because verification is no longer required", slog.Int64("count", st.RowsAffected)) + } + return nil + } + + // Load the overdue accounts + // Admins are exempt from email verification, so an account promoted to admin before its deadline is kept + var userIDs []string + err = j.db. + WithContext(ctx). + Model(&model.User{}). + Where("email_verification_deadline < ? AND email_verified = ? AND is_admin = ?", datatype.DateTime(time.Now()), false, false). + Pluck("id", &userIDs). + Error + if err != nil { + return fmt.Errorf("failed to list accounts that missed their email verification deadline: %w", err) + } + if len(userIDs) == 0 { + return nil + } + + // Delete each account through the user service so its client sessions, SCIM peers and profile picture are cleaned up as well + // A failure is logged and the remaining accounts are still processed + deleted := 0 + for _, userID := range userIDs { + err = j.users.DeleteUser(ctx, dbConfig, userID, false) + if err != nil { + slog.ErrorContext(ctx, "Failed to delete an account that missed its email verification deadline", slog.String("userID", userID), slog.Any("error", err)) + continue + } + deleted++ + } + + slog.InfoContext(ctx, "Deleted accounts that missed their email verification deadline", slog.Int("count", deleted)) + + return nil +} diff --git a/backend/internal/usersignup/cleanup_job_test.go b/backend/internal/usersignup/cleanup_job_test.go new file mode 100644 index 00000000..52b0d28b --- /dev/null +++ b/backend/internal/usersignup/cleanup_job_test.go @@ -0,0 +1,76 @@ +package usersignup + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + "gorm.io/gorm" + + "github.com/pocket-id/pocket-id/backend/internal/appconfig" + "github.com/pocket-id/pocket-id/backend/internal/model" + datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" + testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" +) + +type testUserDeleter struct { + db *gorm.DB + deleted []string +} + +func (d *testUserDeleter) DeleteUser(ctx context.Context, _ *appconfig.AppConfigModel, userID string, _ bool) error { + d.deleted = append(d.deleted, userID) + return d.db.WithContext(ctx).Delete(&model.User{}, "id = ?", userID).Error +} + +func TestCleanupJobDeletesUnverifiedSignups(t *testing.T) { + // seed creates users whose deadline lies in the past or future, covering every condition the job checks + seed := func(t *testing.T, db *gorm.DB) { + t.Helper() + + past := new(datatype.DateTime(time.Now().Add(-time.Minute))) + future := new(datatype.DateTime(time.Now().Add(time.Hour))) + users := []model.User{ + {Base: model.Base{ID: "overdue"}, Username: "overdue", EmailVerificationDeadline: past}, + {Base: model.Base{ID: "not-yet-due"}, Username: "not-yet-due", EmailVerificationDeadline: future}, + {Base: model.Base{ID: "promoted-admin"}, Username: "promoted-admin", IsAdmin: true, EmailVerificationDeadline: past}, + {Base: model.Base{ID: "verified"}, Username: "verified", EmailVerified: true, EmailVerificationDeadline: past}, + {Base: model.Base{ID: "existing"}, Username: "existing"}, + } + require.NoError(t, db.Create(&users).Error) + } + + newJob := func(db *gorm.DB, mode string) (*cleanupJob, *testUserDeleter) { + cfg := appconfig.NewTestConfig(nil) + cfg.EmailVerification = appconfig.AppConfigValue(mode) + deleter := &testUserDeleter{db: db} + return &cleanupJob{db: db, appConfig: appconfig.StaticResolver{Config: cfg}, users: deleter}, deleter + } + + t.Run("deletes only overdue unverified regular users when verification is required", func(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + seed(t, db) + job, deleter := newJob(db, appconfig.EmailVerificationRequired) + + require.NoError(t, job.deleteUnverifiedSignups(t.Context())) + + require.Equal(t, []string{"overdue"}, deleter.deleted) + var remaining []string + require.NoError(t, db.Model(&model.User{}).Order("id").Pluck("id", &remaining).Error) + require.Equal(t, []string{"existing", "not-yet-due", "promoted-admin", "verified"}, remaining) + }) + + t.Run("clears every deadline instead of deleting when verification is not required", func(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + seed(t, db) + job, deleter := newJob(db, appconfig.EmailVerificationOptional) + + require.NoError(t, job.deleteUnverifiedSignups(t.Context())) + + require.Empty(t, deleter.deleted) + var withDeadline int64 + require.NoError(t, db.Model(&model.User{}).Where("email_verification_deadline IS NOT NULL").Count(&withDeadline).Error) + require.Zero(t, withDeadline) + }) +} diff --git a/backend/internal/usersignup/module.go b/backend/internal/usersignup/module.go index 102968f7..52aeebe8 100644 --- a/backend/internal/usersignup/module.go +++ b/backend/internal/usersignup/module.go @@ -29,6 +29,10 @@ type UserCreator interface { CreateUserInternal(ctx context.Context, dbConfig *appconfig.AppConfigModel, input dto.UserCreateDto, isLdapSync bool, tx *gorm.DB) (model.User, error) } +type EmailVerificationSender interface { + Send(ctx context.Context, dbConfig *appconfig.AppConfigModel, userID string) error +} + // ScimSyncScheduler schedules SCIM after the signup transaction has committed type ScimSyncScheduler interface { ScheduleSync(ctx context.Context) @@ -43,6 +47,12 @@ type Dependencies struct { UserCreator UserCreator AppConfig appconfig.AppConfigResolver ScimSync ScimSyncScheduler + UserDeleter UserDeleter + + EmailVerification EmailVerificationSender + + // CleanupDisabled skips registering the cron job that deletes signups that missed their email verification deadline, for example in tests + CleanupDisabled bool } type Module struct { @@ -58,6 +68,19 @@ func New(deps Dependencies) (*Module, error) { return nil, fmt.Errorf("error registering the %s actor: %w", SignupTokenActorType, err) } + // Register the job that deletes self-signed-up accounts that did not verify their email in time + if !deps.CleanupDisabled { + job, err := newCleanupJob(deps.DB, deps.AppConfig, deps.UserDeleter) + if err != nil { + return nil, err + } + + err = deps.Actors.RegisterBuiltInActor(job) + if err != nil { + return nil, fmt.Errorf("error registering unverified signup cleanup cron actor: %w", err) + } + } + service := newService(deps, deps.Actors.Service()) return &Module{ service: service, diff --git a/backend/internal/usersignup/service.go b/backend/internal/usersignup/service.go index 350a06f7..f1c66ddc 100644 --- a/backend/internal/usersignup/service.go +++ b/backend/internal/usersignup/service.go @@ -17,6 +17,7 @@ import ( "github.com/pocket-id/pocket-id/backend/internal/auditlogs" "github.com/pocket-id/pocket-id/backend/internal/common" "github.com/pocket-id/pocket-id/backend/internal/dto" + "github.com/pocket-id/pocket-id/backend/internal/emailverification" "github.com/pocket-id/pocket-id/backend/internal/model" datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" "github.com/pocket-id/pocket-id/backend/internal/utils" @@ -26,23 +27,28 @@ import ( // It must match the value emitted by the JWT service in the access token's "amr" claim const authenticationMethodOneTimePassword = "otp" +// signupVerificationPeriod is how long a self-signed-up account has to verify its email before it is deleted +const signupVerificationPeriod = emailverification.TokenLifetime + type Service struct { - db *gorm.DB - actorService *actor.Service - userCreator UserCreator - signer TokenService - auditLog AuditLogger - scimSync ScimSyncScheduler + db *gorm.DB + actorService *actor.Service + userCreator UserCreator + signer TokenService + auditLog AuditLogger + scimSync ScimSyncScheduler + emailVerification EmailVerificationSender } func newService(deps Dependencies, actorService *actor.Service) *Service { return &Service{ - db: deps.DB, - actorService: actorService, - userCreator: deps.UserCreator, - signer: deps.Signer, - auditLog: deps.AuditLog, - scimSync: deps.ScimSync, + db: deps.DB, + actorService: actorService, + userCreator: deps.UserCreator, + signer: deps.Signer, + auditLog: deps.AuditLog, + scimSync: deps.ScimSync, + emailVerification: deps.EmailVerification, } } @@ -94,6 +100,13 @@ func (s *Service) SignUp(ctx context.Context, config *appconfig.AppConfigModel, return model.User{}, "", err } + if emailverification.IsPending(config, &user) { + err = s.emailVerification.Send(ctx, config, user.ID) + if err != nil { + slog.WarnContext(ctx, "Failed to send the email verification message after signup", slog.String("userID", user.ID), slog.Any("error", err)) + } + } + return user, accessToken, nil } @@ -110,6 +123,16 @@ func (s *Service) createSignedUpUser(ctx context.Context, config *appconfig.AppC return model.User{}, "", err } + // Give the account a deadline to verify its email, after which it is deleted so abandoned signups cannot squat an address + if emailverification.IsPending(config, &user) { + deadline := datatype.DateTime(time.Now().Add(signupVerificationPeriod)) + err = tx.WithContext(ctx).Model(&user).Update("email_verification_deadline", deadline).Error + if err != nil { + return model.User{}, "", err + } + user.EmailVerificationDeadline = &deadline + } + accessToken, err := s.signer.GenerateAccessToken(user, "", config.SessionDuration.AsDurationMinutes()) if err != nil { return model.User{}, "", err diff --git a/backend/internal/usersignup/service_test.go b/backend/internal/usersignup/service_test.go index 0090f002..aa837ce9 100644 --- a/backend/internal/usersignup/service_test.go +++ b/backend/internal/usersignup/service_test.go @@ -30,6 +30,25 @@ func (f fakeUserCreator) CreateUserInternal(_ context.Context, _ *appconfig.AppC return f.user, nil } +// dbUserCreator stores the user in the signup transaction, so changes made later in that transaction can be asserted +type dbUserCreator struct{} + +func (dbUserCreator) CreateUserInternal(_ context.Context, _ *appconfig.AppConfigModel, input dto.UserCreateDto, _ bool, tx *gorm.DB) (model.User, error) { + user := model.User{Username: input.Username, Email: input.Email, EmailVerified: input.EmailVerified} + err := tx.Create(&user).Error + return user, err +} + +type fakeEmailVerificationSender struct { + err error + userIDs []string +} + +func (f *fakeEmailVerificationSender) Send(_ context.Context, _ *appconfig.AppConfigModel, userID string) error { + f.userIDs = append(f.userIDs, userID) + return f.err +} + type fakeSigner struct{} func (fakeSigner) GenerateAccessToken(_ model.User, _ string, _ time.Duration) (string, error) { @@ -164,3 +183,56 @@ func listAllOptions() utils.ListRequestOptions { opts.Pagination.Limit = 100 return opts } + +func TestSignUpRequiresEmailVerification(t *testing.T) { + signUp := func(t *testing.T, mode string, sender *fakeEmailVerificationSender) (*gorm.DB, model.User) { + t.Helper() + + db := testutils.NewDatabaseForTest(t) + svc := newSignupServiceForTest(t, db, dbUserCreator{}) + svc.emailVerification = sender + + config := appconfig.NewTestConfig(nil) + config.AllowUserSignups = "open" + config.EmailVerification = appconfig.AppConfigValue(mode) + user, _, err := svc.SignUp(t.Context(), config, signUpDto{ + Username: "newuser", + Email: new("newuser@example.com"), + }, "1.2.3.4", "test-agent") + require.NoError(t, err) + + return db, user + } + + t.Run("sets a deadline and sends the verification email when verification is required", func(t *testing.T) { + sender := &fakeEmailVerificationSender{} + before := time.Now() + db, user := signUp(t, appconfig.EmailVerificationRequired, sender) + + require.NotNil(t, user.EmailVerificationDeadline) + require.WithinDuration(t, before.Add(24*time.Hour), user.EmailVerificationDeadline.ToTime(), time.Minute) + + var stored model.User + require.NoError(t, db.First(&stored, "id = ?", user.ID).Error) + require.NotNil(t, stored.EmailVerificationDeadline) + require.Equal(t, []string{user.ID}, sender.userIDs) + }) + + t.Run("keeps the account without a deadline when verification is optional", func(t *testing.T) { + sender := &fakeEmailVerificationSender{} + db, user := signUp(t, appconfig.EmailVerificationOptional, sender) + + var stored model.User + require.NoError(t, db.First(&stored, "id = ?", user.ID).Error) + require.Nil(t, stored.EmailVerificationDeadline) + require.Empty(t, sender.userIDs) + }) + + t.Run("a failed delivery does not fail the signup", func(t *testing.T) { + sender := &fakeEmailVerificationSender{err: errors.New("smtp unavailable")} + _, user := signUp(t, appconfig.EmailVerificationRequired, sender) + + require.NotNil(t, user.EmailVerificationDeadline) + require.Equal(t, []string{user.ID}, sender.userIDs) + }) +} diff --git a/backend/resources/migrations/postgres/20261011120000_email_verification_deadline.down.sql b/backend/resources/migrations/postgres/20261011120000_email_verification_deadline.down.sql new file mode 100644 index 00000000..61a1cd5e --- /dev/null +++ b/backend/resources/migrations/postgres/20261011120000_email_verification_deadline.down.sql @@ -0,0 +1 @@ +ALTER TABLE users DROP COLUMN email_verification_deadline; diff --git a/backend/resources/migrations/postgres/20261011120000_email_verification_deadline.up.sql b/backend/resources/migrations/postgres/20261011120000_email_verification_deadline.up.sql new file mode 100644 index 00000000..9ed2ffb7 --- /dev/null +++ b/backend/resources/migrations/postgres/20261011120000_email_verification_deadline.up.sql @@ -0,0 +1 @@ +ALTER TABLE users ADD COLUMN email_verification_deadline TIMESTAMPTZ; diff --git a/backend/resources/migrations/sqlite/20261011120000_email_verification_deadline.down.sql b/backend/resources/migrations/sqlite/20261011120000_email_verification_deadline.down.sql new file mode 100644 index 00000000..61a1cd5e --- /dev/null +++ b/backend/resources/migrations/sqlite/20261011120000_email_verification_deadline.down.sql @@ -0,0 +1 @@ +ALTER TABLE users DROP COLUMN email_verification_deadline; diff --git a/backend/resources/migrations/sqlite/20261011120000_email_verification_deadline.up.sql b/backend/resources/migrations/sqlite/20261011120000_email_verification_deadline.up.sql new file mode 100644 index 00000000..9927e8fb --- /dev/null +++ b/backend/resources/migrations/sqlite/20261011120000_email_verification_deadline.up.sql @@ -0,0 +1 @@ +ALTER TABLE users ADD COLUMN email_verification_deadline DATETIME; diff --git a/frontend/messages/en.json b/frontend/messages/en.json index cf93d3ae..e3c907f2 100644 --- a/frontend/messages/en.json +++ b/frontend/messages/en.json @@ -567,7 +567,23 @@ "email_verification_warning": "Verify your email address", "email_verification_warning_description": "Your email address is not verified yet. Please verify it as soon as possible.", "email_verification": "Email Verification", - "email_verification_description": "Send a verification email to users when they sign up or change their email address.", + "email_verification_description": "Choose whether users verify their email address and whether unverified accounts are restricted.", + "email_verification_disabled_description": "Users are not asked to verify their email address.", + "email_verification_optional": "Optional", + "email_verification_optional_description": "Users can verify their email address, but unverified accounts keep full access.", + "email_verification_required": "Required", + "email_verification_required_description": "Users can't sign in to applications until they verify their email address. Self-signed-up accounts that aren't verified within 24 hours are deleted.", + "email_verification_required_needs_user_email": "Requiring email verification also requires users to have an email address.", + "email_verification_required_warning": "Verify your email address to use your account", + "email_verification_required_warning_description": "Your access is limited until you verify your email address.", + "email_verification_deadline_description": "If you don't, your account will be deleted {time}.", + "verify_email_address": "Verify email address", + "email_verification_required_error": "You must verify your email address before you can sign in to applications", + "email_change_not_started": "Your other changes were saved, but the email to confirm your new email address couldn't be sent. Please try again.", + "email_change_pending_title": "Confirm your new email address", + "email_change_pending_description": "We sent a confirmation link to {email}. Your current email address stays in use until you confirm the change.", + "cancel_email_change": "Cancel change", + "email_change_canceled": "Email change canceled.", "email_verification_success_title": "Email Verified Successfully", "email_verification_success_description": "Your email address has been verified successfully.", "email_verification_error_title": "Email Verification Failed", diff --git a/frontend/src/lib/components/email-verification-state-box.svelte b/frontend/src/lib/components/email-verification-state-box.svelte index dae8b86a..2ffc73b8 100644 --- a/frontend/src/lib/components/email-verification-state-box.svelte +++ b/frontend/src/lib/components/email-verification-state-box.svelte @@ -7,8 +7,10 @@ import UserService from '#lib/services/user-service.ts'; import appConfigStore from '#lib/stores/application-configuration-store.ts'; import userStore from '#lib/stores/user-store.ts'; + import { isEmailVerificationPending } from '#lib/utils/email-verification-util.ts'; import { axiosErrorToast } from '#lib/utils/error-util.ts'; import { LucideAlertTriangle, LucideCheckCircle2, LucideCircleX } from '@lucide/svelte'; + import { formatDistanceToNow } from 'date-fns'; import { onMount } from 'svelte'; import { toast } from 'svelte-sonner'; import { get } from 'svelte/store'; @@ -17,6 +19,10 @@ let emailVerificationState = $state(page.url.searchParams.get('emailVerificationState')); + const emailVerificationRequired = $derived( + isEmailVerificationPending($userStore, $appConfigStore) + ); + async function sendEmailVerification() { await userService .sendEmailVerification() @@ -60,15 +66,27 @@ {/if} -{:else if $userStore && $appConfigStore.emailVerificationEnabled && !$userStore.emailVerified} - - +{:else if $userStore && $appConfigStore.emailVerification !== 'disabled' && !$userStore.emailVerified} + +
- {m.email_verification_warning()} - - {m.email_verification_warning_description()} - + {#if emailVerificationRequired} + {m.email_verification_required_warning()} + + {m.email_verification_required_warning_description()} + {#if $userStore.emailVerificationDeadline} + {m.email_verification_deadline_description({ + time: formatDistanceToNow($userStore.emailVerificationDeadline, { addSuffix: true }) + })} + {/if} + + {:else} + {m.email_verification_warning()} + + {m.email_verification_warning_description()} + + {/if}
- + {#if emailVerificationRequired} + + {:else} + + {/if} {:else if deviceLoginInfo}
{/if}
- + {#if errorCode === 'email_verification_required'} + + {:else} + + {/if} diff --git a/frontend/src/routes/interaction/error/+page.svelte b/frontend/src/routes/interaction/error/+page.svelte index 26a457ba..196bec7d 100644 --- a/frontend/src/routes/interaction/error/+page.svelte +++ b/frontend/src/routes/interaction/error/+page.svelte @@ -2,6 +2,9 @@ import SignInWrapper from '#lib/components/login-wrapper.svelte'; import { Button } from '#lib/components/ui/button/index.ts'; import { m } from '#lib/paraglide/messages.js'; + import appConfigStore from '#lib/stores/application-configuration-store.ts'; + import userStore from '#lib/stores/user-store.ts'; + import { isEmailVerificationPending } from '#lib/utils/email-verification-util.ts'; import ClientProviderImages from '../../authorize/components/client-provider-images.svelte'; import type { PageProps } from './$types'; @@ -22,7 +25,20 @@ {error}

- +
+ {#if isEmailVerificationPending($userStore, $appConfigStore)} +
+ + +
+ {:else} + + {/if} +
diff --git a/frontend/src/routes/settings/+layout.svelte b/frontend/src/routes/settings/+layout.svelte index fe9073d7..bc551735 100644 --- a/frontend/src/routes/settings/+layout.svelte +++ b/frontend/src/routes/settings/+layout.svelte @@ -7,6 +7,7 @@ import { m } from '#lib/paraglide/messages.js'; import appConfigStore from '#lib/stores/application-configuration-store.ts'; import userStore from '#lib/stores/user-store.ts'; + import { isEmailVerificationPending } from '#lib/utils/email-verification-util.ts'; import { LucideTriangleAlert } from '@lucide/svelte'; import type { Snippet } from 'svelte'; import { fade, fly } from 'svelte/transition'; @@ -28,11 +29,14 @@ children?: NavItem[]; }; - const items: NavItem[] = [ - { href: '/settings/account', label: m.my_account() }, - { href: '/settings/apps', label: m.my_apps() }, - { href: '/settings/audit-log', label: m.audit_log() } - ]; + // A user who still has to verify their email can only reach their account until they do + const items: NavItem[] = isEmailVerificationPending(user, $appConfigStore) + ? [{ href: '/settings/account', label: m.my_account() }] + : [ + { href: '/settings/account', label: m.my_account() }, + { href: '/settings/apps', label: m.my_apps() }, + { href: '/settings/audit-log', label: m.audit_log() } + ]; const adminChildren: NavItem[] = [ { href: '/settings/admin/users', label: m.users() }, diff --git a/frontend/src/routes/settings/account/+page.svelte b/frontend/src/routes/settings/account/+page.svelte index e9c703a9..6ebad2d5 100644 --- a/frontend/src/routes/settings/account/+page.svelte +++ b/frontend/src/routes/settings/account/+page.svelte @@ -11,8 +11,14 @@ import userStore from '#lib/stores/user-store.ts'; import type { Passkey } from '#lib/types/passkey.type.ts'; import type { AccountUpdate } from '#lib/types/user.type.ts'; - import { getWebauthnErrorMessage } from '#lib/utils/error-util.ts'; - import { KeyRound, Languages, LucideAlertTriangle, UserCog } from '@lucide/svelte'; + import { axiosErrorToast, getWebauthnErrorMessage } from '#lib/utils/error-util.ts'; + import { + KeyRound, + Languages, + LucideAlertTriangle, + LucideMailCheck, + UserCog + } from '@lucide/svelte'; import { startRegistration } from '@simplewebauthn/browser'; import { toast } from 'svelte-sonner'; import AccountForm from './account-form.svelte'; @@ -23,6 +29,7 @@ let { data } = $props(); let account = $state(data.account); let passkeys = $state(data.passkeys); + let pendingEmailChange = $state(data.pendingEmailChange); let passkeyToRename: Passkey | null = $state(null); const userService = new UserService(); const webauthnService = new WebAuthnService(); @@ -32,7 +39,25 @@ ); async function updateAccount(user: AccountUpdate) { - userStore.setUser(await userService.updateCurrent(user)); + const updated = await userService.updateCurrent(user); + userStore.setUser(updated); + + // A verified email is only replaced once the new address is confirmed, so the change shows up as pending instead + if (user.email && user.email !== updated.email) { + pendingEmailChange = await userService.getPendingEmailChange().catch(() => null); + } + + return updated; + } + + async function cancelEmailChange() { + await userService + .cancelEmailChange() + .then(() => { + pendingEmailChange = null; + toast.success(m.email_change_canceled()); + }) + .catch(axiosErrorToast); } async function createPasskey() { @@ -55,7 +80,7 @@ {#if passkeys.length == 0} - +
{m.passkey_missing()} @@ -72,7 +97,7 @@ {:else if passkeys.length == 1} - +
{m.single_passkey_configured()} @@ -82,6 +107,30 @@ {/if} +{#if pendingEmailChange} + + +
+
+ {m.email_change_pending_title()} + + {m.email_change_pending_description({ email: pendingEmailChange.email })} + +
+
+ +
+
+ +{/if} + diff --git a/frontend/src/routes/settings/account/+page.ts b/frontend/src/routes/settings/account/+page.ts index 613ec5b0..39b065f4 100644 --- a/frontend/src/routes/settings/account/+page.ts +++ b/frontend/src/routes/settings/account/+page.ts @@ -3,9 +3,13 @@ import type { PageLoad } from './$types'; export const load: PageLoad = async () => { const userService = new UserService(); - const account = await userService.getCurrent(); + const [account, pendingEmailChange] = await Promise.all([ + userService.getCurrent(), + userService.getPendingEmailChange().catch(() => null) + ]); return { - account + account, + pendingEmailChange }; }; diff --git a/frontend/src/routes/settings/account/account-form.svelte b/frontend/src/routes/settings/account/account-form.svelte index 3fcce8c7..e16c8645 100644 --- a/frontend/src/routes/settings/account/account-form.svelte +++ b/frontend/src/routes/settings/account/account-form.svelte @@ -5,7 +5,7 @@ import { m } from '#lib/paraglide/messages.js'; import UserService from '#lib/services/user-service.ts'; import appConfigStore from '#lib/stores/application-configuration-store.ts'; - import type { AccountUpdate } from '#lib/types/user.type.ts'; + import type { AccountUpdate, User } from '#lib/types/user.type.ts'; import { axiosErrorToast } from '#lib/utils/error-util.ts'; import { createForm } from '#lib/utils/form-util.ts'; import { trackFormChanges } from '#lib/utils/unsaved-changes-util.svelte.ts'; @@ -23,7 +23,7 @@ }: { account: AccountUpdate; userId: string; - callback: (user: AccountUpdate) => Promise; + callback: (user: AccountUpdate) => Promise; isLdapUser?: boolean; userInfoInputDisabled?: boolean; } = $props(); @@ -55,7 +55,19 @@ } } - trackFormChanges(() => formStore, callback); + trackFormChanges( + () => formStore, + async (data) => { + const saved = await callback(data); + + // A verified email is only replaced once the new address is confirmed, so keep showing the email that is still in effect + // The saved data becomes the new baseline, so it has to carry that email as well + if (saved.email && data.email && saved.email !== data.email) { + data.email = saved.email; + formStore.setValue('email', saved.email); + } + } + ); async function updateProfilePicture(image: File) { await userService diff --git a/frontend/src/routes/settings/admin/application-configuration/forms/app-config-email-form.svelte b/frontend/src/routes/settings/admin/application-configuration/forms/app-config-email-form.svelte index fe0e337a..8970125d 100644 --- a/frontend/src/routes/settings/admin/application-configuration/forms/app-config-email-form.svelte +++ b/frontend/src/routes/settings/admin/application-configuration/forms/app-config-email-form.svelte @@ -46,6 +46,18 @@ } }); + const emailVerificationOptions = $derived({ + disabled: { label: m.disabled(), description: m.email_verification_disabled_description() }, + optional: { + label: m.email_verification_optional(), + description: m.email_verification_optional_description() + }, + required: { + label: m.email_verification_required(), + description: m.email_verification_required_description() + } + }); + let isSendingTestEmail = $state(false); const formSchema = z @@ -63,7 +75,7 @@ smtpTls: z.enum(['none', 'starttls', 'tls']), smtpSkipCertVerify: z.boolean(), emailOneTimeAccessAsUnauthenticatedEnabled: z.boolean(), - emailVerificationEnabled: z.boolean(), + emailVerification: z.enum(['disabled', 'optional', 'required']), emailOneTimeAccessAsAdminEnabled: z.boolean(), emailLoginNotificationMode: z.enum([ 'disabled', @@ -82,7 +94,6 @@ const emailFields: (keyof z.infer)[] = [ 'emailOneTimeAccessAsUnauthenticatedEnabled', - 'emailVerificationEnabled', 'emailOneTimeAccessAsAdminEnabled', 'emailApiKeyExpirationEnabled' ]; @@ -105,8 +116,19 @@ requireFieldsWhen(anyProvided, m.smtp_field_required_when_other_provided()); const emailEnabled = - data.emailLoginNotificationMode !== 'disabled' || emailFields.some((f) => data[f]); + data.emailLoginNotificationMode !== 'disabled' || + data.emailVerification !== 'disabled' || + emailFields.some((f) => data[f]); requireFieldsWhen(emailEnabled, m.smtp_field_required_when_email_enabled()); + + // Requiring verification only protects accounts that have an email + if (data.emailVerification === 'required' && !data.requireUserEmail) { + ctx.addIssue({ + code: 'custom', + path: ['emailVerification'], + message: m.email_verification_required_needs_user_email() + }); + } }); const formStore = createForm(formSchema, pickSchemaValues(formSchema, appConfig)); @@ -175,10 +197,53 @@ description={m.require_user_email_description()} bind:checked={$inputs.requireUserEmail.value} /> + +
+ {m.email_verification()} + {m.email_verification_description()} +
+ { + $inputs.emailVerification.value = value as AllAppConfig['emailVerification']; + // Emails verified by default would let every new account skip the verification + if (value === 'required') { + $inputs.emailsVerified.value = false; + } + }} + > + + {emailVerificationOptions[$inputs.emailVerification.value].label} + + + + {#each Object.entries(emailVerificationOptions) as [value, option] (value)} + +
+ {option.label} + {option.description} +
+ + {/each} + + + + {#if $inputs.emailVerification.error} + {$inputs.emailVerification.error} + {/if} +
@@ -246,12 +311,6 @@ - { await page.getByLabel('Email Login Code Requested by User').click(); await page.getByLabel('Email Login Code from Admin').click(); await page.getByLabel('API Key Expiration').click(); + await page.getByLabel('Email Verification', { exact: true }).click(); + await page.getByRole('option', { name: /^Required/ }).click(); await saveUnsavedChanges(page); @@ -264,6 +266,26 @@ test('Update email configuration', async ({ page }) => { await expect(page.getByLabel('Email Login Code Requested by User')).toBeChecked(); await expect(page.getByLabel('Email Login Code from Admin')).toBeChecked(); await expect(page.getByLabel('API Key Expiration')).toBeChecked(); + await expect(page.getByLabel('Email Verification', { exact: true })).toContainText('Required'); +}); + +test('Required email verification turns off emails verified by default', async ({ page }) => { + await page.getByRole('tab', { name: 'Email' }).click(); + + const emailsVerifiedByDefault = page.getByLabel('Emails verified by default'); + await emailsVerifiedByDefault.click(); + await expect(emailsVerifiedByDefault).toBeChecked(); + + await page.getByLabel('Email Verification', { exact: true }).click(); + await page.getByRole('option', { name: /^Required/ }).click(); + + await expect(emailsVerifiedByDefault).not.toBeChecked(); + await expect(emailsVerifiedByDefault).toBeDisabled(); + + await page.getByLabel('Email Verification', { exact: true }).click(); + await page.getByRole('option', { name: /^Optional/ }).click(); + + await expect(emailsVerifiedByDefault).toBeEnabled(); }); test('Save LDAP configuration while LDAP remains disabled', async ({ page }) => { diff --git a/tests/specs/email-verification.spec.ts b/tests/specs/email-verification.spec.ts new file mode 100644 index 00000000..45dd92c4 --- /dev/null +++ b/tests/specs/email-verification.spec.ts @@ -0,0 +1,190 @@ +import base, { expect, type APIRequestContext } from '@playwright/test'; +import { emailVerificationTokens, oidcClients, users } from '../data'; +import { setAppConfigValues } from '../utils/app-config.util'; +import authUtil from '../utils/auth.util'; +import { cleanupBackend } from '../utils/cleanup.util'; +import { startSmtpSink, verificationLinkPath, type SmtpSink } from '../utils/smtp.util'; +import { saveUnsavedChanges } from '../utils/unsaved-changes.util'; + +const smtpSinkPort = 18125; + +const test = base.extend<{ smtp: SmtpSink }>({ + smtp: async ({}, use) => { + const sink = await startSmtpSink(smtpSinkPort); + try { + await use(sink); + } finally { + await sink.close(); + } + } +}); + +// Requires email verification and points Pocket ID at the SMTP sink, so the verification links can be followed +async function requireEmailVerificationWithSmtp(request: APIRequestContext) { + await setAppConfigValues(request, { + emailVerification: 'required', + smtpHost: 'host.docker.internal', + smtpPort: String(smtpSinkPort), + smtpFrom: 'pocket-id@test.com', + smtpTls: 'none' + }); +} + +// Builds the query of an authorization code request for the Nextcloud client +function nextcloudAuthorizeParams() { + const oidcClient = oidcClients.nextcloud; + return new URLSearchParams({ + client_id: oidcClient.id, + response_type: 'code', + scope: 'openid profile email', + redirect_uri: oidcClient.callbackUrl, + state: 'nXx-6Qr-owc1SHBa', + nonce: 'P1gN3PtpKHJgKUVcLpLjm' + }); +} + +// Marks Craig's email as verified through the admin API +async function markCraigVerified(request: APIRequestContext) { + const craig = await request.get(`/api/users/${users.craig.id}`).then((r) => r.json()); + const response = await request.put(`/api/users/${users.craig.id}`, { + data: { ...craig, emailVerified: true } + }); + expect(response.ok()).toBe(true); +} + +test.describe('Required email verification', () => { + test.beforeEach(async ({ page }) => { + await cleanupBackend(); + await requireEmailVerificationWithSmtp(page.request); + + // Craig is a regular user whose email is not verified yet + await authUtil.changeUser(page, 'craig'); + }); + + test('Unverified user can only reach their account', async ({ page }) => { + await page.goto('/settings/apps'); + await page.waitForURL('/settings/account'); + + await expect(page.getByText('Verify your email address to use your account')).toBeVisible(); + await expect(page.getByRole('link', { name: 'My Apps' })).toHaveCount(0); + + const response = await page.request.get('/api/oidc/users/me/clients'); + expect(response.status()).toBe(403); + }); + + test('Unverified user cannot sign in to a client', async ({ page }) => { + await page.goto(`/authorize?${nextcloudAuthorizeParams().toString()}`); + + await expect(page).toHaveURL(/\/interaction\/error\?error=/); + await expect( + page.getByRole('paragraph').filter({ + hasText: 'You must verify your email address before you can access this service.' + }) + ).toBeVisible(); + + await page.getByRole('link', { name: 'Verify email address' }).click(); + await page.waitForURL('/settings/account'); + }); + + test('Unverified user is stopped right after signing in during authorization', async ({ + page + }) => { + // Sign out so the sign-in happens on the interaction page, with the passkey the setup already registered for Craig + await page.context().clearCookies(); + await page.goto(`/authorize?${nextcloudAuthorizeParams().toString()}`); + await page.getByRole('button', { name: 'Sign in' }).click(); + + // Completing the sign-in step already requires a verified email, so the consent screen is never shown + await expect( + page.getByText('You must verify your email address before you can sign in to applications.') + ).toBeVisible(); + await expect(page.getByTestId('scopes')).toHaveCount(0); + + await page.getByRole('link', { name: 'Verify email address' }).click(); + await page.waitForURL('/settings/account'); + }); + + test('Verifying the email restores full access', async ({ page }) => { + const token = emailVerificationTokens.find((t) => !t.expired)!.token; + await page.goto(`/verify-email?token=${token}`); + await page.waitForURL('/settings/account?emailVerificationState=success'); + + await page.goto('/settings/apps'); + await expect(page).toHaveURL('/settings/apps'); + await expect(page.getByRole('link', { name: 'My Apps' })).toBeVisible(); + }); +}); + +test.describe('Email change with required verification', () => { + test.beforeEach(async ({ page }) => { + await cleanupBackend(); + await requireEmailVerificationWithSmtp(page.request); + await markCraigVerified(page.request); + await authUtil.changeUser(page, 'craig'); + }); + + test('A new email takes effect once it is confirmed', async ({ page, smtp }) => { + const newEmail = 'craig.new@test.com'; + await page.goto('/settings/account'); + await page.getByLabel('Email').fill(newEmail); + await saveUnsavedChanges(page); + + // The verified email stays in effect and the account keeps full access until the change is confirmed + await expect(page.getByText('Confirm your new email address')).toBeVisible(); + await expect(page.getByLabel('Email')).toHaveValue(users.craig.email); + await page.goto('/settings/apps'); + await expect(page).toHaveURL('/settings/apps'); + + await page.goto(verificationLinkPath(smtp, newEmail)); + await page.waitForURL('/settings/account?emailVerificationState=success'); + + await expect(page.getByLabel('Email')).toHaveValue(newEmail); + await expect(page.getByText('Confirm your new email address')).toHaveCount(0); + }); + + test('A canceled email change can no longer be confirmed', async ({ page, smtp }) => { + const newEmail = 'craig.canceled@test.com'; + await page.goto('/settings/account'); + await page.getByLabel('Email').fill(newEmail); + await saveUnsavedChanges(page); + + await page.getByRole('button', { name: 'Cancel change' }).click(); + await expect(page.getByText('Email change canceled.')).toBeVisible(); + await expect(page.getByText('Confirm your new email address')).toHaveCount(0); + + await page.goto(verificationLinkPath(smtp, newEmail)); + await page.waitForURL( + '/settings/account?emailVerificationState=Email+verification+token+is+invalid' + ); + await expect(page.getByLabel('Email')).toHaveValue(users.craig.email); + }); +}); + +test('Signing up sends a verification link that unlocks the account', async ({ page, smtp }) => { + await cleanupBackend(); + await requireEmailVerificationWithSmtp(page.request); + await setAppConfigValues(page.request, { allowUserSignups: 'open' }); + await page.context().clearCookies(); + + const email = 'verified.signup@test.com'; + await page.goto('/signup'); + await page.getByLabel('First name').fill('Verified'); + await page.getByLabel('Last name').fill('Signup'); + await page.getByLabel('Username').fill('verifiedsignup'); + await page.getByLabel('Email').fill(email); + await page.getByRole('button', { name: 'Sign Up' }).click(); + + await page.waitForURL('/signup/add-passkey'); + await page.getByRole('button', { name: 'Skip for now' }).click(); + await page + .getByRole('alertdialog', { name: 'Skip Passkey Setup' }) + .getByRole('button', { name: 'Skip for now' }) + .click(); + await page.waitForURL('/settings/account'); + + await page.goto(verificationLinkPath(smtp, email)); + await page.waitForURL('/settings/account?emailVerificationState=success'); + + await page.goto('/settings/apps'); + await expect(page).toHaveURL('/settings/apps'); +}); diff --git a/tests/specs/user-signup.spec.ts b/tests/specs/user-signup.spec.ts index 6af65338..ecd95bd1 100644 --- a/tests/specs/user-signup.spec.ts +++ b/tests/specs/user-signup.spec.ts @@ -1,5 +1,6 @@ import test, { expect, type Page } from '@playwright/test'; import { signupTokens, userGroups, users } from '../data'; +import { setAppConfigValues } from '../utils/app-config.util'; import { cleanupBackend } from '../utils/cleanup.util'; import passkeyUtil from '../utils/passkey.util'; import { saveUnsavedChanges } from '../utils/unsaved-changes.util'; @@ -296,6 +297,39 @@ test.describe('User Signup', () => { await expect(page.getByRole('heading', { name: 'Set up your passkey' })).toBeVisible(); }); + test('Open signup with required email verification restricts the new account', async ({ + page + }) => { + // Required verification needs an SMTP server, but nothing listens on it, so the email sent at signup just fails + await setAppConfigValues(page.request, { + emailVerification: 'required', + smtpHost: 'localhost', + smtpFrom: 'pocket-id@test.com' + }); + await setSignupMode(page, 'Open Signup'); + + await page.goto('/signup'); + await page.getByLabel('First name').fill('Pending'); + await page.getByLabel('Last name').fill('User'); + await page.getByLabel('Username').fill('pendinguser'); + await page.getByLabel('Email').fill('pending.user@test.com'); + await page.getByRole('button', { name: 'Sign Up' }).click(); + + await page.waitForURL('/signup/add-passkey'); + await page.getByRole('button', { name: 'Skip for now' }).click(); + await page + .getByRole('alertdialog', { name: 'Skip Passkey Setup' }) + .getByRole('button', { name: 'Skip for now' }) + .click(); + + await page.waitForURL('/settings/account'); + await expect(page.getByText('Verify your email address to use your account')).toBeVisible(); + await expect(page.getByText('your account will be deleted')).toBeVisible(); + + await page.goto('/settings/apps'); + await expect(page).toHaveURL('/settings/account'); + }); + test('Token usage limit is enforced', async ({ page }) => { await setSignupMode(page, 'Signup with token'); diff --git a/tests/utils/app-config.util.ts b/tests/utils/app-config.util.ts new file mode 100644 index 00000000..e8411f0c --- /dev/null +++ b/tests/utils/app-config.util.ts @@ -0,0 +1,22 @@ +import { expect, type APIRequestContext } from '@playwright/test'; + +// Overrides application configuration values through the API, keeping every other value unchanged +// The request context must be authenticated as an admin +export async function setAppConfigValues( + request: APIRequestContext, + values: Record +) { + const configResponse = await request.get('/api/application-configuration/all'); + expect(configResponse.ok()).toBe(true); + + const config = Object.fromEntries( + ((await configResponse.json()) as Array<{ key: string; value: string }>).map( + ({ key, value }) => [key, value] + ) + ); + + const updateResponse = await request.put('/api/application-configuration', { + data: { ...config, ...values } + }); + expect(updateResponse.ok()).toBe(true); +} diff --git a/tests/utils/smtp.util.ts b/tests/utils/smtp.util.ts new file mode 100644 index 00000000..8fc41dde --- /dev/null +++ b/tests/utils/smtp.util.ts @@ -0,0 +1,90 @@ +import { createServer } from 'node:net'; + +export type CapturedEmail = { + recipients: string[]; + body: string; +}; + +export type SmtpSink = { + emails: CapturedEmail[]; + close: () => Promise; +}; + +// Starts a minimal SMTP server that accepts every message, so tests can read the emails Pocket ID sends +// Docker reaches it through the host gateway, like the back-channel logout receiver +export async function startSmtpSink(port: number): Promise { + const emails: CapturedEmail[] = []; + + const server = createServer((socket) => { + let buffer = ''; + let inData = false; + let recipients: string[] = []; + + socket.write('220 smtp-sink ESMTP\r\n'); + socket.on('data', (chunk) => { + buffer += chunk.toString(); + + while (true) { + // The message ends with a line that only holds a dot + if (inData) { + const end = buffer.indexOf('\r\n.\r\n'); + if (end === -1) return; + + emails.push({ recipients, body: decodeQuotedPrintable(buffer.slice(0, end)) }); + buffer = buffer.slice(end + 5); + inData = false; + recipients = []; + socket.write('250 OK\r\n'); + continue; + } + + const lineEnd = buffer.indexOf('\r\n'); + if (lineEnd === -1) return; + const line = buffer.slice(0, lineEnd); + buffer = buffer.slice(lineEnd + 2); + + // Answer just enough of the protocol for a client that neither authenticates nor uses TLS + const command = line.slice(0, 4).toUpperCase(); + if (command === 'EHLO' || command === 'HELO') { + socket.write('250 smtp-sink\r\n'); + } else if (command === 'RCPT') { + recipients.push(line.match(/<(.*)>/)?.[1] ?? ''); + socket.write('250 OK\r\n'); + } else if (command === 'DATA') { + inData = true; + socket.write('354 End data with .\r\n'); + } else if (command === 'QUIT') { + socket.end('221 Bye\r\n'); + return; + } else { + socket.write('250 OK\r\n'); + } + } + }); + }); + + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(port, '0.0.0.0', resolve); + }); + + return { + emails, + close: () => + new Promise((resolve, reject) => server.close((e) => (e ? reject(e) : resolve()))) + }; +} + +// Returns the path of the newest email verification link sent to the recipient +export function verificationLinkPath(sink: SmtpSink, recipient: string) { + const email = sink.emails.findLast((e) => e.recipients.includes(recipient)); + const token = email?.body.match(/\/verify-email\?token=([A-Za-z0-9]+)/)?.[1]; + if (!token) throw new Error(`No verification link was sent to ${recipient}`); + return `/verify-email?token=${token}`; +} + +function decodeQuotedPrintable(value: string) { + return value + .replace(/=\r\n/g, '') + .replace(/=([0-9A-F]{2})/g, (_, hex: string) => String.fromCharCode(parseInt(hex, 16))); +}