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.

Sign up for GitHub

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

Auth middleware (beforeAuthStateChanged) #6068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d1992ad
Bookmark todo
hsubox76 Feb 22, 2022
8554836
wip
hsubox76 Feb 28, 2022
202a114
more wip
hsubox76 Mar 9, 2022
ef9b649
Address PR comments
hsubox76 Mar 28, 2022
06e6a13
Address comments and add tests
hsubox76 Apr 1, 2022
ad74326
Add try/catch
hsubox76 Apr 4, 2022
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
1 change: 1 addition & 0 deletions common/api-review/auth.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

readonly config: Config;
readonly currentUser: User | null;
readonly emulatorConfig: EmulatorConfig | null;
Expand Down
76 changes: 75 additions & 1 deletion packages/auth/src/core/auth/auth_impl.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import * as reload from '../user/reload';
import { AuthImpl, DefaultConfig } from './auth_impl';
import { _initializeAuthInstance } from './initialize';
import { ClientPlatform } from '../util/version';
import { AuthErrorCode } from '../errors';

use(sinonChai);
use(chaiAsPromised);
Expand Down Expand Up @@ -138,6 +139,11 @@ describe('core/auth/auth_impl', () => {
expect(persistenceStub._remove).to.have.been.called;
expect(auth.currentUser).to.be.null;
});
it('is blocked if a beforeAuthStateChanged callback throws', async () => {
await auth._updateCurrentUser(testUser(auth, 'test'));
auth.beforeAuthStateChanged(sinon.stub().throws());
await expect(auth.signOut()).to.be.rejectedWith(AuthErrorCode.LOGIN_BLOCKED);
});
});

