Plume
/
Plume
8
16
Bifurcation 22
Code Tickets 174 Demandes d'ajout 8 Versions 8 Wiki Activité

Étiquettes Jalons

Password reset #448

Fusionnée
elegaanz a fusionné 10 révision(s) à partir de password-reset vers master il y a 5 ans
Discussion 5 Révisions 10 Fichiers Modifiés 21
elegaanz a commenté il y a 5 ans (Migré de github.com)

There is now a link on the login page if you forgot your password. It redirects you to a page asking your email address. An email containing a password reset link will be sent to this address if someone registered with this address. The link is only valid for 15 minutes.

I used the lettre crate for mail finally since I didn't find a way to set a subject with mailstrom (which is very weird). It also allowed me to implement a debug mail sender, that can be enabled with --feature debug-mailer and that will log mails instead of actually sending them.

I should also document the new configuration options for the mail server. Done (but I can still modify it again if you don't like the current approach)

Fixes #298

There is now a link on the login page if you forgot your password. It redirects you to a page asking your email address. An email containing a password reset link will be sent to this address if someone registered with this address. The link is only valid for 15 minutes. I used the `lettre` crate for mail finally since I didn't find a way to set a subject with `mailstrom` (which is very weird). It also allowed me to implement a debug mail sender, that can be enabled with `--feature debug-mailer` and that will log mails instead of actually sending them. ~~I should also document the new configuration options for the mail server.~~ Done (but I can still modify it again if you don't like the current approach) Fixes #298
codecov[bot] a commenté il y a 5 ans (Migré de github.com)

Codecov Report

Merging #448 into master will decrease coverage by 0.49%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master     #448     +/-   ##
=========================================
- Coverage   25.45%   24.96%   -0.5%     
=========================================
  Files          63       64      +1     
  Lines        6285     6409    +124     
=========================================
  Hits         1600     1600             
