Skip to content

Commit 06556ab

Browse files
GustedGusted
Gusted
authored and
Gusted
committed
fix: delay deleting authorization token (go-gitea#6937)
- 1ce33aa 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 <[email protected]> Co-authored-by: Gusted <[email protected]> Co-committed-by: Gusted <[email protected]>
1 parent 1a77edc commit 06556ab

File tree

5 files changed

+63
-26
lines changed

5 files changed

+63
-26
lines changed

‎models/user/user.go‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -865,48 +865,46 @@ func countUsers(ctx context.Context, opts *CountUserFilter) int64 {
865865

866866
// VerifyUserActiveCode verifies that the code is valid for the given purpose for this user.
867867
// If delete is specified, the token will be deleted.
868-
func VerifyUserAuthorizationToken(ctx context.Context, code string, purpose auth.AuthorizationPurpose, delete bool) (*User, error) {
868+
func VerifyUserAuthorizationToken(ctx context.Context, code string, purpose auth.AuthorizationPurpose) (user *User, deleteToken func() error, err error) {
869869
lookupKey, validator, found := strings.Cut(code, ":")
870870
if !found {
871-
return nil, nil
871+
return nil, nil, nil
872872
}
873873

874874
authToken, err := auth.FindAuthToken(ctx, lookupKey, purpose)
875875
if err != nil {
876876
if errors.Is(err, util.ErrNotExist) {
877-
return nil, nil
877+
return nil, nil, nil
878878
}
879-
return nil, err
879+
return nil, nil, err
880880
}
881881

882882
if authToken.IsExpired() {
883-
return nil, auth.DeleteAuthToken(ctx, authToken)
883+
return nil, nil, auth.DeleteAuthToken(ctx, authToken)
884884
}
885885

886886
rawValidator, err := hex.DecodeString(validator)
887887
if err != nil {
888-
return nil, err
888+
return nil, nil, err
889889
}
890890

891891
if subtle.ConstantTimeCompare([]byte(authToken.HashedValidator), []byte(auth.HashValidator(rawValidator))) == 0 {
892-
return nil, errors.New("validator doesn't match")
892+
return nil, nil, errors.New("validator doesn't match")
893893
}
894894

895895
u, err := GetUserByID(ctx, authToken.UID)
896896
if err != nil {
897897
if IsErrUserNotExist(err) {
898-
return nil, nil
898+
return nil, nil, nil
899899
}
900-
return nil, err
900+
return nil, nil, err
901901
}
902902

903-
if delete {
904-
if err := auth.DeleteAuthToken(ctx, authToken); err != nil {
905-
return nil, err
906-
}
903+
deleteToken = func() error {
904+
return auth.DeleteAuthToken(ctx, authToken)
907905
}
908906

909-
return u, nil
907+
return u, deleteToken, nil
910908
}
911909

912910
// ValidateUser check if user is valid to insert / update into database

‎models/user/user_test.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -756,13 +756,13 @@ func TestVerifyUserAuthorizationToken(t *testing.T) {
756756
assert.True(t, ok)
757757

758758
t.Run("Wrong purpose", func(t *testing.T) {
759-
u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.PasswordReset, false)
759+
u, _, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.PasswordReset)
760760
require.NoError(t, err)
761761
assert.Nil(t, u)
762762
})
763763

764764
t.Run("No delete", func(t *testing.T) {
765-
u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation, false)
765+
u, _, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation)
766766
require.NoError(t, err)
767767
assert.EqualValues(t, user.ID, u.ID)
768768

@@ -772,9 +772,10 @@ func TestVerifyUserAuthorizationToken(t *testing.T) {
772772
})
773773

774774
t.Run("Delete", func(t *testing.T) {
775-
u, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation, true)
775+
u, deleteToken, err := user_model.VerifyUserAuthorizationToken(db.DefaultContext, code, auth.UserActivation)
776776
require.NoError(t, err)
777777
assert.EqualValues(t, user.ID, u.ID)
778+
require.NoError(t, deleteToken())
778779

779780
authToken, err := auth.FindAuthToken(db.DefaultContext, lookupKey, auth.UserActivation)
780781
require.ErrorIs(t, err, util.ErrNotExist)

‎routers/web/auth/auth.go‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func autoSignIn(ctx *context.Context) (bool, error) {
6262
return false, nil
6363
}
6464

65-
u, err := user_model.VerifyUserAuthorizationToken(ctx, authCookie, auth.LongTermAuthorization, false)
65+
u, _, err := user_model.VerifyUserAuthorizationToken(ctx, authCookie, auth.LongTermAuthorization)
6666
if err != nil {
6767
return false, fmt.Errorf("VerifyUserAuthorizationToken: %w", err)
6868
}
@@ -672,7 +672,7 @@ func Activate(ctx *context.Context) {
672672
return
673673
}
674674

675-
user, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation, false)
675+
user, deleteToken, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation)
676676
if err != nil {
677677
ctx.ServerError("VerifyUserAuthorizationToken", err)
678678
return
@@ -693,6 +693,11 @@ func Activate(ctx *context.Context) {
693693
return
694694
}
695695

696+
if err := deleteToken(); err != nil {
697+
ctx.ServerError("deleteToken", err)
698+
return
699+
}
700+
696701
handleAccountActivation(ctx, user)
697702
}
698703

@@ -741,7 +746,7 @@ func ActivatePost(ctx *context.Context) {
741746
return
742747
}
743748

744-
user, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation, true)
749+
user, deleteToken, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.UserActivation)
745750
if err != nil {
746751
ctx.ServerError("VerifyUserAuthorizationToken", err)
747752
return
@@ -770,6 +775,11 @@ func ActivatePost(ctx *context.Context) {
770775
}
771776
}
772777

