From 19d08d7fc276e26c79d6fe3a14c47702c8c9c60b Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 6 Mar 2020 10:35:18 -0500 Subject: [PATCH 1/4] Directly initialize properties in UserMetadata Prompted by attempting to enable tsconfig options 'strictPropertyInitialization'. The properties were being indirectly set via Object.defineProperty. Typescript isn't able to follow that. Setting the properties directly resolves it. (And also exposed some minor type errors.) --- src/auth/user-record.ts | 11 +++++++---- src/index.d.ts | 5 ++--- src/utils/index.ts | 14 ++++++++++++++ 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/auth/user-record.ts b/src/auth/user-record.ts index 28f63fc972..ea2db69a7a 100644 --- a/src/auth/user-record.ts +++ b/src/auth/user-record.ts @@ -66,16 +66,19 @@ export interface CreateRequest extends UpdateRequest { * @constructor */ export class UserMetadata { - public readonly creationTime: string; - public readonly lastSignInTime: string; + public readonly creationTime?: string | null; + public readonly lastSignInTime?: string | null; constructor(response: any) { // Creation date should always be available but due to some backend bugs there // were cases in the past where users did not have creation date properly set. // This included legacy Firebase migrating project users and some anonymous users. // These bugs have already been addressed since then. - utils.addReadonlyGetter(this, 'creationTime', parseDate(response.createdAt)); - utils.addReadonlyGetter(this, 'lastSignInTime', parseDate(response.lastLoginAt)); + this.creationTime = parseDate(response.createdAt); + utils.enforceReadonly(this, 'creationTime'); + + this.lastSignInTime = parseDate(response.lastLoginAt); + utils.enforceReadonly(this, 'lastSignInTime'); } /** @return {object} The plain object representation of the user's metadata. */ diff --git a/src/index.d.ts b/src/index.d.ts index af8a938766..c724d30b37 100755 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -491,13 +491,12 @@ declare namespace admin.auth { /** * The date the user last signed in, formatted as a UTC string. */ - lastSignInTime: string; + lastSignInTime?: string | null; /** * The date the user was created, formatted as a UTC string. - * */ - creationTime: string; + creationTime?: string | null; /** * @return A JSON-serializable representation of this object. diff --git a/src/utils/index.ts b/src/utils/index.ts index f6e2f41232..21014c21fe 100755 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -55,6 +55,20 @@ export function addReadonlyGetter(obj: object, prop: string, value: any): void { }); } +/** + * Marks an existing property as readonly. Unlike typescript's "readonly" + * modifier, this will take effect at runtime too (generating a TypeError if + * violated), including when called from javascript. + */ +export function enforceReadonly(obj: object, prop: string): void { + Object.defineProperty(obj, prop, { + // Make this property read-only. + writable: false, + // Include this property during enumeration of obj's properties. + enumerable: true, + }); +} + /** * Returns the Google Cloud project ID associated with a Firebase app, if it's explicitly * specified in either the Firebase app options, credentials or the local environment. From 1f81e99f1898991d1c7c5e7a5eb2cce4a1b1603d Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 6 Mar 2020 14:31:39 -0500 Subject: [PATCH 2/4] review feedback --- src/auth/user-record.ts | 4 ++-- src/index.d.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/auth/user-record.ts b/src/auth/user-record.ts index ea2db69a7a..ec479126ab 100644 --- a/src/auth/user-record.ts +++ b/src/auth/user-record.ts @@ -66,8 +66,8 @@ export interface CreateRequest extends UpdateRequest { * @constructor */ export class UserMetadata { - public readonly creationTime?: string | null; - public readonly lastSignInTime?: string | null; + public readonly creationTime: string | null; + public readonly lastSignInTime: string | null; constructor(response: any) { // Creation date should always be available but due to some backend bugs there diff --git a/src/index.d.ts b/src/index.d.ts index c724d30b37..a6cda515e3 100755 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -491,12 +491,12 @@ declare namespace admin.auth { /** * The date the user last signed in, formatted as a UTC string. */ - lastSignInTime?: string | null; + lastSignInTime: string | null; /** * The date the user was created, formatted as a UTC string. */ - creationTime?: string | null; + creationTime: string | null; /** * @return A JSON-serializable representation of this object. From ffb9cc75bdc4e22e3214963d7546dc08222420ab Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Mon, 9 Mar 2020 15:39:07 -0400 Subject: [PATCH 3/4] review feedback 2 --- src/index.d.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/index.d.ts b/src/index.d.ts index a6cda515e3..9f9e9dbc53 100755 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -489,12 +489,14 @@ declare namespace admin.auth { interface UserMetadata { /** - * The date the user last signed in, formatted as a UTC string. + * The date the user last signed in, formatted as a UTC string, or `null` + * if the user has never signed in. */ lastSignInTime: string | null; /** - * The date the user was created, formatted as a UTC string. + * The date the user was created, formatted as a UTC string. Should + * generally never be `null` except in some legacy situations. */ creationTime: string | null; From 3fbeecd51f9233e121eadd85865185701375513f Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Mon, 9 Mar 2020 16:41:55 -0400 Subject: [PATCH 4/4] review feedback 3 --- src/auth/user-record.ts | 11 +++++++++-- src/index.d.ts | 5 ++--- test/unit/auth/auth.spec.ts | 12 ++++++------ test/unit/auth/user-record.spec.ts | 21 ++++++++++----------- 4 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/auth/user-record.ts b/src/auth/user-record.ts index a484415985..3d6e690b00 100644 --- a/src/auth/user-record.ts +++ b/src/auth/user-record.ts @@ -309,7 +309,7 @@ export class MultiFactor { * @constructor */ export class UserMetadata { - public readonly creationTime: string | null; + public readonly creationTime: string; public readonly lastSignInTime: string | null; constructor(response: GetAccountInfoUserResponse) { @@ -317,7 +317,14 @@ export class UserMetadata { // were cases in the past where users did not have creation date properly set. // This included legacy Firebase migrating project users and some anonymous users. // These bugs have already been addressed since then. - this.creationTime = parseDate(response.createdAt); + const creationTime = parseDate(response.createdAt); + if (creationTime === null) { + throw new FirebaseAuthError( + AuthClientErrorCode.INTERNAL_ERROR, + 'INTERNAL ASSERT FAILED: Unable to parse createdAt time: "' + + response.createdAt + '"'); + } + this.creationTime = creationTime; utils.enforceReadonly(this, 'creationTime'); this.lastSignInTime = parseDate(response.lastLoginAt); diff --git a/src/index.d.ts b/src/index.d.ts index d9374be68e..6f69d5890e 100755 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -495,10 +495,9 @@ declare namespace admin.auth { lastSignInTime: string | null; /** - * The date the user was created, formatted as a UTC string. Should - * generally never be `null` except in some legacy situations. + * The date the user was created, formatted as a UTC string. */ - creationTime: string | null; + creationTime: string; /** * @return A JSON-serializable representation of this object. diff --git a/test/unit/auth/auth.spec.ts b/test/unit/auth/auth.spec.ts index c3a867011f..091ce9baec 100755 --- a/test/unit/auth/auth.spec.ts +++ b/test/unit/auth/auth.spec.ts @@ -1597,17 +1597,17 @@ AUTH_CONFIGS.forEach((testConfig) => { const maxResult = 500; const downloadAccountResponse: any = { users: [ - {localId: 'UID1'}, - {localId: 'UID2'}, - {localId: 'UID3'}, + {createdAt: '1234567890000', localId: 'UID1'}, + {createdAt: '1234567890000', localId: 'UID2'}, + {createdAt: '1234567890000', localId: 'UID3'}, ], nextPageToken: 'NEXT_PAGE_TOKEN', }; const expectedResult: any = { users: [ - new UserRecord({localId: 'UID1'}), - new UserRecord({localId: 'UID2'}), - new UserRecord({localId: 'UID3'}), + new UserRecord({createdAt: '1234567890000', localId: 'UID1'}), + new UserRecord({createdAt: '1234567890000', localId: 'UID2'}), + new UserRecord({createdAt: '1234567890000', localId: 'UID3'}), ], pageToken: 'NEXT_PAGE_TOKEN', }; diff --git a/test/unit/auth/user-record.spec.ts b/test/unit/auth/user-record.spec.ts index fce2041ec6..454bc1c252 100644 --- a/test/unit/auth/user-record.spec.ts +++ b/test/unit/auth/user-record.spec.ts @@ -23,6 +23,7 @@ import { UserInfo, UserMetadata, UserRecord, GetAccountInfoUserResponse, ProviderUserInfoResponse, MultiFactor, PhoneMultiFactorInfo, MultiFactorInfo, MultiFactorInfoResponse, } from '../../../src/auth/user-record'; +import {FirebaseAuthError} from '../../../src/utils/error'; chai.should(); @@ -633,18 +634,15 @@ describe('UserMetadata', () => { }).not.to.throw(Error); }); - it('should set creationTime and lastSignInTime to null when not provided', () => { - const metadata = new UserMetadata({} as any); - expect(metadata.creationTime).to.be.null; + it('should set lastSignInTime to null when not provided', () => { + const metadata = new UserMetadata({createdAt: '1234567890000'} as any); expect(metadata.lastSignInTime).to.be.null; }); - it('should set creationTime to null when creationTime value is invalid', () => { - const metadata = new UserMetadata({ - createdAt: 'invalid', - } as any); - expect(metadata.creationTime).to.be.null; - expect(metadata.lastSignInTime).to.be.null; + it('should throw when createdAt value is invalid', () => { + expect(() => { + new UserMetadata({createdAt: 'invalid', localId: 'uid1'}); + }).to.throw(FirebaseAuthError).with.property('code', 'auth/internal-error'); }); it('should set lastSignInTime to null when lastLoginAt value is invalid', () => { @@ -693,9 +691,9 @@ describe('UserRecord', () => { }).to.throw(Error); }); - it('should succeed when only localId is provided', () => { + it('should succeed when only createdAt and localId is provided', () => { expect(() => { - return new UserRecord({localId: '123456789'}); + return new UserRecord({createdAt: '1234567890000', localId: '123456789'}); }).not.to.throw(Error); }); }); @@ -791,6 +789,7 @@ describe('UserRecord', () => { it('should clear REDACTED passwordHash', () => { const user = new UserRecord({ localId: 'uid1', + createdAt: '1234567890000', passwordHash: Buffer.from('REDACTED').toString('base64'), });