Add User.mfa_required? #3135
Add User.mfa_required? #3135
Conversation
|
Added mfa_required_not_yet_enabled? and mfa_required_weak_level_enabled? and tests. The weak level checks will be deleted after UI Only is removed 8000 , right? |
That is correct 👍 After the migration is done we would probably need to do some cleanup |
e3af4a6 to
037ccbf
Compare
|
🤔 It is hard to do review when those methods are not used yet. Are they going to be part of some next PR as well? |
There was a problem hiding this comment.
Left a few notes, food for thought 😊. Overall, it looks like it sets up the groundwork well for the next set of work.
That said, it'd be great if the PR description was more detailed and provided insight into what these methods are in preparation for (especially since the methods aren't actually being used and are simply being defined in this PR).
@simi good point, sorry for not being transparent. I believe the plan is to do something similar with what we did with the warnings.
Maybe it makes more sense to have 1+2 in this PR as they can be merged before launch. The original rationale to have 1 separate is that we can do 2 and 3 in parallel. (I'll create an issue to track this too) |
|
Implemented all of @bettymakes feedback and also changed: rubygem. I think adding the distinction between policy and metadata mfa_required here makes sense, as Rubygem.mfa_required? and User.mfa_required? are now related, as you would expect, compared to before this refactor. |
003b189 to
b5d0dbc
Compare
|
Removed the strong_mfa_level? check, and rebase/squashed |
|
I've reverted to keep the strong_mfa_level? check. mfa_required? is false because of strong_mfa_level? being true, and then the rubygems.mfa_recommended.any? returns true because 180mil+1 > 165 mil. A different approach could be to make the scope 165million..180million, but it would be easier to keep strong_mfa_level? check in mfa_recommended? in my opinion. What is your opinion? |
Renamed existing mfa_required functions (pertaining to the metadata of gems) to be metadata_mfa_required. Added public interfaces that make use of mfa_required. Added tests
b5d0dbc to
fb0e877
Compare
Adds a function to check if a User requires mfa due to the number of downloads on one of their gems in preparation for future MFA requirement work.
Edit, more info:
The mfa_required function will check if a user needs to enable mfa because one of the packages they own passed the MFA-required downloads threshold. Eventually, this will block them from taking some actions that require authentication.
Doesn't use the function/block the user's access to anything.