- Misses       4685     4809    +124
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/448?src=pr&el=h1) Report > Merging [#448](https://codecov.io/gh/Plume-org/Plume/pull/448?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/7bac70a4830eb660506b9cd9603fe013225a74a0?src=pr&el=desc) will **decrease** coverage by `0.49%`. > The diff coverage is `0%`. ```diff @@ Coverage Diff @@ ## master #448 +/- ## ========================================= - Coverage 25.45% 24.96% -0.5% ========================================= Files 63 64 +1 Lines 6285 6409 +124 ========================================= Hits 1600 1600 - Misses 4685 4809 +124 ```
codecov[bot] a commenté il y a 5 ans (Migré de github.com)

Codecov Report

Merging #448 into master will increase coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #448      +/-   ##
==========================================
+ Coverage   25.45%   25.49%   +0.03%     
==========================================
  Files          63       64       +1     
  Lines        6285     6480     +195     
==========================================
+ Hits         1600     1652      +52     
- Misses       4685     4828     +143
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/448?src=pr&el=h1) Report > Merging [#448](https://codecov.io/gh/Plume-org/Plume/pull/448?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/7bac70a4830eb660506b9cd9603fe013225a74a0?src=pr&el=desc) will **increase** coverage by `0.03%`. > The diff coverage is `0%`. ```diff @@ Coverage Diff @@ ## master #448 +/- ## ========================================== + Coverage 25.45% 25.49% +0.03% ========================================== Files 63 64 +1 Lines 6285 6480 +195 ========================================== + Hits 1600 1652 +52 - Misses 4685 4828 +143 ```
trinity-1686a révisé il y a 5 ans
trinity-1686a laisser un commentaire
Propriétaire

I haven't tested it yet, just made some observations on the code. Most comments are code-style, so this is mostly good

I haven't tested it yet, just made some observations on the code. Most comments are code-style, so this is mostly good
src/mail.rs
trinity-1686a a commenté il y a 5 ans
Propriétaire

you should reorder imports so all feature=debug and all not(feature=debug) are segregated, it'll make thinks easier to read and maintain
The ideal may be to have 2 private mods, one debug and one release (or whatever), and re-export only the one enabled

you should reorder imports so all `feature=debug` and all `not(feature=debug)` are segregated, it'll make thinks easier to read and maintain The ideal may be to have 2 private mods, one `debug` and one `release` (or whatever), and re-export only the one enabled
trinity-1686a a commenté il y a 5 ans
Propriétaire

Maybe this would be more at it's place in plume-model than in plume itself?

Maybe this would be more at it's place in plume-model than in plume itself?
src/routes/session.rs
@ -101,3 +105,140 @@ pub fn delete(mut cookies: Cookies) -> Redirect {
}
trinity-1686a a commenté il y a 5 ans
Propriétaire

I think this statement could be made into a single function in src/mails.rs (or wherever it move), which take in parameter subject and content, so it can be easily reused later if we send other kinds of mails (moderation warnings or whatever when it will get implemented)

I think this statement could be made into a single function in `src/mails.rs` (or wherever it move), which take in parameter subject and content, so it can be easily reused later if we send other kinds of mails (moderation warnings or whatever when it will get implemented)
trinity-1686a a commenté il y a 5 ans
Propriétaire

15 minutes seems shorts, I'd advise something like 1 or 2 hours instead, mails can be long to transmit

15 minutes seems shorts, I'd advise something like 1 or 2 hours instead, mails can be long to transmit
trinity-1686a a commenté il y a 5 ans
Propriétaire

to prevent the State<...> from growing up in memory (basically be a memory leak), you should filter and eliminate old entries either with the internal scheduler, or somewhere like here

to prevent the `State<...>` from growing up in memory (basically be a memory leak), you should filter and eliminate old entries either with the internal scheduler, or somewhere like here
trinity-1686a a commenté il y a 5 ans
Propriétaire

you should check if there haven't already be a link sent recently, otherwise one might use it to spam a user by sending them many reset links

you should check if there haven't already be a link sent recently, otherwise one might use it to spam a user by sending them many reset links
trinity-1686a révisé il y a 5 ans
trinity-1686a laisser un commentaire
Propriétaire

Apparently letter won't compile on Archlinux, it requires an old version of openssl that Arch don't provide. I'll spin up a debian vm this afternoon to actually test the code, and try to fix this with upstream

Apparently letter won't compile on Archlinux, it requires an old version of openssl that Arch don't provide. I'll spin up a debian vm this afternoon to actually test the code, and try to fix this with upstream
src/routes/session.rs
trinity-1686a a commenté il y a 5 ans
Propriétaire

that's 24 minutes, not 24 hours

that's 24 minutes, not 24 hours
trinity-1686a a commenté il y a 5 ans
Propriétaire

that's 2 minutes, not 2 hours

that's 2 minutes, not 2 hours
trinity-1686a a commenté il y a 5 ans
Propriétaire

You should maintain the lock instead of locking and unlocking many times

You should maintain the lock instead of locking and unlocking many times
trinity-1686a a commenté il y a 5 ans
Propriétaire

I think using ! .any(|x|...) would be easier to read

I think using ! .any(|x|...) would be easier to read
trinity-1686a a commenté il y a 5 ans
Propriétaire

you should remove before checking if there is already an entry, otherwise one asking for a second reset link after, says, 12 hours, won't get it's mail. Also this mean the vec is cleaned only when a valid request is made, so if nobody else request a password reset, one wont ever get unlocked

you should remove before checking if there is already an entry, otherwise one asking for a second reset link after, says, 12 hours, won't get it's mail. Also this mean the vec is cleaned only when a valid request is made, so if nobody else request a password reset, one wont ever get unlocked
trinity-1686a a commenté il y a 5 ans
Propriétaire

Instead of building a Vec, then "substracting" if from the one stored, building directly the right one and mem::replace-ing it seems easier to read, and don't require to enable another feature
snippet :

let a = Mutex::new(vec![1,2,3]);
std::mem::replace(&mut *a.lock().unwrap(), vec![4,5,6]);
Instead of building a Vec, then "substracting" if from the one stored, building directly the right one and mem::replace-ing it seems easier to read, and don't require to enable another feature snippet : ```rust let a = Mutex::new(vec![1,2,3]); std::mem::replace(&mut *a.lock().unwrap(), vec![4,5,6]); ```
elegaanz (Migré de github.com) révisé il y a 5 ans
src/routes/session.rs
elegaanz (Migré de github.com) a commenté il y a 5 ans

Oops looks like I don't know how time is working 😬

Oops looks like I don't know how time is working :grimacing:
elegaanz (Migré de github.com) révisé il y a 5 ans
src/routes/session.rs
elegaanz (Migré de github.com) a commenté il y a 5 ans

I can't because I need to borrow it both mutably and immutably and Rust won't let me do that. But maybe there is a solution to do it I didn't find?

I can't because I need to borrow it both mutably and immutably and Rust won't let me do that. But maybe there is a solution to do it I didn't find?
trinity-1686a révisé il y a 5 ans
src/routes/session.rs
trinity-1686a a commenté il y a 5 ans
Propriétaire

I've removed a lot of code, but here is a poc of how I would do it (and some other things, trashing old things sooner and don't use the feature) https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=2c32af184c99ff62ee99f94aa2e4fa13
Sadly I can't compile this branch so I can't give you a clean commit, but it should put you on the right track

I've removed a lot of code, but here is a poc of how I would do it (and some other things, trashing old things sooner and don't use the feature) https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=2c32af184c99ff62ee99f94aa2e4fa13 Sadly I can't compile this branch so I can't give you a clean commit, but it should put you on the right track
trinity-1686a révisé il y a 5 ans
src/routes/session.rs
trinity-1686a a commenté il y a 5 ans
Propriétaire

(I found a better way to do it, see the link to the playground)

