one-time passwords (e.g. for public file shares) - #61722
Conversation
adbd854 to
ae1bd6a
Compare
ae1bd6a to
5035daf
Compare
5035daf to
99f7621
Compare
edb4db5 to
27730a7
Compare
bc04d3a to
9a9f4d3
Compare
|
From my side, this PR is ready for review. :) The only thing missing, is documentation, but I would want to wait for feedback before working on that. Let me know if there is anything you require from me. Here are curl commands to create an OTP protected share on a Nextcloud instance running at http://nextcloud.local with user=admin and password=admin (requires the otp_provider_debug and otp_provider_email apps to be enabled): For email: curl http://nextcloud.local/ocs/v2.php/apps/files_sharing/api/v1/shares -u 'admin:admin' -H 'Content-Type: application/json' -H "Accept: application/json" -H "OCS-APIRequest: true" --data '{"path": "/Nextcloud_Server_Administration_Manual.pdf", "attributes": "[]", "shareType": 3, "otpProvider": "email", "otpRecipient": "your@email.address"}'For debug (OTPs will be written to Nextcloud logs, requires https://github.com/theCalcaholic/otp_provider_debug): curl http://nextcloud.local/ocs/v2.php/apps/files_sharing/api/v1/shares -u 'admin:admin' -H 'Content-Type: application/json' -H "Accept: application/json" -H "OCS-APIRequest: true" --data '{"path": "/Nextcloud_Server_Administration_Manual.pdf", "attributes": "[]", "shareType": 3, "otpProvider": "debug", "otpRecipient": "foobar"}'If the file |
f79b8ba to
702cee9
Compare
|
@susnux I hope it's ok to tag you (I wasn't sure if the update messages above would reach you otherwise). |
1c2d696 to
271c553
Compare
|
@miaulalala I have now implemented the requested changes apart from those that aren't yet fully clear to me: Security-related
Regular issues
|
f583d80 to
e1b1aa5
Compare
|
Another day, another rebase (significantly reducing the changed files in dist/, this time) 🙃 |
049e0bf to
faf1609
Compare
|
Sorry for the new pipeline failures. Apparently I introduced them during merge. I rebased again and fixed the issues (hopefully, couldn't test all of them reliably). |
|
@miaulalala Hi, I don't want to create any pressure, however, I wanted to ask if there's anything you need from me right now? Other than that, please let me know, when you want me to rebase again (I'd rather not do that every day, since that means more work and always creates the risk of introducing errors). |
|
/compile amend |
|
@theCalcaholic can you rebase your PR and squash? Then I'll retrigger CI 🙏 |
|
@miaulalala Sure, will do! |
…ords Signed-off-by: Tobias Knöppler <tobias@knoeppler.org>
Signed-off-by: Tobias Knöppler <tobias@knoeppler.org>
…ackend/API) Signed-off-by: Tobias Knöppler <tobias@knoeppler.org>
…ge (if available) Signed-off-by: Tobias Knöppler <tobias@knoeppler.org>
Signed-off-by: Tobias Knöppler <tobias@knoeppler.org>
Signed-off-by: Tobias Knöppler <tobias@knoeppler.org>
Signed-off-by: Tobias Knöppler <tobias@knoeppler.org>
faf1609 to
7136cc7
Compare
|
@miaulalala Done. I hope I didn't miss anything this time :) |
Signed-off-by: Tobias Knöppler <tobias@knoeppler.org>
7136cc7 to
41321ee
Compare
|
@miaulalala 🙈 And it's outdated again. But at least there are no conflicts, so I think, this is still fine? |
|
@theCalcaholic you do not need to rebase if only |
| * @psalm-import-type Files_SharingOTPSendError from ResponseDefinitions | ||
| * @psalm-import-type Files_SharingOTPProvider from ResponseDefinitions | ||
| */ | ||
| class ShareOTPController extends ApiController { |
There was a problem hiding this comment.
I think this should be an OCS controller instead - OCS would be the standard for all our "officially" provided APIs
There was a problem hiding this comment.
Alright, I remember, that it was a deliberate decision to not implement this as OCS controller, but I don't remember the reasons anymore (it's been some time :D).
I'll try to change it and if it comes up again I'll discuss it with you.
There was a problem hiding this comment.
I dont think we need an empty routes file?
There was a problem hiding this comment.
Originally I had no routes.php, but it broke the CI checks. Not quite sure anymore, but it might have been the OpenAPI generator that complains if we remove it.
There was a problem hiding this comment.
I do not really get why this interface provides the setters?
Isnt the workflow:
- app requests OTP from manager
- manager generates an OTP
- app checks OTP later?
In this case the interface never needs setters?
There was a problem hiding this comment.
OTPs are created during share creation without password and expiration time (but providing OTP configuration, i.e. provider, recipient) while passwords are set at a later point, when requested by the recipient (see sequence diagram below). Also, the setters are used by the OTP manager when loading and creating OTPs (see here).
There was a problem hiding this comment.
How to get the available providers?
Would you need to know them / hardcode them?
Why not use the common registration approach? Meaning add something like
addOtpProvider to the BootstrapContext so that an provider app would enable itself.
This way the otp manager can:
- check there is at least one provider enabled
- check if a provider id is valid
- get a list of available providers
In general I am not sure I correctly understood the workflow that should be done here.
From how I understand the feature, there are following entities:
- Manager - handling OTP from within OCP API
- (multiple) Providers - handling delivery of OTPs to the user
- OTP - The password object
- App - Requests a new OTP and checks user input against OTP
I would expect the workflow to be something like
sequenceDiagram
App->>+Manager: Request OTP for Subject and Receiver
Manager->>+App: Responds with OTP
App-->>+App: Stores that OTP is used
Manager->>+Receiver: Delivers OTP
Receiver->>+App: Requests access to Subject with OTP
App->>+Manager: Checks validity of OTP or Subject and Receiver
Manager->>+App: Confirms access
Manager-->>+Manager: Deletes OTP
Meaning
- The manager handles generation and storage of OTPs (secret, subject, receiver).
- Providers add support for different receivers.
- Manager provides list of available providers so apps can offer OTP for specific receivers
- App only stores that an OTP is used -> checks manager if OTP is correct
|
@susnux Thanks so much for the review! I'll try to answer your questions below.
Nothing is hardcoded. Instead, the OTPManager creates a GetOneTimePasswordProvidersEvent to collect a list of providers. Providers, in turn, need to implement and register listeners to the event.
All of this is already possible (and working this way).
Yes, handling delivery + validation of recipients. They are using events to communicate both back to the OTP manager (or whoever needs to access them).
OTP is an object containing both the configuration for the OTP (provider + allowed recipient + (optional) expiry date) as well as the password. Initially, the password is empty until requested by a client.
The actual validation logic is implemented in the OTP Manager, but it is called from the app (or in the implemented use case - i.e. public shares - from the Share20 Manager).
The workflow for public shares is slightly different, because it is not the sharee who decides whether OTPs are used, but the sharer. So, the actual process looks as follows (but mind, that the implementation is designed to be flexible enough to allow other use cases to implement support in different ways). I have split it into share creation and retrieval for better readability. Share CreationsequenceDiagram
actor Sharer
Sharer->>+FilesSharingApp: Request dialog for public share creation
FilesSharingApp-)+EventBus: Request list of OTP providers with UI metadata to provide options for Sharer
EventBus-)+ProviderApp: Receive list-otp-providers request
ProviderApp--)+EventBus: Respond with providers + metadata
EventBus--)+FilesSharingApp: Receive providers + metadata
FilesSharingApp-->>+Sharer: Offer list of OTP providers for public share creation
Sharer->>+FilesSharingApp: Request OTP protected share for given provider + recipient
FilesSharingApp->>+Manager: Create OTP configuration for the share (with empty password)
Share RetrievalsequenceDiagram
actor Sharee
Sharee->>+FilesSharingApp: Request OTP for configured recipient
FilesSharingApp->>+Manager: Request OTP to be generated (sets password + expiration time)
Manager-)+EventBus: Request OTP provider to be sent to recipient
EventBus-)+ProviderApp: Receive send request with matching provider id
ProviderApp--)+Sharee: Sends OTP
ProviderApp--)+EventBus: Responds with send success/failure status
EventBus--)+Manager: Returns send success/failure status
Sharee->>+FilesSharingApp: Asks for access with (OTP) password
FilesSharingApp->>+Manager: Checks OTP validity
Manager->>+Manager: Deletes OTP
Manager-->>+FilesSharingApp: Confirms access
FilesSharingApp-->>+Sharee: Grants access to share
Note: I have merged the responsibilities of the files_sharing app and the Share20/Manager here a bit, but I hope, the picture still becomes clear.
correct
correct
yes and no, the providers themselves add themselves to the list for anyone asking (via event bus, see here). This doesn't require the manager, but it provides a method for this purpose, for convenience.
The app stores the OTP id (as foreign key), which is required to retrieve recipient and provider ids. |
Summary
This PR adds one-time password management to Nextcloud server and integrates them with the files_sharing app.
(the actual form in the screenshot is part of #61733)
Notes
make build-js-production).IShare->isPasswordProtected()(Refactor: Centralize logic for checking if a share is password protected #61946)TODO
Architecture and Rationale
General concepts
OTPs (one-time password) are short-lived, single use credentials sent to users via an (according to a given threat model) trusted channel (e.g. a specific email address).
OTP Providers define a method of sending OTPs to users.
OTP Recipients are valid address definitions within the scope of an OTP provider that can be sent OTPs.
Core/Server Changes
Generic
One-time passwords are implemented with generic interfaces so that they can be used by other parts of Nextcloud than sharing.
The core functionality for one-time passwords is implemented within the \OCP and \OC namespaces. OTPs are stored within a new database table
one_time_passwordand have a providerID, a recipient string, an expiration date and a password. The idea here is, that the OTP configuration (i.e. provider+recipient) can be long lived, while the credentials (password+expiration date) are (re-)generated per use.Management of OTPs is implemented in
\OC\OneTimePassword\Manager(implementing the injectable interface at\OCP\OneTimePassword\IManager).\OCP\Security\PasswordContexthas been extended by anOTPcase to allow the creation of password policies specifically for OTPs.Events are used to allow apps to register OTP providers. They need to hook into the
GetOneTimePasswordProvidersand theSendOneTimePasswordevents to provider their functionality. Providers also need to implement the interface\OCP\OneTimePassword\IOneTimePasswordProvider, which defines methods that allow theManagerto select providers and provide information about them.Sharing specific
Shares (see
\OCP\Share\IShare) have been extended with anone_time_passwordfield.The
\OC\Share20\Managerhas been adjusted to prioritize OTPs when checking the authentication for a share.The template
publicshareauth.phphas been adjusted to receive and display OTP related information (used in #61733 to show the OTP specific password form).files_sharing Changes
The
ShareAPIControllerhas been extended to allow creating and updating OTP protected shares and returning the otp configuration when fetching shares. OTPs and passwords are mutually exclusive and an error will be returned when attempting to create a share with both.The
ShareControllerhas been extended to supply template responses for public shares with otp related information.A new
ShareOTPControllerhas been implemented that allows users to request OTPs for a share.OTP Providers
An OTP provider, which allows sending OTPs via email has been implemented as (core) app:
otp_provider_email. It can be disabled or restricted by administrators to disallow this functionality.UI changes
The authentication page for public shares has been updated to accommodate OTPs (see screenshot).
When an OTP is required for a share, the authentication form will change to present a button for requesting an OTP in addition to the normal password field. It will also display a different title above the password field.
Password authentication page for OTP protected shares
The email template for sending OTPs to users is located in
apps/otp_provider_email/lib/listener/SendOneTimePasswordEventListener.phpand uses Nextclouds usual Mailer interface for templating emails.Example OTP email sent to users
The logo is not displayed, because I'm testing with a local Nextcloud instance at a non-public URL.
The custom permissions in the sidebar sharing tab (files->sidebar->sharing->custom permissions) has been adjusted to allow choosing between no password protection, setting a normal password and configuring an OTP. The descriptions of the OTP are defined and localized by the OTP provider.
New UI for selecting an authentication method during share creation
Previous UI for selecting an auth method for comparison
Checklist
3. to review, feature component)stable32)AI (if applicable)