8
16
Fork 22

Store password reset requests in database #610

Zusammengeführt
rfwatson hat 4 Commits von feature/persist_password_reset nach master vor 5 Jahren zusammengeführt
rfwatson hat vor 5 Jahren kommentiert (Migriert von github.com)

Had a go at closing #600.

New to lots of stuff here (Rust, Diesel, Rocket, Plume...) so all feedback appreciated.

TODO

  • SQLite migrations
  • resolve inline TODOs
  • integration tests?
  • fix CI issues
Had a go at closing #600. New to lots of stuff here (Rust, Diesel, Rocket, Plume...) so all feedback appreciated. ## TODO * [x] SQLite migrations * [x] resolve inline TODOs * [ ] integration tests? * [x] fix CI issues
rfwatson (Migriert von github.com) hat vor 5 Jahren überprüft
@ -243,4 +223,4 @@
form: Form<NewPasswordForm>,
rockets: PlumeRocket,
) -> Result<Flash<Redirect>, Ructe> {
form.validate()
rfwatson (Migriert von github.com) hat vor 5 Jahren kommentiert

The logic to detect an expired token is private to the model now. Not sure how important having a specific error is. We could maybe return a custom Diesel error?

The logic to detect an expired token is private to the model now. Not sure how important having a specific error is. We could maybe return a custom Diesel error?
rfwatson (Migriert von github.com) hat vor 5 Jahren überprüft
rfwatson (Migriert von github.com) hat vor 5 Jahren kommentiert

It would be good to test that old password reset requests are ignored.

The simplest way I can think of would be to insert a record, and then update its creation_date with some raw SQL in the test. It feels pretty dirty though. Better ideas appreciated.

It would be good to test that old password reset requests are [ignored](https://github.com/Plume-org/Plume/pull/610/files#diff-a248869f1983a92462c39c8b2ea8a66eR44). The simplest way I can think of would be to insert a record, and then update its `creation_date` with some raw SQL in the test. It feels pretty dirty though. Better ideas appreciated.
elegaanz (Migriert von github.com) hat vor 5 Jahren überprüft
elegaanz (Migriert von github.com) hat einen Kommentar hinterlassen

What you did so far looks great! Thank you for helping with that.

What you did so far looks great! Thank you for helping with that.
elegaanz (Migriert von github.com) hat vor 5 Jahren kommentiert

thread::sleep(2 * 60 * 60 * 1000)

More seriously, I think you can pass tuples to diesel::insert_into(...).values(), like:

diesel::insert_into(password_reset_requests::table)
    .values((
        password_reset_requests::email.eq("foo@bar.org"),
        password_reset_requests::token.eq("aaaaaaaaa"),
        password_reset_requests::creation_date.eq(now - 3.hours()),
    ))

You can probably use it instead of PasswordResetRequest::insert in this test.

~~`thread::sleep(2 * 60 * 60 * 1000)`~~ More seriously, I think you can pass tuples to `diesel::insert_into(...).values()`, like: ```rust diesel::insert_into(password_reset_requests::table) .values(( password_reset_requests::email.eq("foo@bar.org"), password_reset_requests::token.eq("aaaaaaaaa"), password_reset_requests::creation_date.eq(now - 3.hours()), )) ``` You can probably use it instead of `PasswordResetRequest::insert` in this test.
@ -243,4 +223,4 @@
form: Form<NewPasswordForm>,
rockets: PlumeRocket,
) -> Result<Flash<Redirect>, Ructe> {
form.validate()
elegaanz (Migriert von github.com) hat vor 5 Jahren kommentiert

Maybe, instead of mapping the error to to_validation, you could have a closure that returns the specific error that was previously returned?

Maybe, instead of mapping the error to `to_validation`, you could have a closure that returns the specific error that was previously returned?
codecov[bot] hat vor 5 Jahren kommentiert (Migriert von github.com)

Codecov Report

No coverage uploaded for pull request base (master@b2312d7). Click here to learn what that means.
The diff coverage is 66.94%.

@@            Coverage Diff            @@
##             master     #610   +/-   ##
=========================================
  Coverage          ?   35.31%           
=========================================
  Files             ?       68           
  Lines             ?     7907           
  Branches          ?     1893           
=========================================
  Hits              ?     2792           
  Misses            ?     4345           
  Partials          ?      770
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/610?src=pr&el=h1) Report > :exclamation: No coverage uploaded for pull request base (`master@b2312d7`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit). > The diff coverage is `66.94%`. ```diff @@ Coverage Diff @@ ## master #610 +/- ## ========================================= Coverage ? 35.31% ========================================= Files ? 68 Lines ? 7907 Branches ? 1893 ========================================= Hits ? 2792 Misses ? 4345 Partials ? 770 ```
rfwatson (Migriert von github.com) hat vor 5 Jahren überprüft
@ -243,4 +223,4 @@
form: Form<NewPasswordForm>,
rockets: PlumeRocket,
) -> Result<Flash<Redirect>, Ructe> {
form.validate()
rfwatson (Migriert von github.com) hat vor 5 Jahren kommentiert

I ended up adding a new Expired error variant - it felt generic enough to be useful elsewhere in the app too. Happy to iterate on this more if it doesn't feel like a good approach though.

I ended up adding a new `Expired` error variant - it felt generic enough to be useful elsewhere in the app too. Happy to iterate on this more if it doesn't feel like a good approach though.
rfwatson hat vor 5 Jahren kommentiert (Migriert von github.com)

Pretty happy with this now. Still a couple of CI failures but they look random to me

Pretty happy with this now. Still a couple of CI failures but they look random to me
elegaanz hat vor 5 Jahren kommentiert (Migriert von github.com)

Yes the CI fails sometimes because it doesn't have enough memory to build…

Yes the CI fails sometimes because it doesn't have enough memory to build…
elegaanz (Migriert von github.com) hat die Änderungen vor 5 Jahren genehmigt
elegaanz (Migriert von github.com) hat einen Kommentar hinterlassen

Everything seems to work fine. Thank you! 😊

Everything seems to work fine. Thank you! :blush:

Reviewer

Der Pull Request wurde als 4b205fa995 gemergt.

Schritt 1:

Wechsle auf einen neuen Branch in deinem lokalen Repository und teste die Änderungen.
git checkout -b feature/persist_password_reset master
git pull origin feature/persist_password_reset

Schritt 2:

Führe die Änderungen zusammen und aktualisiere den Stand online auf Forgejo.
git checkout master
git merge --no-ff feature/persist_password_reset
git push origin master
Anmelden, um an der Diskussion teilzunehmen.
Keine Reviewer
Kein Meilenstein
Niemand zuständig
1 Beteiligte
Nachrichten
Fällig am
Das Fälligkeitsdatum ist ungültig oder außerhalb des zulässigen Bereichs. Bitte verwende das Format „jjjj-mm-tt“.

Kein Fälligkeitsdatum gesetzt.

Abhängigkeiten

Keine Abhängigkeiten gesetzt.

Referenz: Plume/Plume#610
Laden…
Hier gibt es bis jetzt noch keinen Inhalt.