describe('#useDeviceLanguage', () => {
Expand Down Expand Up @@ -208,20 +214,24 @@ describe('core/auth/auth_impl', () => {
let user: UserInternal;
let authStateCallback: sinon.SinonSpy;
let idTokenCallback: sinon.SinonSpy;
let beforeAuthCallback: sinon.SinonSpy;

beforeEach(() => {
user = testUser(auth, 'uid');
authStateCallback = sinon.spy();
idTokenCallback = sinon.spy();
beforeAuthCallback = sinon.spy();
});

context('initially currentUser is null', () => {
beforeEach(async () => {
auth.onAuthStateChanged(authStateCallback);
auth.onIdTokenChanged(idTokenCallback);
auth.beforeAuthStateChanged(beforeAuthCallback);
await auth._updateCurrentUser(null);
authStateCallback.resetHistory();
idTokenCallback.resetHistory();
beforeAuthCallback.resetHistory();
});

it('onAuthStateChange triggers on log in', async () => {
Expand All @@ -233,15 +243,22 @@ describe('core/auth/auth_impl', () => {
await auth._updateCurrentUser(user);
expect(idTokenCallback).to.have.been.calledWith(user);
});

it('beforeAuthStateChanged triggers on log in', async () => {
await auth._updateCurrentUser(user);
expect(beforeAuthCallback).to.have.been.calledWith(user);
});
});

context('initially currentUser is user', () => {
beforeEach(async () => {
auth.onAuthStateChanged(authStateCallback);
auth.onIdTokenChanged(idTokenCallback);
auth.beforeAuthStateChanged(beforeAuthCallback);
await auth._updateCurrentUser(user);
authStateCallback.resetHistory();
idTokenCallback.resetHistory();
beforeAuthCallback.resetHistory();
});

it('onAuthStateChange triggers on log out', async () => {
Expand All @@ -254,6 +271,11 @@ describe('core/auth/auth_impl', () => {
expect(idTokenCallback).to.have.been.calledWith(null);
});

it('beforeAuthStateChanged triggers on log out', async () => {
await auth._updateCurrentUser(null);
expect(beforeAuthCallback).to.have.been.calledWith(null);
});

it('onAuthStateChange does not trigger for user props change', async () => {
user.photoURL = 'blah';
await auth._updateCurrentUser(user);
Expand Down Expand Up @@ -300,21 +322,61 @@ describe('core/auth/auth_impl', () => {
expect(cb1).to.have.been.calledWith(user);
expect(cb2).to.have.been.calledWith(user);
});

it('beforeAuthStateChange works for multiple listeners', async () => {
const cb1 = sinon.spy();
const cb2 = sinon.spy();
auth.beforeAuthStateChanged(cb1);
auth.beforeAuthStateChanged(cb2);
await auth._updateCurrentUser(null);
cb1.resetHistory();
cb2.resetHistory();

await auth._updateCurrentUser(user);
expect(cb1).to.have.been.calledWith(user);
expect(cb2).to.have.been.calledWith(user);
});

it('_updateCurrentUser throws if a beforeAuthStateChange callback throws', async () => {
await auth._updateCurrentUser(null);
const cb1 = sinon.stub().throws();
const cb2 = sinon.spy();
auth.beforeAuthStateChanged(cb1);
auth.beforeAuthStateChanged(cb2);

await expect(auth._updateCurrentUser(user)).to.be.rejectedWith(AuthErrorCode.LOGIN_BLOCKED);
expect(cb2).not.to.be.called;
});

it('_updateCurrentUser throws if a beforeAuthStateChange callback rejects', async () => {
await auth._updateCurrentUser(null);
const cb1 = sinon.stub().rejects();
const cb2 = sinon.spy();
auth.beforeAuthStateChanged(cb1);
auth.beforeAuthStateChanged(cb2);

await expect(auth._updateCurrentUser(user)).to.be.rejectedWith(AuthErrorCode.LOGIN_BLOCKED);
expect(cb2).not.to.be.called;
});
});
});

describe('#_onStorageEvent', () => {
let authStateCallback: sinon.SinonSpy;
let idTokenCallback: sinon.SinonSpy;
let beforeStateCallback: sinon.SinonSpy;

beforeEach(async () => {
authStateCallback = sinon.spy();
idTokenCallback = sinon.spy();
beforeStateCallback = sinon.spy();
auth.onAuthStateChanged(authStateCallback);
auth.onIdTokenChanged(idTokenCallback);
auth.beforeAuthStateChanged(beforeStateCallback);
await auth._updateCurrentUser(null); // force event handlers to clear out
authStateCallback.resetHistory();
idTokenCallback.resetHistory();
beforeStateCallback.resetHistory();
});

context('previously logged out', () => {
Expand All @@ -324,6 +386,7 @@ describe('core/auth/auth_impl', () => {

expect(authStateCallback).not.to.have.been.called;
expect(idTokenCallback).not.to.have.been.called;
expect(beforeStateCallback).not.to.have.been.called;
});
});

Expand All @@ -341,6 +404,8 @@ describe('core/auth/auth_impl', () => {
expect(auth.currentUser?.toJSON()).to.eql(user.toJSON());
expect(authStateCallback).to.have.been.called;
expect(idTokenCallback).to.have.been.called;
// This should never be called on a storage event.
expect(beforeStateCallback).not.to.have.been.called;
});
});
});
Expand All @@ -353,6 +418,7 @@ describe('core/auth/auth_impl', () => {
await auth._updateCurrentUser(user);
authStateCallback.resetHistory();
idTokenCallback.resetHistory();
beforeStateCallback.resetHistory();
});

context('now logged out', () => {
Expand All @@ -366,6 +432,8 @@ describe('core/auth/auth_impl', () => {
expect(auth.currentUser).to.be.null;
expect(authStateCallback).to.have.been.called;
expect(idTokenCallback).to.have.been.called;
// This should never be called on a storage event.
expect(beforeStateCallback).not.to.have.been.called;
});
});

Expand All @@ -378,6 +446,7 @@ describe('core/auth/auth_impl', () => {
expect(auth.currentUser?.toJSON()).to.eql(user.toJSON());
expect(authStateCallback).not.to.have.been.called;
expect(idTokenCallback).not.to.have.been.called;
expect(beforeStateCallback).not.to.have.been.called;
});

it('should update fields if they have changed', async () => {
Expand All @@ -391,6 +460,7 @@ describe('core/auth/auth_impl', () => {
expect(auth.currentUser?.displayName).to.eq('other-name');
expect(authStateCallback).not.to.have.been.called;
expect(idTokenCallback).not.to.have.been.called;
expect(beforeStateCallback).not.to.have.been.called;
});

it('should update tokens if they have changed', async () => {
Expand All @@ -407,6 +477,8 @@ describe('core/auth/auth_impl', () => {
).to.eq('new-access-token');
expect(authStateCallback).not.to.have.been.called;
expect(idTokenCallback).to.have.been.called;
// This should never be called on a storage event.
expect(beforeStateCallback).not.to.have.been.called;
});
});

Expand All @@ -420,6 +492,8 @@ describe('core/auth/auth_impl', () => {
expect(auth.currentUser?.toJSON()).to.eql(newUser.toJSON());
expect(authStateCallback).to.have.been.called;
expect(idTokenCallback).to.have.been.called;
// This should never be called on a storage event.
expect(beforeStateCallback).not.to.have.been.called;
});
});
});
Expand Down Expand Up @@ -461,7 +535,7 @@ describe('core/auth/auth_impl', () => {
});
});

context ('#_getAdditionalHeaders', () => {
context('#_getAdditionalHeaders', () => {
it('always adds the client version', async () => {
expect(await auth._getAdditionalHeaders()).to.eql({
'X-Client-Version': 'v',
Expand Down
70 changes: 64 additions & 6 deletions packages/auth/src/core/auth/auth_impl.ts
View file
Open in desktop
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 @@ -181,7 +182,8 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
}

// Update current Auth state. Either a new login or logout.
await this._updateCurrentUser(user);
// Skip blocking callbacks, they should not apply to a change in another tab.
await this._updateCurrentUser(user, /* skipBeforeStateCallbacks */ true);
}

private async initializeCurrentUser(
Expand Down Expand Up @@ -223,6 +225,14 @@ 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.
try {
await this._runBeforeStateCallbacks(storedUser);
} catch(e) {
return;
}

// If the redirect user's event ID matches the current user's event ID,
// DO NOT reload the current user, otherwise they'll be cleared from storage.
// This is important for the reauthenticateWithRedirect() flow.
Expand Down Expand Up @@ -313,7 +323,7 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
return this._updateCurrentUser(user && user._clone(this));
}

async _updateCurrentUser(user: User | null): Promise<void> {
async _updateCurrentUser(user: User | null, skipBeforeStateCallbacks: boolean = false): Promise<void> {
if (this._deleted) {
return;
}
Expand All @@ -325,19 +335,41 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
);
}

if (!skipBeforeStateCallbacks) {
await this._runBeforeStateCallbacks(user);
}

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

async _runBeforeStateCallbacks(user: User | null): Promise<void> {
if (this.currentUser === user) {
return;
}
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, { originalMessage: e.message });
}
}

async signOut(): Promise<void> {
// Run first, to block _setRedirectUser() if any callbacks fail.
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);
}

return this._updateCurrentUser(null);
// Prevent callbacks from being called again in _updateCurrentUser, as
// they were already called in the first line.
return this._updateCurrentUser(null, /* skipBeforeStateCallbacks */ true);
}

setPersistence(persistence: Persistence): Promise<void> {
Expand Down Expand Up @@ -371,6 +403,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 +487,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 +558,7 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
completed?: CompleteFn
): Unsubscribe {
if (this._deleted) {
return () => {};
return () => { };
}

const cb =
Expand Down Expand Up @@ -607,7 +665,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
Loading