Skip to content

Clean up feature flag for second factor reauthentication#8707

Merged
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/remove-reauthentication-2fa-feature-flag
Jul 5, 2023
Merged

Clean up feature flag for second factor reauthentication#8707
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/remove-reauthentication-2fa-feature-flag

Conversation

@mitchellhenke
Copy link
Contributor

🛠 Summary of changes

This was added in #8037 and has been enabled in the major environments for awhile, so this PR enables it by default and removes the configuration for it.

@mitchellhenke mitchellhenke requested a review from a team July 3, 2023 15:31
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/remove-reauthentication-2fa-feature-flag branch from b5448b7 to 3c6127c Compare July 3, 2023 15:57
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only place this is assigned, but it's still referenced in a view? 🤔

<%= t('help_text.change_factor', factor: user_session[:factor_to_change]) %>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this controller has been re-used in other ways where it's not changing factors and the change_factor does not exist. It's also not translated and the content should be re-written to be more generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://gsa-tts.slack.com/archives/CEUQ9FXNJ/p1643994476402979 is a Slack thread that discusses it a bit (with an associated ticket that's linked above that line)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll un-icebox that ticket and get it in front of the team.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, was the _2fa suffix added to help with clarity, or just something we added to avoid conflict with the original method name (i.e. we'd want to circle back and rename to drop it)?

Personally I think it's an improvement though 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added to differentiate from the previous method (which intended to require both password and 2FA).

I like the idea of keeping it as well for descriptiveness' sake.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this ticket be cancelled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yeah. I can do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part about the untranslated string can be cancelled, but does the copy on the page still say "sign in with your phone" for all other MFAs? If so then that part of the ticket should stay

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hope is to remove this page since it'll be unused after this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right!!! thx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mitchell Henke added 2 commits July 5, 2023 14:02
changelog: Internal, Maintenance, Clean up feature flag for second factor reauthentication
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/remove-reauthentication-2fa-feature-flag branch from 4a46d73 to 1b1c49d Compare July 5, 2023 19:03
@mitchellhenke mitchellhenke merged commit 2d51b7d into main Jul 5, 2023
@mitchellhenke mitchellhenke deleted the mitchellhenke/remove-reauthentication-2fa-feature-flag branch July 5, 2023 21:22
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.

3 participants