From 78f79accb5cfacb70d596adf9ee115144977db08 Mon Sep 17 00:00:00 2001 From: kolaente Date: Fri, 12 Jun 2026 10:28:21 +0200 Subject: [PATCH] refactor(auth): extract transport-agnostic login, logout and OIDC cores Pull the credential/TOTP check, session deletion, user-token issuance and OIDC callback flow out of the v1 echo handlers and into reusable helpers so both /api/v1 and the upcoming /api/v2 share one implementation: - auth.IssueUserToken + auth.WriteUserAuthCookies split the token/cookie machinery from the echo response; NewUserAuthTokenResponse now wraps them. - auth.SessionIDFromContext reads the sid claim for logout. - shared.AuthenticateUserCredentials, shared.DeleteSession hold the login and logout cores. - openid.AuthenticateCallback holds the OIDC exchange/getOrCreate/TOTP/team sync, returning the user; HandleCallback issues the token as before. v1 behaviour is unchanged on the wire. --- pkg/modules/auth/auth.go | 77 ++++++++++++++---- pkg/modules/auth/openid/openid.go | 81 +++++++++++-------- pkg/routes/api/shared/auth.go | 125 ++++++++++++++++++++++++++++++ pkg/routes/api/v1/login.go | 97 +---------------------- 4 files changed, 241 insertions(+), 139 deletions(-) diff --git a/pkg/modules/auth/auth.go b/pkg/modules/auth/auth.go index 97429aa13..f94537158 100644 --- a/pkg/modules/auth/auth.go +++ b/pkg/modules/auth/auth.go @@ -100,46 +100,75 @@ func ClearRefreshTokenCookie(c *echo.Context) { SetRefreshTokenCookie(c, "", -1) } -// NewUserAuthTokenResponse creates a new user auth token response from a user object. -func NewUserAuthTokenResponse(u *user.User, c *echo.Context, long bool) error { +// IssuedUserToken bundles a freshly minted access token with the matching +// refresh token and the cookie max-age both v1 and v2 use to set the +// HttpOnly refresh cookie. +type IssuedUserToken struct { + AccessToken string + RefreshToken string + CookieMaxAge int +} + +// IssueUserToken creates a session for the user and mints a JWT access token plus +// a refresh token for it. It is the transport-agnostic core both v1 (which writes +// the echo response) and v2 (Huma) call; callers set the refresh cookie and the +// Cache-Control header themselves via WriteUserAuthCookies. +func IssueUserToken(ctx context.Context, u *user.User, deviceInfo, ipAddress string, long bool) (*IssuedUserToken, error) { s := db.NewSession() defer s.Close() - deviceInfo := c.Request().UserAgent() - ipAddress := c.RealIP() - session, err := models.CreateSession(s, u.ID, deviceInfo, ipAddress, long) if err != nil { _ = s.Rollback() - return err + return nil, err } t, err := NewUserJWTAuthtoken(u, session.ID) if err != nil { _ = s.Rollback() - return err + return nil, err } if err := s.Commit(); err != nil { _ = s.Rollback() - return err + return nil, err } - if err := events.DispatchWithContext(c.Request().Context(), &user.LoginSucceededEvent{User: u}); err != nil { + if err := events.DispatchWithContext(ctx, &user.LoginSucceededEvent{User: u}); err != nil { log.Errorf("Could not dispatch login succeeded event: %s", err) } - // Set the refresh token as an HttpOnly cookie. The cookie is path-scoped - // to the refresh endpoint, so the browser only sends it there. JavaScript - // never sees the refresh token — this protects it from XSS. cookieMaxAge := int(config.ServiceJWTTTL.GetInt64()) if long { cookieMaxAge = int(config.ServiceJWTTTLLong.GetInt64()) } - SetRefreshTokenCookie(c, session.RefreshToken, cookieMaxAge) + return &IssuedUserToken{ + AccessToken: t, + RefreshToken: session.RefreshToken, + CookieMaxAge: cookieMaxAge, + }, nil +} + +// WriteUserAuthCookies sets the HttpOnly refresh-token cookie and the +// Cache-Control: no-store header on a response. The cookie is path-scoped to the +// refresh endpoint, so the browser only sends it there; JavaScript never sees the +// refresh token, which protects it from XSS. Shared by the v1 echo handlers and +// the v2 Huma handlers (which reach the echo context via humaecho5.Unwrap). +func WriteUserAuthCookies(c *echo.Context, token *IssuedUserToken) { + SetRefreshTokenCookie(c, token.RefreshToken, token.CookieMaxAge) c.Response().Header().Set("Cache-Control", "no-store") - return c.JSON(http.StatusOK, Token{Token: t}) +} + +// NewUserAuthTokenResponse creates a new user auth token response from a user object. +func NewUserAuthTokenResponse(u *user.User, c *echo.Context, long bool) error { + token, err := IssueUserToken(c.Request().Context(), u, c.Request().UserAgent(), c.RealIP(), long) + if err != nil { + return err + } + + WriteUserAuthCookies(c, token) + return c.JSON(http.StatusOK, Token{Token: token.AccessToken}) } // NewUserJWTAuthtoken generates and signs a new short-lived jwt token for a user. @@ -392,6 +421,26 @@ func RefreshSession(rawRefreshToken string) (*RefreshResult, error) { }, nil } +// SessionIDFromContext reads the session id (the `sid` claim) off the user JWT +// in the echo context. It returns "" when there is no user JWT or no sid claim +// (API tokens and link shares carry no session), which callers treat as a no-op. +func SessionIDFromContext(c *echo.Context) string { + raw := c.Get("user") + if raw == nil { + return "" + } + jwtinf, ok := raw.(*jwt.Token) + if !ok { + return "" + } + claims, ok := jwtinf.Claims.(jwt.MapClaims) + if !ok { + return "" + } + sid, _ := claims["sid"].(string) + return sid +} + // GetAuthFromContext retrieves the authenticated web.Auth from a plain // context.Context, bridging Huma handlers to Vikunja's echo JWT flow. The // humaecho5 adapter stashes the *echo.Context under EchoContextKey first. diff --git a/pkg/modules/auth/openid/openid.go b/pkg/modules/auth/openid/openid.go index b1fa3961a..47ade7dc9 100644 --- a/pkg/modules/auth/openid/openid.go +++ b/pkg/modules/auth/openid/openid.go @@ -168,8 +168,12 @@ func enforceTOTPIfRequired(s *xorm.Session, u *user.User, totpPasscode string) e // @Failure 500 {object} models.Message "Internal error" // @Router /auth/openid/{provider}/callback [post] func HandleCallback(c *echo.Context) error { + cb := &Callback{} + if err := c.Bind(cb); err != nil { + return &models.ErrOpenIDBadRequest{Message: "Bad data"} + } - provider, cb, oauthToken, idToken, err := getProviderAndOidcTokens(c) + u, err := AuthenticateCallback(c.Request().Context(), cb, c.Param("provider")) if err != nil { var detailedErr *models.ErrOpenIDBadRequestWithDetails if errors.As(err, &detailedErr) { @@ -181,9 +185,29 @@ func HandleCallback(c *echo.Context) error { return err } - cl, err := getClaims(provider, oauthToken, idToken) + // Create token + return auth.NewUserAuthTokenResponse(u, c, false) +} + +// AuthenticateCallback resolves an OpenID Connect callback to an authenticated +// user: it exchanges the auth code, verifies the ID token, creates or updates the +// matching local user, enforces the account-status and TOTP gates, and syncs the +// user's external teams. It is the transport-agnostic core shared by the v1 echo +// handler and the v2 Huma handler; the caller issues the auth token. The +// ErrOpenIDBadRequestWithDetails error keeps its provider detail so v1 can render +// its bespoke body and v2 can map it to RFC 9457. +func AuthenticateCallback(ctx context.Context, cb *Callback, providerKey string) (*user.User, error) { + // ctx is threaded through only to dispatch the login event; the OIDC token + // exchange, claim verification and user/avatar sync run on their own + // background contexts, exactly as the v1 callback always did. + provider, oauthToken, idToken, err := exchangeOidcTokens(cb, providerKey) //nolint:contextcheck if err != nil { - return err + return nil, err + } + + cl, err := getClaims(provider, oauthToken, idToken) //nolint:contextcheck + if err != nil { + return nil, err } s := db.NewSession() @@ -193,20 +217,20 @@ func HandleCallback(c *echo.Context) error { defer events.CleanupPending(s) // Check if we have seen this user before - u, err := getOrCreateUser(s, cl, provider, idToken) + u, err := getOrCreateUser(s, cl, provider, idToken) //nolint:contextcheck if err != nil { _ = s.Rollback() log.Errorf("Error creating new user for provider %s: %v", provider.Name, err) - return err + return nil, err } if u.Status == user.StatusDisabled { _ = s.Rollback() - return &user.ErrAccountDisabled{UserID: u.ID} + return nil, &user.ErrAccountDisabled{UserID: u.ID} } if u.Status == user.StatusAccountLocked { _ = s.Rollback() - return &user.ErrAccountLocked{UserID: u.ID} + return nil, &user.ErrAccountLocked{UserID: u.ID} } // Must run before team sync so a failed 2FA attempt cannot mutate team @@ -218,32 +242,31 @@ func HandleCallback(c *echo.Context) error { log.Errorf("Error committing session after failed OIDC TOTP attempt for user %d: %v", u.ID, commitErr) } else { // The user creation above was committed, so its events are real. - events.DispatchPending(c.Request().Context(), s) + events.DispatchPending(ctx, s) } if user.IsErrInvalidTOTPPasscode(err) { user.HandleFailedTOTPAuth(u) } - return err + return nil, err } teamData := getTeamDataFromToken(cl.VikunjaGroups, provider) err = models.SyncExternalTeamsForUser(s, u, teamData, idToken.Issuer, provider.Name) if err != nil { - return err + return nil, err } err = s.Commit() if err != nil { _ = s.Rollback() log.Errorf("Error creating new team for provider %s: %v", provider.Name, err) - return err + return nil, err } - events.DispatchPending(c.Request().Context(), s) + events.DispatchPending(ctx, s) - // Create token - return auth.NewUserAuthTokenResponse(u, c, false) + return u, nil } func getTeamDataFromToken(groups []map[string]interface{}, provider *Provider) (teamData []*models.Team) { @@ -516,21 +539,17 @@ func getClaims(provider *Provider, oauth2Token *oauth2.Token, idToken *oidc.IDTo return cl, nil } -func getProviderAndOidcTokens(c *echo.Context) (*Provider, *Callback, *oauth2.Token, *oidc.IDToken, error) { - - cb := &Callback{} - if err := c.Bind(cb); err != nil { - return nil, nil, nil, nil, &models.ErrOpenIDBadRequest{Message: "Bad data"} - } - - // Check if the provider exists - providerKey := c.Param("provider") +// exchangeOidcTokens resolves the provider, exchanges the callback's auth code, +// and verifies the returned ID token. It takes an already-bound Callback so it +// can be shared by the v1 echo handler (which binds from the request) and the v2 +// Huma handler (which binds via its typed body). +func exchangeOidcTokens(cb *Callback, providerKey string) (*Provider, *oauth2.Token, *oidc.IDToken, error) { provider, err := GetProvider(providerKey) if err != nil { - return nil, cb, nil, nil, err + return nil, nil, nil, err } if provider == nil { - return nil, cb, nil, nil, &models.ErrOpenIDBadRequest{Message: "Provider does not exist"} + return nil, nil, nil, &models.ErrOpenIDBadRequest{Message: "Provider does not exist"} } log.Debugf("Trying to authenticate user using provider: %s", provider.Key) @@ -546,25 +565,25 @@ func getProviderAndOidcTokens(c *echo.Context) (*Provider, *Callback, *oauth2.To if err := json.Unmarshal(rerr.Body, &details); err != nil { log.Errorf("Error unmarshalling token for provider %s: %v", provider.Name, err) log.Debugf("Raw token value is %s", rerr.Body) - return nil, cb, nil, nil, err + return nil, nil, nil, err } log.Errorf("Error retrieving token: %s", err) log.Debugf("Raw token value is %s", rerr.Body) - return nil, cb, nil, nil, &models.ErrOpenIDBadRequestWithDetails{ + return nil, nil, nil, &models.ErrOpenIDBadRequestWithDetails{ Message: "Could not authenticate against third party.", Details: details, } } - return nil, cb, nil, nil, err + return nil, nil, nil, err } // Extract the ID Token from OAuth2 token. rawIDToken, ok := oauth2Token.Extra("id_token").(string) if !ok { log.Debugf("Could not get id_token, raw token is %v", oauth2Token) - return nil, cb, nil, nil, &models.ErrOpenIDBadRequest{Message: "Missing token"} + return nil, nil, nil, &models.ErrOpenIDBadRequest{Message: "Missing token"} } verifier := provider.openIDProvider.Verifier(&oidc.Config{ClientID: provider.ClientID}) @@ -573,8 +592,8 @@ func getProviderAndOidcTokens(c *echo.Context) (*Provider, *Callback, *oauth2.To idToken, err := verifier.Verify(context.Background(), rawIDToken) if err != nil { log.Errorf("Error verifying token for provider %s: %v", provider.Name, err) - return nil, cb, nil, nil, err + return nil, nil, nil, err } - return provider, cb, oauth2Token, idToken, nil + return provider, oauth2Token, idToken, nil } diff --git a/pkg/routes/api/shared/auth.go b/pkg/routes/api/shared/auth.go index 925a533d8..acfa47ce0 100644 --- a/pkg/routes/api/shared/auth.go +++ b/pkg/routes/api/shared/auth.go @@ -26,7 +26,11 @@ import ( "code.vikunja.io/api/pkg/metrics" "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/modules/auth" + "code.vikunja.io/api/pkg/modules/auth/ldap" + "code.vikunja.io/api/pkg/modules/keyvalue" "code.vikunja.io/api/pkg/user" + + "xorm.io/xorm" ) // UserRegister carries the fields accepted by the public registration endpoint: @@ -78,6 +82,127 @@ func RegisterUser(ctx context.Context, in *UserRegister) (*user.User, error) { return newUser, nil } +// AuthenticateUserCredentials verifies a login against local (and, if configured, +// LDAP) credentials and enforces the account-status and TOTP gates, returning the +// authenticated user on success. It is the transport-agnostic core of the login +// flow shared by v1 and v2; the caller issues the token and sets the cookie. The +// returned errors carry their own HTTP semantics (wrong credentials, disabled +// account, missing/invalid TOTP) so both APIs surface them identically. +func AuthenticateUserCredentials(ctx context.Context, login *user.Login) (*user.User, error) { + s := db.NewSession() + defer s.Close() + // Discards events queued during a rolled-back transaction (e.g. LDAP user + // creation); a no-op once DispatchPending has run. + defer events.CleanupPending(s) + + u, err := resolveLoginUser(ctx, s, login) + if err != nil { + _ = s.Rollback() + return nil, err + } + + if u.Status == user.StatusDisabled || u.Status == user.StatusAccountLocked { + _ = s.Rollback() + return nil, &user.ErrAccountDisabled{UserID: u.ID} + } + + if err := enforceLoginTOTP(s, u, login.TOTPPasscode); err != nil { + return nil, err + } + + if err := keyvalue.Del(u.GetFailedTOTPAttemptsKey()); err != nil { + return nil, err + } + if err := keyvalue.Del(u.GetFailedPasswordAttemptsKey()); err != nil { + return nil, err + } + + if err := s.Commit(); err != nil { + _ = s.Rollback() + return nil, err + } + + events.DispatchPending(ctx, s) + + return u, nil +} + +// resolveLoginUser authenticates the credentials against LDAP (when enabled) and +// then against local accounts, mirroring v1's order so local users keep working +// alongside LDAP. Bots are rejected before bcrypt runs because they have no +// password hash. +func resolveLoginUser(ctx context.Context, s *xorm.Session, login *user.Login) (*user.User, error) { + if config.AuthLdapEnabled.GetBool() { + u, err := ldap.AuthenticateUserInLDAP(s, login.Username, login.Password, config.AuthLdapGroupSyncEnabled.GetBool(), config.AuthLdapAvatarSyncAttribute.GetString()) + if err != nil && !user.IsErrWrongUsernameOrPassword(err) { + return nil, err + } + if u != nil { + return u, nil + } + } + + existingUser, lookupErr := user.GetUserByUsername(s, login.Username) + if lookupErr == nil && existingUser.IsBot() { + return nil, &user.ErrAccountIsBot{UserID: existingUser.ID} + } + + return user.CheckUserCredentials(ctx, s, login) +} + +// enforceLoginTOTP runs the TOTP gate for users who have it enabled, mirroring +// v1: a missing passcode is rejected, and a wrong one trips the failed-attempt +// lockout via HandleFailedTOTPAuth. The session is rolled back before +// HandleFailedTOTPAuth so its dedicated session can acquire a write lock on +// SQLite shared-cache (the lockout write is decoupled from this transaction — +// see GHSA-fgfv-pv97-6cmj). +func enforceLoginTOTP(s *xorm.Session, u *user.User, passcode string) error { + totpEnabled, err := user.TOTPEnabledForUser(s, u) + if err != nil { + _ = s.Rollback() + return err + } + if !totpEnabled { + return nil + } + + if passcode == "" { + _ = s.Rollback() + return user.ErrInvalidTOTPPasscode{} + } + + _, err = user.ValidateTOTPPasscode(s, &user.TOTPPasscode{User: u, Passcode: passcode}) + if err != nil { + _ = s.Rollback() + if user.IsErrInvalidTOTPPasscode(err) { + user.HandleFailedTOTPAuth(u) + } + return err + } + + return nil +} + +// DeleteSession removes the session with the given id, logging the user out +// server-side. An empty sid is a no-op (the token carried no session, e.g. an +// API token or a link share), matching v1. Shared by v1 and v2; the caller is +// responsible for clearing the refresh cookie. +func DeleteSession(sid string) error { + if sid == "" { + return nil + } + + s := db.NewSession() + defer s.Close() + + if _, err := s.Where("id = ?", sid).Delete(&models.Session{}); err != nil { + _ = s.Rollback() + return err + } + + return s.Commit() +} + // ResetPassword resets a user's password from a previously issued reset token // and invalidates all of that user's sessions, so a leaked password cannot be // used after a reset. Shared by v1 and v2. diff --git a/pkg/routes/api/v1/login.go b/pkg/routes/api/v1/login.go index ae92e1d72..2d740ffdf 100644 --- a/pkg/routes/api/v1/login.go +++ b/pkg/routes/api/v1/login.go @@ -25,8 +25,7 @@ import ( "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/modules/auth" - "code.vikunja.io/api/pkg/modules/auth/ldap" - "code.vikunja.io/api/pkg/modules/keyvalue" + "code.vikunja.io/api/pkg/routes/api/shared" user2 "code.vikunja.io/api/pkg/user" "github.com/golang-jwt/jwt/v5" @@ -51,87 +50,11 @@ func Login(c *echo.Context) (err error) { return c.JSON(http.StatusBadRequest, models.Message{Message: "Please provide a username and password."}) } - s := db.NewSession() - defer s.Close() - // Discards events queued during a rolled-back transaction (e.g. LDAP user - // creation); a no-op once DispatchPending has run. - defer events.CleanupPending(s) - - var user *user2.User - if config.AuthLdapEnabled.GetBool() { - user, err = ldap.AuthenticateUserInLDAP(s, u.Username, u.Password, config.AuthLdapGroupSyncEnabled.GetBool(), config.AuthLdapAvatarSyncAttribute.GetString()) - if err != nil && !user2.IsErrWrongUsernameOrPassword(err) { - _ = s.Rollback() - return err - } - } - - if user == nil { - // Check if the user is a bot before attempting password verification, - // because bots have no password hash and bcrypt would fail with a - // misleading error. - existingUser, lookupErr := user2.GetUserByUsername(s, u.Username) - if lookupErr == nil && existingUser.IsBot() { - _ = s.Rollback() - return &user2.ErrAccountIsBot{UserID: existingUser.ID} - } - - // This allows us to still have local users while ldap is enabled - user, err = user2.CheckUserCredentials(c.Request().Context(), s, &u) - if err != nil { - _ = s.Rollback() - return err - } - } - - if user.Status == user2.StatusDisabled || user.Status == user2.StatusAccountLocked { - _ = s.Rollback() - return &user2.ErrAccountDisabled{UserID: user.ID} - } - - totpEnabled, err := user2.TOTPEnabledForUser(s, user) + user, err := shared.AuthenticateUserCredentials(c.Request().Context(), &u) if err != nil { - _ = s.Rollback() return err } - if totpEnabled { - if u.TOTPPasscode == "" { - _ = s.Rollback() - return user2.ErrInvalidTOTPPasscode{} - } - - _, err = user2.ValidateTOTPPasscode(s, &user2.TOTPPasscode{ - User: user, - Passcode: u.TOTPPasscode, - }) - if err != nil { - // Rollback before HandleFailedTOTPAuth so its dedicated session - // can acquire a write lock on SQLite shared-cache. The lockout - // write is decoupled from this handler's transaction — see - // GHSA-fgfv-pv97-6cmj. - _ = s.Rollback() - if user2.IsErrInvalidTOTPPasscode(err) { - user2.HandleFailedTOTPAuth(user) - } - return err - } - } - - if err := keyvalue.Del(user.GetFailedTOTPAttemptsKey()); err != nil { - return err - } - if err := keyvalue.Del(user.GetFailedPasswordAttemptsKey()); err != nil { - return err - } - - if err := s.Commit(); err != nil { - _ = s.Rollback() - return err - } - - events.DispatchPending(c.Request().Context(), s) - // Create token return auth.NewUserAuthTokenResponse(user, c, u.LongToken) } @@ -254,21 +177,7 @@ func Logout(c *echo.Context) (err error) { } } - if sid == "" { - return c.JSON(http.StatusOK, models.Message{Message: "Successfully logged out."}) - } - - s := db.NewSession() - defer s.Close() - - _, err = s.Where("id = ?", sid).Delete(&models.Session{}) - if err != nil { - _ = s.Rollback() - return err - } - - if err := s.Commit(); err != nil { - _ = s.Rollback() + if err := shared.DeleteSession(sid); err != nil { return err }