Merge App Check feature branch #4833
Conversation
hsubox76
requested review from
Feiyang1,
hiranya911,
jsdt and
schmidt-sebastian
as code owners
|
Binary Size ReportAffected SDKs
Test Logs |
| @@ -154,6 +166,12 @@ export class WebSocketConnection implements Transport { | |||
| } | |||
| }; | |||
|
|
|||
| if (this.nodeAdmin) { | |||
| options.headers['Authorization'] = this.authToken || ''; | |||
There was a problem hiding this comment.
Do we not need the Authorization header if it's the web client?
There was a problem hiding this comment.
If I am reading correctly, this entire thing is inside the if (isNodeSdk()) {} block starting on line 159 and this.nodeAdmin means it's the Admin SDK and not the client Node SDK. Which also means, in light of the comment above about App Check not working in Node, maybe the appcheck header doesn't need to be added in this block at all.
There was a problem hiding this comment.
Do we not need the header for the client Node SDK?
Should we send the dummy token for AppCheck in Nodejs?
There was a problem hiding this comment.
Will address this in the follow-up PR that also includes Node debug token support.
packages/firebase/app-check/index.ts
Outdated
| * limitations under the License. | ||
| */ | ||
|
|
||
| import '@firebase/app-check'; |
There was a problem hiding this comment.
We should not add AppCheck to the firebase package until we are ready to release.
There was a problem hiding this comment.
nvm, I just realized you have another PR to address this.
There was a problem hiding this comment.
I guess CI won't pass if this isn't taken care of so will address in this PR.
| @@ -104,7 +105,13 @@ export function sharedErrorHandler( | |||
| ): FirebaseStorageError { | |||
| let newErr; | |||
| if (xhr.getStatus() === 401) { | |||
| newErr = unauthenticated(); | |||
| if ( | |||
| xhr.getResponseText().includes('Firebase App Check token is invalid') | |||
There was a problem hiding this comment.
Is there a code that we can check against?
There was a problem hiding this comment.
We could make this string into a local constant but it would have to be manually kept in sync. This is where the string comes from in the backend, we don't have any access to it here. The status code is 401 which is shared with some other errors. https://source.corp.google.com/piper///depot/google3/java/com/google/firebase/storage/authorization/appcheck/AppCheckAuthorizer.java;rcl=369703237;l=26
There was a problem hiding this comment.
I was hoping server sends back a json object with a code property that we can check against. The error text is subject to change in nature, so I'm worried we will get out of sync at one point.
There was a problem hiding this comment.
It doesn't seem like anything consistent is sent back except the error code and that string https://source.corp.google.com/piper///depot/google3/java/com/google/firebase/storage/authorization/appcheck/AppCheckAuthorizer.java;rcl=370565192;l=78
There was a problem hiding this comment.
Hm. Can you add a comment to the above linked file to state that the text of the error message cannot be changed?
Size Analysis Report |
packages/database/exp/register.ts
Outdated
| const appCheckProvider = | ||
| variant === 'node' | ||
| ? undefined | ||
| : container.getProvider('app-check-internal'); |
There was a problem hiding this comment.
I think in practice it doesn't make any difference because appCheckProvider.get() will not resolve in Nodejs and database will just work without AppCheck. And since it is a shared code, I would actually prefer to not have the special handling for nodejs.
There was a problem hiding this comment.
I would also prefer that we simplify this again and either call container.getProvider('app-check') directly and rely on the fact that Node will return undefined here or pass a "isNode" boolean into registerDatabase (with different values from index.ts and index.node.ts).
The question is though if the debug AppCheck will ever work in Node. If that is the case then we should just let container decide if AppCheck is available.
There was a problem hiding this comment.
Reverted to simple container.getProvider.
There was a problem hiding this comment.
Debug AppCheck can work in Node, we just need to read from globalThis ( need a polyfill) instead of self. I think it'll be useful for server side rendering, though it sounds like an abuse of the debug token.
Also custom providers should already be working in Nodejs.
Therefore, I think we should
- Read debug token from
globalThis. We need to define a polyfill forglobalThis, probably in@firebase/util. Here is a reference implementation. - Always pass the AppCheckProvider database
There was a problem hiding this comment.
Ok, will make a separate PR for Node support after merge. Reverting other Node-related change above.
hsubox76
force-pushed
the
ch-appcheck-feature
branch
from
0568ba5 to
b15a4ee
Compare
Merge change that adds App Check package as well as App Check enabled versions of functions, storage, and database.