Skip to content

Auth middleware (beforeAuthStateChanged) #6068

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 6 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from 3 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
46 changes: 43 additions & 3 deletions packages/auth/src/core/auth/auth_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
private redirectPersistenceManager?: PersistenceUserManager;
private authStateSubscription = new Subscription<User>(this);
private idTokenSubscription = new Subscription<User>(this);
private beforeStateQueue: Array<(user: User | null) => Promise<void>> = [];
private redirectUser: UserInternal | null = null;
private isProactiveRefreshEnabled = false;

Expand Down Expand Up @@ -324,14 +325,26 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
AuthErrorCode.TENANT_ID_MISMATCH
);
}
await this._runBeforeStateCallbacks(user);
Copy link
Contributor

Choose a reason for hiding this comment

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

_updateCurrentUser is also called by the storage onevent callbacks, which means a persistence change in another tab could trigger a stop here and the tabs would get out of sync. I'm not sure what the right behavior should be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this, and we want the callbacks to not run if called by _onStorageEvent(), so I made use of the optional skipBeforeStateCallbacks argument I added to _updateCurrentUser when called by _onStorageEvent().


return this.queue(async () => {
await this.directlySetCurrentUser(user as UserInternal | null);
this.notifyAuthListeners();
});
}

async _runBeforeStateCallbacks(user: User | null): Promise<void> {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method should probably check first whether or not the user is actually changing (like directlySetCurrentUser does).

Right now with this code, if you call auth.signOut() twice in a row, it will call the middleware twice in a row

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check.

for (const beforeStateCallback of this.beforeStateQueue) {
await beforeStateCallback(user);
}
} catch (e) {
throw this._errorFactory.create(AuthErrorCode.LOGIN_BLOCKED, { message: e.message });
}
}

async signOut(): Promise<void> {
await this._runBeforeStateCallbacks(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will run twice since it's also in _updateCurrentUser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like it needs to run before _setRedirectUser() so it can block it, so I added an optional arg to _updateCurrentUser() to skip running the callbacks inside it.

// Clear the redirect user when signOut is called
if (this.redirectPersistenceManager || this._popupRedirectResolver) {
await this._setRedirectUser(null);
Expand Down Expand Up @@ -371,6 +384,32 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
);
}

beforeAuthStateChanged(
callback: (user: User | null) => void | Promise<void>
): Unsubscribe {
// The callback could be sync or async. Wrap it into a
// function that is always async.
const wrappedCallback =
(user: User | null): Promise<void> => new Promise((resolve, reject) => {
try {
const result = callback(user);
// Either resolve with existing promise or wrap a non-promise
// return value into a promise.
resolve(result);
} catch (e) {
// Sync callback throws.
reject(e);
}
});
this.beforeStateQueue.push(wrappedCallback);
const index = this.beforeStateQueue.length - 1;
return () => {
// Unsubscribe. Replace with no-op. Do not remove from array, or it will disturb
// indexing of other elements.
this.beforeStateQueue[index] = () => Promise.resolve();
};
}

onIdTokenChanged(
nextOrObserver: NextOrObserver<User>,
error?: ErrorFn,
Expand Down Expand Up @@ -429,7 +468,7 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
// Make sure we've cleared any pending persistence actions if we're not in
// the initializer
if (this._isInitialized) {
await this.queue(async () => {});
await this.queue(async () => { });
}

if (this._currentUser?._redirectEventId === id) {
Expand Down Expand Up @@ -500,7 +539,7 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
completed?: CompleteFn
): Unsubscribe {
if (this._deleted) {
return () => {};
return () => { };
}

const cb =
Expand Down Expand Up @@ -528,6 +567,7 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
* should only be called from within a queued callback. This is necessary
* because the queue shouldn't rely on another queued callback.
*/
// TODO: Find where this is called and see if we can run the middleware before it
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only used during initialization and then in a few select cases within this file. I need to do some thinking about the appropriate places in the init phase to use middleware. Off the top of my head, the only place init should run middleware is when signing in a user from redirect (i.e. first page load after a signInWithRedirect() call)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inserted it in what I think is the correct place in initializeCurrentUser().

private async directlySetCurrentUser(
user: UserInternal | null
): Promise<void> {
Expand Down Expand Up @@ -607,7 +647,7 @@ class Subscription<T> {
observer => (this.observer = observer)
);

constructor(readonly auth: AuthInternal) {}
constructor(readonly auth: AuthInternal) { }

get next(): NextFn<T | null> {
_assert(this.observer, this.auth, AuthErrorCode.INTERNAL_ERROR);
Expand Down
6 changes: 5 additions & 1 deletion packages/auth/src/core/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export const enum AuthErrorCode {
INVALID_SENDER = 'invalid-sender',
INVALID_SESSION_INFO = 'invalid-verification-id',
INVALID_TENANT_ID = 'invalid-tenant-id',
LOGIN_BLOCKED = 'login-blocked',
MFA_INFO_NOT_FOUND = 'multi-factor-info-not-found',
MFA_REQUIRED = 'multi-factor-auth-required',
MISSING_ANDROID_PACKAGE_NAME = 'missing-android-pkg-name',
Expand Down Expand Up @@ -245,6 +246,7 @@ function _debugErrorMap(): ErrorMap<AuthErrorCode> {
'The verification ID used to create the phone auth credential is invalid.',
[AuthErrorCode.INVALID_TENANT_ID]:
"The Auth instance's tenant ID is invalid.",
[AuthErrorCode.LOGIN_BLOCKED]: "Login blocked by user-provided method.",
[AuthErrorCode.MISSING_ANDROID_PACKAGE_NAME]:
'An Android Package Name must be provided if the Android App is required to be installed.',
[AuthErrorCode.MISSING_AUTH_DOMAIN]:
Expand Down Expand Up @@ -414,9 +416,10 @@ type GenericAuthErrorParams = {
| AuthErrorCode.NO_AUTH_EVENT
| AuthErrorCode.OPERATION_NOT_SUPPORTED
>]: {
appName: AppName;
appName?: AppName;
email?: string;
phoneNumber?: string;
message?: string;
};
};

Expand All @@ -427,6 +430,7 @@ export interface AuthErrorParams extends GenericAuthErrorParams {
[AuthErrorCode.ARGUMENT_ERROR]: { appName?: AppName };
[AuthErrorCode.DEPENDENT_SDK_INIT_BEFORE_AUTH]: { appName?: AppName };
[AuthErrorCode.INTERNAL_ERROR]: { appName?: AppName };
[AuthErrorCode.LOGIN_BLOCKED]: { message?: string };
[AuthErrorCode.OPERATION_NOT_SUPPORTED]: { appName?: AppName };
[AuthErrorCode.NO_AUTH_EVENT]: { appName?: AppName };
[AuthErrorCode.MFA_REQUIRED]: {
Expand Down