Use SQL for password reset token queries#3546
Conversation
dnephin
requested review from
BruceMacD,
jmorganca and
pdevine
as code owners
| // TODO: must use errors.As for error types | ||
| if tries <= 3 && errors.Is(err, UniqueConstraintError{}) { |
There was a problem hiding this comment.
I also resolved this TODO at the same time. Previously the errors.Is would never return true because the err would have the Table and Column field set.
dnephin
force-pushed
the
dnephin/sql-for-passwordreset
branch
from
1325cb5 to
234975b
Compare
|
I added some direct test coverage of these two functions and fixed the conflicts. |
internal/access/passwordreset.go
Outdated
| ) | ||
|
|
||
| func PasswordResetRequest(c *gin.Context, email string, ttl time.Duration) (token string, user *models.Identity, err error) { | ||
| func PasswordResetRequest(c *gin.Context, email string, ttl time.Duration) (string, *models.Identity, error) { |
There was a problem hiding this comment.
unnamed string return arguments are less clear than named ones.
There was a problem hiding this comment.
Ya, especially true when there are multiple return values with the same type. Usually when they have different types it's pretty clear.
It's unfortunate that naming them also extends the scope of the variable (to any place in the function, instead of only after declaration). I can add these back
| return tx.Commit() | ||
| } | ||
|
|
||
| func getDefaultSortFromType(t interface{}) string { |
There was a problem hiding this comment.
Every list function has a sort, but there's no central function that uses reflection to try and guess at which field to use.
| _, err = GetUserIDForPasswordResetToken(tx, token) | ||
| assert.ErrorIs(t, err, internal.ErrExpired) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
might be worth adding a case where the record is gone because the background job trimmed it already.
There was a problem hiding this comment.
I believe that case is covered by the second call in deletes token. It's calling the function with a token that was already deleted.
dnephin
force-pushed
the
dnephin/sql-for-passwordreset
branch
from
eda75b6 to
82b82f5
Compare
Also remove the model. No other packages care about this model, so we can use a unexported type in the data package.
Now that this function does a delete, rename it from Get to Claim to indicate that it's more than just returning data.
dnephin
force-pushed
the
dnephin/sql-for-passwordreset
branch
from
82b82f5 to
05fc867
Compare
| // ClaimPasswordResetToken deletes the password reset token, and returns the | ||
| // user ID that was associated with the token. Returns an error if the token | ||
| // does not exist or has expired. | ||
| func ClaimPasswordResetToken(tx WriteTxn, token string) (uid.ID, error) { |
There was a problem hiding this comment.
I pushed one more commit which renames this function from Get to Claim. Now that it does a delete, I think calling it Get could easily be misleading.
2 participants
Summary
This PR converts the two data queries used for password reset tokens to use SQL.
I also made the following changes:
RETURNINGto return the fields we care about at the same time. This allows us to remove one of the data functions.models.PasswordResetToken. With a couple small changes to the return values this type became an implementation detail of thedatapackage. The caller only needs theuser.IDand thetoken, which we can return without a type. The model was replaced with an unexported struct in thedatapackage.