Skip to content

Fix for authStateReady for FirebaseServerApp #8085

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

Merged
merged 4 commits into from
Mar 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions packages/auth/src/core/auth/auth_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ import { _logWarn } from '../util/log';
import { _getPasswordPolicy } from '../../api/password_policy/get_password_policy';
import { PasswordPolicyInternal } from '../../model/password_policy';
import { PasswordPolicyImpl } from './password_policy_impl';
import { getAccountInfo } from '../../api/account_management/account';
import { UserImpl } from '../user/user_impl';

interface AsyncAction {
(): Promise<void>;
Expand Down Expand Up @@ -174,10 +176,7 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
}
}

// Skip loading users from persistence in FirebaseServerApp Auth instances.
if (!_isFirebaseServerApp(this.app)) {
await this.initializeCurrentUser(popupRedirectResolver);
}
await this.initializeCurrentUser(popupRedirectResolver);

this.lastNotifiedUid = this.currentUser?.uid || null;

Expand Down Expand Up @@ -221,9 +220,47 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
await this._updateCurrentUser(user, /* skipBeforeStateCallbacks */ true);
}

private async initializeCurrentUserFromIdToken(
idToken: string
): Promise<void> {
try {
const response = await getAccountInfo(this, { idToken });
const user = await UserImpl._fromGetAccountInfoResponse(
this,
response,
idToken
);
await this.directlySetCurrentUser(user);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use directlySetCurrentUser instead of updateCurrentUser? directlySetCurrentUser is rarely used.

Copy link
Member Author

@jamesdaniels jamesdaniels Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noted that directlySetCurrentUser was what was being used elsewhere in initializeCurrentUser where I am calling this from. FWIW we have chosen to disable updateCurrentUser in FirebaseServerApp. I could use _updateCurrentUser as @DellaBitta did before but I was concerned that the side effects of notifying listeners and running middleware were undesirable during auth initialization and may lead to more race conditions and ill effects in developer's code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I should rename this method, as it is more appropriately initializeCurrentUserFromIdToken

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seconded the rename.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the concern of side effects of _updateCurrentUser makes sense to me. Thanks, let's keep directlySetCurrentUser then.

} catch (err) {
console.warn(
'FirebaseServerApp could not login user with provided authIdToken: ',
err
);
await this.directlySetCurrentUser(null);
}
}

private async initializeCurrentUser(
popupRedirectResolver?: PopupRedirectResolver
): Promise<void> {
if (_isFirebaseServerApp(this.app)) {
const idToken = this.app.settings.authIdToken;
if (idToken) {
// Start the auth operation in the next tick to allow a moment for the customer's app to
// attach an emulator, if desired.
return new Promise<void>(resolve => {
setTimeout(() =>
this.initializeCurrentUserFromIdToken(idToken).then(
resolve,
resolve
)
);
});
} else {
return this.directlySetCurrentUser(null);
}
}

// First check to see if we have a pending redirect event.
const previouslyStoredUser =
(await this.assertedPersistence.getCurrentUser()) as UserInternal | null;
Expand Down
37 changes: 2 additions & 35 deletions packages/auth/src/core/auth/initialize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,15 @@
* limitations under the License.
*/

import { _getProvider, _isFirebaseServerApp, FirebaseApp } from '@firebase/app';
import { _getProvider, 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, _castAuth } from './auth_impl';
import { UserImpl } from '../user/user_impl';
import { getAccountInfo } from '../../api/account_management/account';
import { AuthImpl } from './auth_impl';

/**
* Initializes an {@link Auth} instance with fine-grained control over
Expand Down Expand Up @@ -67,40 +65,9 @@ export function initializeAuth(app: FirebaseApp, deps?: Dependencies): Auth {

const auth = provider.initialize({ options: deps }) as AuthImpl;

if (_isFirebaseServerApp(app)) {
if (app.settings.authIdToken !== undefined) {
const idToken = app.settings.authIdToken;
// Start the auth operation in the next tick to allow a moment for the customer's app to
// attach an emulator, if desired.
setTimeout(() => void _loadUserFromIdToken(auth, idToken), 0);
}
}

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);
} catch (err) {
console.warn(
'FirebaseServerApp could not login user with provided authIdToken: ',
err
);
}
}

export function _initializeAuthInstance(
auth: AuthImpl,
deps?: Dependencies
Expand Down