Skip to content

Commit 06e6a13

Browse files
hsubox76hsubox76
committed
Address comments and add tests
1 parent ef9b649 commit 06e6a13

File tree

4 files changed

+93
-2
lines changed

4 files changed

+93
-2
lines changed

‎common/api-review/auth.api.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ export function applyActionCode(auth: Auth, oobCode: string): Promise<void>;
8181
// @public
8282
export interface Auth {
8383
readonly app: FirebaseApp;
84+
beforeAuthStateChanged(callback: (user: User | null) => void | Promise<void>): Unsubscribe;
8485
readonly config: Config;
8586
readonly currentUser: User | null;
8687
readonly emulatorConfig: EmulatorConfig | null;

‎packages/auth/src/core/auth/auth_impl.test.ts

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import * as reload from '../user/reload';
3434
import { AuthImpl, DefaultConfig } from './auth_impl';
3535
import { _initializeAuthInstance } from './initialize';
3636
import { ClientPlatform } from '../util/version';
37+
import { AuthErrorCode } from '../errors';
3738

3839
use(sinonChai);
3940
use(chaiAsPromised);
@@ -138,6 +139,11 @@ describe('core/auth/auth_impl', () => {
138139
expect(persistenceStub._remove).to.have.been.called;
139140
expect(auth.currentUser).to.be.null;
140141
});
142+
it('is blocked if a beforeAuthStateChanged callback throws', async () => {
143+
await auth._updateCurrentUser(testUser(auth, 'test'));
144+
auth.beforeAuthStateChanged(sinon.stub().throws());
145+
await expect(auth.signOut()).to.be.rejectedWith(AuthErrorCode.LOGIN_BLOCKED);
146+
});
141147
});
142148

143149
describe('#useDeviceLanguage', () => {
@@ -208,20 +214,24 @@ describe('core/auth/auth_impl', () => {
208214
let user: UserInternal;
209215
let authStateCallback: sinon.SinonSpy;
210216
let idTokenCallback: sinon.SinonSpy;
217+
let beforeAuthCallback: sinon.SinonSpy;
211218

212219
beforeEach(() => {
213220
user = testUser(auth, 'uid');
214221
authStateCallback = sinon.spy();
215222
idTokenCallback = sinon.spy();
223+
beforeAuthCallback = sinon.spy();
216224
});
217225

218226
context('initially currentUser is null', () => {
219227
beforeEach(async () => {
220228
auth.onAuthStateChanged(authStateCallback);
221229
auth.onIdTokenChanged(idTokenCallback);
230+
auth.beforeAuthStateChanged(beforeAuthCallback);
222231
await auth._updateCurrentUser(null);
223232
authStateCallback.resetHistory();
224233
idTokenCallback.resetHistory();
234+
beforeAuthCallback.resetHistory();
225235
});
226236

227237
it('onAuthStateChange triggers on log in', async () => {
@@ -233,15 +243,22 @@ describe('core/auth/auth_impl', () => {
233243
await auth._updateCurrentUser(user);
234244
expect(idTokenCallback).to.have.been.calledWith(user);
235245
});
246+
247+
it('beforeAuthStateChanged triggers on log in', async () => {
248+
await auth._updateCurrentUser(user);
249+
expect(beforeAuthCallback).to.have.been.calledWith(user);
250+
});
236251
});
237252

238253
context('initially currentUser is user', () => {
239254
beforeEach(async () => {
240255
auth.onAuthStateChanged(authStateCallback);
241256
auth.onIdTokenChanged(idTokenCallback);
257+
auth.beforeAuthStateChanged(beforeAuthCallback);
242258
await auth._updateCurrentUser(user);
243259
authStateCallback.resetHistory();
244260
idTokenCallback.resetHistory();
261+
beforeAuthCallback.resetHistory();
245262
});
246263

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

274+
it('beforeAuthStateChanged triggers on log out', async () => {
275+
await auth._updateCurrentUser(null);
276+
expect(beforeAuthCallback).to.have.been.calledWith(null);
277+
});
278+
257279
it('onAuthStateChange does not trigger for user props change', async () => {
258280
user.photoURL = 'blah';
259281
await auth._updateCurrentUser(user);
@@ -300,21 +322,61 @@ describe('core/auth/auth_impl', () => {
300322
expect(cb1).to.have.been.calledWith(user);
301323
expect(cb2).to.have.been.calledWith(user);
302324
});
325+
326+
it('beforeAuthStateChange works for multiple listeners', async () => {
327+
const cb1 = sinon.spy();
328+
const cb2 = sinon.spy();
329+
auth.beforeAuthStateChanged(cb1);
330+
auth.beforeAuthStateChanged(cb2);
331+
await auth._updateCurrentUser(null);
332+
cb1.resetHistory();
333+
cb2.resetHistory();
334+
335+
await auth._updateCurrentUser(user);
336+
expect(cb1).to.have.been.calledWith(user);
337+
expect(cb2).to.have.been.calledWith(user);
338+
});
339+
340+
it('_updateCurrentUser throws if a beforeAuthStateChange callback throws', async () => {
341+
await auth._updateCurrentUser(null);
342+
const cb1 = sinon.stub().throws();
343+
const cb2 = sinon.spy();
344+
auth.beforeAuthStateChanged(cb1);
345+
auth.beforeAuthStateChanged(cb2);
346+
347+
await expect(auth._updateCurrentUser(user)).to.be.rejectedWith(AuthErrorCode.LOGIN_BLOCKED);
348+
expect(cb2).not.to.be.called;
349+
});
350+
351+
it('_updateCurrentUser throws if a beforeAuthStateChange callback rejects', async () => {
352+
await auth._updateCurrentUser(null);
353+
const cb1 = sinon.stub().rejects();
354+
const cb2 = sinon.spy();
355+
auth.beforeAuthStateChanged(cb1);
356+
auth.beforeAuthStateChanged(cb2);
357+
358+
await expect(auth._updateCurrentUser(user)).to.be.rejectedWith(AuthErrorCode.LOGIN_BLOCKED);
359+
expect(cb2).not.to.be.called;
360+
});
303361
});
304362
});
305363

306364
describe('#_onStorageEvent', () => {
307365
let authStateCallback: sinon.SinonSpy;
308366
let idTokenCallback: sinon.SinonSpy;
367+
let beforeStateCallback: sinon.SinonSpy;
309368

310369
beforeEach(async () => {
311370
authStateCallback = sinon.spy();
312371
idTokenCallback = sinon.spy();
372+
beforeStateCallback = sinon.spy();
313373
auth.onAuthStateChanged(authStateCallback);
314374
auth.onIdTokenChanged(idTokenCallback);
375+
auth.beforeAuthStateChanged(beforeStateCallback);
315376
await auth._updateCurrentUser(null); // force event handlers to clear out
316377
authStateCallback.resetHistory();
317378
idTokenCallback.resetHistory();
379+
beforeStateCallback.resetHistory();
318380
});
319381

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

325387
expect(authStateCallback).not.to.have.been.called;
326388
expect(idTokenCallback).not.to.have.been.called;
389+
expect(beforeStateCallback).not.to.have.been.called;
327390
});
328391
});
329392

@@ -341,6 +404,8 @@ describe('core/auth/auth_impl', () => {
341404
expect(auth.currentUser?.toJSON()).to.eql(user.toJSON());
342405
expect(authStateCallback).to.have.been.called;
343406
expect(idTokenCallback).to.have.been.called;
407+
// This should never be called on a storage event.
408+
expect(beforeStateCallback).not.to.have.been.called;
344409
});
345410
});
346411
});
@@ -353,6 +418,7 @@ describe('core/auth/auth_impl', () => {
353418
await auth._updateCurrentUser(user);
354419
authStateCallback.resetHistory();
355420
idTokenCallback.resetHistory();
421+
beforeStateCallback.resetHistory();
356422
});
357423

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

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

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

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

