Issue when upgrading TOTP from v0.4.3 to v0.4.4

[pako81]

Upgrading TOTP from v0.4.3 to v0.4.4 result in the app to be automatically disabled. Users will need to reactivate it and re-enroll any device. Tested on 10.0.7.

@DeepDiver1975 FYI

[pako81]

To be more precise the app stays enabled but the user is required to untick and tick again the "Activate TOTP" checkbox on the Personal settings page and reverify the authentication code.

If the user does not perform this action login over two-factor auth does not work anymore.

[karakayasemi]

@pako81 thanks for reporting. Since 0.4.4, verification has become mandatory to prevent possible lockout scenario. To achieve that we added a new boolean column ("verified") to the database. To activation "verified" column should be true. In the database migration from v0.4.3 to v0.4.4 the column is created with false initial value for existing rows. That is why your issue is happening.
I am not sure, should we take action for it? @DeepDiver1975 FYI

[pako81]

@karakayasemi thanks for your feedback, I was not aware of the change in the DB structure since v0.4.4. So I guess that updating the boolean column like

UPDATE oc_twofactor_totp_secrets SET verified=1 WHERE verified=0;

right after the upgrade would make it work normally. And indeed I have tested this out and it seems users can log in without the need to reverify the code. Not sure however if recommended to manually hack the DB this way..

[felixboehm]

@PVince81 please clarify how to resolve this issue for next version.

[PVince81]

@karakayasemi if we decided to make this field "verified" set to true, is there a way for the admin over the UI to force reverification is they choose so ? Goal would be mostly to prevent such upgrade experience.

Do i understand it correctly that in past version the verification wasn't guaranteed so setting it to true on upgrade would be wrong and a potential security flaw ?

I see two possible solutions:

1. make the upgrade more smooth by setting that value to true on upgrade only but not on new installs: has potential security flaw as we'd be setting to true without guarantee that verification was done

or

2. write a release note and maybe a more visible message somewhere to let the admin know they should set this value to "true" if they are confident that clients are already trustworthy and do not require reverification

[PVince81]

Talked with @DeepDiver1975 and he suggested to go the release notes route now.

@pmaier1

[karakayasemi]

As far as I can see, setting this value to "true" only works right after the upgrade. For already updated servers, setting this value true can cause a lockout for users in verification step. In the already updated servers, every user should recreate totp.

[PVince81]

Hmmm... I can see how admins might miss doing this on time... Can it be done manually before the upgrade ? Is the config value used by the old version ?

[karakayasemi]

No, the column "verified" was newly added. We could write a migration to set this value true for existing rows. Now the admins should set true for existing rows right after the update because we did not.

[PVince81]

Even if we did now the app is already released, so whatever logic we put in will only be effective when migrating from v0.4.3 to v0.4.5, because changing the setting in v0.4.4 to v0.4.5 would still require all users to reverify.

It looks like we could have this proposed automatic switch when migrating from v0.4.3 to v0.4.5+ and release notes for people already on v0.4.4 that say "sorry, you'll need all clients to reverify now"

[PVince81]

moving to backlog, need to schedule some time to look at this...

[PVince81]

Ok, maybe an occ command that an admin could run if they choose to automatically set the verification to true for existing users ? By default users would need to reverify but the release notes should state that an admin might want to run this command.

The command should allow to set for all users --all or for specific user ids. Let the command also set the state (verified vs non-verified). This way admins have finer control over the state in case they'd want to reset.

Thoughts ? @karakayasemi @pako81

[PVince81]

also cc @pmaier1 as the above was already change of behavior

[karakayasemi]

@PVince81 It sounds good to me. In addition, we can add an optional timestamp argument for --all. In this way, the command can set as verified or unverified all entries which created before the timestamp.

Since admins know when they upgraded the app, they can specify upgrade timestamp and the command only affects rows created before updating.

[PVince81]

@karakayasemi sounds great. Glad to hear there's also a timestamp in there.

Is this something you'd have time to work on ?

[karakayasemi]

I can work on it this weekend. If it is okay, you can assign it to me.

[PVince81]

Awesome, thanks