Auth middleware (beforeAuthStateChanged) #6068
Conversation
hsubox76
requested review from
avolkovi,
lisajian,
sam-gc and
yuchenshi
as code owners
|
| async signOut(): Promise<void> { | ||
| await this._runBeforeStateCallbacks(null); |
There was a problem hiding this comment.
This will run twice since it's also in _updateCurrentUser
There was a problem hiding this comment.
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.
| @@ -324,14 +325,26 @@ export class AuthImpl implements AuthInternal, _FirebaseService { | |||
| AuthErrorCode.TENANT_ID_MISMATCH | |||
| ); | |||
| } | |||
| await this._runBeforeStateCallbacks(user); | |||
There was a problem hiding this comment.
_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
There was a problem hiding this comment.
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().
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (795,406 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
| 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.
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
| @@ -528,6 +575,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.
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)
There was a problem hiding this comment.
Inserted it in what I think is the correct place in initializeCurrentUser().
| @@ -223,6 +225,10 @@ export class AuthImpl implements AuthInternal, _FirebaseService { | |||
| _assert(this._popupRedirectResolver, this, AuthErrorCode.ARGUMENT_ERROR); | |||
| await this.getOrInitRedirectPersistenceManager(); | |||
|
|
|||
| // At this point in the flow, this is a redirect user. Run blocking | |||
| // middleware callbacks before setting the user. | |||
| await this._runBeforeStateCallbacks(storedUser); | |||
There was a problem hiding this comment.
I think here we should wrap it in a try/catch. Initialization swallows errors. Ideally we would surface the error in getRedirectResult but that will require some extra thinking
| @@ -81,6 +81,7 @@ export function applyActionCode(auth: Auth, oobCode: string): Promise<void>; | |||
| // @public | |||
| export interface Auth { | |||
| readonly app: FirebaseApp; | |||
| beforeAuthStateChanged(callback: (user: User | null) => void | Promise<void>): Unsubscribe; | |||
There was a problem hiding this comment.
Did we want to tackle the second argument, the optional onAbort callback in this PR?
That is just a () => void that is called useful if beforeAuthStateChanged is called multiple times and a subsequent callback rejects the state change. This gives developers a chance to walk back any side effects.
Useful to me as I integrate with the web frameworks work, for authenticated server-context. onAbort would allow me the chance to destroy the just minted cookie, if the developer is using beforeAuthStateChanged too and rejected the sign in.
hsubox76
changed the title
|
Merging into |
Create
beforeAuthStateChanged()method per internal design doc go/firebase-auth-middleware.TODO in a separate PR: implement
onAbort().