feat: allow occ admin to delete user's secrets

[jvillafanez]

This allows admins to reset the secret in order to allow the user to login in case the user has lost the phone and he's locked out of his account.

Rel: https://github.com/owncloud/enterprise/issues/6252

[pako81]

Looks good. Just a small remark when passing multiple users:

twofactor_totp:delete-secret bob,alice

does not delete the secrets for the two users.

twofactor_totp:delete-secret bob, alice

(note the blank space) does delete the secret for alice only, so basically only for the user being passed as last in the list.

Maybe add an hint at https://github.com/owncloud/twofactor_totp/pull/311/files#diff-db86218d6d77891d5678b5cb1f7ff3d8240fe0591d2ec4b9d8c3ff16d6b3dce3R50 to clarify this must not be a comma-separated list of users.

[phil-davis]

I suppose that deleteSecretsByUserId fails for non-existent users, such as a user with UID bob,alice or bob, in the examples above. twofactor_totp:delete-secret bob alice should work to delete the secrets of both users.

What should the command do if there are non-existent users specified? Nothing? Or report the users that were invalid (and process the others), or stop completely without doing anything?

[pako81]

What should the command do if there are non-existent users specified? Nothing? Or report the users that were invalid (and process the others), or stop completely without doing anything?

I would say report the users that were invalid (and process the others).

Also, the command currently does not give any feedback when succeeding. May be worth to add a message Secret for user $user deleted.

[jvillafanez]

Looks good. Just a small remark when passing multiple users

Command's help should help :sweat_smile: . It should be clear there.

What should the command do if there are non-existent users specified? Nothing? Or report the users that were invalid (and process the others), or stop completely without doing anything?

I would say report the users that were invalid (and process the others).

The command could be used to remove secrets from already-deleted users, in case the secrets aren't removed automatically. Non-existing users will likely be reported as invalid, so we might not be able to remove their secrets otherwise.

I think it's important to find a place for this command to co-exists with the twofactor_totp:delete-redundant-secret one. Unconditionally remove the secrets of a couple of targeted users should be fine.

In addition, for the --all option, we're deleting the secrets of the "seen" users. We might need to wipe out the whole table to be consistent with removing data from users that have been deleted.

Also, the command currently does not give any feedback when succeeding. May be worth to add a message Secret for user $user deleted.

Right now, the query we're using doesn't give any feedback, so we don't know if the data was deleted or not. It might be good to adjust the code in order to return something, along with some changes to wipe out the data for the --all option.

[jvillafanez]

New commit should give better feedback to the admin. I still think we should be able to remove secrets from non-existing users, so we aren't checking the user.

[pako81]

AFAICT secrets are deleted upon user's removal (https://github.com/owncloud/twofactor_totp/blob/master/lib/Hooks.php#L67-L69), so not sure what "non-existing users" exactly are in this context.

Anyway, admin at least gets now the feedback that 0 secrets were deleted when inputting a wrong user:

sudo -u www-data php occ twofactor_totp:delete-secret alice,
0 secrets deleted for alice,

I guess this would be sufficient?

[phil-davis]

I added some acceptance test scenarios. They help verify the various combinations of messages emitted by the delete-secret command.

[sonarcloud[bot]]

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[pako81]

merge this?