[ServerApp] Update feature branch Auth implementation to use the authIdToken #7944
Conversation
…from app tsconfig
revert name: undefined.
Swap undefineds.
Remove the new FirebaseServerApp from the app-types. App-types are meant only for compat, and FirebaseServerApp will not be part of compat.
Add integration tests for the automatic login of Auth users when initializing Auth with an instance of FirebaseServerApp.
DellaBitta
requested review from
lisajian,
Xiaoshouzi-gh,
renkelvin,
sam-gc and
a team
as code owners
common/api-review/app.api.md
Outdated
| @@ -119,6 +119,11 @@ export function initializeServerApp(options: FirebaseOptions | FirebaseApp, conf | |||
| // @internal (undocumented) | |||
| export function _isFirebaseApp(obj: FirebaseApp | FirebaseOptions): obj is FirebaseApp; | |||
|
|
|||
| // Warning: (ae-forgotten-export) The symbol "FirebaseServerAppImpl" needs to be exported by the entry point index.d.ts | |||
There was a problem hiding this comment.
If you're exporting this out of @firebase/app you should also export FirebaseServerAppImpl, as its typing depends on that type, otherwise you could run into some TS errors. The other alternative is to be a little looser with the typings and not do the obj is FirebaseServerAppImpl and just do boolean.
There was a problem hiding this comment.
Ah yes, I was exporting FirebaseServerAppImpl so that Auth could get the authIdToken from it. I didn't realize that it would then be part of the public API, too, but that makes total sense that it is.
I'll work to make it FirebaseServerAppImpl to say withinapp only, and figure out something else for auth.
There was a problem hiding this comment.
No, to be clear, FirebaseServerAppImpl isn't being exported at the moment, as far as I can tell, which is what api-extractor is warning you about. _isFirebaseServerAppImpl is being exported. But since the typing of _isFirebaseServerAppImpl depends on FirebaseServerAppImpl, api-extractor is complaining that you have to export that too. The way to get around it is to not use the obj is return type and return a boolean instead.
The internal.ts file isn't "public" in that it's not meant to be used be users of the SDK, it's meant to export "internal" methods used by other packages, like auth, so I think this is fine to export this function in that file (if you check dist/ after building you should see it show up in the "internal" bundle and not in the main bundle). The only problem is that there may possibly be some TS issue at some point if you don't resolve this warning, so I think the simplest way is to change the return type to boolean.
There was a problem hiding this comment.
Right, I was saying to fix the error I would have needed to export FirebaseServerAppImpl, and I didn't want to do that.
This should be fixed now. I've updated the function to simple by isFirebaseServerApp and not isFirebaseServerAppImpl. FirebasServerApp is already a public interface so we should be good to go here.
| response, | ||
| idToken | ||
| ); | ||
| await authInternal._updateCurrentUser(user); |
There was a problem hiding this comment.
Could there be a race condition here? _initializeAuth also calls another hanging promise, provider.initialize which I think calls _initializeAuthInstance which eventually tries to check for a user in persistent storage and will try to _updateCurrentUser if it finds anything?
There was a problem hiding this comment.
FirebaseServerApps cannot be created in Browser environments. Is there persistence outside of browser environments?
I did some testing and printed out the persistence array in _initializeAuthInstance and it appears to be empty.
There was a problem hiding this comment.
Whether it finds a user or not, does it eventually call _updateCurrentUser() anyway, with null if the user isn't found? Not sure if it does, but if it does, seems like that could overwrite a value you set here.
There was a problem hiding this comment.
It doesn't seem to, still I think it should have a more definitive implementation. I'll work on this topic on Friday.
There was a problem hiding this comment.
Ok, I now suppress loading users from persistence upon Auth init for FirebaseServerApps. Looks like it works properly with multiple apps not interfering with each other, etc.
In the tests, attach emulators to Auth instances created via FirebaseServerApps. In Auth initialization, wait a tick before kicking off the automatic loading of users by idToken. This should give the app enough time to attach an emulator.
DellaBitta
requested review from
egilmorez,
markarndt and
kevinthecheung
as code owners
Discussion
Add support for logging-in users with the FirebaseServerApp's authIdToken.
Testing
Local project testing client-side created users, passing idTokens to serverApps, and logging in the user. Tested with multiple users and multiple instances of FirebaseServerApps w/ Auth.
CI tests (added integration tests).
API Changes
N/A