Add App Check token to FirebaseServerApp #8651
Conversation
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (416,182 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 |
🦋 Changeset detectedLatest commit: 3352b7f The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 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 |
| constructor( | ||
| private appName_: string, |
There was a problem hiding this comment.
This value was never used.
| private appCheckProvider?: Provider<AppCheckInternalComponentName> | ||
| ) { | ||
| if (_isFirebaseServerApp(app) && app.settings.appCheckToken) { | ||
| this.serverAppAppCheckToken = app.settings.appCheckToken; | ||
| } |
There was a problem hiding this comment.
During the initialization of the Data Connect-specific AppCheckTokenProvider, check if the provided app is a FirebaseServerApp that contains an App Check token. If it does, store the token locally so getToken can return it (below).
| this.appName = app.name; | ||
| if (_isFirebaseServerApp(app) && app.settings.appCheckToken) { | ||
| this.serverAppAppCheckToken = app.settings.appCheckToken; | ||
| } |
There was a problem hiding this comment.
Like Data Connect above, check if the provided app is a FirebaseServerApp that contains an App Check token. If it does, store the token locally so getToken can return it (below).
| if (_isFirebaseServerApp(app) && app.settings.appCheckToken) { | ||
| this.serverAppAppCheckToken = app.settings.appCheckToken; | ||
| } | ||
| } |
There was a problem hiding this comment.
Like the other SDKs (above), for Firestore check if the provided app is a FirebaseServerApp that contains an App Check token. If it does, store the token locally so getToken can return it (below).
There was a problem hiding this comment.
Is there any concern of this token expiring?
There was a problem hiding this comment.
I attempted to answer this in the main thread of the PR.
| authProvider: Provider<FirebaseAuthInternalName>, | ||
| messagingProvider: Provider<MessagingInternalComponentName>, | ||
| appCheckProvider: Provider<AppCheckInternalComponentName> | ||
| ) { | ||
| if (_isFirebaseServerApp(app) && app.settings.appCheckToken) { | ||
| this.serverAppAppCheckToken = app.settings.appCheckToken; | ||
| } |
There was a problem hiding this comment.
Like the other SDKs (above), for Functions check if the provided app is a FirebaseServerApp that contains an App Check token. If it does, store the token locally so getToken can return it (below).
| @@ -262,6 +266,9 @@ export class FirebaseStorageImpl implements FirebaseStorage { | |||
| } | |||
|
|
|||
| async _getAppCheckToken(): Promise<string | null> { | |||
| if (_isFirebaseServerApp(this.app) && this.app.settings.appCheckToken) { | |||
| return this.app.settings.appCheckToken; | |||
| } | |||
There was a problem hiding this comment.
Check to see if Storage's app is a FirebaseServerApp that contains an App Check token. If it does, return the token instead of invoking App Check.
| this._apiSettings.getAppCheckToken = () => { | ||
| return Promise.resolve({ token }); | ||
| }; | ||
| } else if ((vertexAI as VertexAIService).appCheck) { |
There was a problem hiding this comment.
Similiar to the other SDKs (above) check to see if the provided instance of FirebaseApp is one of FirebaseServerApp, and if it contains an App Check token, then return it instead of invoking the App Check SDK.
This implementation differes only slightly to the other SDKs (which use Providers) becuase we already have a direct reference to the FirebaseApp instance itself, similiar to the Storage implementation.
There was a problem hiding this comment.
I don't think this PR is the time to do it, but we should probably abstract all this logic (maybe also the model name processing) out of GenerativeModel to some kind of shared function that ImagenModel can also use, will give @dlarocque a heads up.
It's covered elsewhere now that we do exp testing.
|
|
||
| await deleteApp(serverApp); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Invalid tokens are now handled by a new flow, which we test in app/src/FirebaseServerApp.test.ts, so I'm removing this test here.
DellaBitta
requested review from
sam-gc,
maneesht,
tonyjhuang and
aashishpatil-g
as code owners
| this.appName = app.name; | ||
| if (_isFirebaseServerApp(app) && app.settings.appCheckToken) { | ||
| this.serverAppAppCheckToken = app.settings.appCheckToken; | ||
| } | ||
| this.appCheck = appCheckProvider?.getImmediate({ optional: true }); | ||
| if (!this.appCheck) { | ||
| appCheckProvider?.get().then(appCheck => (this.appCheck = appCheck)); | ||
| } | ||
| } | ||
|
|
||
| getToken(forceRefresh?: boolean): Promise<AppCheckTokenResult> { |
There was a problem hiding this comment.
if forceRefresh && this.serverAppAppCheckToken should this throw or otherwise indicate that an unsupported call was made?
There was a problem hiding this comment.
Just checking: is the only time that forceRefresh is set to true for the App Check token is when the service returns an UNAUTHENTICATED error?
There was a problem hiding this comment.
It looks like, in RTDB, it will be set to true on any failed request (status !== 'ok') to the auth or app check endpoints.
There was a problem hiding this comment.
I suppose that throwing here would then be valid, as it would reduce the burden on the service on subsequent requests, if the app attempts to continue to use the defunct App Check Token. WDYT @hsubox76 ?
It's assumed that they will be created via We'll post a best practice that apps attempt to use that App Check token on the server side, and either check the validity of the token themselves, or call |
| this.appName = app.name; | ||
| if (_isFirebaseServerApp(app) && app.settings.appCheckToken) { | ||
| this.serverAppAppCheckToken = app.settings.appCheckToken; | ||
| } | ||
| this.appCheck = appCheckProvider?.getImmediate({ optional: true }); | ||
| if (!this.appCheck) { | ||
| appCheckProvider?.get().then(appCheck => (this.appCheck = appCheck)); | ||
| } | ||
| } | ||
|
|
||
| getToken(forceRefresh?: boolean): Promise<AppCheckTokenResult> { |
There was a problem hiding this comment.
It looks like, in RTDB, it will be set to true on any failed request (status !== 'ok') to the auth or app check endpoints.
| authProvider: Provider<FirebaseAuthInternalName>, | ||
| messagingProvider: Provider<MessagingInternalComponentName>, | ||
| appCheckProvider: Provider<AppCheckInternalComponentName> | ||
| ) { | ||
| if (_isFirebaseServerApp(app) && app.settings.appCheckToken) { | ||
| this.serverAppAppCheckToken = app.settings.appCheckToken; | ||
| } |
There was a problem hiding this comment.
Can't attach comment to line 85 but can we put a ? there so it's appCheckProvider?.get() like RTDB and DataConnect etc seem to have done and I just noticed in this PR, for max efficiency in not bothering to even call get() in the case of no provider.
| if ((vertexAI as VertexAIService).appCheck) { | ||
|
|
||
| if ( | ||
| vertexAI.app && |
There was a problem hiding this comment.
Can there be a vertexAI instance that doesn't have an app property? How does that happen? Actually come to think of it, what do you think about putting a ? here https://github.com/firebase/firebase-js-sdk/blob/main/packages/app/src/internal.ts#L173 so that it can handle being passed undefined/null, so it would cover if any other caller happens to pass it that.
There was a problem hiding this comment.
To answer your first question, I'm not sure. I don't think it's possible but the rest of the code (above) checks for it.
As for the latter, I'll add the ability for the function to take null / undefined!
| this._apiSettings.getAppCheckToken = () => { | ||
| return Promise.resolve({ token }); | ||
| }; | ||
| } else if ((vertexAI as VertexAIService).appCheck) { |
There was a problem hiding this comment.
I don't think this PR is the time to do it, but we should probably abstract all this logic (maybe also the model name processing) out of GenerativeModel to some kind of shared function that ImagenModel can also use, will give @dlarocque a heads up.
| @@ -42,7 +47,11 @@ export class AppCheckTokenProvider { | |||
| } | |||
| } | |||
|
|
|||
| getToken(forceRefresh?: boolean): Promise<AppCheckTokenResult> { | |||
| getToken(): Promise<AppCheckTokenResult> { | |||
There was a problem hiding this comment.
forceRefresh was never used, so I've removed it for now.
| @@ -67,6 +93,16 @@ export class FirebaseServerAppImpl | |||
| ...serverConfig | |||
| }; | |||
|
|
|||
| // Ensure that the current time is within the authIdtoken window of validity. | |||
There was a problem hiding this comment.
authIdtoken and appChecktoken look like literals that should be backticked.
packages/app/src/public-types.ts
Outdated
| @@ -196,6 +196,12 @@ export interface FirebaseServerAppSettings | |||
| */ | |||
| authIdToken?: string; | |||
|
|
|||
| /** | |||
| * An optional App Check token. If provided, the Firebase SDKs that use App Check will utilizze | |||
There was a problem hiding this comment.
Typo "utilize."
Personally I like in lieu, but I'll bet good money our style guide advises something more like "instead of" or "in place of."
There was a problem hiding this comment.
Fixed! I went with "in place of".
Thanks @egilmorez. Could you give it a review again? I fixed the problems you found and updated some other comments, so it's worth a full review again. |
.changeset/kind-pets-sin.md
Outdated
| '@firebase/auth': patch | ||
| --- | ||
|
|
||
| `FirebaseServerApp` may now be initalized with an App Check token in leu of invoking the App Check |
There was a problem hiding this comment.
Do these strings get pulled into release notes or other public docs?
If so, I'd go with "can now be initialized" and "instead of invoking."
There was a problem hiding this comment.
They don't directly. Later, the release engineer crafts the release notes using these as ... inspiration. Updated!
Discussion
Currently a customer enforces App Check for a product in the Firebase console then there's no way to successfully use that product in SSR environments. This is due to the requirement for products SDKs to internally invoke
getTokenon the a instance of the App Check SDK that the customer's app has initialized. However, the App Check SDK can only be initialized in browser environments.This PR aims to fix this. FirebaseServerApp will now accept an optional App Check token at initialization. The product SDKs will look for this token, and if it's present, the SDKs will use this value in lieu of calling
getTokenon App Check.This change affects the following SDKs:
Testing
Added tests to FirebaseServerApp's handling of the token. The product SDKs currently don't have viable App Check tests in our test suite, so I manually tested all of the SDKs by enabling App Check enforcement in the Firebase Console, watching them fail due to permissions, and then provided the App Check token to Firebase Server app and watched them succeed.
API Changes
This conforms to the previoulsy approved Existing FirebaseServerApp API proposal (internal) for
FirebaseServerAppwhich includes App Check token support.