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

Distinguish 'done' from 'configuring' in 2FA #37913

Closed
wants to merge 3 commits into from

Conversation

mrvahedi68
Copy link

@mrvahedi68 mrvahedi68 commented Apr 25, 2023

Summary

When there is a token in the session for which the user is still setting up 2FA, setting self::SESSION_UID_DONE ("two_factor_auth_passed") is a misnomer.

AFAICT, everything works fine if you set nothing into the session and just return 'false' from this if-statement, but in case there is some code (now or in the future) that needs to know if the user is configuring 2FA, to play it safe I would suggest storing self::SESSION_UID_CONFIGURING ("two_factor_auth_configuring") into the session.

TODO

  • add tests
  • test manually
  • consider removing this session variable once configuration is successfully completed

Checklist

mrvahedi68 and others added 3 commits April 25, 2023 08:12
When there is a token in the session for which the user is still [setting up 2FA](https://raw.githubusercontent.com/nextcloud/twofactor_totp/master/screenshots/settings.png), setting `self::SESSION_UID_DONE` ("two_factor_auth_passed") is a misnomer.

AFAICT, everything works fine if you set nothing into the session and just return 'false' from this if-statement, but in case there is some code (now or in the future) that needs to know if the user is configuring 2FA, to play it safe I would suggest storing `self::SESSION_UID_CONFIGURING` ("two_factor_auth_configuring") into the session.

Signed-off-by: Michiel de Jong <michiel@unhosted.org>
Signed-off-by: MohammadReza vahedi <34796044+mrvahedi68@users.noreply.github.com>
Signed-off-by: MohammadReza vahedi <34796044+mrvahedi68@users.noreply.github.com>
@solracsf solracsf added the 3. to review Waiting for reviews label Apr 25, 2023
@solracsf solracsf added this to the Nextcloud 27 milestone Apr 25, 2023
@mrvahedi68
Copy link
Author

Closed because of duplication.

@mrvahedi68 mrvahedi68 closed this Apr 25, 2023
@mrvahedi68 mrvahedi68 deleted the patch-2 branch April 25, 2023 07:46
@solracsf solracsf removed the 3. to review Waiting for reviews label Apr 25, 2023
@solracsf solracsf removed this from the Nextcloud 27 milestone Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing variable name in 2FA
3 participants