User Validation on MFA Level

[ericherscovich]

Contributes to #3800

What problem are you solving?

MFA Level could be set without having any MFA device, such as not having a mfa_seed (-> totp_seed) set, nor webauthn credentials. Users could also have MFA disabled while still having devices. This could lead to data inconsistencies.

What approach did you choose and why?

Adds a validation on a user to ensure that mfa_level can only be set if there is a MFA device present and can only be disabled if there is no MFA device.

What should reviewers focus on?

The approach to validating that MFA level is set in the correct scenarios.

Testing

Can attempt to set MFA impromperly, for example by doing create(:user, mfa_level: :ui_and_api) and observing it fails validation.

⚠️ Pre-merging considerations ⚠️

We should validate if this is somehow present in production. Specifically, we should check if there exists any user that has mfa_level somehow set while not having any MFA devices. We should also check that there isn't any user that has mfa_level set to disabled, but has an MFA device present. It's possible that merging this would cause some funky behaviour for users if they somehow got into a weird state like this. I don't have the ability to check production in this way, so will have to defer to other maintainers to validate this before merging.

[codecov]

Codecov Report

Merging #3905 (c255e8b) into master (642b650) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head c255e8b differs from pull request most recent head 3486573. Consider uploading reports for the commit 3486573 to get more accurate results

@@           Coverage Diff           @@
##           master    #3905   +/-   ##
=======================================
  Coverage   98.79%   98.79%           
=======================================
  Files         217      217           
  Lines        5406     5414    +8     
=======================================
+ Hits         5341     5349    +8     
  Misses         65       65           

Impacted Files	Coverage Δ
app/models/concerns/user_multifactor_methods.rb	100.00% <100.00%> (ø)

[simi]

@ericherscovich I did some initial checks.

Specifically, we should check if there exists any user that has mfa_level somehow set while not having any MFA devices.

User.where.not(mfa_level: 'disabled').each_with_object(0) {|u,m| m=m+1 if u.webauthn_credentials.none?}
#=> 0 # yes, I was lazy to write JOIN logic in here

We should also check that there isn't any user that has mfa_level set to disabled, but has an MFA device present.

User.where(mfa_level: 'disabled').joins(:webauthn_credentials).count
#=> 0

[ericherscovich]

@ericherscovich I did some initial checks.

Specifically, we should check if there exists any user that has mfa_level somehow set while not having any MFA devices.

User.where.not(mfa_level: 'disabled').each_with_object(0) {|u,m| m=m+1 if u.webauthn_credentials.none?}
#=> 0 # yes, I was lazy to write JOIN logic in here

We should also check that there isn't any user that has mfa_level set to disabled, but has an MFA device present.

User.where(mfa_level: 'disabled').joins(:webauthn_credentials).count
#=> 0

Thank you! Could you do a more generic check that would check for TOTP as well? You should be able to check something along the lines of

1. User.where.not(mfa_level: 'disabled') and check u.no_mfa_devices?
2. User.where(mfa_level: 'disabled') and u.!no_mfa_devices?

[simi]

@ericherscovich like this?

User.where.not(mfa_level: 'disabled') and check u.no_mfa_devices?

User.where.not(mfa_level: 'disabled').each_with_object(0) {|u,m| m=m+1 if u.no_mfa_devices?}
=> 0

User.where(mfa_level: 'disabled') and u.!no_mfa_devices?

User.where(mfa_level: 'disabled').each_with_object(0) {|u,m| m=m+1 if !u.no_mfa_devices?}
=> 0

[ericherscovich]

@ericherscovich like this?

User.where.not(mfa_level: 'disabled') and check u.no_mfa_devices?

User.where.not(mfa_level: 'disabled').each_with_object(0) {|u,m| m=m+1 if u.no_mfa_devices?}
=> 0

User.where(mfa_level: 'disabled') and u.!no_mfa_devices?

User.where(mfa_level: 'disabled').each_with_object(0) {|u,m| m=m+1 if !u.no_mfa_devices?}
=> 0

Yes that looks like what I was looking for! Thank you for doing this check :D

[segiddins]

irb(main):002:0> User.where.not(mfa_level: 'disabled').find_each.count(&:no_mfa_devices?)
=> 0
irb(main):003:0> User.where(mfa_level: 'disabled').find_each.count { !_1.no_mfa_devices? }

=> 0