778+
if err := deleteToken(); err != nil {
779+
ctx.ServerError("deleteToken", err)
780+
return
781+
}
782+
773783
handleAccountActivation(ctx, user)
774784
}
775785

@@ -830,7 +840,7 @@ func ActivateEmail(ctx *context.Context) {
830840
code := ctx.FormString("code")
831841
emailStr := ctx.FormString("email")
832842

833-
u, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.EmailActivation(emailStr), true)
843+
u, deleteToken, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.EmailActivation(emailStr))
834844
if err != nil {
835845
ctx.ServerError("VerifyUserAuthorizationToken", err)
836846
return
@@ -840,6 +850,11 @@ func ActivateEmail(ctx *context.Context) {
840850
return
841851
}
842852

853+
if err := deleteToken(); err != nil {
854+
ctx.ServerError("deleteToken", err)
855+
return
856+
}
857+
843858
email, err := user_model.GetEmailAddressOfUser(ctx, emailStr, u.ID)
844859
if err != nil {
845860
ctx.ServerError("GetEmailAddressOfUser", err)

‎routers/web/auth/password.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ func commonResetPassword(ctx *context.Context, shouldDeleteToken bool) (*user_mo
116116
}
117117

118118
// Fail early, don't frustrate the user
119-
u, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.PasswordReset, shouldDeleteToken)
119+
u, deleteToken, err := user_model.VerifyUserAuthorizationToken(ctx, code, auth.PasswordReset)
120120
if err != nil {
121121
ctx.ServerError("VerifyUserAuthorizationToken", err)
122122
return nil, nil
@@ -127,6 +127,13 @@ func commonResetPassword(ctx *context.Context, shouldDeleteToken bool) (*user_mo
127127
return nil, nil
128128
}
129129

130+
if shouldDeleteToken {
131+
if err := deleteToken(); err != nil {
132+
ctx.ServerError("deleteToken", err)
133+
return nil, nil
134+
}
135+
}
136+
130137
twofa, err := auth.GetTwoFactorByUID(ctx, u.ID)
131138
if err != nil {
132139
if !auth.IsErrTwoFactorNotEnrolled(err) {

‎tests/integration/user_test.go‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -920,11 +920,27 @@ func TestUserActivate(t *testing.T) {
920920
assert.False(t, authToken.IsExpired())
921921
assert.EqualValues(t, authToken.HashedValidator, auth_model.HashValidator(rawValidator))
922922

923-
req = NewRequest(t, "POST", "/user/activate?code="+code)
924-
session.MakeRequest(t, req, http.StatusOK)
923+
t.Run("No password", func(t *testing.T) {
924+
defer tests.PrintCurrentTest(t)()
925925

926-
unittest.AssertNotExistsBean(t, &auth_model.AuthorizationToken{ID: authToken.ID})
927-
unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "doesnotexist", IsActive: true})
926+
req = NewRequest(t, "POST", "/user/activate?code="+code)
927+
session.MakeRequest(t, req, http.StatusOK)
928+
929+
unittest.AssertExistsIf(t, true, &auth_model.AuthorizationToken{ID: authToken.ID})
930+
unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "doesnotexist"}, "is_active = false")
931+
})
932+
933+
t.Run("With password", func(t *testing.T) {
934+
defer tests.PrintCurrentTest(t)()
935+
936+
req = NewRequestWithValues(t, "POST", "/user/activate?code="+code, map[string]string{
937+
"password": "examplePassword!1",
938+
})
939+
session.MakeRequest(t, req, http.StatusSeeOther)
940+
941+
unittest.AssertExistsIf(t, false, &auth_model.AuthorizationToken{ID: authToken.ID})
942+
unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "doesnotexist"}, "is_active = true")
943+
})
928944
}
929945

930946
func TestUserPasswordReset(t *testing.T) {

0 commit comments

Comments
 (0)