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 protected non-anonymous shares #31005

Closed
StCyr opened this issue Feb 4, 2022 · 11 comments
Closed

Temporary passwords for protected non-anonymous shares #31005

StCyr opened this issue Feb 4, 2022 · 11 comments
Assignees
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap enhancement

Comments

@StCyr
Copy link
Contributor

StCyr commented Feb 4, 2022

How to use GitHub

  • Please use the 👍 reaction to show that you are interested into the same feature.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

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

  1. When sharing a file via an email recipient, the password input field is not displayed anymore as the user sharing the file may not set its password anymore (except in case of "video verification". See next case)

image

  1. When the "video verification" checkbox is checked, the password input field is displayed again to let the user sharing the file specify a temporary password during a "Request password" Talk session. Clicking on the arrow button would just store the password in the database but not send it to the recipient's email address.

image

  1. Setting password for protected anonymous share stays unchanged

image

Changes to the protected non-anonymous share access screen

The protected non-anonymous share access screen would always show the “request password” button:

image

The following flowchart shows the new "request password" process:

image

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.

@StCyr StCyr added enhancement 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Feb 4, 2022
@StCyr
Copy link
Contributor Author

StCyr commented Feb 4, 2022

ping @jospoortvliet and @usselite ;-)

@StCyr StCyr self-assigned this Feb 8, 2022
StCyr added a commit that referenced this issue Feb 16, 2022
…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>
StCyr added a commit that referenced this issue Feb 16, 2022
…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>
StCyr added a commit that referenced this issue Feb 16, 2022
…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>
StCyr added a commit that referenced this issue Feb 16, 2022
…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>
StCyr added a commit that referenced this issue Feb 16, 2022
…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>
StCyr added a commit that referenced this issue Feb 16, 2022
This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Feb 17, 2022
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Feb 17, 2022
This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Feb 17, 2022
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Feb 17, 2022
…ds respecting the password policy (this commit is part of #31005)

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Feb 17, 2022
… of #31005)

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Feb 18, 2022
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Feb 23, 2022
…ystem value.

This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Feb 23, 2022
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>
StCyr added a commit that referenced this issue Mar 6, 2022
…ystem value.

This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Mar 6, 2022
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>
StCyr added a commit that referenced this issue Mar 9, 2022
…in the

email input field and add a 'Back' button.

This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Mar 9, 2022
… 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>
StCyr added a commit that referenced this issue Mar 9, 2022
…220208195521' is unknown."

(by running build/autoloadchecker.sh)

This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Mar 18, 2022
… via the

"Create a new Share" API

this commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Mar 23, 2022
This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
…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>
StCyr added a commit that referenced this issue Apr 7, 2022
…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>
StCyr added a commit that referenced this issue Apr 7, 2022
…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>
StCyr added a commit that referenced this issue Apr 7, 2022
…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>
StCyr added a commit that referenced this issue Apr 7, 2022
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
…ds respecting the password policy (this commit is part of #31005)

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
… of #31005)

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
…ystem value.

This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
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>
StCyr added a commit that referenced this issue Apr 7, 2022
…in the

email input field and add a 'Back' button.

This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
… 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>
StCyr added a commit that referenced this issue Apr 7, 2022
…220208195521' is unknown."

(by running build/autoloadchecker.sh)

This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
… via the

"Create a new Share" API

this commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
StCyr added a commit that referenced this issue Apr 7, 2022
This commit is part of #31005

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
PVince81 pushed a commit that referenced this issue Apr 7, 2022
…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>
PVince81 pushed a commit that referenced this issue Apr 7, 2022
…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>
StCyr added a commit that referenced this issue Apr 11, 2022
…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
Copy link

Is this supposed to be working fine in NC24.0.0?

I create a new email share and I see the password field (not expected?):

image

The link and the password are sent in two separate emails (note the missing "or:")

image

Pretend I don't have the password so click on Request Password, provide the email and I get "Password Sent!" message, but no email is sent (not expected).
image

Nonetheless I already had the password so I type it in and I have access to the document (expected).

Now, I close the browser reopen it and I'm not asked to provide a password, access to the document is not protected anymore (not expected).

@PVince81
Copy link
Member

@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

I create a new email share and I see the password field (not expected?):

this is expected, you can set an initial password if you want, if you keep what's there it's already a generated password

The link and the password are sent in two separate emails (note the missing "or:")

is this in the email ? or tooltip ?

Pretend I don't have the password so click on Request Password, provide the email and I get "Password Sent!" message, but no email is sent (not expected).

if email sending working with your setup at all ? please check the logs

Now, I close the browser reopen it and I'm not asked to provide a password, access to the document is not protected anymore (not expected).

this is public link functionality, a different unrelated bug that likely exists also for non-email shares.
when entering a password a PHP session is started and perhaps the cookie scope has been set to have an expiration time instead of "expire on browser close". Not sure if this was by design, @jancborchardt might know

@pmarini-nc
Copy link

Thanks @PVince81 for the quick feedback.

@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

I create a new email share and I see the password field (not expected?):

this is expected, you can set an initial password if you want, if you keep what's there it's already a generated password

The link and the password are sent in two separate emails (note the missing "or:")

is this in the email ? or tooltip ?

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:")?

Pretend I don't have the password so click on Request Password, provide the email and I get "Password Sent!" message, but no email is sent (not expected).

if email sending working with your setup at all ? please check the logs

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


  "reqId": "FRaZ5t7Hr7VogT6Di2np",
  "level": 3,
  "time": "2022-05-10T08:10:02+00:00",
  "remoteAddr": "",
  "user": "--",
  "app": "files",
  "method": "",
  "url": "--",
  "message": "Backends provided no user object for ",
  "userAgent": "--",
  "version": "24.0.0.12",
  "exception": {
    "Exception": "OC\\User\\NoUserException",
    "Message": "Backends provided no user object",
    "Code": 0,
    "Trace": [
      {
        "function": "getUserFolder",
        "class": "OC\\Files\\Node\\Root",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/lib/private/Files/Node/LazyFolder.php",
        "line": 72,
        "function": "call_user_func_array"
      },
      {
        "file": "/var/www/nextcloud/lib/private/Files/Node/LazyRoot.php",
        "line": 40,
        "function": "__call",
        "class": "OC\\Files\\Node\\LazyFolder",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/apps/text/lib/Service/DocumentService.php",
        "line": 388,
        "function": "getUserFolder",
        "class": "OC\\Files\\Node\\LazyRoot",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/apps/text/lib/Service/DocumentService.php",
        "line": 525,
        "function": "getFileById",
        "class": "OCA\\Text\\Service\\DocumentService",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/apps/text/lib/Cron/Cleanup.php",
        "line": 75,
        "function": "unlock",
        "class": "OCA\\Text\\Service\\DocumentService",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/lib/public/BackgroundJob/Job.php",
        "line": 79,
        "function": "run",
        "class": "OCA\\Text\\Cron\\Cleanup",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/lib/public/BackgroundJob/TimedJob.php",
        "line": 95,
        "function": "execute",
        "class": "OCP\\BackgroundJob\\Job",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/cron.php",
        "line": 151,
        "function": "execute",
        "class": "OCP\\BackgroundJob\\TimedJob",
        "type": "->"
      }
    ],
    "File": "/var/www/nextcloud/lib/private/Files/Node/Root.php",
    "Line": 368,
    "CustomMessage": "Backends provided no user object for "
  }
}
{
  "reqId": "FRaZ5t7Hr7VogT6Di2np",
  "level": 3,
  "time": "2022-05-10T08:10:02+00:00",
  "remoteAddr": "",
  "user": "--",
  "app": "core",
  "method": "",
  "url": "--",
  "message": "Error while running background job (class: OCA\\Text\\Cron\\Cleanup, arguments: )",
  "userAgent": "--",
  "version": "24.0.0.12",
  "exception": {
    "Exception": "OCP\\Files\\NotFoundException",
    "Message": "",
    "Code": 0,
    "Trace": [
      {
        "file": "/var/www/nextcloud/apps/text/lib/Service/DocumentService.php",
        "line": 525,
        "function": "getFileById",
        "class": "OCA\\Text\\Service\\DocumentService",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/apps/text/lib/Cron/Cleanup.php",
        "line": 75,
        "function": "unlock",
        "class": "OCA\\Text\\Service\\DocumentService",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/lib/public/BackgroundJob/Job.php",
        "line": 79,
        "function": "run",
        "class": "OCA\\Text\\Cron\\Cleanup",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/lib/public/BackgroundJob/TimedJob.php",
        "line": 95,
        "function": "execute",
        "class": "OCP\\BackgroundJob\\Job",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/cron.php",
        "line": 151,
        "function": "execute",
        "class": "OCP\\BackgroundJob\\TimedJob",
        "type": "->"
      }
    ],
    "File": "/var/www/nextcloud/apps/text/lib/Service/DocumentService.php",
    "Line": 391,
    "CustomMessage": "Error while running background job (class: OCA\\Text\\Cron\\Cleanup, arguments: )"
  }
}

Now, I close the browser reopen it and I'm not asked to provide a password, access to the document is not protected anymore (not expected).

this is public link functionality, a different unrelated bug that likely exists also for non-email shares. when entering a password a PHP session is started and perhaps the cookie scope has been set to have an expiration time instead of "expire on browser close". Not sure if this was by design, @jancborchardt might know

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.

@StCyr
Copy link
Contributor Author

StCyr commented May 10, 2022

Regarding the session cookie, we had a (unfinished) discussion on this topic in the PR => #31220 (comment)

@PVince81
Copy link
Member

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.

@PVince81
Copy link
Member

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

@Zegorax
Copy link

Zegorax commented May 12, 2022

@PVince81 I think there is a problem with the implementation. I've just tested it and here is how it went :

  1. Shared a file with an email address, checking the box "password protected"
  2. The recipient successfully receive the link to the shared file
  3. The recipient also receives the password right afterwards (In my opinion, this is a wrong behavior, the user should click on the link, and then request the password)
  4. When the link is clicked, the user can either put the password he has received, or request a new one
  5. "Request a password" works. The new password is sent via email and then the user can use it
  6. Opening a new incognito tab and pasting the link. In theory, the password in the last email (after the request password) should not be able to work, since it is one-time only. However, if I paste again the same password, the access is granted.

In my opinion, there is two wrong behaviors :

  • The first password should not be sent via email when the share is created
  • Each requested password should be one-time use only (cannot re-use them)
  • The requested password should expire after some time if it is not used

What do you think ?

@StCyr
Copy link
Contributor Author

StCyr commented May 12, 2022

* The first password should not be sent via email when the share is created

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....

* Each requested password should be one-time use only (cannot re-use them)

This is an interesting feature but it is not how it has been implemented. Please create a new feature request.

@Zegorax
Copy link

Zegorax commented May 12, 2022

I will create a new feature request then, thanks!

@camilasan
Copy link
Member

3. The recipient also receives the password right afterwards (In my opinion, this is a wrong behavior, the user should click on the link, and then request the password)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap enhancement
Projects
None yet
Development

No branches or pull requests

5 participants