Fix getMFAOptions data extraction

[sjawhar]

Description of changes:
cognitoUser.getMFAOptions() is currently broken. It always returns undefined, because userData.MFAOptions is not defined. Instead, there are two keys in userData containing MFA info: UserMFASettingList and PreferredMfaSetting. So we should return those instead.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[haverchuck]

@sjawhar - Currently reviewing and testing this.

[haverchuck]

@sjawhar - Since the name of the function you've updated is getMFAOptions, I think it makes sense that it returns the MFAOptions value. What are your thoughts on creating a similar function called getMFASettings to return the values that your PR currently adds to getMFAOptions? If you do not want to make this update it's ok - we can take it on.

Thanks again for submitting this PR and apologies for the delayed response.

[sjawhar]

The problem is that MFAOptions is undefined, or at least was when I opened the pull request. There was no MFAOptions attribute of userOptions

[haverchuck]

@sjawhar Yes - MFAOptions will only be defined if SMS MFA settings are enabled. Getting the setting variables that you are using in the PR is a fine solution for more comprehensive data; however, I think it would be good to retrieve these in a seperate function. There may be some developers who are using MFAOptions in scenarios where it does bring back data, and suddently giving them additional properties in the response is less desirable than providing a seperate function for developers to use going forward (IMO).

[sjawhar]

@haverchuck I just ran through and tested it real quick. Tried when I have no MFA, when I have SMS, and when I have TOTP. userData.MFAOptions was undefined in all three cases. Can you help me understand what I'm missing? Using version 3.0.12

[haverchuck]

@sjawhar I'm contacting the service team with this for additional info, please stay tuned.

[mlabieniec]

Just FYI on this the cognito team has an update that they will be pushing out to address this asap, will keep the thread posted once the update is complete!

[rachitdhall]

We have identified an issue in Amazon Cognito where the Admin/GetUser service response doesn’t correctly populate MFAOptions when on a pool with MFA required and the end user uses SMS MFA. We are actively working on the fix and will circle back when the service update is completed.

Rachit,
Development Manager, Amazon Cognito

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[davidgatti]

LOL :D new technique to pretend Issues are being solved? Love It! :D

[codecov]

Codecov Report

Merging #2707 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2707   +/-   ##
=======================================
  Coverage   73.74%   73.74%           
=======================================
  Files         203      203           
  Lines       11956    11956           
  Branches     2249     2335   +86     
=======================================
  Hits         8817     8817           
+ Misses       2977     2957   -20     
- Partials      162      182   +20     

Impacted Files	Coverage Δ
packages/auth/src/OAuth/OAuth.ts	48.12% <0.00%> (ø)
packages/core/src/Credentials.ts	31.48% <0.00%> (ø)
packages/analytics/src/Analytics.ts	64.81% <0.00%> (ø)
packages/datastore/src/sync/outbox.ts	25.00% <0.00%> (ø)
packages/datastore/src/storage/storage.ts	67.59% <0.00%> (ø)
packages/core/src/OAuthHelper/GoogleOAuth.ts	32.65% <0.00%> (ø)
packages/core/src/Util/Reachability.native.ts	37.50% <0.00%> (ø)
packages/xr/src/Providers/SumerianProvider.ts	47.55% <0.00%> (ø)
packages/core/src/OAuthHelper/FacebookOAuth.ts	35.55% <0.00%> (ø)
packages/datastore/src/sync/processors/sync.ts	17.33% <0.00%> (ø)
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9019acf...4441c46. Read the comment docs.

[svidgen]

I'm touching base with @rachitdhall internally. The Cognito service is still behaving as described in the OP from what I can tell. But, I'm not actually clear as to what the correct behavior is there.

I'll also need to base with @haverchuck, I think. I'm honestly not 100% sure how getMFAOptions() is intended to be use. So, I more background to provide guidance on this PR.

[svidgen]

As it turns out, MFAOptionType is no longer supported at all. So, this method is being deprecated in the client library, and I'll be closing this particular PR.

You could always use user.getUserData() directly. But, I'm not actually clear on what the intended use-case is here. So, I'd suggest checking the MFA docs for the recommended MFA setup and filing a separate issue if something is missing or unclear in those docs.

[github-actions]

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.