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

[Mandatory 2FA] Spec: Revamp 2FA occ interface #11019

Closed
ChristophWurst opened this issue Sep 3, 2018 · 1 comment
Closed

[Mandatory 2FA] Spec: Revamp 2FA occ interface #11019

ChristophWurst opened this issue Sep 3, 2018 · 1 comment

Comments

@ChristophWurst
Copy link
Member

ChristophWurst commented Sep 3, 2018

Mandatory 2FA in Nextcloud 15

Overview/progress board: https://github.com/orgs/nextcloud/projects/17

🚀


Specification: Revamp 2FA occ interface

In order to improve the admin experience with two-factor authentication, I think it would make sense to discuss possible changes to the occ commands.

Current state

The Nextcloud 14 command line interface occ currently has three commands dedicated to 2FA:

twofactorauth:disable

When I first implemented 2FA, I added two occ commands that allowed admins to overrule the state of 2FA of a user. The goal was to offer a way of temporarily disabling 2FA for an account so that login was possible again.

twofactorauth:enable

This command reverses the changes of the previous command. It should be used once the user gained back access of their account and the admin can re-enable 2FA.

While technically this worked fine, it caused lots of confusion for users and admins. It was not clearly communicated/documented that this command cannot be used to enable 2FA in the first place.

twofactorauth:state

This command was added in Nextcloud 15 when the provider state got persisted in the server component. Since this change, the server (with a few exception) knows which specific provider are enabled/disabled for a user without having to load the actual provider apps.

The output looks as follows.

$ php occ twofactorauth:state admin
Two-factor authentication is not enabled for user admin

Disabled providers:
- backup_codes
- totp
- u2f

Proposed changes

An alternative to twofactorauth:enable

As mentioned above, the enable/disable commands were confusing for admins/users. Hence, I would suggest to remove them and add a solution that is easier to use and less prone to (user) errors.

The admin support provider could be a perfect replacement for this. With that new provider, admins wouldn't have to disable the existing providers, but could offer users another way to gain back access to their account. IMO this is clean, secure and less error-prone.

A way to disable (uninstalled) providers

There are scenarios where the provider state (which provider is active for a given user) is outdated in our database. This could, for example, happen if a user has previously offered their admins to use the TOTP app but for some reason it had to be removed. Unless the admin asked the users to disable the provider on their account and they disabled it manually, Nextcloud will still force them to use 2FA on login, even if that was the only active user. The reason for this is now that Nextcloud knows about the state of 2FA providers, it makes sure to never let user log in if a provider fails to load. Failing providers can either be caused by a bug in the software or a malicious instance.

To offer a nice solution for this, I would suggest to have an occ command to disable specific providers for a user. This way the admin can first check if/which providers are active and then disabled those without having the user to do that manually.

Another slightly different approach would be to add a command that removed the provider state of all users of a given provider id. In the example above, an admin could run something like

./occ app:disable twofactor_totp
./occ twofactorauth:cleanup totp

to cleanly remove the association between users and a (now unavailable) provider.

Requirements: changes to the OCP interface of providers for the case where an admin disables a provider for a specific user which will still be used by other users. For that case, providers must be aware of the state transition to ensure their internal state stays in sync. This change will be tracked in #11018

@rullzer
Copy link
Member

rullzer commented Sep 4, 2018

I would like to suggest as well to (as a next step) have buttons for this as well in the admin interface.

First occ is fine I think. but would be good to have this in the admin interface for people that use the terminal less.

ChristophWurst added a commit that referenced this issue Sep 10, 2018
Fixes #11018.
Required for #11019.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst added a commit that referenced this issue Sep 10, 2018
Fixes #11018.
Required for #11019.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst added a commit that referenced this issue Sep 10, 2018
Fixes #11018.
Required for #11019.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst added a commit that referenced this issue Sep 10, 2018
Fixes #11018.
Required for #11019.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst added a commit that referenced this issue Sep 10, 2018
Fixes #11018.
Required for #11019.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst added a commit that referenced this issue Sep 10, 2018
Ref #11019.

This ensures that when an admin tries to enable or disable a provider,
the provider actually allows the state change without user interaction.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst added a commit that referenced this issue Sep 14, 2018
Ref #11019.

This ensures that when an admin tries to enable or disable a provider,
the provider actually allows the state change without user interaction.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst added a commit that referenced this issue Sep 17, 2018
Ref #11019.

This ensures that when an admin tries to enable or disable a provider,
the provider actually allows the state change without user interaction.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst added a commit that referenced this issue Sep 19, 2018
Ref #11019.

Add `twofactorauth:cleanup` command

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst added a commit that referenced this issue Sep 19, 2018
Ref #11019.

Add `twofactorauth:cleanup` command

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
rullzer pushed a commit that referenced this issue Sep 25, 2018
Ref #11019.

Add `twofactorauth:cleanup` command

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants