From 7d2a7b855986b2cfdede8d69abd3cb329f072785 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 28 May 2025 14:46:23 +0200 Subject: [PATCH] feat: add validating user password as trace region (#7981) - Password hashing can take a measurable amount of time, make this more visible in the trace by capturing the computations done in the password hash in their own region. - Ref: forgejo/forgejo#6470 ## Screenshot ![image](/attachments/9834b094-a78f-4ac2-847e-91f221a84833) The upper part are where the tasks are shown (and nothing else). The bottom part is where the interesting execution tracing happens and the part where the user password hashing happens is now properly indicated/highlighted and does not need to be inferred by looking at the stack traces. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7981 Reviewed-by: Earl Warren Co-authored-by: Gusted Co-committed-by: Gusted --- models/user/user.go | 4 +++- models/user/user_test.go | 2 +- routers/web/auth/auth.go | 2 +- routers/web/user/setting/account.go | 2 +- services/auth/source/db/authenticate.go | 2 +- tests/integration/user_test.go | 2 +- 6 files changed, 8 insertions(+), 6 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index d75fe56a20..eedd1db80e 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -15,6 +15,7 @@ import ( "net/url" "path/filepath" "regexp" + "runtime/trace" "strings" "time" "unicode" @@ -397,7 +398,8 @@ func (u *User) SetPassword(passwd string) (err error) { } // ValidatePassword checks if the given password matches the one belonging to the user. -func (u *User) ValidatePassword(passwd string) bool { +func (u *User) ValidatePassword(ctx context.Context, passwd string) bool { + defer trace.StartRegion(ctx, "Validate user password").End() return hash.Parse(u.PasswdHashAlgo).VerifyPassword(passwd, u.Passwd, u.Salt) } diff --git a/models/user/user_test.go b/models/user/user_test.go index 3c302864c7..2a9e652a35 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -267,7 +267,7 @@ func TestHashPasswordDeterministic(t *testing.T) { r2 := u.Passwd assert.NotEqual(t, r1, r2) - assert.True(t, u.ValidatePassword(pass)) + assert.True(t, u.ValidatePassword(t.Context(), pass)) } } } diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 5ee80769d3..dbb6665398 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -764,7 +764,7 @@ func ActivatePost(ctx *context.Context) { ctx.HTML(http.StatusOK, TplActivate) return } - if !user.ValidatePassword(password) { + if !user.ValidatePassword(ctx, password) { ctx.Data["IsPasswordInvalid"] = true ctx.HTML(http.StatusOK, TplActivate) return diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index 4967b5f26e..1dfcc90e35 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -57,7 +57,7 @@ func AccountPost(ctx *context.Context) { return } - if ctx.Doer.IsPasswordSet() && !ctx.Doer.ValidatePassword(form.OldPassword) { + if ctx.Doer.IsPasswordSet() && !ctx.Doer.ValidatePassword(ctx, form.OldPassword) { ctx.Flash.Error(ctx.Tr("settings.password_incorrect")) } else if form.Password != form.Retype { ctx.Flash.Error(ctx.Tr("form.password_not_match")) diff --git a/services/auth/source/db/authenticate.go b/services/auth/source/db/authenticate.go index 7c18540a10..b1d8eae6ae 100644 --- a/services/auth/source/db/authenticate.go +++ b/services/auth/source/db/authenticate.go @@ -50,7 +50,7 @@ func Authenticate(ctx context.Context, user *user_model.User, login, password st if !user.IsPasswordSet() { return nil, ErrUserPasswordNotSet{UID: user.ID, Name: user.Name} - } else if !user.ValidatePassword(password) { + } else if !user.ValidatePassword(ctx, password) { return nil, ErrUserPasswordInvalid{UID: user.ID, Name: user.Name} } diff --git a/tests/integration/user_test.go b/tests/integration/user_test.go index 9da62c1cae..0f659e6b02 100644 --- a/tests/integration/user_test.go +++ b/tests/integration/user_test.go @@ -1062,7 +1062,7 @@ func TestUserPasswordReset(t *testing.T) { session.MakeRequest(t, req, http.StatusSeeOther) unittest.AssertNotExistsBean(t, &auth_model.AuthorizationToken{ID: authToken.ID}) - assert.True(t, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).ValidatePassword("new_password")) + assert.True(t, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).ValidatePassword(t.Context(), "new_password")) } func TestActivateEmailAddress(t *testing.T) {