From f33cde82e2a637a98df577442fe22c00738725e3 Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 11 Jun 2026 21:34:45 +0200 Subject: [PATCH] feat(audit): attribute failed logins to the originating request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thread the request context through CheckUserCredentials so the LoginFailedEvent carries IP, user agent and request id — without it, failed logins were the one auth event useless for brute-force tracing. All four callers have the request at hand. --- pkg/models/user_settings.go | 6 ++++-- pkg/routes/api/v1/login.go | 2 +- pkg/routes/api/v1/user_update_email.go | 2 +- pkg/routes/api/v1/user_update_password.go | 2 +- pkg/routes/api/v2/user_settings.go | 4 ++-- pkg/routes/caldav/auth.go | 2 +- pkg/user/update_email.go | 6 ++++-- pkg/user/user.go | 12 +++++++----- pkg/user/user_test.go | 19 ++++++++++--------- 9 files changed, 31 insertions(+), 24 deletions(-) diff --git a/pkg/models/user_settings.go b/pkg/models/user_settings.go index cba87cdb5..0d905cd1a 100644 --- a/pkg/models/user_settings.go +++ b/pkg/models/user_settings.go @@ -17,6 +17,8 @@ package models import ( + "context" + "code.vikunja.io/api/pkg/modules/avatar" "code.vikunja.io/api/pkg/user" @@ -66,12 +68,12 @@ func NewUserGeneralSettings(u *user.User) *UserGeneralSettings { // ChangeUserPassword verifies the old password, sets the new one, and // invalidates all of the user's sessions. Lives here (not in pkg/user) because // it needs DeleteAllUserSessions, which pkg/user cannot import. -func ChangeUserPassword(s *xorm.Session, u *user.User, oldPassword, newPassword string) error { +func ChangeUserPassword(ctx context.Context, s *xorm.Session, u *user.User, oldPassword, newPassword string) error { if oldPassword == "" { return user.ErrEmptyOldPassword{} } - if _, err := user.CheckUserCredentials(s, &user.Login{Username: u.Username, Password: oldPassword}); err != nil { + if _, err := user.CheckUserCredentials(ctx, s, &user.Login{Username: u.Username, Password: oldPassword}); err != nil { return err } diff --git a/pkg/routes/api/v1/login.go b/pkg/routes/api/v1/login.go index 8bcde0dcf..ae92e1d72 100644 --- a/pkg/routes/api/v1/login.go +++ b/pkg/routes/api/v1/login.go @@ -77,7 +77,7 @@ func Login(c *echo.Context) (err error) { } // This allows us to still have local users while ldap is enabled - user, err = user2.CheckUserCredentials(s, &u) + user, err = user2.CheckUserCredentials(c.Request().Context(), s, &u) if err != nil { _ = s.Rollback() return err diff --git a/pkg/routes/api/v1/user_update_email.go b/pkg/routes/api/v1/user_update_email.go index e73ba6f89..7e03b250a 100644 --- a/pkg/routes/api/v1/user_update_email.go +++ b/pkg/routes/api/v1/user_update_email.go @@ -62,7 +62,7 @@ func UpdateUserEmail(c *echo.Context) (err error) { s := db.NewSession() defer s.Close() - if err := user.ChangeUserEmail(s, emailUpdate.User, emailUpdate.Password, emailUpdate.NewEmail); err != nil { + if err := user.ChangeUserEmail(c.Request().Context(), s, emailUpdate.User, emailUpdate.Password, emailUpdate.NewEmail); err != nil { _ = s.Rollback() return err } diff --git a/pkg/routes/api/v1/user_update_password.go b/pkg/routes/api/v1/user_update_password.go index 52941a48a..87b372aff 100644 --- a/pkg/routes/api/v1/user_update_password.go +++ b/pkg/routes/api/v1/user_update_password.go @@ -66,7 +66,7 @@ func UserChangePassword(c *echo.Context) error { s := db.NewSession() defer s.Close() - if err := models.ChangeUserPassword(s, doer, newPW.OldPassword, newPW.NewPassword); err != nil { + if err := models.ChangeUserPassword(c.Request().Context(), s, doer, newPW.OldPassword, newPW.NewPassword); err != nil { _ = s.Rollback() return err } diff --git a/pkg/routes/api/v2/user_settings.go b/pkg/routes/api/v2/user_settings.go index a1f5bbee4..35a366644 100644 --- a/pkg/routes/api/v2/user_settings.go +++ b/pkg/routes/api/v2/user_settings.go @@ -176,7 +176,7 @@ func userChangePassword(ctx context.Context, in *struct { s := db.NewSession() defer s.Close() - if err := models.ChangeUserPassword(s, doer, in.Body.OldPassword, in.Body.NewPassword); err != nil { + if err := models.ChangeUserPassword(ctx, s, doer, in.Body.OldPassword, in.Body.NewPassword); err != nil { _ = s.Rollback() return nil, translateDomainError(err) } @@ -206,7 +206,7 @@ func userUpdateEmail(ctx context.Context, in *struct { s := db.NewSession() defer s.Close() - if err := user.ChangeUserEmail(s, doer, in.Body.Password, in.Body.NewEmail); err != nil { + if err := user.ChangeUserEmail(ctx, s, doer, in.Body.Password, in.Body.NewEmail); err != nil { _ = s.Rollback() return nil, translateDomainError(err) } diff --git a/pkg/routes/caldav/auth.go b/pkg/routes/caldav/auth.go index 930b8f013..fc89d7555 100644 --- a/pkg/routes/caldav/auth.go +++ b/pkg/routes/caldav/auth.go @@ -88,7 +88,7 @@ func BasicAuth(c *echo.Context, username, password string) (bool, error) { return false, nil } if u == nil { - u, err = user.CheckUserCredentials(s, credentials) + u, err = user.CheckUserCredentials(c.Request().Context(), s, credentials) if err != nil { log.Errorf("Error during basic auth for caldav: %v", err) return false, nil diff --git a/pkg/user/update_email.go b/pkg/user/update_email.go index b721ba518..26606c152 100644 --- a/pkg/user/update_email.go +++ b/pkg/user/update_email.go @@ -17,6 +17,8 @@ package user import ( + "context" + "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/notifications" "xorm.io/xorm" @@ -34,8 +36,8 @@ type EmailUpdate struct { // ChangeUserEmail verifies the user's password, then sets a new email address // (kicking off confirmation when the mailer is enabled). Shared by the v1 and // v2 email-update handlers; only HTTP input binding stays in the handlers. -func ChangeUserEmail(s *xorm.Session, u *User, password, newEmail string) error { - verified, err := CheckUserCredentials(s, &Login{Username: u.Username, Password: password}) +func ChangeUserEmail(ctx context.Context, s *xorm.Session, u *User, password, newEmail string) error { + verified, err := CheckUserCredentials(ctx, s, &Login{Username: u.Username, Password: password}) if err != nil { return err } diff --git a/pkg/user/user.go b/pkg/user/user.go index ab2912982..09fef2565 100644 --- a/pkg/user/user.go +++ b/pkg/user/user.go @@ -17,6 +17,7 @@ package user import ( + "context" "encoding/json" "errors" "fmt" @@ -363,8 +364,9 @@ func getUserByUsernameOrEmail(s *xorm.Session, usernameOrEmail string) (u *User, return } -// CheckUserCredentials checks user credentials -func CheckUserCredentials(s *xorm.Session, u *Login) (*User, error) { +// CheckUserCredentials checks user credentials. The context carries request +// metadata for the audit trail of failed attempts. +func CheckUserCredentials(ctx context.Context, s *xorm.Session, u *Login) (*User, error) { // Check if we have any credentials if u.Password == "" || u.Username == "" { return nil, ErrNoUsernamePassword{} @@ -391,7 +393,7 @@ func CheckUserCredentials(s *xorm.Session, u *Login) (*User, error) { err = CheckUserPassword(user, u.Password) if err != nil { if IsErrWrongUsernameOrPassword(err) { - handleFailedPassword(user) + handleFailedPassword(ctx, user) } return user, err } @@ -411,8 +413,8 @@ func (u *User) IsLocalUser() bool { return u.Issuer == IssuerLocal } -func handleFailedPassword(user *User) { - if err := events.Dispatch(&LoginFailedEvent{User: user}); err != nil { +func handleFailedPassword(ctx context.Context, user *User) { + if err := events.DispatchWithContext(ctx, &LoginFailedEvent{User: user}); err != nil { log.Errorf("Could not dispatch login failed event: %s", err) } diff --git a/pkg/user/user_test.go b/pkg/user/user_test.go index 776a60b5d..38287c6e0 100644 --- a/pkg/user/user_test.go +++ b/pkg/user/user_test.go @@ -17,6 +17,7 @@ package user import ( + "context" "testing" "code.vikunja.io/api/pkg/db" @@ -357,7 +358,7 @@ func TestCheckUserCredentials(t *testing.T) { s := db.NewSession() defer s.Close() - _, err := CheckUserCredentials(s, &Login{Username: "user1", Password: "12345678"}) + _, err := CheckUserCredentials(context.Background(), s, &Login{Username: "user1", Password: "12345678"}) require.NoError(t, err) }) t.Run("unverified email", func(t *testing.T) { @@ -365,7 +366,7 @@ func TestCheckUserCredentials(t *testing.T) { s := db.NewSession() defer s.Close() - _, err := CheckUserCredentials(s, &Login{Username: "user5", Password: "12345678"}) + _, err := CheckUserCredentials(context.Background(), s, &Login{Username: "user5", Password: "12345678"}) require.Error(t, err) assert.True(t, IsErrEmailNotConfirmed(err)) }) @@ -374,7 +375,7 @@ func TestCheckUserCredentials(t *testing.T) { s := db.NewSession() defer s.Close() - _, err := CheckUserCredentials(s, &Login{Username: "user1", Password: "12345"}) + _, err := CheckUserCredentials(context.Background(), s, &Login{Username: "user1", Password: "12345"}) require.Error(t, err) assert.True(t, IsErrWrongUsernameOrPassword(err)) }) @@ -383,7 +384,7 @@ func TestCheckUserCredentials(t *testing.T) { s := db.NewSession() defer s.Close() - _, err := CheckUserCredentials(s, &Login{Username: "dfstestuu", Password: "12345678"}) + _, err := CheckUserCredentials(context.Background(), s, &Login{Username: "dfstestuu", Password: "12345678"}) require.Error(t, err) assert.True(t, IsErrWrongUsernameOrPassword(err)) }) @@ -392,7 +393,7 @@ func TestCheckUserCredentials(t *testing.T) { s := db.NewSession() defer s.Close() - _, err := CheckUserCredentials(s, &Login{Username: "user1"}) + _, err := CheckUserCredentials(context.Background(), s, &Login{Username: "user1"}) require.Error(t, err) assert.True(t, IsErrNoUsernamePassword(err)) }) @@ -401,7 +402,7 @@ func TestCheckUserCredentials(t *testing.T) { s := db.NewSession() defer s.Close() - _, err := CheckUserCredentials(s, &Login{Password: "12345678"}) + _, err := CheckUserCredentials(context.Background(), s, &Login{Password: "12345678"}) require.Error(t, err) assert.True(t, IsErrNoUsernamePassword(err)) }) @@ -410,7 +411,7 @@ func TestCheckUserCredentials(t *testing.T) { s := db.NewSession() defer s.Close() - _, err := CheckUserCredentials(s, &Login{Username: "user1@example.com", Password: "12345678"}) + _, err := CheckUserCredentials(context.Background(), s, &Login{Username: "user1@example.com", Password: "12345678"}) require.NoError(t, err) }) t.Run("disabled user", func(t *testing.T) { @@ -419,7 +420,7 @@ func TestCheckUserCredentials(t *testing.T) { defer s.Close() // user17 is disabled (status=2), password is "12345678" - _, err := CheckUserCredentials(s, &Login{Username: "user17", Password: "12345678"}) + _, err := CheckUserCredentials(context.Background(), s, &Login{Username: "user17", Password: "12345678"}) require.Error(t, err) assert.True(t, IsErrAccountDisabled(err)) }) @@ -429,7 +430,7 @@ func TestCheckUserCredentials(t *testing.T) { defer s.Close() // user18 is locked (status=3), password is "12345678" - _, err := CheckUserCredentials(s, &Login{Username: "user18", Password: "12345678"}) + _, err := CheckUserCredentials(context.Background(), s, &Login{Username: "user18", Password: "12345678"}) require.Error(t, err) assert.True(t, IsErrAccountLocked(err)) })