-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 protected non-anonymous shares #31005
Comments
ping @jospoortvliet and @usselite ;-) |
…age for shares of type TYPE_EMAIL, when the "video verification" checkbox isn't checked. This to support sending temporary passwords to recipients for non-anonymous shares (TYPE_EMAIL shares). See #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…troller.php Users accessing non-anonymous public shares can now request a temporary password themselves. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…rd_expiration_time' attribute to the oc_shares table. This is needed to support the temporary password feature. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…ent and whose 'video verification' checkbox isn't checked. This supports the 'temporary password for email shares' feature, and this commit relates to #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…cipient and set a password expiration time when the password has requested explicitly (either via a Talk session or by the user having followed the temporary password self- provisioning process). This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…ds respecting the password policy (this commit is part of #31005) Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
… of #31005) Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…ystem value. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
This also deletes the ResetExpiredPasswordsJob.php as it is not needed anymore. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…ystem value. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
This also deletes the ResetExpiredPasswordsJob.php as it is not needed anymore. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…in the email input field and add a 'Back' button. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
… public authenticated share. I initially thought it was necessary to prevent users from setting an arbitrary password in such case (except when the password is given via Talk). But, eventually, I've figured out it was not necessary to drop support for that scenario. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…220208195521' is unknown." (by running build/autoloadchecker.sh) This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
… via the "Create a new Share" API this commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…age for shares of type TYPE_EMAIL, when the "video verification" checkbox isn't checked. This to support sending temporary passwords to recipients for non-anonymous shares (TYPE_EMAIL shares). See #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…troller.php Users accessing non-anonymous public shares can now request a temporary password themselves. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…rd_expiration_time' attribute to the oc_shares table. This is needed to support the temporary password feature. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…ent and whose 'video verification' checkbox isn't checked. This supports the 'temporary password for email shares' feature, and this commit relates to #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…ds respecting the password policy (this commit is part of #31005) Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
… of #31005) Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…ystem value. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
This also deletes the ResetExpiredPasswordsJob.php as it is not needed anymore. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…in the email input field and add a 'Back' button. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
… public authenticated share. I initially thought it was necessary to prevent users from setting an arbitrary password in such case (except when the password is given via Talk). But, eventually, I've figured out it was not necessary to drop support for that scenario. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…220208195521' is unknown." (by running build/autoloadchecker.sh) This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
… via the "Create a new Share" API this commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…age for shares of type TYPE_EMAIL, when the "video verification" checkbox isn't checked. This to support sending temporary passwords to recipients for non-anonymous shares (TYPE_EMAIL shares). See #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Implements the "Request password" functionality in AuthPublicShareController.php Users accessing non-anonymous public shares can now request a temporary password themselves. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Creates a migration step for the files_sharing app to add the 'password_expiration_time' attribute to the oc_shares table. This is needed to support the temporary password feature. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Hides the password input field for shares shared with an email recipient and whose 'video verification' checkbox isn't checked. This supports the 'temporary password for email shares' feature, and this commit relates to #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Fixes a typo in a comment in apps/files_sharing/src/components/SharingEntryLink.vue Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> When updating a share of type EMAIL, only send the password to the recipient and set a password expiration time when the password has requested explicitly (either via a Talk session or by the user having followed the temporary password self- provisioning process). This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Creates a new background job to reset expired share temporary passwords. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Respect password policies (if any) when generating a temporary password. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Moves the newly created validateIdentity function away from the IManager interface and rather implement it in ShareController, so as to avoid the need for creating a new version of the IManager interface. Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Makes share temporary passwords' expiration time configurable via a system value. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Makes sure password hasn't expired when verifying a share's password. This also deletes the ResetExpiredPasswordsJob.php as it is not needed anymore. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Refactors the publicshareauth template to have the Enter key working in the email input field and add a 'Back' button. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Fixes "Migration step 'OCA\Files_Sharing\Migration\Version24000Date20220208195521' is unknown." (by running build/autoloadchecker.sh) This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Adds an option to the "Create a new Share" OCS Share API to allow not sending the share password by email. Rationale: With the introduction of temporary share passwords, it's not so much usefull to send the password to the guest at share creation since she may request it later anyway. Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Adds a system config value to allow permanent share passwords This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> fixes DefaultShareProvider class is not compatible with IShareProvider Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Fixes FederatedShareProvider class is not compatible with IShareProvider Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Reverts the addition of a 'sendPassword' parameter to the ShareAPIController::create() method. The behaviour is now: If the password is a temporary one, do not send it to the guest, else send it. Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Updates ShareByMailProviderTest tests to take into account the fact that temporary passwords shall not be sent by email anymore upon share creation. Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…age for shares of type TYPE_EMAIL, when the "video verification" checkbox isn't checked. This to support sending temporary passwords to recipients for non-anonymous shares (TYPE_EMAIL shares). See #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Implements the "Request password" functionality in AuthPublicShareController.php Users accessing non-anonymous public shares can now request a temporary password themselves. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Creates a migration step for the files_sharing app to add the 'password_expiration_time' attribute to the oc_shares table. This is needed to support the temporary password feature. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Hides the password input field for shares shared with an email recipient and whose 'video verification' checkbox isn't checked. This supports the 'temporary password for email shares' feature, and this commit relates to #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Fixes a typo in a comment in apps/files_sharing/src/components/SharingEntryLink.vue Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> When updating a share of type EMAIL, only send the password to the recipient and set a password expiration time when the password has requested explicitly (either via a Talk session or by the user having followed the temporary password self- provisioning process). This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Creates a new background job to reset expired share temporary passwords. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Respect password policies (if any) when generating a temporary password. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Moves the newly created validateIdentity function away from the IManager interface and rather implement it in ShareController, so as to avoid the need for creating a new version of the IManager interface. Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Makes share temporary passwords' expiration time configurable via a system value. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Makes sure password hasn't expired when verifying a share's password. This also deletes the ResetExpiredPasswordsJob.php as it is not needed anymore. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Refactors the publicshareauth template to have the Enter key working in the email input field and add a 'Back' button. This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Fixes "Migration step 'OCA\Files_Sharing\Migration\Version24000Date20220208195521' is unknown." (by running build/autoloadchecker.sh) This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Adds an option to the "Create a new Share" OCS Share API to allow not sending the share password by email. Rationale: With the introduction of temporary share passwords, it's not so much usefull to send the password to the guest at share creation since she may request it later anyway. Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Adds a system config value to allow permanent share passwords This commit is part of #31005 Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> fixes DefaultShareProvider class is not compatible with IShareProvider Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Fixes FederatedShareProvider class is not compatible with IShareProvider Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Reverts the addition of a 'sendPassword' parameter to the ShareAPIController::create() method. The behaviour is now: If the password is a temporary one, do not send it to the guest, else send it. Signed-off-by: Cyrille Bollu <cyrpub@bollu.be> Updates ShareByMailProviderTest tests to take into account the fact that temporary passwords shall not be sent by email anymore upon share creation. Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
…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>
@pmarini-nc the feature was merged and shipped as part of 24.0.0 and it worked fine last I tested, you seem to have found unrelated bugs
this is expected, you can set an initial password if you want, if you keep what's there it's already a generated password
is this in the email ? or tooltip ?
if email sending working with your setup at all ? please check the logs
this is public link functionality, a different unrelated bug that likely exists also for non-email shares. |
Thanks @PVince81 for the quick feedback.
I was comparing with https://user-images.githubusercontent.com/8179031/152499873-0cc0ac0b-30c5-4981-92d4-fe0ac636f11a.png but maybe is not relevant anymore (just a design tweak the missing "or:")?
The email sending works as I receive the email with the link and the first email with the password, however it works fine with c.nc.com so I guess something is wrong with my settings. With loglevel=2, I don't see any related entry but I see some cron related errors
In this context, I see it as a security issue. Again, I'm not able to replicate this behaviour with c.nc.com. Maybe some sharing security option? In the instance where I notice the issue, the sharing security options are the default ones. |
Regarding the session cookie, we had a (unfinished) discussion on this topic in the PR => #31220 (comment) |
@StCyr I tried this again and could not reproduce the issue. Now when I open the link it goes directly to the files page and doesn't show the form, so the behavior is ok. Discussion about session expiration should be separate, I believe that for public links we always kept the session even when the browser is closed. So the issue is not related to this addition. |
I've tested #31981 and it all worked fine, so I believe perhaps @pmarini-nc had some local setup issues. I'm closing this ticket since the implementation was done and merged already for 24.0.0 as part of #31220 |
@PVince81 I think there is a problem with the implementation. I've just tested it and here is how it went :
In my opinion, there is two wrong behaviors :
What do you think ? |
Agree. I remember though that I've tried to implement this behaviour and that it was more difficult than expected.... Maybe I should give it a second look....
This is an interesting feature but it is not how it has been implemented. Please create a new feature request. |
I will create a new feature request then, thanks! |
It seems that removing the check from the checkbox "send password by mail" in the settings for sharing, does not send the e-mail even if the user requests it. If you check that, then the user gets the password right away as you described - which doesn't make sense with this feature or does it? Maybe I am missing something @StCyr? There are people trying to use this feature expecting that by removing the check from the checkbox "send password by mail", the server will only send the password once the user requests it. |
How to use GitHub
Feature request
Is your feature request related to a problem? Please describe.
Currently, protected non-anonymous shares (ie: shares shared via email) are protected through the use of permanent passwords: Once a password is set to protect a public share it stays valid forever. This way of working makes these shares sensible to confidentiality issues as there’s plenty of time for the password to be leaked and the share accessed by unintended parties.
Though Nextcloud has some controls to limit these possible confidentiality issues, like setting an expiration date for a share or explicitly changing a share’s password, these controls rely on the users making an explicit security-focused choice which is rarely the case.
Describe the solution you'd like
To improve this situation, I propose to drop permanent password support for protected non-anonymous shares: Protecting a non-anonymous share would not create a permanent password anymore. Instead, guests would request a temporary password each time they want to access the share.
Additional context
Here are some mockups of the proposal.
Changes to the file details right-side panel 'sharing tab
Changes to the protected non-anonymous share access screen
The protected non-anonymous share access screen would always show the “request password” button:
The following flowchart shows the new "request password" process:
Technical notes
The "request password" button is implemented via the Talk application. Hence, the changes here in server shall be coordinated with the corresponding changes in spreed.
The text was updated successfully, but these errors were encountered: