fix(auth): Allow to update MFA #530
Conversation
|
Thanks for the fix! |
| @@ -170,14 +174,16 @@ func convertMultiFactorInfoToServerFormat(mfaInfo MultiFactorInfo) (multiFactorI | |||
| if mfaInfo.EnrollmentTimestamp != 0 { | |||
| authFactorInfo.EnrolledAt = time.Unix(mfaInfo.EnrollmentTimestamp, 0).Format("2006-01-02T15:04:05Z07:00Z") | |||
| } | |||
| if mfaInfo.UID != "" { | |||
There was a problem hiding this comment.
Since phone is the only factorId, is it ok to put this in the if block below?
Or is this to cover the case where a factorId is not specified?
There was a problem hiding this comment.
It can be moved to the block bellow of course, but why we have the EnrollmentTimestamp at the top for example?
I just thought that UID will probably exist for every "factor", because we will need it when updating something, so it's safe to move it from the block
There was a problem hiding this comment.
Good point. We will likely move it out of the if block when we add another MFA (like TOTP).
Similarly, DisplayName is a generic field too. So you could move that out too.
| @@ -333,7 +339,7 @@ func (u *UserToUpdate) validatedRequest() (map[string]interface{}, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| req["mfaInfo"] = mfaInfo | |||
| req["mfa"] = multiFactorEnrollments{mfaInfo} | |||
There was a problem hiding this comment.
nit: Can you link to the docs https://cloud.google.com/identity-platform/docs/reference/rest/v1/accounts/update for easy reference next time? Thanks!
There was a problem hiding this comment.
Super nit: Please add this link as a code comment if possible.
There was a problem hiding this comment.
This change is not showing up in the diff yet.. otherwise lgtm
There was a problem hiding this comment.
Have added the link as you suggested @prameshj . Please take a look
auth/user_mgt.go
Outdated
| default: | ||
| return nil, fmt.Errorf("unsupported methodType: %s", methodType) | ||
| } | ||
| if err := validateDisplayName(multiFactorInfo.DisplayName); err != nil { |
There was a problem hiding this comment.
Wouldn't we want to include a display name for a (new or existing) mfa option? Any downside to requiring this?
There was a problem hiding this comment.
the only downside it that it's not really required from the API point of view
There was a problem hiding this comment.
I think it would be ok to require it on the SDK, since this field helps identify the MFA option easily. @lahirumaramba any thoughts?
| @@ -894,17 +873,6 @@ func TestInvalidUpdateUser(t *testing.T) { | |||
| }, | |||
| }), | |||
| `the second factor "phoneNumber" for "invalid" must be a non-empty E.164 standard compliant identifier string`, | |||
| }, { | |||
There was a problem hiding this comment.
nit: I would prefer to keep the test cases related to display name field being required.
|
Thanks for finding this fix! |
|
Thanks! I have one final nit, LGTM otherwise. @lahirumaramba for final approval/merge. |
|
can this be merged @lahirumaramba ? |
|
Hi team, can you please take a look at the PR. I have made the changes as requested. @prameshj @lahirumaramba |
|
Hello team, can we please move forward with this PR? Let me know if there is anything I should do |
|
Thanks for checking, Ivan! The changes look good to me. I think we only need @lahirumaramba to approve. Lahiru, can you help with this? Thanks! |
|
Thank you for the update @prameshj |
|
Are my proposed changes outdated and do not comply with expected release? Should I create a new PR and revisit what was done? Please give at least some direction @prameshj @pragatimodi @lahirumaramba , thanks. |
|
Thanks folks! We will include this in the next release. |
UpdateUsercan update MFAUIDis not required when updating MFADisplayNameis not required fieldFixes #529.
RELEASE NOTE: Allow updating multi factor enrollments in
UpdateUser()API