Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(auth): Add TotpInfo field to UserRecord #558

Closed
wants to merge 39 commits into from
Closed

feat(auth): Add TotpInfo field to UserRecord #558

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
54b8114
Merge dev into master
google-oss-bot May 21, 2020
cef91ac
Merge dev into master
google-oss-bot Jun 16, 2020
77177c7
Merge dev into master
google-oss-bot Oct 22, 2020
a957589
Merge dev into master
google-oss-bot Jan 28, 2021
eb0d2a0
Merge dev into master
google-oss-bot Mar 24, 2021
05378ef
Merge dev into master
google-oss-bot Mar 29, 2021
4121c50
Merge dev into master
google-oss-bot Apr 14, 2021
928b104
Merge dev into master
google-oss-bot Jun 2, 2021
02cde4f
Merge dev into master
google-oss-bot Nov 4, 2021
6b40682
Merge dev into master
google-oss-bot Dec 15, 2021
e60757f
Merge dev into master
google-oss-bot Jan 20, 2022
bb055ed
Merge dev into master
google-oss-bot Apr 6, 2022
23a1f17
Merge dev into master
google-oss-bot Oct 6, 2022
1d24577
Merge dev into master
google-oss-bot Nov 10, 2022
61c6c04
Merge dev into master
google-oss-bot Apr 6, 2023
3f140e0
Adding TotpInfo to userRecord
pragatimodi May 31, 2023
e178eb3
Add MFA fixes
pragatimodi Jun 1, 2023
8c41cc7
Addressing feedback
pragatimodi Jun 5, 2023
4c5e08f
Adding struct "`TOTPMultiFactorInfo`" as per review discussion.
pragatimodi Jun 6, 2023
d29e057
Addressing PR feedback
pragatimodi Jun 22, 2023
6921afb
Addressing PR feedback
pragatimodi Jun 22, 2023
ce0bac0
adding `deprecated` tag to `PhoneNumber` field
pragatimodi Jul 21, 2023
79321fe
Merge branch 'totp-user-record' of https://github.com/firebase/fireba…
pragatimodi Jul 21, 2023
b756b71
Merge branch 'dev' into totp-user-record
pragatimodi Jul 21, 2023
939d6c0
Keep `PhoneNumber` field in `MultiFactorInfo` for compatibility
pragatimodi Jul 24, 2023
09d8eaa
Merge branch 'totp-user-record' of https://github.com/firebase/fireba…
pragatimodi Jul 24, 2023
f448782
Validation logic fix
pragatimodi Jul 24, 2023
336ca6d
lint fix
pragatimodi Jul 24, 2023
587f0df
Adding TotpInfo to userRecord
pragatimodi May 31, 2023
34b2f98
Add MFA fixes
pragatimodi Jun 1, 2023
3535bbd
Addressing feedback
pragatimodi Jun 5, 2023
e13f10c
Adding struct "`TOTPMultiFactorInfo`" as per review discussion.
pragatimodi Jun 6, 2023
b1a5ac8
Addressing PR feedback
pragatimodi Jun 22, 2023
7495d4f
adding `deprecated` tag to `PhoneNumber` field
pragatimodi Jul 21, 2023
b4a93cf
Addressing PR feedback
pragatimodi Jun 22, 2023
afce475
Keep `PhoneNumber` field in `MultiFactorInfo` for compatibility
pragatimodi Jul 24, 2023
afdd057
Validation logic fix
pragatimodi Jul 24, 2023
b9aa6d6
lint fix
pragatimodi Jul 24, 2023
939a446
Merge branch 'totp-user-record' of https://github.com/firebase/fireba…
pragatimodi Jul 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 64 additions & 30 deletions auth/user_mgt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ const (
createUserMethod = "createUser"
updateUserMethod = "updateUser"
phoneMultiFactorID = "phone"
totpMultiFactorID = "totp"
)

// 'REDACTED', encoded as a base64 string.
Expand All @@ -62,20 +63,36 @@ type UserInfo struct {

// multiFactorInfoResponse describes the `mfaInfo` of the user record API response
type multiFactorInfoResponse struct {
MFAEnrollmentID string `json:"mfaEnrollmentId,omitempty"`
DisplayName string `json:"displayName,omitempty"`
PhoneInfo string `json:"phoneInfo,omitempty"`
EnrolledAt string `json:"enrolledAt,omitempty"`
MFAEnrollmentID string `json:"mfaEnrollmentId,omitempty"`
DisplayName string `json:"displayName,omitempty"`
PhoneInfo string `json:"phoneInfo,omitempty"`
TOTPInfo *TOTPInfo `json:"totpInfo,omitempty"`
EnrolledAt string `json:"enrolledAt,omitempty"`
}

// TOTPInfo describes a server side user enrolled second totp factor.
type TOTPInfo struct{}

// PhoneMultiFactorInfo describes a user enrolled second phone factor.
type PhoneMultiFactorInfo struct {
PhoneNumber string
}

// TOTPMultiFactorInfo describes a user enrolled second totp factor.
type TOTPMultiFactorInfo struct{}

type multiFactorEnrollments struct {
Enrollments []*multiFactorInfoResponse `json:"enrollments"`
}

// MultiFactorInfo describes a user enrolled second phone factor.
// TODO : convert PhoneNumber to PhoneMultiFactorInfo struct
type MultiFactorInfo struct {
UID string
DisplayName string
EnrollmentTimestamp int64
FactorID string
PhoneNumber string
UID string
DisplayName string
EnrollmentTimestamp int64
FactorID string
PhoneMultiFactorInfo *PhoneMultiFactorInfo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a user-facing field? If yes, we need an API review for it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we cannot merge this until the API change is approved, right? Can we keep PhoneNumber for now and merge the rest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on the API proposal for this.

TOTPMultiFactorInfo *TOTPMultiFactorInfo
}

// MultiFactorSettings describes the multi-factor related user settings.
Expand Down Expand Up @@ -166,18 +183,27 @@ func (u *UserToCreate) set(key string, value interface{}) *UserToCreate {

// Converts a client format second factor object to server format.
func convertMultiFactorInfoToServerFormat(mfaInfo MultiFactorInfo) (multiFactorInfoResponse, error) {
var authFactorInfo multiFactorInfoResponse
authFactorInfo := multiFactorInfoResponse{DisplayName: mfaInfo.DisplayName}
if mfaInfo.EnrollmentTimestamp != 0 {
authFactorInfo.EnrolledAt = time.Unix(mfaInfo.EnrollmentTimestamp, 0).Format("2006-01-02T15:04:05Z07:00Z")
}
if mfaInfo.FactorID == phoneMultiFactorID {
authFactorInfo.PhoneInfo = mfaInfo.PhoneNumber
authFactorInfo.DisplayName = mfaInfo.DisplayName
if mfaInfo.UID != "" {
authFactorInfo.MFAEnrollmentID = mfaInfo.UID
}
authFactorInfo.DisplayName = mfaInfo.DisplayName
pragatimodi marked this conversation as resolved.
Show resolved Hide resolved
authFactorInfo.MFAEnrollmentID = mfaInfo.UID

switch mfaInfo.FactorID {
case phoneMultiFactorID:
authFactorInfo.PhoneInfo = mfaInfo.PhoneMultiFactorInfo.PhoneNumber
return authFactorInfo, nil
case totpMultiFactorID:
authFactorInfo.TOTPInfo = (*TOTPInfo)(mfaInfo.TOTPMultiFactorInfo)
return authFactorInfo, nil
}

out, _ := json.Marshal(mfaInfo)
return multiFactorInfoResponse{}, fmt.Errorf("Unsupported second factor %s provided", string(out))
return multiFactorInfoResponse{}, fmt.Errorf("unsupported second factor %s provided", string(out))
pragatimodi marked this conversation as resolved.
Show resolved Hide resolved
}

func (u *UserToCreate) validatedRequest() (map[string]interface{}, error) {
Expand Down Expand Up @@ -333,7 +359,8 @@ func (u *UserToUpdate) validatedRequest() (map[string]interface{}, error) {
if err != nil {
return nil, err
}
req["mfaInfo"] = mfaInfo
// Request body ref: https://cloud.google.com/identity-platform/docs/reference/rest/v1/accounts/update
req["mfa"] = multiFactorEnrollments{mfaInfo}
} else {
req[k] = v
}
Expand Down Expand Up @@ -665,18 +692,15 @@ func validateAndFormatMfaSettings(mfaSettings MultiFactorSettings, methodType st
return nil, fmt.Errorf("\"uid\" is not supported when adding second factors via \"createUser()\"")
}
case updateUserMethod:
if multiFactorInfo.UID == "" {
pragatimodi marked this conversation as resolved.
Show resolved Hide resolved
return nil, fmt.Errorf("the second factor \"uid\" must be a valid non-empty string when adding second factors via \"updateUser()\"")
}
default:
return nil, fmt.Errorf("unsupported methodType: %s", methodType)
}
if err := validateDisplayName(multiFactorInfo.DisplayName); err != nil {
return nil, fmt.Errorf("the second factor \"displayName\" for \"%s\" must be a valid non-empty string", multiFactorInfo.DisplayName)
}
if multiFactorInfo.FactorID == phoneMultiFactorID {
if err := validatePhone(multiFactorInfo.PhoneNumber); err != nil {
return nil, fmt.Errorf("the second factor \"phoneNumber\" for \"%s\" must be a non-empty E.164 standard compliant identifier string", multiFactorInfo.PhoneNumber)
if err := validatePhone(multiFactorInfo.PhoneMultiFactorInfo.PhoneNumber); err != nil {
pragatimodi marked this conversation as resolved.
Show resolved Hide resolved
return nil, fmt.Errorf("the second factor \"phoneNumber\" for \"%s\" must be a non-empty E.164 standard compliant identifier string", multiFactorInfo.PhoneMultiFactorInfo.PhoneNumber)
}
}
obj, err := convertMultiFactorInfoToServerFormat(*multiFactorInfo)
Expand Down Expand Up @@ -1075,17 +1099,27 @@ func (r *userQueryResponse) makeExportedUserRecord() (*ExportedUserRecord, error
enrollmentTimestamp = t.Unix() * 1000
}

if factor.PhoneInfo == "" {
if factor.PhoneInfo != "" {
enrolledFactors = append(enrolledFactors, &MultiFactorInfo{
UID: factor.MFAEnrollmentID,
DisplayName: factor.DisplayName,
EnrollmentTimestamp: enrollmentTimestamp,
FactorID: phoneMultiFactorID,
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: factor.PhoneInfo,
},
})
} else if factor.TOTPInfo != nil {
enrolledFactors = append(enrolledFactors, &MultiFactorInfo{
UID: factor.MFAEnrollmentID,
DisplayName: factor.DisplayName,
EnrollmentTimestamp: enrollmentTimestamp,
FactorID: totpMultiFactorID,
TOTPMultiFactorInfo: &TOTPMultiFactorInfo{},
})
} else {
return nil, fmt.Errorf("unsupported multi-factor auth response: %#v", factor)
}

enrolledFactors = append(enrolledFactors, &MultiFactorInfo{
UID: factor.MFAEnrollmentID,
DisplayName: factor.DisplayName,
EnrollmentTimestamp: enrollmentTimestamp,
FactorID: phoneMultiFactorID,
PhoneNumber: factor.PhoneInfo,
})
}

return &ExportedUserRecord{
Expand Down
112 changes: 75 additions & 37 deletions auth/user_mgt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,20 @@ var testUser = &UserRecord{
MultiFactor: &MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
UID: "0aaded3f-5e73-461d-aef9-37b48e3769be",
UID: "enrolledPhoneFactor",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we keep the previous UID which is more representative of what the UID looks like?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer it this way since we can test UIDs with integration tests but for more clarity as we add unit tests this might be better. What do you think? We can revert back to the previous UID and add comments if you feel strongly about this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we can test UIDs with integration tests but for more clarity as we add unit tests this might be better

I am not sure what the human-readable UID gives us that the other alphanumeric value does not. Can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like you said, human-readable. It is just easier to write unit test cases as we add more factors here. I guess it doesn't make a lot of difference to have alphanumeric strings for unit tests as such because these are generated by the backend in the actual flow.

FactorID: "phone",
EnrollmentTimestamp: 1614776780000,
PhoneNumber: "+1234567890",
DisplayName: "My MFA Phone",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+1234567890",
},
DisplayName: "My MFA Phone",
},
{
UID: "enrolledTOTPFactor",
FactorID: "totp",
EnrollmentTimestamp: 1614776780000,
TOTPMultiFactorInfo: &TOTPMultiFactorInfo{},
DisplayName: "My MFA TOTP",
},
},
},
Expand Down Expand Up @@ -646,8 +655,10 @@ func TestInvalidCreateUser(t *testing.T) {
(&UserToCreate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
UID: "EnrollmentID",
PhoneNumber: "+11234567890",
UID: "EnrollmentID",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
DisplayName: "Spouse's phone number",
FactorID: "phone",
},
Expand All @@ -658,7 +669,9 @@ func TestInvalidCreateUser(t *testing.T) {
(&UserToCreate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
PhoneNumber: "invalid",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "invalid",
},
DisplayName: "Spouse's phone number",
FactorID: "phone",
},
Expand All @@ -669,7 +682,9 @@ func TestInvalidCreateUser(t *testing.T) {
(&UserToCreate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
PhoneNumber: "+11234567890",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
DisplayName: "Spouse's phone number",
FactorID: "phone",
EnrollmentTimestamp: time.Now().UTC().Unix(),
Expand All @@ -681,7 +696,9 @@ func TestInvalidCreateUser(t *testing.T) {
(&UserToCreate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
PhoneNumber: "+11234567890",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
DisplayName: "Spouse's phone number",
FactorID: "",
},
Expand All @@ -692,8 +709,10 @@ func TestInvalidCreateUser(t *testing.T) {
(&UserToCreate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
PhoneNumber: "+11234567890",
FactorID: "phone",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
FactorID: "phone",
},
},
}),
Expand Down Expand Up @@ -773,7 +792,9 @@ var createUserCases = []struct {
(&UserToCreate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
PhoneNumber: "+11234567890",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
DisplayName: "Spouse's phone number",
FactorID: "phone",
},
Expand All @@ -790,12 +811,16 @@ var createUserCases = []struct {
(&UserToCreate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
PhoneNumber: "+11234567890",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
DisplayName: "number1",
FactorID: "phone",
},
{
PhoneNumber: "+11234567890",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
DisplayName: "number2",
FactorID: "phone",
},
Expand Down Expand Up @@ -875,9 +900,11 @@ func TestInvalidUpdateUser(t *testing.T) {
(&UserToUpdate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
UID: "enrolledSecondFactor1",
PhoneNumber: "+11234567890",
FactorID: "phone",
UID: "enrolledSecondFactor1",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
FactorID: "phone",
},
},
}),
Expand All @@ -886,25 +913,16 @@ func TestInvalidUpdateUser(t *testing.T) {
(&UserToUpdate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
UID: "enrolledSecondFactor1",
PhoneNumber: "invalid",
UID: "enrolledSecondFactor1",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "invalid",
},
DisplayName: "Spouse's phone number",
FactorID: "phone",
},
},
}),
`the second factor "phoneNumber" for "invalid" must be a non-empty E.164 standard compliant identifier string`,
}, {
(&UserToUpdate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
PhoneNumber: "+11234567890",
FactorID: "phone",
DisplayName: "Spouse's phone number",
},
},
}),
`the second factor "uid" must be a valid non-empty string when adding second factors via "updateUser()"`,
}, {
(&UserToUpdate{}).ProviderToLink(&UserProvider{UID: "google_uid"}),
"user provider must specify a provider ID",
Expand Down Expand Up @@ -1049,20 +1067,30 @@ var updateUserCases = []struct {
(&UserToUpdate{}).MFASettings(MultiFactorSettings{
EnrolledFactors: []*MultiFactorInfo{
{
UID: "enrolledSecondFactor1",
PhoneNumber: "+11234567890",
UID: "enrolledSecondFactor1",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
DisplayName: "Spouse's phone number",
FactorID: "phone",
EnrollmentTimestamp: time.Now().Unix(),
}, {
UID: "enrolledSecondFactor2",
PhoneNumber: "+11234567890",
UID: "enrolledSecondFactor2",
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
DisplayName: "Spouse's phone number",
FactorID: "phone",
}, {
PhoneMultiFactorInfo: &PhoneMultiFactorInfo{
PhoneNumber: "+11234567890",
},
DisplayName: "Spouse's phone number",
FactorID: "phone",
},
},
}),
map[string]interface{}{"mfaInfo": []*multiFactorInfoResponse{
map[string]interface{}{"mfa": multiFactorEnrollments{Enrollments: []*multiFactorInfoResponse{
{
MFAEnrollmentID: "enrolledSecondFactor1",
PhoneInfo: "+11234567890",
Expand All @@ -1074,12 +1102,16 @@ var updateUserCases = []struct {
DisplayName: "Spouse's phone number",
PhoneInfo: "+11234567890",
},
},
{
DisplayName: "Spouse's phone number",
PhoneInfo: "+11234567890",
},
}},
},
},
{
(&UserToUpdate{}).MFASettings(MultiFactorSettings{}),
map[string]interface{}{"mfaInfo": nil},
map[string]interface{}{"mfa": multiFactorEnrollments{Enrollments: nil}},
},
{
(&UserToUpdate{}).ProviderToLink(&UserProvider{
Expand Down Expand Up @@ -1886,10 +1918,16 @@ func TestMakeExportedUser(t *testing.T) {
MFAInfo: []*multiFactorInfoResponse{
{
PhoneInfo: "+1234567890",
MFAEnrollmentID: "0aaded3f-5e73-461d-aef9-37b48e3769be",
MFAEnrollmentID: "enrolledPhoneFactor",
DisplayName: "My MFA Phone",
EnrolledAt: "2021-03-03T13:06:20.542896Z",
},
{
TOTPInfo: &TOTPInfo{},
MFAEnrollmentID: "enrolledTOTPFactor",
DisplayName: "My MFA TOTP",
EnrolledAt: "2021-03-03T13:06:20.542896Z",
},
},
}

Expand Down
Loading