From 61e652d41124bcce0a51a2795a8cb895a78ef6d3 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 3 Sep 2020 14:31:55 +0100 Subject: [PATCH 1/6] Remove usage of the NODE_ADMIN global in RTDB --- packages/database/index.node.ts | 13 ++++++++++--- packages/database/src/core/PersistentConnection.ts | 12 +++++++----- packages/database/src/core/RepoInfo.ts | 7 ++++++- packages/database/src/core/RepoManager.ts | 9 +++++++-- .../database/src/realtime/WebSocketConnection.ts | 4 ++-- 5 files changed, 32 insertions(+), 13 deletions(-) diff --git a/packages/database/index.node.ts b/packages/database/index.node.ts index c0ea32a0ab5..d354ca82058 100644 --- a/packages/database/index.node.ts +++ b/packages/database/index.node.ts @@ -51,13 +51,19 @@ const ServerValue = Database.ServerValue; * @param app A valid FirebaseApp-like object * @param url A valid Firebase databaseURL * @param version custom version e.g. firebase-admin version + * @param nodeAdmin true if the SDK is being initialized from Firebase Admin. */ -export function initStandalone(app: FirebaseApp, url: string, version: string) { +export function initStandalone( + app: FirebaseApp, + url: string, + version: string, + nodeAdmin: boolean = true +) { /** * This should allow the firebase-admin package to provide a custom version * to the backend */ - CONSTANTS.NODE_ADMIN = true; + CONSTANTS.NODE_ADMIN = nodeAdmin; setSDKVersion(version); /** @@ -84,7 +90,8 @@ export function initStandalone(app: FirebaseApp, url: string, version: string) { instance: RepoManager.getInstance().databaseFromApp( app, authProvider, - url + url, + nodeAdmin ) as types.Database, namespace: { Reference, diff --git a/packages/database/src/core/PersistentConnection.ts b/packages/database/src/core/PersistentConnection.ts index a79d20b8d96..30f08c93470 100644 --- a/packages/database/src/core/PersistentConnection.ts +++ b/packages/database/src/core/PersistentConnection.ts @@ -796,7 +796,7 @@ export class PersistentConnection extends ServerActions { .then(null, error => { self.log_('Failed to get token: ' + error); if (!canceled) { - if (CONSTANTS.NODE_ADMIN) { + if (this.repoInfo_.nodeAdmin) { // This may be a critical error for the Admin Node.js SDK, so log a warning. // But getToken() may also just have temporarily failed, so we still want to // continue retrying. @@ -959,10 +959,12 @@ export class PersistentConnection extends ServerActions { const stats: { [k: string]: number } = {}; let clientName = 'js'; - if (CONSTANTS.NODE_ADMIN) { - clientName = 'admin_node'; - } else if (CONSTANTS.NODE_CLIENT) { - clientName = 'node'; + if (isNodeSdk()) { + if (this.repoInfo_.nodeAdmin) { + clientName = 'admin_node'; + } else { + clientName = 'node'; + } } stats['sdk.' + clientName + '.' + SDK_VERSION.replace(/\./g, '-')] = 1; diff --git a/packages/database/src/core/RepoInfo.ts b/packages/database/src/core/RepoInfo.ts index a1b55bfcd08..d2adf5c3305 100644 --- a/packages/database/src/core/RepoInfo.ts +++ b/packages/database/src/core/RepoInfo.ts @@ -43,7 +43,8 @@ export class RepoInfo { public namespace: string, public webSocketOnly: boolean, public persistenceKey: string = '', - public includeNamespaceInQueryParams: boolean = false + public includeNamespaceInQueryParams: boolean = false, + public nodeAdmin: boolean = false ) { this.host = host.toLowerCase(); this.domain = this.host.substr(this.host.indexOf('.') + 1); @@ -73,6 +74,10 @@ export class RepoInfo { ); } + setNodeAdmin(nodeAdmin: boolean) { + this.nodeAdmin = nodeAdmin; + } + updateHost(newHost: string) { if (newHost !== this.internalHost) { this.internalHost = newHost; diff --git a/packages/database/src/core/RepoManager.ts b/packages/database/src/core/RepoManager.ts index 5ec5e7bc753..625b6885131 100644 --- a/packages/database/src/core/RepoManager.ts +++ b/packages/database/src/core/RepoManager.ts @@ -99,7 +99,8 @@ export class RepoManager { databaseFromApp( app: FirebaseApp, authProvider: Provider, - url?: string + url?: string, + nodeAdmin?: boolean ): Database { let dbUrl: string | undefined = url || app.options[DATABASE_URL_OPTION]; if (dbUrl === undefined) { @@ -129,8 +130,12 @@ export class RepoManager { isEmulator = !parsedUrl.repoInfo.secure; } + if (nodeAdmin) { + repoInfo.setNodeAdmin(nodeAdmin); + } + const authTokenProvider = - CONSTANTS.NODE_ADMIN && isEmulator + nodeAdmin && isEmulator ? new EmulatorAdminTokenProvider() : new FirebaseAuthTokenProvider(app, authProvider); diff --git a/packages/database/src/realtime/WebSocketConnection.ts b/packages/database/src/realtime/WebSocketConnection.ts index 0e59e0d111d..71458aaea2e 100644 --- a/packages/database/src/realtime/WebSocketConnection.ts +++ b/packages/database/src/realtime/WebSocketConnection.ts @@ -87,7 +87,7 @@ export class WebSocketConnection implements Transport { */ constructor( public connId: string, - repoInfo: RepoInfo, + private repoInfo: RepoInfo, private applicationId?: string, transportSessionId?: string, lastSessionId?: string @@ -151,7 +151,7 @@ export class WebSocketConnection implements Transport { try { if (isNodeSdk()) { - const device = ENV_CONSTANTS.NODE_ADMIN ? 'AdminNode' : 'Node'; + const device = this.repoInfo.nodeAdmin ? 'AdminNode' : 'Node'; // UA Format: Firebase//// const options: { [k: string]: object } = { headers: { From c142c568d95ecde1738a8eba9ac2bb0d8f56c8dd Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 3 Sep 2020 15:04:38 +0100 Subject: [PATCH 2/6] Add new unit test --- .../rules-unit-testing/test/database.test.ts | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/packages/rules-unit-testing/test/database.test.ts b/packages/rules-unit-testing/test/database.test.ts index 3846e31df10..6bac58310b5 100644 --- a/packages/rules-unit-testing/test/database.test.ts +++ b/packages/rules-unit-testing/test/database.test.ts @@ -152,6 +152,44 @@ describe('Testing Module Tests', function () { ); }); + it('initializeAdminApp() and initializeTestApp() work together', async function () { + await firebase.loadDatabaseRules({ + databaseName: 'foo', + rules: JSON.stringify({ + 'rules': { + 'public': { '.read': true, '.write': true }, + 'private': { '.read': false, '.write': false } + } + }) + }); + + const adminApp = firebase.initializeAdminApp({ + projectId: 'foo', + databaseName: 'foo' + }); + + const testApp = firebase.initializeTestApp({ + projectId: 'foo', + databaseName: 'foo' + }); + + // Admin app can write anywhere + await firebase.assertSucceeds( + adminApp.database().ref().child('/public/doc').set({ hello: 'admin' }) + ); + await firebase.assertSucceeds( + adminApp.database().ref().child('/private/doc').set({ hello: 'admin' }) + ); + + // Test app can only write to public, not to private + await firebase.assertSucceeds( + testApp.database().ref().child('/public/doc').set({ hello: 'test' }) + ); + await firebase.assertFails( + testApp.database().ref().child('/private/doc').set({ hello: 'test' }) + ); + }); + it('loadDatabaseRules() throws if no databaseName or rules', async function () { // eslint-disable-next-line @typescript-eslint/no-explicit-any await expect((firebase as any).loadDatabaseRules.bind(null, {})).to.throw( From 91166f97b83b68ee6eb81efbc173de13394a957b Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 3 Sep 2020 17:06:22 +0100 Subject: [PATCH 3/6] Review comments --- packages/database/src/realtime/WebSocketConnection.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/database/src/realtime/WebSocketConnection.ts b/packages/database/src/realtime/WebSocketConnection.ts index 71458aaea2e..07295460334 100644 --- a/packages/database/src/realtime/WebSocketConnection.ts +++ b/packages/database/src/realtime/WebSocketConnection.ts @@ -76,6 +76,7 @@ export class WebSocketConnection implements Transport { private stats_: StatsCollection; private everConnected_: boolean; private isClosed_: boolean; + private nodeAdmin: boolean; /** * @param connId identifier for this transport @@ -87,7 +88,7 @@ export class WebSocketConnection implements Transport { */ constructor( public connId: string, - private repoInfo: RepoInfo, + repoInfo: RepoInfo, private applicationId?: string, transportSessionId?: string, lastSessionId?: string @@ -99,6 +100,7 @@ export class WebSocketConnection implements Transport { transportSessionId, lastSessionId ); + this.nodeAdmin = repoInfo.nodeAdmin; } /** @@ -151,7 +153,7 @@ export class WebSocketConnection implements Transport { try { if (isNodeSdk()) { - const device = this.repoInfo.nodeAdmin ? 'AdminNode' : 'Node'; + const device = this.nodeAdmin ? 'AdminNode' : 'Node'; // UA Format: Firebase//// const options: { [k: string]: object } = { headers: { From 2ae1b8d26527fdc5aad777e342d0dad751ab6b30 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 9 Sep 2020 16:46:10 +0100 Subject: [PATCH 4/6] Apply Sebastian's fixes --- packages/database/src/api/Database.ts | 4 +-- packages/database/src/core/RepoInfo.ts | 27 +++++++++---------- packages/database/src/core/RepoManager.ts | 8 ++---- .../database/src/core/util/libs/parser.ts | 9 +++---- 4 files changed, 19 insertions(+), 29 deletions(-) diff --git a/packages/database/src/api/Database.ts b/packages/database/src/api/Database.ts index 0a974f6beff..59cc873d3d1 100644 --- a/packages/database/src/api/Database.ts +++ b/packages/database/src/api/Database.ts @@ -105,11 +105,11 @@ export class Database implements FirebaseService { const apiName = 'database.refFromURL'; this.checkDeleted_(apiName); validateArgCount(apiName, 1, 1, arguments.length); - const parsedURL = parseRepoInfo(url); + const parsedURL = parseRepoInfo(url, this.repo_.repoInfo_.nodeAdmin); validateUrl(apiName, 1, parsedURL); const repoInfo = parsedURL.repoInfo; - if (repoInfo.host !== (this.repo_.repoInfo_ as RepoInfo).host) { + if (repoInfo.host !== this.repo_.repoInfo_.host) { fatal( apiName + ': Host name does not match the current database: ' + diff --git a/packages/database/src/core/RepoInfo.ts b/packages/database/src/core/RepoInfo.ts index d2adf5c3305..79acc631dff 100644 --- a/packages/database/src/core/RepoInfo.ts +++ b/packages/database/src/core/RepoInfo.ts @@ -31,20 +31,21 @@ export class RepoInfo { internalHost: string; /** - * @param {string} host Hostname portion of the url for the repo - * @param {boolean} secure Whether or not this repo is accessed over ssl - * @param {string} namespace The namespace represented by the repo - * @param {boolean} webSocketOnly Whether to prefer websockets over all other transports (used by Nest). - * @param {string=} persistenceKey Override the default session persistence storage key + * @param host Hostname portion of the url for the repo + * @param secure Whether or not this repo is accessed over ssl + * @param namespace The namespace represented by the repo + * @param webSocketOnly Whether to prefer websockets over all other transports (used by Nest). + * @param nodeAdmin Whether this instance uses Admin SDK credentials + * @param persistenceKey Override the default session persistence storage key */ constructor( host: string, - public secure: boolean, - public namespace: string, - public webSocketOnly: boolean, - public persistenceKey: string = '', - public includeNamespaceInQueryParams: boolean = false, - public nodeAdmin: boolean = false + public readonly secure: boolean, + public readonly namespace: string, + public readonly webSocketOnly: boolean, + public readonly nodeAdmin: boolean, + public readonly persistenceKey: string = '', + public readonly includeNamespaceInQueryParams: boolean = false ) { this.host = host.toLowerCase(); this.domain = this.host.substr(this.host.indexOf('.') + 1); @@ -74,10 +75,6 @@ export class RepoInfo { ); } - setNodeAdmin(nodeAdmin: boolean) { - this.nodeAdmin = nodeAdmin; - } - updateHost(newHost: string) { if (newHost !== this.internalHost) { this.internalHost = newHost; diff --git a/packages/database/src/core/RepoManager.ts b/packages/database/src/core/RepoManager.ts index 625b6885131..76d02593849 100644 --- a/packages/database/src/core/RepoManager.ts +++ b/packages/database/src/core/RepoManager.ts @@ -111,7 +111,7 @@ export class RepoManager { ); } - let parsedUrl = parseRepoInfo(dbUrl); + let parsedUrl = parseRepoInfo(dbUrl, nodeAdmin); let repoInfo = parsedUrl.repoInfo; let isEmulator: boolean; @@ -124,16 +124,12 @@ export class RepoManager { if (dbEmulatorHost) { isEmulator = true; dbUrl = `http://${dbEmulatorHost}?ns=${repoInfo.namespace}`; - parsedUrl = parseRepoInfo(dbUrl); + parsedUrl = parseRepoInfo(dbUrl, nodeAdmin); repoInfo = parsedUrl.repoInfo; } else { isEmulator = !parsedUrl.repoInfo.secure; } - if (nodeAdmin) { - repoInfo.setNodeAdmin(nodeAdmin); - } - const authTokenProvider = nodeAdmin && isEmulator ? new EmulatorAdminTokenProvider() diff --git a/packages/database/src/core/util/libs/parser.ts b/packages/database/src/core/util/libs/parser.ts index 5ae1979eaef..bda26166c6f 100644 --- a/packages/database/src/core/util/libs/parser.ts +++ b/packages/database/src/core/util/libs/parser.ts @@ -61,13 +61,9 @@ function decodeQuery(queryString: string): { [key: string]: string } { return results; } -/** - * - * @param {!string} dataURL - * @return {{repoInfo: !RepoInfo, path: !Path}} - */ export const parseRepoInfo = function ( - dataURL: string + dataURL: string, + nodeAdmin: boolean ): { repoInfo: RepoInfo; path: Path } { const parsedUrl = parseDatabaseURL(dataURL), namespace = parsedUrl.namespace; @@ -101,6 +97,7 @@ export const parseRepoInfo = function ( parsedUrl.host, parsedUrl.secure, namespace, + nodeAdmin, webSocketOnly, /*persistenceKey=*/ '', /*includeNamespaceInQueryParams=*/ namespace !== parsedUrl.subdomain From 8a65ae626021f0141a820d48c17ad802cfe35b8e Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 9 Sep 2020 16:51:31 +0100 Subject: [PATCH 5/6] Changeset --- .changeset/polite-readers-wait.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/polite-readers-wait.md diff --git a/.changeset/polite-readers-wait.md b/.changeset/polite-readers-wait.md new file mode 100644 index 00000000000..6608f1998f4 --- /dev/null +++ b/.changeset/polite-readers-wait.md @@ -0,0 +1,6 @@ +--- +'@firebase/database': patch +'@firebase/rules-unit-testing': patch +--- + +Fix detection of admin context in Realtime Database SDK From 6a0270a90dafe35522f827dc2cf8970f9e3076c3 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 9 Sep 2020 17:00:57 +0100 Subject: [PATCH 6/6] Fix compile --- packages/database/src/core/RepoInfo.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/database/src/core/RepoInfo.ts b/packages/database/src/core/RepoInfo.ts index 79acc631dff..25bcef2c609 100644 --- a/packages/database/src/core/RepoInfo.ts +++ b/packages/database/src/core/RepoInfo.ts @@ -43,7 +43,7 @@ export class RepoInfo { public readonly secure: boolean, public readonly namespace: string, public readonly webSocketOnly: boolean, - public readonly nodeAdmin: boolean, + public readonly nodeAdmin: boolean = false, public readonly persistenceKey: string = '', public readonly includeNamespaceInQueryParams: boolean = false ) {