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

Temporary passwords for public non-anonymous protected shares (ie: files shared with an email recipient). #31220

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

StCyr
Copy link
Contributor

@StCyr StCyr commented Feb 16, 2022

This PR implements #31005

With this change, passwords protecting public non-anonymous shares become temporary: They have an expiration time and a background job changes them to some random undisclosed value when their expiration time is passed.

To request a new temporary password, recipients use a new "request password" button that is added to protected public non-anonymous shares' authentication page. This button serves the same functionality as the "request password" button implemented by Talk (ie: To identify the person requesting the password), except that, here, the person identifies herself by proving she knows the email address with which the file is shared with (ie: she types her email address, and if it matches the email address with which the file is shared the person is considered "identified" and a new temporary password is sent to that email address). Of course, identification via a Talk session takes precedence when the share has its "video verification" attribute checked.

I'm waiting for your reviews and welcome your comments which I hope will be constructive

Cyrille

===============================
UPDATE 20220217:

  1. The existing protected public non-anonymous shares are not impacted by this change (they don't have a password_expiration_time, so the expiration background job doesn't consider them)
  2. The temporary passwords are valid for 1 day.

===============================
UPDATE 20220223:
I've created a new branch: https://github.com/nextcloud/server/tree/enhancement/31005/temporary-passwords-v2

Compared to this branch, this new branch brings the following:

  1. password_expiration_time is now checked during password check. This also makes the expiration background job not necessary anymore;
  2. Temporary passwords validity period is now configurable via a system value

@PVince81
Copy link
Member

PVince81 commented Mar 1, 2022

did you forget to push something ?
image

  • please run build/autoloaderchecker.sh

Fixed in7fffe20e0c324c158d60b51799ec5f0f1a8f6230

use \OCP\Security\IHasher;
use \OCP\Security\ISecureRandom;

class ResetExpiredPasswordsJob extends TimedJob {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can ignore these warnings, we'll need to fix the base class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I've a general remark about the necessity of this background job:

I've created another branch (https://github.com/nextcloud/server/commits/enhancement/31005/temporary-passwords-v2), building on this one and adding 2 commits, where password's expiration time is checked in IManager::checkPassword (see 576de4b) hence making this background job unnecessary anymore.

I believe it's a better solution than this background job. Could you please have a look and take a stand?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StCyr yes, let's use your approach without the background job which checks on-demand

Copy link
Contributor Author

@StCyr StCyr Mar 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on-demand check implemented (via 71ae5a0 , 9ef1c59, and 63d478b), together with configurable password expiration time

$qb = $this->connection->getQueryBuilder();

// QUESTION: DOES THE DATETIME COMPARAISON WORK WELL WHEN TIMEZONES ENTER THE GAME?
// I THINK SO, BECAUSE EVERYTHING HAPPENS ON THE SERVER, HENCE ON THE SAME TZ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually the timezone of server is supposed to be UTC, so should be fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove the comment from the code when ready 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the background job idea is discarded => #31220 (comment)

@PVince81
Copy link
Member

PVince81 commented Mar 1, 2022

  • BUG: enter key not working when entering email address to request from

Fixed in 592a1ad.

I'm not very fond of this fix -I find it a bit ugly- but it works; I've tried to create a new route to implement this 2-steps authentication (/s/{token}/authenticate/{redirect} for the password prompt and /s/{token}/identify/{redirect} for the identity/email prompt) but redirection made things difficult and I gave up.

@PVince81
Copy link
Member

PVince81 commented Mar 1, 2022

  • BUG: need to clear the (public) session info as soon as opening the "request password" page

Steps:

