Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn for password reset when files_encryption is enabled #11696

Merged
merged 11 commits into from
Nov 19, 2014

Conversation

LukasReschke
Copy link
Member

This patch will warn the user of the consequences when resetting the password and requires checking a checkbox (as we had in the past) to reset a password.

Furthermore I updated the code to use our new classes and added some unit tests for it 👯

Fixes #11438

@jancborchardt Requires some UI love.

\cc @gig13

@LukasReschke LukasReschke force-pushed the addWarningToEncryptionLostPassword branch from 8948871 to 5ff95ef Compare October 21, 2014 16:18
@jancborchardt
Copy link
Member

Code-wise it looks good and not too much of a UI change. Can you supply a screenshot?

One thing: core/lostpassword/templates/lostpassword.php wasn’t needed anymore after the lost-password-ajaxification, right? Cause it being removed in this context seems odd.

@LukasReschke
Copy link
Member Author

Can you supply a screenshot?

Of course, please note that the encryption warning is shown at the password reset step, not the login screen. This has the reason that the potential data destroying step is the password reset and not clicking the "reset password button":

When opening password reset:
screen shot 2014-10-22 at 10 30 32

When clicking reset:
screen shot 2014-10-22 at 10 30 37

After the user checked the proceed box and clicked the "reset password" button again the password is reset.

One thing: core/lostpassword/templates/lostpassword.php wasn’t needed anymore after the lost-password-ajaxification, right? Cause it being removed in this context seems odd.

Yep. Let's kill all the legacy 🔥 🔥 🔥

@LukasReschke
Copy link
Member Author

You can also take a look by enabling encryption and opening /index.php/lostpassword/reset/form/foo/foo - the password reset will obviously not work, but the message will be shown anyways.

@jancborchardt
Copy link
Member

Looks ok design-wise. Appropriate to the cause. ;)

@LukasReschke
Copy link
Member Author

@schiesbn @th3fallen Up for a review? - Testing this is simple, just enable the encryption app and try the password reset functionality. It should show a warning in the last final step when resetting the password.
Also regular password resets should still work etc… - The usual stuff ;-)

@etiess
Copy link

etiess commented Oct 23, 2014

Looks good. Isn't it possible to detect if the user does have a recovery key? And then display a message adapted to the user's situation?

@LukasReschke
Copy link
Member Author

Looks good. Isn't it possible to detect if the user does have a recovery key? And then display a message adapted to the user's situation?

Problem here is that I do not really want to leak to a potential adversary whether a recovery key is in place or not for specific users. Unless it really makes really easier to understand. - Furthermore, this would even add even more code from files_encryption into core.

@jancborchardt Your call.

@etiess
Copy link

etiess commented Oct 23, 2014

I understand your point concerning the potential adversary, but does it really make a difference? I mean, is it really easier for him to break the security if he knows that? I hope it's not ;)

@LukasReschke
Copy link
Member Author

I understand your point concerning the potential adversary, but does it really make a difference? I mean, is it really easier for him to break the security if he knows that? I hope it's not ;)

Well, it at least allows you to force somebody physically to give the recovery password (i.e. an administrator) :-)

But maybe that's just my hyper paranoid stance together with the reluctance to add more files_encryption code into core :-)

@jancborchardt
Copy link
Member

Let’s first improve this bit and then see separately if anything else needs to be done.

@schiessle
Copy link
Contributor

I just tried it, but I don't get the warning, instead I get directly redirected to:

The link to reset your password has been sent to your email. If you do not receive it within a reasonable amount of time, check your spam/junk folders.
If it is not there ask your local administrator.

@schiessle
Copy link
Contributor

BTW: If I look at the diff I don't see a string with the warning, maybe you forget to attach a file?

@LukasReschke
Copy link
Member Author

I just tried it, but I don't get the warning, instead I get directly redirected to:

Well, because it appears within the next step. Just click the link (or get the token out of the DB :))

BTW: If I look at the diff I don't see a string with the warning, maybe you forget to attach a file?

Nope. It was already there since ages and just unused code :-)

@LukasReschke
Copy link
Member Author

Actually the reason for having it in the other step was for me that the destructive action ("changing the password") is the one when changing it - not requesting it.

@schiessle
Copy link
Contributor

hm, If I try to open the link I get a empty page. No errors in the log files or the JS console.

@LukasReschke
Copy link
Member Author

That's strange. Can you paste the link here?

@schiessle
Copy link
Contributor

This is the link:

http://localhost/oc/index.php/lostpassword/set/azJtPaCcPBnn8yC3hsJw6/schiesbn

I verified the token, it is the same as stored in the database

@LukasReschke
Copy link
Member Author

@schiesbn Damn, that happens when one does not have a local mail system setup :-/

81ddc20 - sorry again...

@schiessle
Copy link
Contributor

OK, now it works. Just two things.

If I hover over the "I know what I'm doing" the font colour becomes really dark and it stays dark after the checkbox was clicked. This is really hard to read:

reset

After login with the new password the user now gets "Invalid private key for Encryption App. Please update your private key password in your personal settings to recover access to your encrypted files."

But there is no way to create a new key pair. Either we should have a option in the personal settings to create a new key pair or create it on-the-fly if the user reset the password. The code to create a new key-pair should already be there, this also happens when the admin set a new password. We probably just need a hook the encryption app can listen to and trigger the generation of a new key pair.

@schiessle
Copy link
Contributor

If you add a hook for the successful password recovery (the hooks needs to contain the new password as parameter), I can take care of the encryption app to listen to it and to perform the necessary steps.

@LukasReschke
Copy link
Member Author

If you add a hook for the successful password recovery, I can take care of the encryption app to listen to it.

Sure. That sounds like a good idea!

@LukasReschke
Copy link
Member Author

@schiesbn I added a hook as suggested with a9e4ea4.

However, I did not really use hooks much in the past. Would be great if you could let me know if this is sufficient! :-) - That said, feel free to commit any changes regarding your above pointed out issue directly to this branch.

@LukasReschke LukasReschke force-pushed the addWarningToEncryptionLostPassword branch 3 times, most recently from 43e1e43 to 092a5ef Compare October 29, 2014 11:32
@schiessle
Copy link
Contributor

Just to summarize the behaviour:

If a user agree to reset the password we will first make a backup of all encryption keys. Then we will create a new private/public key for the user, based on his new login password. After login he will no longer be able to read his old files. But can upload new files with his new key.
If the recovery key was enabled the admin can still recover all files. Afterwards the user will be able to read all files again, the old ones and the new ones.

@schiessle
Copy link
Contributor

@LukasReschke I discovered one problem. Creating a new key pair can take 1-2 seconds. So it could happen that the user click the "Reset password" button again which can lead to many nasty things, e.g. we could create two new key pairs in parallel.

Can you disable the button after the user clicked it the first time to avoid such race conditions? Thanks!

@LukasReschke
Copy link
Member Author

@schiesbn Done with 293b75c - great that you noticed that. Would have created nasty bugs and hard to debug bugs :-)

LukasReschke and others added 8 commits November 17, 2014 17:50
THX @schiesbn
(I should setup a mail server on my local system...)
Creating a new key pair can take 1-2 seconds. So it could happen that the user click the "Reset password" button again which can lead to many nasty things, e.g. we could create two new key pairs in parallel.
@LukasReschke LukasReschke force-pushed the addWarningToEncryptionLostPassword branch from 6707765 to 9eeea57 Compare November 17, 2014 16:50
@LukasReschke
Copy link
Member Author

I added a spinner with 9eeea57 - but it is not centered, anybody knows how to do that properly within our code base?

@LukasReschke
Copy link
Member Author

Summoning @MorrisJobke.

@scrutinizer-notifier
Copy link

A new inspection was created.

@MorrisJobke
Copy link
Contributor

@LukasReschke fixed with 345eb62

Still 👍

@ghost
Copy link

ghost commented Nov 18, 2014

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/2755/
🚀 Test PASSed. 🚀

@MorrisJobke
Copy link
Contributor

@schiesbn Wanna review?

@schiessle
Copy link
Contributor

👍

LukasReschke added a commit that referenced this pull request Nov 19, 2014
…sword

Warn for password reset when files_encryption is enabled
@LukasReschke LukasReschke merged commit 1c8f956 into master Nov 19, 2014
@LukasReschke LukasReschke deleted the addWarningToEncryptionLostPassword branch November 19, 2014 12:05
@hostingnuggets
Copy link

Where is this nice warning message when a user tries to recover his password and does not have the recovery password enabled you mention @schiesbn ? I am using 7.0.4 and just tried the reset password function with no such warning. This is a catastrophy in terms of UX as his account is all messed up after. How do you expect a user remembering his old password in order to reset his private key? This user just clicked "reset password" for a reason... because he forgot it...

@RandieM
Copy link

RandieM commented Jun 4, 2015

Hello everyone.

Having the warning message back is really great. However, there will still be MANY users that will just tick the box and proceed and will then complain to us (their admin) for not being able to access their old files. Password reset through an email link -at least the way it is is right now- is quite of a problem for us. It significantly increases the administration work on our side, as we have to reply to emails and phone calls and explain to technical or non-technical customers why they no longer have access to their files, no matter if we clearly stress out in our instructions and FAQ that password recovery disabled together with a weak memory can be a catastrophic combination.

Most importantly, we have several customers that forget their password/encryption key, reset their log-in password via the email link, realise that they no longer have access to their files and instead of contacting us, they decide to re-upload them. This means that they end up using double the storage space that they are paying us for (e.g. if they have a 10 GB storage package, they can end up using 20 GB; 10 GB for their newly uploaded unencrypted files and 10 GB for the inaccessible encrypted files).

For us, an "easy" solution to this would be to have the possibility to disable password reset via email link for customers with password recovery disabled and enable password reset via email link for those that have password recovery enabled.

Thanks.

@MorrisJobke
Copy link
Contributor

@RandieM Can you open this as a feature request with a new ticket in this bug tracker. Otherwise it will get lost. Thanks

@RandieM
Copy link

RandieM commented Jun 9, 2015

@MorrisJobke thanks for the advice. I opened this issue #16839. I was a bit unsure how to do what you suggested, so I hope it's OK like that.

MorrisJobke added a commit that referenced this pull request Jan 20, 2016
* lostpassword.css is unneeded since #11696 is merged - 1b50d4f
* js is already in core/js
* css is moved to core/css/lostpassword
* template is moved to core/templates/lostpassword
@lock lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web UI lostpassword.php is not calling warning message when encryption app is enabled
8 participants