From 06556abb6d07c379dab4958e62f08bedff4f1d6d Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 17 Feb 2025 11:09:33 +0000 Subject: [PATCH] fix: delay deleting authorization token (#6937) - 1ce33aa38d1d258d14523ff2c7c2dbf339f22b74 extended the LTA table with a purpose column so it could be extended to other tokens. However some are single-use tokens and should be deleted after use. - This did not result in a good UX for activating user as they needed to also fill in their passwords and in the case that the password was incorrect the token would no longer be usable. - This patch modifies the code to allow for a little delay before deleting authorization tokens to do additional verification such as the password check. This cannot be done before the authorization token check as that the authorization token determines who the user is. - Resolves forgejo/forgejo#6912 - Adjusted existing unit test. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6937 Reviewed-by: Otto Co-authored-by: Gusted Co-committed-by: Gusted --- models/user/user.go | 26 ++++++++++++-------------- models/user/user_test.go | 7 ++++--- routers/web/auth/auth.go | 23 +++++++++++++++++++---- routers/web/auth/password.go | 9 ++++++++- tests/integration/user_test.go | 24 ++++++++++++++++++++---- 5 files changed, 63 insertions(+), 26 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index 846190a648..37dd69f42d 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -865,48 +865,46 @@ func countUsers(ctx context.Context, opts *CountUserFilter) int64 { // VerifyUserActiveCode verifies that the code is valid for the given purpose for this user. // If delete is specified, the token will be deleted. -func VerifyUserAuthorizationToken(ctx context.Context, code string, purpose auth.AuthorizationPurpose, delete bool) (*User, error) { +func VerifyUserAuthorizationToken(ctx context.Context, code string, purpose auth.AuthorizationPurpose) (user *User, deleteToken func() error, err error) { lookupKey, validator, found := strings.Cut(code, ":") if !found { - return nil, nil + return nil, nil, nil } authToken, err := auth.FindAuthToken(ctx, lookupKey, purpose) if err != nil { if errors.Is(err, util.ErrNotExist) { - return nil, nil + return nil, nil, nil } - return nil, err + return nil, nil, err } if authToken.IsExpired() { - return nil, auth.DeleteAuthToken(ctx, authToken) + return nil, nil, auth.DeleteAuthToken(ctx, authToken) } rawValidator, err := hex.DecodeString(validator) if err != nil { - return nil, err + return nil, nil, err } if subtle.ConstantTimeCompare([]byte(authToken.HashedValidator), []byte(auth.HashValidator(rawValidator))) == 0 { - return nil, errors.New("validator doesn't match") + return nil, nil, errors.New("validator doesn't match") } u, err := GetUserByID(ctx, authToken.UID) if err != nil { if IsErrUserNotExist(err) { - return nil, nil + return nil, nil, nil } - return nil, err + return nil, nil, err } - if delete { - if err := auth.DeleteAuthToken(ctx, authToken); err != nil { - return nil, err - } + deleteToken = func() error { + return auth.DeleteAuthToken(ctx, authToken) } - return u, nil + return u, deleteToken, nil } // ValidateUser check if user is valid to insert / update into database diff --git a/models/user/user_test.go b/models/user/user_test.go index e3479943e3..9ac63b5a72 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -756,13 +756,13 @@ func TestVerifyUserAuthorizationToken(t *testing.T) { assert.True(t, ok) t.Run("Wrong purpose", func(t *testing.T) { - u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.PasswordReset, false) + u, _, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.PasswordReset) require.NoError(t, err) assert.Nil(t, u) }) t.Run("No delete", func(t *testing.T) { - u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation, false) + u, _, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation) require.NoError(t, err) assert.EqualValues(t, user.ID, u.ID) @@ -772,9 +772,10 @@ func TestVerifyUserAuthorizationToken(t *testing.T) { }) t.Run("Delete", func(t *testing.T) { - u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation, true) + u, deleteToken, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation) require.NoError(t, err) assert.EqualValues(t, user.ID, u.ID) + require.NoError(t, deleteToken()) authToken, err := auth.FindAuthToken(db.DefaultContext, lookupKey, auth.UserActivation) require.ErrorIs(t, err, util.ErrNotExist) diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 92305ad26a..d592dff077 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -62,7 +62,7 @@ func autoSignIn(ctx *context.Context) (bool, error) { return false, nil } - u, err := user_model.VerifyUserAuthorizationToken(ctx, authCookie, auth.LongTermAuthorization, false) + u, _, err := user_model.VerifyUserAuthorizationToken(ctx, authCookie, auth.LongTermAuthorization) if err != nil { return false, fmt.Errorf("VerifyUserAuthorizationToken: %w", err) } @@ -672,7 +672,7 @@ func Activate(ctx *context.Context) { return } - user, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation, false) + user, deleteToken, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation) if err != nil { ctx.ServerError("VerifyUserAuthorizationToken", err) return @@ -693,6 +693,11 @@ func Activate(ctx *context.Context) { return } + if err := deleteToken(); err != nil { + ctx.ServerError("deleteToken", err) + return + } + handleAccountActivation(ctx, user) } @@ -741,7 +746,7 @@ func ActivatePost(ctx *context.Context) { return } - user, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation, true) + user, deleteToken, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation) if err != nil { ctx.ServerError("VerifyUserAuthorizationToken", err) return @@ -770,6 +775,11 @@ func ActivatePost(ctx *context.Context) { } } + if err := deleteToken(); err != nil { + ctx.ServerError("deleteToken", err) + return + } + handleAccountActivation(ctx, user) } @@ -830,7 +840,7 @@ func ActivateEmail(ctx *context.Context) { code := ctx.FormString("code") emailStr := ctx.FormString("email") - u, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.EmailActivation(emailStr), true) + u, deleteToken, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.EmailActivation(emailStr)) if err != nil { ctx.ServerError("VerifyUserAuthorizationToken", err) return @@ -840,6 +850,11 @@ func ActivateEmail(ctx *context.Context) { return } + if err := deleteToken(); err != nil { + ctx.ServerError("deleteToken", err) + return + } + email, err := user_model.GetEmailAddressOfUser(ctx, emailStr, u.ID) if err != nil { ctx.ServerError("GetEmailAddressOfUser", err) diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index 363c01c6a8..84f343bfca 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -116,7 +116,7 @@ func commonResetPassword(ctx *context.Context, shouldDeleteToken bool) (*user_mo } // Fail early, don't frustrate the user - u, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.PasswordReset, shouldDeleteToken) + u, deleteToken, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.PasswordReset) if err != nil { ctx.ServerError("VerifyUserAuthorizationToken", err) return nil, nil @@ -127,6 +127,13 @@ func commonResetPassword(ctx *context.Context, shouldDeleteToken bool) (*user_mo return nil, nil } + if shouldDeleteToken { + if err := deleteToken(); err != nil { + ctx.ServerError("deleteToken", err) + return nil, nil + } + } + twofa, err := auth.GetTwoFactorByUID(ctx, u.ID) if err != nil { if !auth.IsErrTwoFactorNotEnrolled(err) { diff --git a/tests/integration/user_test.go b/tests/integration/user_test.go index 3da8a3ae1d..04ba690886 100644 --- a/tests/integration/user_test.go +++ b/tests/integration/user_test.go @@ -920,11 +920,27 @@ func TestUserActivate(t *testing.T) { assert.False(t, authToken.IsExpired()) assert.EqualValues(t, authToken.HashedValidator, auth_model.HashValidator(rawValidator)) - req = NewRequest(t, "POST", "/user/activate?code="+code) - session.MakeRequest(t, req, http.StatusOK) + t.Run("No password", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() - unittest.AssertNotExistsBean(t, &auth_model.AuthorizationToken{ID: authToken.ID}) - unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "doesnotexist", IsActive: true}) + req = NewRequest(t, "POST", "/user/activate?code="+code) + session.MakeRequest(t, req, http.StatusOK) + + unittest.AssertExistsIf(t, true, &auth_model.AuthorizationToken{ID: authToken.ID}) + unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "doesnotexist"}, "is_active = false") + }) + + t.Run("With password", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req = NewRequestWithValues(t, "POST", "/user/activate?code="+code, map[string]string{ + "password": "examplePassword!1", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + unittest.AssertExistsIf(t, false, &auth_model.AuthorizationToken{ID: authToken.ID}) + unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "doesnotexist"}, "is_active = true") + }) } func TestUserPasswordReset(t *testing.T) {