(I found a better way to do it, see the link to the playground)
trinity-1686a révisé il y a 5 ans
src/routes/session.rs
trinity-1686a a commenté il y a 5 ans
Propriétaire

and again, that's 2.4 hours

and again, that's 2.4 hours
igalic (Migré de github.com) révisé il y a 5 ans
src/routes/session.rs
igalic (Migré de github.com) a commenté il y a 5 ans

why is math so hard? why are there no as_minutes() and as_hours() functions?!??

why is math so hard? why are there no `as_minutes()` and `as_hours()` functions?!??
trinity-1686a révisé il y a 5 ans
trinity-1686a laisser un commentaire
Propriétaire

👍

:+1:
trinity-1686a a commenté il y a 5 ans
Propriétaire

It doesn't seems to compile with debug-mailer on

It doesn't seems to compile with debug-mailer on
trinity-1686a ces changements ont été approuvés il y a 5 ans
trinity-1686a laisser un commentaire
Propriétaire

Something should also be added in the documentation regarding new environment variables

Something should also be added in the documentation regarding new environment variables
elegaanz a commenté il y a 5 ans (Migré de github.com)

Already done 😁, see https://docs.joinplu.me/environment/ and https://docs.joinplu.me/installation/config/

Already done :grin:, see https://docs.joinplu.me/environment/ and https://docs.joinplu.me/installation/config/
👍 1
trinity-1686a a commenté il y a 5 ans
Propriétaire

Mb, I did not check it before. I think however Docker documentation should be updated, as it says only 2 vars need changes, whereas MAIL_* do too

Mb, I did not check it before. I think however Docker documentation should be updated, as it says only 2 vars need changes, whereas MAIL_* do too
👍 1

Relecteurs

trinity-1686a ces changements ont été approuvés il y a 5 ans
La demande d'ajout a été fusionnée en a2b9d7ec44.
Vous pouvez également voir les instructions en ligne de commande.

Étape 1:

Depuis le dépôt de votre projet, sélectionnez une nouvelle branche et testez les modifications.
git checkout -b password-reset master
git pull origin password-reset

Étape 2:

Fusionner les modifications et mettre à jour sur Forgejo.
git checkout master
git merge --no-ff password-reset
git push origin master
Connectez-vous pour rejoindre cette conversation.
Relecteurs
Demander une revue
Aucune évaluation
trinity-1686a
Étiquettes
Appliquer une étiquette
Effacer les étiquettes   
A: API

Related to the REST API   
A: Backend

Code running on the server   
A: Federation

Stuff related to Federation   
A: Front-End

Related to the front-end   
A: I18N

Translations, and related code   
A: Meta

More about project management or code than the project itself   
A: Security
   
Build

The building, or installation process of Plume   
C: Bug

Something isn't working   
C: Discussion

We need to talk   
C: Enhancement

New feature or request   
C: Feature

This is a new feature   
Compatibility

Compatibility with different browsers, readers and OS   
Dependency

Related to an external package that Plume uses   
Design

UI/UX related issues and PRs   
Documentation
   
Good first issue

Good for newcomers   
Help welcome

Extra attention is needed   
Mobile

Issues affecting only mobile UX   
Rendering

How elements're rendered out for the end user   
S: Blocked

Something else needs to be fixed first   
S: Duplicate

This issue or pull request already exists   
S: Incomplete

This PR is not complete yet   
S: Instance specific

Issues concern a limited number of instances   
S: Invalid

This doesn't seem right   
S: Needs Voting/Discussion

Need to be discussed by the community (on Loomio)   
S: Ready for review

This PR is ready to be reviewed   
Suggestion

Proposed ideas worth considering   
S: Voted on Loomio

This is issue has been created after a vote on Loomio   
S: Wontfix

This will not be worked on
Pas d'étiquette
A: API
A: Backend
A: Federation
A: Front-End
A: I18N
A: Meta
A: Security
Build
C: Bug
C: Discussion
C: Enhancement
C: Feature
Compatibility
Dependency
Design
Documentation
Good first issue
Help welcome
Mobile
Rendering
S: Blocked
S: Duplicate
S: Incomplete
S: Instance specific
S: Invalid
S: Needs Voting/Discussion
S: Ready for review
Suggestion
S: Voted on Loomio
S: Wontfix
Jalon
Affecter un jalon
Effacer le jalon
Pas d'élément
Aucun jalon
Affecté à
Assigner des utilisateurs
Supprimer les affectations
Pas d'assignataires
2 participants
Notifications
Échéance
La date d’échéance est invalide ou hors plage. Veuillez utiliser le format 'aaaa-mm-dd'.

Aucune échéance n'a été définie.

Dépendances

No dependencies set.

Reference: Plume/Plume#448
Écrire Aperçu
Chargement…
Annuler
Enregistrer
Il n'existe pas encore de contenu.
Supprimer la branche 'password-reset'

Supprimer une branche est permanent. Cela NE PEUVENT être annulées. Continuer ?

Non
Oui