Extends createUser to support multi-factor user creation.#718
Extends createUser to support multi-factor user creation.#718bojeil-google merged 3 commits intoauth-mfafrom
Conversation
Note that only phoneNumber and displayName are allowed to be passed in this operation. Adds relevant unit and integration tests. This capability is only available in staging.
hiranya911
left a comment
hiranya911
left a comment
There was a problem hiding this comment.
Looks pretty good. Just a few nits to improve on.
src/auth/auth-api-request.ts
Outdated
| } catch (e) { | ||
| return Promise.reject(e); | ||
| } | ||
| if (request.mfaInfo.length === 0) { |
There was a problem hiding this comment.
Nit: To keep the mutations to request to a minimum, you can do this:
const mfaInfo = [];
// Populate mfaInfo
if (request.mfaInfo.length > 0) {
// You can drop the if condition, if you checked for isNonEmptyArray earlier.
request.mfaInfo = mfaInfo;
}
src/auth/auth-api-request.ts
Outdated
| } | ||
| // Construct mfa related user data. | ||
| if (validator.isNonNullObject(request.multiFactor)) { | ||
| if (validator.isArray(request.multiFactor.enrolledFactors)) { |
There was a problem hiding this comment.
isNonEmptyArray makes more sense here.
test/integration/auth.spec.ts
Outdated
| // Confirm second factors added to user. | ||
| expect(userRecord.multiFactor.enrolledFactors.length).to.equal(2); | ||
| // Confirm first enrolled second factor. | ||
| expect(userRecord.multiFactor.enrolledFactors[0].uid).not.to.be.undefined; |
There was a problem hiding this comment.
const firstFactor = enrolledFactors[0];
There was a problem hiding this comment.
I set firstMultiFactor to userRecord.multiFactor.enrolledFactors[0]. That improves readability.
test/integration/auth.spec.ts
Outdated
| expect(userRecord.multiFactor.enrolledFactors[0].factorId) | ||
| .to.equal(enrolledFactors[0].factorId); | ||
| // Confirm second enrolled second factor. | ||
| expect(userRecord.multiFactor.enrolledFactors[1].uid).not.to.be.undefined; |
There was a problem hiding this comment.
const secondFactor = enrolledFactors[1];
There was a problem hiding this comment.
I set secondMultiFactor to userRecord.multiFactor.enrolledFactors[1]. It is easier to read this way.
| }); | ||
| }); | ||
|
|
||
| it('should be fulfilled given empty enrolled factors array', () => { |
There was a problem hiding this comment.
This seems identical to the previous test case. Can we consolidate with an array?
const noEnrolledFactors = [ [], null ];
noEnrolledFactors.forEach((arg) => {
...
});
| const unsupportedSecondFactor = { | ||
| secret: 'SECRET', | ||
| displayName: 'Google Authenticator on personal phone', | ||
| factorId: 'totp', |
There was a problem hiding this comment.
Add a comment here indicating totp is not yet supported.
|
Thanks for the quick review! |
2 participants
Extends createUser to support multi-factor user creation.
Note that only phoneNumber and displayName are allowed to be passed in this operation.
Adds relevant unit and integration tests.
This capability is only available in staging.