-
Notifications
You must be signed in to change notification settings - Fork 938
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
Changes from 3 commits
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 |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -324,14 +325,26 @@ export class AuthImpl implements AuthInternal, _FirebaseService { | |
AuthErrorCode.TENANT_ID_MISMATCH | ||
); | ||
} | ||
await this._runBeforeStateCallbacks(user); | ||
|
||
return this.queue(async () => { | ||
await this.directlySetCurrentUser(user as UserInternal | null); | ||
this.notifyAuthListeners(); | ||
}); | ||
} | ||
|
||
async _runBeforeStateCallbacks(user: User | null): Promise<void> { | ||
try { | ||
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. This method should probably check first whether or not the user is actually changing (like Right now with this code, if you call auth.signOut() twice in a row, it will call the middleware twice in a row 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. 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); | ||
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. This will run twice since it's also in _updateCurrentUser 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 seems like it needs to run before |
||
// Clear the redirect user when signOut is called | ||
if (this.redirectPersistenceManager || this._popupRedirectResolver) { | ||
await this._setRedirectUser(null); | ||
|
@@ -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, | ||
|
@@ -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) { | ||
|
@@ -500,7 +539,7 @@ export class AuthImpl implements AuthInternal, _FirebaseService { | |
completed?: CompleteFn | ||
): Unsubscribe { | ||
if (this._deleted) { | ||
return () => {}; | ||
return () => { }; | ||
} | ||
|
||
const cb = | ||
|
@@ -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 | ||
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. 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 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. Inserted it in what I think is the correct place in |
||
private async directlySetCurrentUser( | ||
user: UserInternal | null | ||
): Promise<void> { | ||
|
@@ -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); | ||
|
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.
_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 beThere 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.
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().