owncloud / twofactor_totp

🔑 Second factor TOTP (Google Authenticator) provider for ownCloud
GNU Affero General Public License v3.0
9 stars 9 forks source link

[UX] 2F TOTP confuses Guest Users in default configuration #247

Closed ChrisEdS closed 2 years ago

ChrisEdS commented 2 years ago

Steps to reproduce

  1. Enable the Guests App
  2. Enable 2FA TOTP
  3. Log in as guest
  4. Try to setup 2FA TOTP via User -> Security -> TOTP Second-factor Auth

Current behaviour

In the ownCloud default configuration, 2FA is active for all groups, including the guests_app group. When a guest goes to the Security tab he has the possibility to select Activate TOTP but the QR code does not appear as usual - instead there is no further action. When the page is reloaded, the check mark disappears.

image

This is due to the fact that the use of the app is not whitelisted for the Guests App by default. It works as expected when twofactor_totp will be added to the whitelist.

image

Expected behaviour

If the app is not in the guest app whitelist, the UI to turn on and configure the guest app should not be displayed to avoid confusion of guest users.

jnweiger commented 2 years ago

More expected behaviours: *A warning message should be displayed "You are logged in as a guest user. twofactor_totp is not whitelisted for guests. Please consult with your administrator"

(In general, the guest user scenario is not very well covered in manual tests, currently. Should we add something to the testplan template?)

NannaBarz commented 2 years ago

Add the twofactor_totp to the white list

ChrisEdS commented 2 years ago

Add the twofactor_totp to the white list

?

ChrisEdS commented 2 years ago

You mean the solution should be to add the Guest_App to the Whitelist? But that doesn't solve the user experience if the app is not whitelisted.

phil-davis commented 2 years ago

In general, the guest user scenario is not very well covered in manual tests, currently. Should we add something to the testplan template?

I also raised #248 - it is easy enough to run with the guests app in CI. We do this for password_policy, and can do it here, and think about what other apps would be good to test with scenarios that involve a guest user.

AlexAndBear commented 2 years ago

You mean the solution should be to add the Guest_App to the Whitelist? But that doesn't solve the user experience if the app is not whitelisted.

No, the whitelist is in the guest app context, so we should add totp to the default whitelist. I think it is NOT good, just hiding the whole guests config section, the admin will not be able to add or remove totp from the guests app whitelist.

ChrisEdS commented 2 years ago

You mean the solution should be to add the Guest_App to the Whitelist? But that doesn't solve the user experience if the app is not whitelisted.

No, the whitelist is in the guest app context, so we should add totp to the default whitelist. I think it is NOT good, just hiding the whole guests config section, the admin will not be able to add or remove totp from the guests app whitelist.

I don't understand that sentence. The admin can always remove the app from the whitelist?

What if an admin does not want to enable 2FA for guests? Then we are at a remove whitelisting and a bad UX. In this case we should at least show a hint that 2FA is active but cannot be enabled for guests.

pmaier1 commented 2 years ago

2FA is not mandatory, even when enabled. The user needs to set it up for themself. There is not much point in removing it from the guests allow list. I'll not start adding hints or hiding config sections for each and every app that the admin could potentially remove from the guests allow list.

Please add twofactor_totp to the default allow list and close the case. Thanks!

AlexAndBear commented 2 years ago

Please add twofactor_totp to the default allow list and close the case. Thanks!

done