  1. Share a folder with user "another@example.com"
  2. Set a password "test"
  3. Open the email
  4. Open the link
  5. Enter the password "test" => this authenticates you for the link
  6. Open the link again
  7. Click "request access"
  8. Enter email address

Expected result: receive email with new password
Actual result: no email and the share folder appears directly, because of the previous session data

========================

I'm sorry I can't reproduce the bug: If I follow the steps precisely, I'm getting automatically authenticated at step 7, rather than being presented with the 'authenticate' page and the "request password" button. So I can't proceed with step 8 and get your actual result.

This looks like a normal behaviour to me: After being authenticated for the link (at step 5), I receive a session token which remains valid for some time. So, it's normal that I'm getting automatically authenticated the second time I open the link, isn't?

===

Vincent: also cannot reproduce any more in 24.0.0, probably it was fixed through a recent change

emailPrompt.style.display="block";

// Hides password prompt
var passwordRequestButton = document.getElementById('request-password-button-not-talk');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/-not-talk/-by-email/ ? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want. But, I believe it's best to keep it technology-agnostic for if later we implement other means to request passwords (eg: by sms)

@nickvergessen nickvergessen removed their request for review March 1, 2022 14:25
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some comments and found some issues, please have a look

* @return IShare The share object
* @throws \InvalidArgumentException
* @since 9.0.0
*/
public function updateShare(IShare $share);
public function updateShare(IShare $share, bool $sendPassword = false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will likely break implementations and might need a new version of the interface

wondering if there's another way by having or deriving this info from the IShare object directly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before adding a new interface let's check if there are really more implementations of IManager or just one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be ok, see #31447

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm.... Anyway, I'm wondering if this new feature (ie: not sending a temporary password upon public authenticate share creation) is really needed... Eventually, it's not that much of an issue if a temporary password is sent to the recipient upon share creation, rather than waiting for the recipient to explicitely requesting it.

I'm thinking about reverting this change. What's your opinion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are scenarios where one might want to manually set a specific password to make it easier for the other person to type (or it's something they gave them already on paper or another medium), so we should allow for still supporting such scenarios

now, I'm not sure if this applies to this piece of code specifically ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi Cyrille, you're right about misalignment, thanks for spotting

I'll get this clarified internally as this would be a change of behavior

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @StCyr, after a bit of thoughts, we decided we would need a config switch.
Old behaviour would be off by default. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @StCyr, after a bit of thoughts, we decided we would need a config switch. Old behaviour would be off by default. :)

A new checkbox in the share menu?

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather a config switch in config.php to switch it globally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implemented in c9940b1

I believe the PR is feature complete now. Do you want me to rebase and squash it?

lib/public/Share/IManager.php Outdated Show resolved Hide resolved
* @param string $identityToken
* @return bool
*/
protected function validateIdentity(string $identityToken): bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove the type hint to allow null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean the @return bool?

@skjnldsv skjnldsv removed the 3. to review Waiting for reviews label Mar 2, 2022
@PVince81 PVince81 force-pushed the enhancement/31005/temporary-passwords branch from 1ae14b4 to 70e790c Compare April 7, 2022 18:25
@PVince81
Copy link
Member

PVince81 commented Apr 7, 2022

ran php:cs, rebuilt assets, merged suggestions from @CarlSchwan

now hoping for green CI

@PVince81
Copy link
Member

PVince81 commented Apr 7, 2022

not that easy

"PHP Fatal error:  Declaration of OC\Share20\Share::setPasswordExpirationTime($passwordExpirationTime = null) must be compatible with OCP\Share\IShare::setPasswordExpirationTime($passwordExpirationTime = null): OCP\Share\IShare in /home/runner/work/server/server/lib/private/Share20/Share.php on line 469"

I'll have a look later

lib/public/Share/IShare.php Show resolved Hide resolved
lib/public/Share/IShare.php Show resolved Hide resolved
lib/public/AppFramework/AuthPublicShareController.php Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan force-pushed the enhancement/31005/temporary-passwords branch 5 times, most recently from fd1b563 to 94c8c50 Compare April 8, 2022 11:17
@StCyr StCyr force-pushed the enhancement/31005/temporary-passwords branch 3 times, most recently from 71391aa to 2aa9885 Compare April 9, 2022 19:10
@StCyr
Copy link
Contributor Author

StCyr commented Apr 9, 2022

Hi @PVince81 @CarlSchwan,

I've fixed the unit tests (and added a new one to validate that password is not sent via email when temporary passwords are enabled).

Also, the test testCreateSendPasswordByTalkWithEnforcedPasswordProtection was failing because of an unexpected functional change imho. So, I've updated the function ShareByMailProvider::create() to allow sending password to the share owner in that case.

@PVince81
Copy link
Member

@StCyr thanks a lot!

…age for shares

of type TYPE_EMAIL, when the "video verification" checkbox isn't checked. Users accessing
non-anonymous public shares (TYPE_EMAIL shares) can now request a temporary password themselves.

- Creates a migration step for the files_sharing app to add the 'password_expiration_time'
  attribute to the oc_shares table.
- Makes share temporary passwords' expiration time configurable via a system value.
- Adds a system config value to allow permanent share passwords

-Fixes a typo in a comment in apps/files_sharing/src/components/SharingEntryLink.vue

See #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
@StCyr StCyr force-pushed the enhancement/31005/temporary-passwords branch from f4abed9 to c6a5c07 Compare April 11, 2022 20:05
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@PVince81
Copy link
Member

failing test unrelated and has just been fixed on master

merging

@PVince81 PVince81 merged commit 483741f into master Apr 12, 2022
@welcome
Copy link

welcome bot commented Apr 12, 2022

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@PVince81 PVince81 deleted the enhancement/31005/temporary-passwords branch April 12, 2022 07:46
@PVince81
Copy link
Member

seems a bug has sneaked in, the expiration date column never changes for me on master, raised here: #31951

@PVince81
Copy link
Member

and some UX feedback: #31952

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants