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

Prevent changing MFA to ui_only #3084

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

kevinlinxc
Copy link
Contributor

@kevinlinxc kevinlinxc commented Jun 6, 2022

Addresses the remaining half of #2968 (flash messages already resolved/being resolved)

Hides ui_only option for MFA, unless the user is currently using ui_only, ultimately preventing people from switching to ui_only.

Testing:
MFA set to something else:
image (3)

Set MFA to ui_only:
image (5)

Result:
image (4)

@kevinlinxc
Copy link
Contributor Author

kevinlinxc commented Jun 6, 2022

Considered making a less DRY version, maybe make a list, and insert at position 2 if mfa_ui_only? Let me know if that would be better. Alright I just did it

@jchestershopify
Copy link
Contributor

Code looks fine, but I think we should add a test as there is some logic added.

@jenshenny
Copy link
Member

Looking good! Along with tests, it'll be great to add a check in the backend to make sure they aren't updating to ui_only (possibly adding a flash banner if someone tries to).

@kevinlinxc
Copy link
Contributor Author

Added backend code, flash banner, and tests. Had to refactor a bit because of codeclimate

Copy link
Member

@jenshenny jenshenny left a comment

Choose a reason for hiding this comment

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

A few suggestions that should be addressed but otherwise LGTM! Could you also refactor/squash your commits?
Also, I think an update to the guides would be needed 🤔

app/views/settings/edit.html.erb Outdated Show resolved Hide resolved
config/locales/en.yml Outdated Show resolved Hide resolved
Copy link
Member

@jenshenny jenshenny left a comment

Choose a reason for hiding this comment

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

✨ 🚀

@sonalkr132 are we ok with merging and deprecating ui_only right now (with the only heads up being an update to the guides). I think it would be ok as a small percentage are at this level + we are now recommending ui_and_gem_signin and ui_and_api.

@simi
Copy link
Member

simi commented Jun 14, 2022

🙏 please squash commits before merge

@kevinlinxc
Copy link
Contributor Author

Rebased and squashed

@kevinlinxc
Copy link
Contributor Author

Went with Updating multi-factor authentication to "UI Only" is no longer supported. Please use "UI and gem signin" or "UI and API". RubyGems latest failing though

@sonalkr132 sonalkr132 merged commit 9a2d3db into rubygems:master Jun 17, 2022
@sonalkr132
Copy link
Member

Thank you @kevinlinxc

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.

5 participants