Skip to content

Conversation

@mrvahedi68
Copy link

@mrvahedi68 mrvahedi68 commented Jul 16, 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

@szaimen szaimen added the 3. to review Waiting for reviews label Jul 16, 2023
@szaimen szaimen added this to the Nextcloud 28 milestone Jul 16, 2023
@szaimen szaimen requested review from a team, ArtificialOwl, come-nc and nfebe and removed request for a team July 16, 2023 22:36
@kesselb kesselb mentioned this pull request Jul 17, 2023
5 tasks
@kesselb kesselb requested review from ChristophWurst, miaulalala, nickvergessen and st3iny and removed request for ArtificialOwl, come-nc and nfebe July 17, 2023 18:47
@michielbdejong
Copy link
Contributor

Can someone review this please?
@miaulalala you already gave this code a "looks good", this is the rebase you requested.

@mickenordin
Copy link
Contributor

Is there anything blocking the merge of this? It would be great if this could be merged.

@ChristophWurst
Copy link
Member

See the open todos in the PR description

@ChristophWurst
Copy link
Member

For the tests: let's also add one that tests the change of session data IDs during an authentication. That can happen when an instance is upgraded in the middle of a user's authentication attempt.

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 26, 2023
@ChristophWurst ChristophWurst marked this pull request as draft September 26, 2023 08:26
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 31 milestone Aug 14, 2024
@sorbaugh
Copy link
Contributor

sorbaugh commented Aug 16, 2024

Hello @michielbdejong, I'm reopening your PR and requesting reviews! :) Is there anything left blocking you? We'd like to help you get this one in!

@sorbaugh sorbaugh reopened this Aug 16, 2024
@sorbaugh sorbaugh marked this pull request as ready for review August 16, 2024 07:56
@skjnldsv skjnldsv added this to the Nextcloud 32 milestone Jan 30, 2025
This was referenced Aug 22, 2025
This was referenced Sep 2, 2025
This was referenced Sep 25, 2025
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Confusing variable name in 2FA

10 participants