Expire password reset tokens when used (fixes #3270). #3388

AndrolGenhald · 2023-06-28T05:58:39Z

Added an expires_at column for password reset requests that gets set to a few seconds in the past whenever a new password reset request is created for a user, or when it's used.
I also added a hash index on the hashed token since that's primarily what's going to be queried and a full table scan isn't great once the table gets larger.

Nutomic · 2023-06-28T09:34:21Z

crates/db_schema/src/source/password_reset_request.rs

@@ -10,11 +10,13 @@ pub struct PasswordResetRequest {
  pub token_encrypted: String,
  pub published: chrono::NaiveDateTime,
  pub local_user_id: LocalUserId,
+  pub expires_at: chrono::NaiveDateTime,


I dont see any reason to add a new column here. Would be enough to delete the row when the password reset was performed successfully.

Note that deleting rows would enable malicious actors to bypass the daily limit I added in #3344

What @sunaurus said, I don't see any real harm in adding the extra column. If expiration duration ever becomes configurable it might be nice to be able to tell when the actual expiration time was, vs having to figure out what the expiration duration was configured to at the time and add it to the published timestamp.

Makes sense about the daily limit. But for that a boolean column expired would be enough, I dont see any reason for a second timestamp.

I could change it to bool if you like, I just figured storing when it expired might be useful information in some case.

Yeah, I suppose a bool would work fine, I've just always seen it implemented with a timestamp so that's what I went with 🤷 I'll change it later when I have time.

We should also add a daily job to delete these based on their published time > than a day, otherwise the password reset request table will grow pointlessly large.

Just got some time to work on this again and I realized why I'm not so much a fan of a boolean column. If we go with "expired" as the name then it's confusing because you'll end up with rows that are expired when the expired column is still false. If we go with "used" we'll be marking rows as used when they weren't actually used, they're just no longer valid. Is there a way to name this that's not confusing?

Why would the column be marked as used before its been used? It should be set to true after its been used.

It would be marked used if a new token is generated before 24 hours. Maybe it's not too big of a deal and I'm overthinking it, I'll get it updated this weekend.

crates/db_schema/src/impls/password_reset_request.rs

dessalines

I agree with nutomic that a simple used boolean column would be good.

Another thing: Why are we pointlessly converting the token back and forth to from bytes to hex? What would be wrong about just using an autogenerated uuid column in postgres, emailing them that, and checking it?

dessalines · 2023-07-03T16:44:09Z

migrations/2023-06-23-172550_expire_password_reset_tokens/up.sql

@@ -0,0 +1,2 @@
+alter table password_reset_request add column expires_at timestamp not null;


Adding a not null column without a default means that any existing password_reset_request rows will result in a broken migration. I suggest either deleting them all before running this, or adding a default.

dessalines · 2023-07-03T16:47:36Z

migrations/2023-06-23-172550_expire_password_reset_tokens/up.sql

@@ -0,0 +1,2 @@
+alter table password_reset_request add column expires_at timestamp not null;
+create index idx_password_reset_request_token_encrypted on password_reset_request using hash (token_encrypted);


using hash? Why not just a regular index?

Another thing: why not just generate the UUID in postgres, instead of rust?

using hash? Why not just a regular index?

Btree index seems a waste since ordering by hashed tokens doesn't seem useful in any way, hash index is more performant for direct access.

Another thing: why not just generate the UUID in postgres, instead of rust?

A couple of reasons, one is so that it can be hashed before inserting, and another is that it's not clear if postgres's UUID4 generation is guaranteed to be cryptographically secure.

crates/db_schema/src/impls/password_reset_request.rs

dessalines · 2023-07-03T16:53:07Z

crates/db_schema/src/source/password_reset_request.rs

@@ -10,11 +10,13 @@ pub struct PasswordResetRequest {
  pub token_encrypted: String,
  pub published: chrono::NaiveDateTime,
  pub local_user_id: LocalUserId,
+  pub expires_at: chrono::NaiveDateTime,


We should also add a daily job to delete these based on their published time > than a day, otherwise the password reset request table will grow pointlessly large.

Nutomic · 2023-07-13T10:13:03Z

crates/api/src/local_user/change_password_after_reset.rs

@@ -33,11 +33,16 @@ impl Perform for PasswordChangeAfterReset {
      return Err(LemmyError::from_message("passwords_dont_match"));
    }

+    // Expire reset token
+    // TODO do this in a transaction along with the user update (blocked by https://github.com/LemmyNet/lemmy/issues/1161)
+    PasswordResetRequest::expire(context.pool(), reset_request.id).await?;


Transactions should be doable now that #3420 is merged.

AndrolGenhald requested review from Nutomic and dessalines as code owners June 28, 2023 05:58

Nutomic reviewed Jun 28, 2023

View reviewed changes

Nutomic reviewed Jun 29, 2023

View reviewed changes

crates/db_schema/src/impls/password_reset_request.rs Outdated Show resolved Hide resolved

AndrolGenhald added 2 commits June 30, 2023 00:57

Expire password reset tokens when used (fixes LemmyNet#3270).

ad18c03

Improve password reset token expiry.

c82f2d3

AndrolGenhald force-pushed the expire-password-reset-tokens branch from baf80cf to c82f2d3 Compare June 30, 2023 05:57

dessalines reviewed Jul 3, 2023

View reviewed changes

dessalines mentioned this pull request Jul 5, 2023

Reduce complexity by making short-term password reset tokens use UUIDs only. #3491

Closed

4 tasks

Nutomic reviewed Jul 13, 2023

View reviewed changes

Nutomic closed this Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expire password reset tokens when used (fixes #3270). #3388

Expire password reset tokens when used (fixes #3270). #3388

AndrolGenhald commented Jun 28, 2023

Nutomic Jun 28, 2023

sunaurus Jun 28, 2023 •

edited

Loading

AndrolGenhald Jun 29, 2023

Nutomic Jun 29, 2023

AndrolGenhald Jun 30, 2023

AndrolGenhald Jun 30, 2023

dessalines Jul 3, 2023

AndrolGenhald Jul 10, 2023

dessalines Jul 12, 2023

AndrolGenhald Jul 13, 2023

dessalines left a comment

dessalines Jul 3, 2023

dessalines Jul 3, 2023

dessalines Jul 3, 2023

AndrolGenhald Jul 3, 2023

dessalines Jul 3, 2023

Nutomic Jul 13, 2023

		@@ -0,0 +1,2 @@
		alter table password_reset_request add column expires_at timestamp not null;

		@@ -0,0 +1,2 @@
		alter table password_reset_request add column expires_at timestamp not null;
		create index idx_password_reset_request_token_encrypted on password_reset_request using hash (token_encrypted);

Expire password reset tokens when used (fixes #3270). #3388

Expire password reset tokens when used (fixes #3270). #3388

Conversation

AndrolGenhald commented Jun 28, 2023

Choose a reason for hiding this comment

sunaurus Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunaurus Jun 28, 2023 •

edited

Loading