PER-9943-cant-delete-sms-mfa #503
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #503 +/- ##
==========================================
+ Coverage 43.19% 43.28% +0.09%
==========================================
Files 363 363
Lines 11104 11123 +19
Branches 1810 1818 +8
==========================================
+ Hits 4796 4815 +19
- Misses 6146 6148 +2
+ Partials 162 160 -2 ☔ View full report in Codecov by Sentry. |
| const match = numbers.match(/^(\d{1,3})/); | ||
| if (match) { | ||
| countryCode = `+${match[1]}`; | ||
| numbers = numbers.substring(match[1].length); | ||
| } |
There was a problem hiding this comment.
Can you add coverage for these paths of code? The other ones have examples provided in the tests and it would be nice to see that spelled out for this branch as well.
|
@meisekimiu you are right, test added :) |
There was a problem hiding this comment.
A few things:
- @meisekimiu I'm not entirely sure how to test the actual bug for this, because I no longer have an account set up with a SMS 2FA number with the country code. I'm also not able to set that up directly in the webapp because we don't allow country codes. Is there a way to do this in FusionAuth, that you're aware of?
- @crisnicandrei I did notice that when I try to add a new SMS 2FA method, I enter my phone number but the button to submit the form to request a code requires 13 characters before it becomes active. It should require only 10 numerical characters. This appears to be a new change as this is not the case on prod. This might solve the issue to point 1 above, though, if I am able to submit a phone number here that looks like #-###-###-####.
|
@k8lyn6 I did not understand the last part. Should the button become active after 10 characters? |
|
@crisnicandrei yes, US numbers are ten digits long (in the format XXX-XXX-XXXX). I'm not sure if you adjusted the form to allow for a few extra numbers, which I think is okay to solve the bug reported, but it should also be able to accept 10 digits. Sorry for the confusion! |
|
@k8lyn6 It now allows 10 digit long numbers :) |
I'm not sure, but you might be able to override that as an admin in FusionAuth dev. |
There was a problem hiding this comment.
The bug itself is proving challenging to QA test, because I'm not able to get my account into that state anymore and it's difficult to search for an account currently in that state. That being said, 2FA functionality seems to work as expected on this pull request, so I'm going to approve this.
Handle 11 character phone numbers(with country code) in SMS MFA setup
crisnicandrei
force-pushed
the
PER-9943-cant-delete-sms-mfa
branch
from
186a7a1 to
241e1fb
Compare
Handle 11 character phone numbers(with country code) in SMS MFA setup
Steps to test: