Enable MFA on specific API keys #2846
Conversation
jenshenny
force-pushed
the
mfa-specific-keys
branch
from
f041888 to
b2516b2
Compare
app/views/api_keys/new.html.erb
Outdated
| <%= label_tag :mfa, t("api_keys.index.multifactor_auth"), class: "form__label" %> | ||
| <% if current_user.mfa_ui_and_api? %> | ||
| <p><%= t("api_keys.index.mfa_api_enabled") %></p> | ||
| <%= f.hidden_field :mfa, value: true %> |
There was a problem hiding this comment.
when a user has mfa_ui_and_api, all keys they create will have mfa enabled. is this desired behavior? this seems inconsistent with existing keys, where mfa is null for user with mfa_ui_and_api. when user enables mfa with ui_only, it may be unexpected that his existing keys are not working without otp.
also, if we really want this to be true, we should set this in the controller and not view (attackers can mess with this logic).
There was a problem hiding this comment.
My thought process is that a user that has mfa ui and api enabled would downgrade if one of their keys can't work with otp. It would be a pain to downgrade and enable mfa on most of their existing keys if they have many.
I agree that the inconsistencies between new/existing keys isn't great. On the new page, we are communicating this inconsistency by saying mfa will be turned on for that key. One possibility to try to make it more consistent (other than backfilling) is to set mfa to true when updating existing keys if the user has mfa_ui_and_api. But that could make it even more confusing if the user downgrades.
I removed the hidden value that sets this in the view and will add it to the controller if we decide to go this way.
There was a problem hiding this comment.
to make it more consistent (other than backfilling) is to set mfa to true when updating existing keys if the user has mfa_ui_and_api.
agreed, we can make this consistent.
My thought process is that a user that has mfa ui and api enabled would downgrade if one of their keys can't work with otp.
got it. thank you for explaining. Perhaps we need more opinion on this, however, I feel if the user is changing their mfa level, they would expect that existing keys would use the default behavior for that level (default mfa false for ui_only). we would preserve api_key.mfa only where it was explicitly enabled.
If we agree on the above proposition, setting api_key.mfa, when user has mfa UI and API enabled, would be redundant. mfa will be enforced anyway for all keys.
There was a problem hiding this comment.
Perhaps we need more opinion on this, however, I feel if the user is changing their mfa level, they would expect that existing keys would use the default behavior for that level (default mfa false for ui_only). we would preserve api_key.mfa only where it was explicitly enabled.
That makes a lot of sense, I agree with your view now. I’m happy to leave it the way it is right now and not set api_key.mfa unless the user explicitly sets it in the ui only level. I suppose it also isn’t difficult to change the behaviour if we change our minds about this in the future?
There was a problem hiding this comment.
yes, I would prefer that we don't set mfa when the user already has MFA UI and api.
There was a problem hiding this comment.
👍 I removed the checkbox associated with enabling MFA on the UI and API level.
jenshenny
force-pushed
the
mfa-specific-keys
branch
2 times, most recently
from
3a397bb to
0420b51
Compare
|
Apologies for the delay @sonalkr132, I've been away for a while since you last reviewed. I think this PR is ready for another look. |
jenshenny
force-pushed
the
mfa-specific-keys
branch
from
0420b51 to
dc9e57c
Compare
What: A users profile can currently be queried by their id or handle (`api/v1/profiles/:id|:handle`). This adds the ability for an authenticated user to also query their own profile without needing to know their id or pass their handle. Why: Once MFA can be [enabled on specific API keys](rubygems#2846) through the UI, a user should also be able to enable it on keys that they create during `gem signin` in the CLI. However, we only want to ask a user if they would like to enable mfa on new keys if they have account mfa levels of `ui_only` or `ui_and_gem_sign`. Users that have MFA disabled or have it enabled for `ui_and_api ` should not be prompted, as it should be auto enabled or disabled for those levels. Once [an owners mfa level can be queried through the API](rubygems#2837), then enabling an authed user to pull their profile will return their MFA level and help us determine if we should ask them to set enable MFA on new keys created during gem signin.
Co-authored-by: Betty Li <betty.li@shopify.com>
sonalkr132
force-pushed
the
mfa-specific-keys
branch
from
dc9e57c to
8cd8b2c
Compare
There was a problem hiding this comment.
Hi, I have updated the view a bit so that we only show api_key.mfa enabled. Previously, it was showing mfa enabled for all API keys when user has ui_and_api mfa. It may have lead user to belive that they have mfa enabled on all keys even when they downgrade their mfa level.
Thank you so much for working on this. Apologies for the delay in getting this merged.
Please do send the PR for rubygems cli as well.
|
🎉 Awesome, thank you! FYI for the rubygems cli PR, I believe we would need to first add an API endpoint to retrieve the user's MFA status to determine whether or not to ask the user to enable MFA on their API key. There's changes for adding an auth endpoint |
|
Here's the open PR for adding the authed endpoint if you want to take a look @sonalkr132 |
Part of: #2755
This PR allows users to enable or disable MFA requirement on owners that have MFA enabled for
UIandUI and gem signin. Owners cannot disable MFA on keys if they haveUI and APIand cannot enable MFA if they didn't have MFA setup.MFA disabled
IndexMFA UI only
IndexEdit
Create
MFA UI and API
IndexEdit
Create
