Skip to content

Remove usage of the NODE_ADMIN global in RTDB #3736

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
Sep 9, 2020
Merged

Remove usage of the NODE_ADMIN global in RTDB #3736

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
61e652d
Remove usage of the NODE_ADMIN global in RTDB
samtstern Sep 3, 2020
c142c56
Add new unit test
samtstern Sep 3, 2020
91166f9
Review comments
samtstern Sep 3, 2020
2ae1b8d
Apply Sebastian's fixes
samtstern Sep 9, 2020
8a65ae6
Changeset
samtstern Sep 9, 2020
6a0270a
Fix compile
samtstern Sep 9, 2020
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
13 changes: 10 additions & 3 deletions packages/database/index.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this constant since RTDB is the only consumer of it. If any other JS SDKs were used by the admin SDK in the future, they should not rely on this constant either.

Copy link
Member

Choose a reason for hiding this comment

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

We should hold off on this. firebase-admin does not pin its dependency against @firebase/database, so we can only remove this in a major version bump to avoid breaking customers.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I think I misunderstood -- this is all internal in this package and utils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Feiyang1 so I still want isNodeSdk() to work ... where does NODE_ADMIN actually come from when it's not set in initStandalone? Is it set in part of the build process?

Copy link
Member

Choose a reason for hiding this comment

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

It defaults to false unless set by initStandalone. https://github.com/firebase/firebase-js-sdk/blob/master/packages/util/src/constants.ts#L30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not comfortable removing this at the moment ... it's used within isNodeSdk() which is used in many places including one place where module.exports is redefined. I think removing this is likely to break something I don't understand. Maybe someone from JSCore feels confident enough to do it as a follow up.

Copy link
Member

Choose a reason for hiding this comment

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

The check for CONSTANTS.NODE_ADMIN is unnecessary in isNodeSdk() because CONSTANTS.NODE_CLIENT will be always true when CONSTANTS.NODE_ADMIN is true. Sure, I will do a follow up PR for this.

setSDKVersion(version);

/**
Expand All @@ -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,
Expand Down
12 changes: 7 additions & 5 deletions packages/database/src/core/PersistentConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down
7 changes: 6 additions & 1 deletion packages/database/src/core/RepoInfo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -73,6 +74,10 @@ export class RepoInfo {
);
}

setNodeAdmin(nodeAdmin: boolean) {
this.nodeAdmin = nodeAdmin;
}

updateHost(newHost: string) {
if (newHost !== this.internalHost) {
this.internalHost = newHost;
Expand Down
9 changes: 7 additions & 2 deletions packages/database/src/core/RepoManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ export class RepoManager {
databaseFromApp(
app: FirebaseApp,
authProvider: Provider<FirebaseAuthInternalName>,
url?: string
url?: string,
nodeAdmin?: boolean
): Database {
let dbUrl: string | undefined = url || app.options[DATABASE_URL_OPTION];
if (dbUrl === undefined) {
Expand Down Expand Up @@ -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);

Expand Down
4 changes: 3 additions & 1 deletion packages/database/src/realtime/WebSocketConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -99,6 +100,7 @@ export class WebSocketConnection implements Transport {
transportSessionId,
lastSessionId
);
this.nodeAdmin = repoInfo.nodeAdmin;
}

/**
Expand Down Expand Up @@ -151,7 +153,7 @@ export class WebSocketConnection implements Transport {

try {
if (isNodeSdk()) {
const device = ENV_CONSTANTS.NODE_ADMIN ? 'AdminNode' : 'Node';
const device = this.nodeAdmin ? 'AdminNode' : 'Node';
// UA Format: Firebase/<wire_protocol>/<sdk_version>/<platform>/<device>
const options: { [k: string]: object } = {
headers: {
Expand Down
38 changes: 38 additions & 0 deletions packages/rules-unit-testing/test/database.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down