Added Firebase App Check support to Firebase Auth. #7191
Conversation
🦋 Changeset detectedLatest commit: 2338348 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 |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (235,243 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
| config | ||
| ); | ||
| _initializeAuthInstance(authInstance, deps); | ||
| _assert( |
There was a problem hiding this comment.
We were previously wrapping it as an anonymous function and invoking it right away, now we are just invoking it without that wrapper, correct?
Adding @sam-gc to look at this section, since we had some past discussion about whether initializeAuthInstance needs to be invoked here.
There was a problem hiding this comment.
Yep, just invoking without that wrapper - no changes have been made since https://github.com/firebase/firebase-js-sdk/pull/6982/files#r1089404274. _initializeAuthInstance() is called on line 95
There was a problem hiding this comment.
Yeah I think this is fine to do. It was unnecessarily complicated creating a throwaway anonymous function that's immediately called.
| return `${getHandlerBase(auth)}?${querystring(paramsDict).slice(1)}`; | ||
|
|
||
| // Sets the App Check token to pass to the widget | ||
| const appCheckToken = await auth._getAppCheckToken(); |
There was a problem hiding this comment.
What will happen if _getAppCheckToken() failed? Should we return a url without an app check token, or throw an exception?
There was a problem hiding this comment.
_getAppCheckToken() will generally return a token (either a real one or a dummy token, for more info see the comment in auth_impl.ts) and we will always pass that real / dummy token to the widget. If there's an error, we simply log it client side, and when the backend receives a dummy token, it would prevent the request from succeeding if App Check is enforced.
| config | ||
| ); | ||
| _initializeAuthInstance(authInstance, deps); | ||
| _assert( |
There was a problem hiding this comment.
Yeah I think this is fine to do. It was unnecessarily complicated creating a throwaway anonymous function that's immediately called.
| expect(matches).to.be.null; | ||
| }); | ||
|
|
||
| it('does not add the App Check token in the url fragment if controller unavailable', async () => { |
There was a problem hiding this comment.
Have you run yarn lint:fix ? (Or maybe it's yarn format?) to auto-wrap this
There was a problem hiding this comment.
Hmm, the linter isn't marking this as an issue (neither prettify nor yarn lint:fix)? I'll leave it as is for now
* Add App Check token to headers of Auth requests * Add App Check token to widget url fragment
lisajian
force-pushed
the
lj-appcheck-feature
branch
from
15badef to
2338348
Compare
Discussion
As the title implies
Internal tracking bug: b/265453815
Testing
Unit tests pass and manually tested endpoints as part of go/gcip-app-check-bug-bash
API Changes
N/A