@@ -420,6 +492,8 @@ describe('core/auth/auth_impl', () => {
420492
expect(auth.currentUser?.toJSON()).to.eql(newUser.toJSON());
421493
expect(authStateCallback).to.have.been.called;
422494
expect(idTokenCallback).to.have.been.called;
495+
// This should never be called on a storage event.
496+
expect(beforeStateCallback).not.to.have.been.called;
423497
});
424498
});
425499
});
@@ -461,7 +535,7 @@ describe('core/auth/auth_impl', () => {
461535
});
462536
});
463537

464-
context ('#_getAdditionalHeaders', () => {
538+
context('#_getAdditionalHeaders', () => {
465539
it('always adds the client version', async () => {
466540
expect(await auth._getAdditionalHeaders()).to.eql({
467541
'X-Client-Version': 'v',

‎packages/auth/src/core/auth/auth_impl.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,10 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
225225
_assert(this._popupRedirectResolver, this, AuthErrorCode.ARGUMENT_ERROR);
226226
await this.getOrInitRedirectPersistenceManager();
227227

228+
// At this point in the flow, this is a redirect user. Run blocking
229+
// middleware callbacks before setting the user.
230+
await this._runBeforeStateCallbacks(storedUser);
231+
228232
// If the redirect user's event ID matches the current user's event ID,
229233
// DO NOT reload the current user, otherwise they'll be cleared from storage.
230234
// This is important for the reauthenticateWithRedirect() flow.
@@ -338,6 +342,9 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
338342
}
339343

340344
async _runBeforeStateCallbacks(user: User | null): Promise<void> {
345+
if (this.currentUser === user) {
346+
return;
347+
}
341348
try {
342349
for (const beforeStateCallback of this.beforeStateQueue) {
343350
await beforeStateCallback(user);
@@ -575,7 +582,6 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
575582
* should only be called from within a queued callback. This is necessary
576583
* because the queue shouldn't rely on another queued callback.
577584
*/
578-
// TODO: Find where this is called and see if we can run the middleware before it
579585
private async directlySetCurrentUser(
580586
user: UserInternal | null
581587
): Promise<void> {

‎packages/auth/src/model/public_types.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,16 @@ export interface Auth {
254254
error?: ErrorFn,
255255
completed?: CompleteFn
256256
): Unsubscribe;
257+
/**
258+
* Adds a blocking callback that runs before an auth state change
259+
* sets a new user.
260+
*
261+
* @param callback - callback triggered before new user value is set.
262+
* If this throws, it will block the user from being set.
263+
*/
264+
beforeAuthStateChanged(
265+
callback: (user: User | null) => void | Promise<void>
266+
): Unsubscribe;
257267
/**
258268
* Adds an observer for changes to the signed-in user's ID token.
259269
*

0 commit comments

Comments
 (0)