Password reset #448

trinity-1686a · 2019-02-24T19:18:14Z

elegaanz

2019-02-24 19:18:14 +00:00

yorum yaptı (github.com konumundan göç edildi)

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]

2019-02-24 19:50:49 +00:00

yorum yaptı (github.com konumundan göç edildi)

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]

2019-02-24 19:52:56 +00:00

yorum yaptı (github.com konumundan göç edildi)

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 2019-02-25 21:14:38 +00:00 incelendi

trinity-1686a bir yorum yaptı

Sahibi

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

2019-02-25 20:46:38 +00:00

yorum yaptı

Sahibi

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

2019-02-25 20:50:20 +00:00

yorum yaptı

Sahibi

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

2019-02-25 20:57:32 +00:00

yorum yaptı

Sahibi

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

2019-02-25 21:04:11 +00:00

yorum yaptı

Sahibi

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

trinity-1686a

2019-02-25 21:06:50 +00:00

yorum yaptı

Sahibi

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

2019-02-25 21:09:25 +00:00

yorum yaptı

Sahibi

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 2019-02-26 10:39:21 +00:00 incelendi

trinity-1686a bir yorum yaptı

Sahibi

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

2019-02-26 09:44:37 +00:00

yorum yaptı

Sahibi

that's 24 minutes, not 24 hours

trinity-1686a

2019-02-26 09:49:47 +00:00

yorum yaptı

Sahibi

that's 2 minutes, not 2 hours

trinity-1686a

2019-02-26 10:11:26 +00:00

yorum yaptı

Sahibi

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

trinity-1686a

2019-02-26 10:15:51 +00:00

yorum yaptı

Sahibi

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

trinity-1686a

2019-02-26 10:19:41 +00:00

yorum yaptı

Sahibi

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

2019-02-26 10:33:16 +00:00

yorum yaptı

Sahibi

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 (github.com konumundan göç edildi) 2019-02-26 12:09:50 +00:00 incelendi

src/routes/session.rs

elegaanz (github.com konumundan göç edildi)

2019-02-26 12:09:49 +00:00

yorum yaptı

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

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

elegaanz (github.com konumundan göç edildi) 2019-02-26 12:11:13 +00:00 incelendi

src/routes/session.rs

elegaanz (github.com konumundan göç edildi)

2019-02-26 12:11:13 +00:00

yorum yaptı

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 2019-02-26 12:46:52 +00:00 incelendi

src/routes/session.rs

trinity-1686a

2019-02-26 12:46:51 +00:00

yorum yaptı

Sahibi

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