Include a reference to AuthInternal in MultiFactorSessionImpl. #6594
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
prameshj
requested review from
avolkovi,
lisajian,
sam-gc and
yuchenshi
as code owners
🦋 Changeset detectedLatest commit: ec9be42 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This is needed for TOTP support where the function to generate TOTP Secret (by invoking startEnrollment API) needs the Auth reference, but rather than pass in a parameter, we can derive it from the multiFactorSession that is already passed in as a param. This simplifies the API for the developer, by requiring one less paramter.
prameshj
force-pushed
the
mfasession-auth
branch
from
0f84366 to
4cc1e49
Compare
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
sam-gc
approved these changes
Sep 12, 2022
bhparijat
reviewed
Sep 13, 2022
| @@ -31,11 +32,19 @@ interface SerializedMultiFactorSession { | |||
| export class MultiFactorSessionImpl implements MultiFactorSession { | |||
| private constructor( | |||
| readonly type: MultiFactorSessionType, | |||
| readonly credential: string | |||
| readonly credential: string, | |||
| readonly auth?: AuthInternal | |||
There was a problem hiding this comment.
Wondering if this change need to be documented somewhere or any proto files need to be changed as we added a new param
There was a problem hiding this comment.
This is an implementation detail, a developer will not directly interact with this field, hence it is not documented.
There was a problem hiding this comment.
What about internal customers?
Merged
bhparijat
pushed a commit
that referenced
this pull request
Sep 23, 2022
* Include a reference to AuthInternal in MultiFactorSessionImpl. This is needed for TOTP support where the function to generate TOTP Secret (by invoking startEnrollment API) needs the Auth reference, but rather than pass in a parameter, we can derive it from the multiFactorSession that is already passed in as a param. This simplifies the API for the developer, by requiring one less paramter. * Added changeset.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is needed for TOTP support where the function to generate TOTP Secret (by invoking startEnrollment API) needs the Auth reference, but rather than pass in a parameter, we can derive it from the multiFactorSession that is already passed in as a param. This simplifies the TOTP enrollment API for the developer, by requiring one less parameter.
The usage for auth can be seen in this WIP change - mfa-totp...mfa-totp-prameshj#diff-e9ae122839df9e758f95dd35db60444dd51b73a319b02a9ef36b747dc9889fbaR81
Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:
Discussion
If not, go file an issue about this before creating a pull request to discuss.
Testing
Added new unit tests.