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.

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

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

Closed
wants to merge 39 commits into from
Closed

Conversation

pragatimodi
Copy link
Contributor

@pragatimodi pragatimodi commented May 31, 2023

Adding a TotpInfo field to UserRecord for Admin users to view a user's MFA configuration. TOTP enrollment for users is enabled via client SDKs only.
Testing was done by manually enrolling a user on TOTP using curl and creating a test app to get the corresponding user's UserRecord configuration
Manual testing output:

{
  "MultiFactor": {
    "EnrolledFactors": [
      {
        "UID": "d3dc0070-665b-42e1-b3ec-18f98af224fb",
        "DisplayName": "totp-enroll-test",
        "EnrollmentTimestamp": 1685671931000,
        "FactorID": "totp",
        "PhoneMultiFactorInfo": null,
        "TOTPMultiFactorInfo": {}
      }
    ]
  }
}

@pragatimodi pragatimodi marked this pull request as ready for review June 2, 2023 03:37
@pragatimodi pragatimodi changed the title Adding TotpInfo to userRecord feat(auth): Add TotpInfo field to UserRecord Jun 5, 2023
Copy link

@renkelvin renkelvin left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

auth/user_mgt.go Outdated Show resolved Hide resolved
auth/user_mgt.go Outdated Show resolved Hide resolved
testdata/get_user.json Outdated Show resolved Hide resolved
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

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.

auth/user_mgt.go Outdated Show resolved Hide resolved
auth/user_mgt.go Outdated Show resolved Hide resolved
auth/user_mgt.go Show resolved Hide resolved
auth/user_mgt.go Outdated Show resolved Hide resolved
@@ -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.

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.

@lahirumaramba
Copy link
Member

Please rebase this to use the dev branch as the base. Thank you!

@pragatimodi pragatimodi changed the base branch from master to dev July 12, 2023 19:27
@pragatimodi
Copy link
Contributor Author

Moving this to #573 as per offline sync with @lahirumaramba to base it off dev branch. Closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants