-
Notifications
You must be signed in to change notification settings - Fork 915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ServerApp] Update feature branch Auth implementation to use the authIdToken #7944
Changes from 43 commits
1216622
2efe043
fda3060
5fa9c21
0ef3bb2
8cd1072
d42f676
612f990
76f2fe1
d68f986
09e3157
3750aa4
316309c
6cc428b
53c15e2
22b9bd9
3c39866
d5e461e
a7d111a
0427934
db9b941
e146815
e9f28ed
a13f418
8a745d1
7b02583
d86eeb7
b34b878
4aeb9a6
04af931
968e176
b5a84b0
826a3c4
e2a0f9a
a6bdf3e
c323811
e8151d6
8e6ab2d
1980c7b
6543206
29fa942
bb9ba8a
940ec40
8bf5286
3b066fa
020687b
63cf1fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,15 +15,21 @@ | |
* limitations under the License. | ||
*/ | ||
|
||
import { _getProvider, FirebaseApp } from '@firebase/app'; | ||
import { | ||
_getProvider, | ||
_isFirebaseServerAppImpl, | ||
FirebaseApp | ||
} from '@firebase/app'; | ||
import { deepEqual } from '@firebase/util'; | ||
import { Auth, Dependencies } from '../../model/public_types'; | ||
|
||
import { AuthErrorCode } from '../errors'; | ||
import { PersistenceInternal } from '../persistence'; | ||
import { _fail } from '../util/assert'; | ||
import { _getInstance } from '../util/instantiator'; | ||
import { AuthImpl } from './auth_impl'; | ||
import { AuthImpl, _castAuth } from './auth_impl'; | ||
import { UserImpl } from '../user/user_impl'; | ||
import { getAccountInfo } from '../../api/account_management/account'; | ||
|
||
/** | ||
* Initializes an {@link Auth} instance with fine-grained control over | ||
|
@@ -65,9 +71,37 @@ export function initializeAuth(app: FirebaseApp, deps?: Dependencies): Auth { | |
|
||
const auth = provider.initialize({ options: deps }) as AuthImpl; | ||
|
||
if (_isFirebaseServerAppImpl(app)) { | ||
if (app.authIdToken !== undefined) { | ||
DellaBitta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
void _loadUserFromIdToken(auth, app.authIdToken); | ||
} | ||
} | ||
|
||
return auth; | ||
} | ||
|
||
export async function _loadUserFromIdToken( | ||
auth: Auth, | ||
idToken: string | ||
): Promise<void> { | ||
try { | ||
const response = await getAccountInfo(auth, { idToken }); | ||
const authInternal = _castAuth(auth); | ||
await authInternal._initializationPromise; | ||
const user = await UserImpl._fromGetAccountInfoResponse( | ||
authInternal, | ||
response, | ||
idToken | ||
); | ||
await authInternal._updateCurrentUser(user); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could there be a race condition here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} catch (err) { | ||
console.warn( | ||
'FirebaseServerApp could not login user with provided authIdToken: ', | ||
err | ||
); | ||
} | ||
} | ||
|
||
export function _initializeAuthInstance( | ||
auth: AuthImpl, | ||
deps?: Dependencies | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're exporting this out of
@firebase/app
you should also exportFirebaseServerAppImpl
, 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 theobj is FirebaseServerAppImpl
and just doboolean
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 forauth
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 onFirebaseServerAppImpl
, api-extractor is complaining that you have to export that too. The way to get around it is to not use theobj 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 notisFirebaseServerAppImpl
.FirebasServerApp
is already a public interface so we should be good to go here.