From 9b1a25a133d7e9a15c5558a979046d09ba752c2e Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Wed, 24 Jul 2019 16:35:46 -0700 Subject: [PATCH 1/7] Implementing the Firebase Security Rules API --- .../security-rules-api-client.ts | 86 ++++++ src/security-rules/security-rules-utils.ts | 33 +++ src/security-rules/security-rules.ts | 178 +++++++++++++ src/utils/error.ts | 2 +- test/unit/index.spec.ts | 4 + .../security-rules-api-client.spec.ts | 103 ++++++++ .../security-rules/security-rules.spec.ts | 248 ++++++++++++++++++ 7 files changed, 653 insertions(+), 1 deletion(-) create mode 100644 src/security-rules/security-rules-api-client.ts create mode 100644 src/security-rules/security-rules-utils.ts create mode 100644 src/security-rules/security-rules.ts create mode 100644 test/unit/security-rules/security-rules-api-client.spec.ts create mode 100644 test/unit/security-rules/security-rules.spec.ts diff --git a/src/security-rules/security-rules-api-client.ts b/src/security-rules/security-rules-api-client.ts new file mode 100644 index 0000000000..b2f2a2db58 --- /dev/null +++ b/src/security-rules/security-rules-api-client.ts @@ -0,0 +1,86 @@ +/*! + * Copyright 2019 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { HttpRequestConfig, HttpClient, HttpError } from '../utils/api-request'; +import { PrefixedFirebaseError } from '../utils/error'; +import { FirebaseSecurityRulesError, SecurityRulesErrorCode } from './security-rules-utils'; + +const RULES_API_URL = 'https://firebaserules.googleapis.com/v1/'; + +/** + * Class that facilitates sending requests to the Firebase security rules backend API. + * + * @private + */ +export class SecurityRulesApiClient { + + constructor(private readonly httpClient: HttpClient) { } + + /** + * Gets the specified resource from the rules API. Resource name must be full qualified names with project + * ID prefix (e.g. `projects/project-id/rulesets/ruleset-name`). + * + * @param {string} name Full qualified name of the resource to get. + * @returns {Promise} A promise that fulfills with the resource. + */ + public getResource(name: string): Promise { + const request: HttpRequestConfig = { + method: 'GET', + url: `${RULES_API_URL}${name}`, + }; + return this.httpClient.send(request) + .then((resp) => { + return resp.data as T; + }) + .catch((err) => { + throw this.toFirebaseError(err); + }); + } + + private toFirebaseError(err: HttpError): PrefixedFirebaseError { + if (err instanceof PrefixedFirebaseError) { + return err; + } + + const response = err.response; + if (!response.isJson()) { + return new FirebaseSecurityRulesError( + 'unknown-error', + `Unexpected response with status: ${response.status} and body: ${response.text}`); + } + + const error: Error = (response.data as ErrorResponse).error || {}; + const code = ERROR_CODE_MAPPING[error.status] || 'unknown-error'; + const message = error.message || `Unknown server error: ${response.text}`; + return new FirebaseSecurityRulesError(code, message); + } +} + +interface ErrorResponse { + error?: Error; +} + +interface Error { + code?: number; + message?: string; + status?: string; +} + +const ERROR_CODE_MAPPING: {[key: string]: SecurityRulesErrorCode} = { + NOT_FOUND: 'not-found', + UNAUTHENTICATED: 'authentication-error', + UNKNOWN: 'unknown-error', +}; diff --git a/src/security-rules/security-rules-utils.ts b/src/security-rules/security-rules-utils.ts new file mode 100644 index 0000000000..21f013f311 --- /dev/null +++ b/src/security-rules/security-rules-utils.ts @@ -0,0 +1,33 @@ +/*! + * Copyright 2019 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { PrefixedFirebaseError } from '../utils/error'; + +export type SecurityRulesErrorCode = + 'already-exists' + | 'authentication-error' + | 'internal-error' + | 'invalid-argument' + | 'invalid-server-response' + | 'not-found' + | 'service-unavailable' + | 'unknown-error'; + +export class FirebaseSecurityRulesError extends PrefixedFirebaseError { + constructor(code: SecurityRulesErrorCode, message: string) { + super('security-rules', code, message); + } +} diff --git a/src/security-rules/security-rules.ts b/src/security-rules/security-rules.ts new file mode 100644 index 0000000000..fc336c9dbb --- /dev/null +++ b/src/security-rules/security-rules.ts @@ -0,0 +1,178 @@ +/*! + * Copyright 2019 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { FirebaseServiceInterface, FirebaseServiceInternalsInterface } from '../firebase-service'; +import { FirebaseApp } from '../firebase-app'; +import * as utils from '../utils/index'; +import * as validator from '../utils/validator'; +import { SecurityRulesApiClient } from './security-rules-api-client'; +import { AuthorizedHttpClient } from '../utils/api-request'; +import { FirebaseSecurityRulesError } from './security-rules-utils'; + +/** + * A source file containing some Firebase security rules. + */ +export interface RulesFile { + readonly name: string; + readonly content: string; +} + +/** + * Additional metadata associated with a Ruleset. + */ +export interface RulesetMetadata { + readonly name: string; + readonly createTime: string; +} + +interface Release { + readonly name: string; + readonly rulesetName: string; + readonly createTime: string; + readonly updateTime: string; +} + +interface RulesetResponse { + readonly name: string; + readonly createTime: string; + readonly source: { + readonly files: RulesFile[]; + }; +} + +/** + * Representa a set of Firebase security rules. + */ +export class Ruleset implements RulesetMetadata { + + public readonly name: string; + public readonly createTime: string; + public readonly source: RulesFile[]; + + constructor(ruleset: RulesetResponse) { + if (!validator.isNonNullObject(ruleset) || + !validator.isNonEmptyString(ruleset.name) || + !validator.isNonEmptyString(ruleset.createTime) || + !validator.isNonNullObject(ruleset.source)) { + throw new FirebaseSecurityRulesError( + 'invalid-argument', + `Invalid Ruleset response: ${JSON.stringify(ruleset)}`); + } + + this.name = stripProjectIdPrefix(ruleset.name); + this.createTime = new Date(ruleset.createTime).toUTCString(); + this.source = ruleset.source.files || []; + } +} + +/** + * SecurityRules service bound to the provided app. + */ +export class SecurityRules implements FirebaseServiceInterface { + + private static readonly CLOUD_FIRESTORE = 'cloud.firestore'; + + public readonly INTERNAL = new SecurityRulesInternals(); + + private readonly client: SecurityRulesApiClient; + private readonly projectId: string; + + /** + * @param {object} app The app for this SecurityRules service. + * @constructor + */ + constructor(readonly app: FirebaseApp) { + if (!validator.isNonNullObject(app) || !('options' in app)) { + throw new FirebaseSecurityRulesError( + 'invalid-argument', + 'First argument passed to admin.securityRules() must be a valid Firebase app ' + + 'instance.'); + } + + const projectId = utils.getProjectId(app); + if (!validator.isNonEmptyString(projectId)) { + throw new FirebaseSecurityRulesError( + 'invalid-argument', + 'Failed to determine project ID. Initialize the SDK with service account credentials, or ' + + 'set project ID as an app option. Alternatively, set the GOOGLE_CLOUD_PROJECT ' + + 'environment variable.'); + } + + this.projectId = projectId; + this.client = new SecurityRulesApiClient(new AuthorizedHttpClient(app)); + } + + /** + * Gets the Ruleset identified by the given name. The input name should be the short name string without + * the project ID prefix. Rejects with a `not-found` error if the specified Ruleset cannot be found. + * + * @param {string} name Name of the Ruleset to retrieve. + * @returns {Promise} A promise that fulfills with the specified Ruleset. + */ + public getRuleset(name: string): Promise { + if (!validator.isNonEmptyString(name)) { + const err = new FirebaseSecurityRulesError( + 'invalid-argument', 'Ruleset name must be a non-empty string.'); + return Promise.reject(err); + } + + if (name.indexOf('/') !== -1) { + const err = new FirebaseSecurityRulesError( + 'invalid-argument', 'Ruleset name must not contain any "/" characters.'); + return Promise.reject(err); + } + + const resource = `projects/${this.projectId}/rulesets/${name}`; + return this.client.getResource(resource) + .then((rulesetResponse) => { + return new Ruleset(rulesetResponse); + }); + } + + /** + * Gets the Ruleset currently applied to Cloud Firestore. Rejects with a `not-found` error if no Ruleset is + * applied on Firestore. + * + * @returns {Promise} A promise that fulfills with the Firestore Ruleset. + */ + public getFirestoreRuleset(): Promise { + return this.getRulesetForService(SecurityRules.CLOUD_FIRESTORE); + } + + private getRulesetForService(name: string): Promise { + const resource = `projects/${this.projectId}/releases/${name}`; + return this.client.getResource(resource) + .then((release) => { + const rulesetName = release.rulesetName; + if (!validator.isNonEmptyString(rulesetName)) { + throw new FirebaseSecurityRulesError( + 'not-found', `Ruleset name not found for ${name}.`); + } + + return this.getRuleset(stripProjectIdPrefix(rulesetName)); + }); + } +} + +class SecurityRulesInternals implements FirebaseServiceInternalsInterface { + public delete(): Promise { + return Promise.resolve(); + } +} + +function stripProjectIdPrefix(name: string): string { + return name.split('/').pop(); +} diff --git a/src/utils/error.ts b/src/utils/error.ts index 88e5b72335..08b3222bcf 100755 --- a/src/utils/error.ts +++ b/src/utils/error.ts @@ -80,7 +80,7 @@ export class FirebaseError extends Error { * @param {string} message The error message. * @constructor */ -class PrefixedFirebaseError extends FirebaseError { +export class PrefixedFirebaseError extends FirebaseError { constructor(private codePrefix: string, code: string, message: string) { super({ code: `${codePrefix}/${code}`, diff --git a/test/unit/index.spec.ts b/test/unit/index.spec.ts index e8e73a3aa3..b6b7e48ef1 100755 --- a/test/unit/index.spec.ts +++ b/test/unit/index.spec.ts @@ -58,3 +58,7 @@ import './project-management/project-management.spec'; import './project-management/project-management-api-request.spec'; import './project-management/android-app.spec'; import './project-management/ios-app.spec'; + +// SecurityRules +import './security-rules/security-rules.spec'; +import './security-rules/security-rules-api-client.spec'; diff --git a/test/unit/security-rules/security-rules-api-client.spec.ts b/test/unit/security-rules/security-rules-api-client.spec.ts new file mode 100644 index 0000000000..86c2933db6 --- /dev/null +++ b/test/unit/security-rules/security-rules-api-client.spec.ts @@ -0,0 +1,103 @@ +/*! + * Copyright 2019 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +import * as _ from 'lodash'; +import * as chai from 'chai'; +import * as sinon from 'sinon'; +import { SecurityRulesApiClient } from '../../../src/security-rules/security-rules-api-client'; +import { FirebaseSecurityRulesError } from '../../../src/security-rules/security-rules-utils'; +import { HttpClient } from '../../../src/utils/api-request'; +import * as utils from '../utils'; +import { FirebaseAppError } from '../../../src/utils/error'; + +const expect = chai.expect; + +describe('SecurityRulesApiClient', () => { + + const ERROR_RESPONSE = { + error: { + code: 404, + message: 'Requested entity not found', + status: 'NOT_FOUND', + }, + }; + + const apiClient: SecurityRulesApiClient = new SecurityRulesApiClient(new HttpClient()); + + // Stubs used to simulate underlying api calls. + let stubs: sinon.SinonStub[] = []; + + afterEach(() => { + _.forEach(stubs, (stub) => stub.restore()); + stubs = []; + }); + + describe('getResource', () => { + it('should resolve with the requested resource on success', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .resolves(utils.responseFrom({foo: 'bar'})); + stubs.push(stub); + return apiClient.getResource<{foo: string}>('foo') + .then((resp) => { + expect(resp.foo).to.equal('bar'); + }); + }); + + it('should throw when a full platform error response is received', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(utils.errorFrom(ERROR_RESPONSE, 404)); + stubs.push(stub); + const expected = new FirebaseSecurityRulesError('not-found', 'Requested entity not found'); + return apiClient.getResource('foo') + .should.eventually.be.rejected.and.deep.equal(expected); + }); + + it('should throw unknown-error when error code is not present', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(utils.errorFrom({}, 404)); + stubs.push(stub); + const expected = new FirebaseSecurityRulesError('unknown-error', 'Unknown server error: {}'); + return apiClient.getResource('foo') + .should.eventually.be.rejected.and.deep.equal(expected); + }); + + it('should throw unknown-error for non-json response', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(utils.errorFrom('not json', 404)); + stubs.push(stub); + const expected = new FirebaseSecurityRulesError( + 'unknown-error', 'Unexpected response with status: 404 and body: not json'); + return apiClient.getResource('foo') + .should.eventually.be.rejected.and.deep.equal(expected); + }); + + it('should throw when rejected with a FirebaseAppError', () => { + const expected = new FirebaseAppError('network-error', 'socket hang up'); + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(expected); + stubs.push(stub); + return apiClient.getResource('foo') + .should.eventually.be.rejected.and.deep.equal(expected); + }); + }); +}); diff --git a/test/unit/security-rules/security-rules.spec.ts b/test/unit/security-rules/security-rules.spec.ts new file mode 100644 index 0000000000..89a6a8d0ca --- /dev/null +++ b/test/unit/security-rules/security-rules.spec.ts @@ -0,0 +1,248 @@ +/*! + * Copyright 2019 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +import * as _ from 'lodash'; +import * as chai from 'chai'; +import * as sinon from 'sinon'; +import { SecurityRules } from '../../../src/security-rules/security-rules'; +import { FirebaseApp } from '../../../src/firebase-app'; +import * as mocks from '../../resources/mocks'; +import { SecurityRulesApiClient } from '../../../src/security-rules/security-rules-api-client'; +import { FirebaseSecurityRulesError } from '../../../src/security-rules/security-rules-utils'; +import { deepCopy } from '../../../src/utils/deep-copy'; + +const expect = chai.expect; + +describe('SecurityRules', () => { + + const NO_PROJECT_ID = 'Failed to determine project ID. Initialize the SDK with service ' + + 'account credentials, or set project ID as an app option. Alternatively, set the ' + + 'GOOGLE_CLOUD_PROJECT environment variable.'; + const EXPECTED_ERROR = new FirebaseSecurityRulesError('internal-error', 'message'); + const FIRESTORE_RULESET_RESPONSE = { + name: 'projects/test-project/rulesets/foo', + createTime: '2019-03-08T23:45:23.288047Z', + source: { + files: [ + { + name: 'firestore.rules', + content: 'service cloud.firestore{\n}\n', + }, + ], + }, + }; + + let securityRules: SecurityRules; + let mockApp: FirebaseApp; + let mockCredentialApp: FirebaseApp; + + // Stubs used to simulate underlying api calls. + let stubs: sinon.SinonStub[] = []; + + before(() => { + mockApp = mocks.app(); + mockCredentialApp = mocks.mockCredentialApp(); + securityRules = new SecurityRules(mockApp); + }); + + after(() => { + return mockApp.delete(); + }); + + afterEach(() => { + _.forEach(stubs, (stub) => stub.restore()); + stubs = []; + }); + + describe('Constructor', () => { + const invalidApps = [null, NaN, 0, 1, true, false, '', 'a', [], [1, 'a'], {}, { a: 1 }, _.noop]; + invalidApps.forEach((invalidApp) => { + it('should throw given invalid app: ' + JSON.stringify(invalidApp), () => { + expect(() => { + const securityRulesAny: any = SecurityRules; + return new securityRulesAny(invalidApp); + }).to.throw( + 'First argument passed to admin.securityRules() must be a valid Firebase app ' + + 'instance.'); + }); + }); + + it('should throw given no app', () => { + expect(() => { + const securityRulesAny: any = SecurityRules; + return new securityRulesAny(); + }).to.throw( + 'First argument passed to admin.securityRules() must be a valid Firebase app ' + + 'instance.'); + }); + + it('should throw when initialized without project ID', () => { + // Project ID not set in the environment. + delete process.env.GOOGLE_CLOUD_PROJECT; + delete process.env.GCLOUD_PROJECT; + expect(() => { + return new SecurityRules(mockCredentialApp); + }).to.throw(NO_PROJECT_ID); + }); + + it('should not throw given a valid app', () => { + expect(() => { + return new SecurityRules(mockApp); + }).not.to.throw(); + }); + }); + + describe('app', () => { + it('returns the app from the constructor', () => { + // We expect referential equality here + expect(securityRules.app).to.equal(mockApp); + }); + }); + + describe('getRuleset', () => { + const invalidNames: any[] = [null, '', 1, true, {}, []]; + invalidNames.forEach((invalidName) => { + it(`should reject when called with: ${JSON.stringify(invalidName)}`, () => { + return securityRules.getRuleset(invalidName) + .should.eventually.be.rejected.and.have.property( + 'message', 'Ruleset name must be a non-empty string.'); + }); + }); + + it(`should reject when called with prefixed name`, () => { + return securityRules.getRuleset('projects/foo/rulesets/bar') + .should.eventually.be.rejected.and.have.property( + 'message', 'Ruleset name must not contain any "/" characters.'); + }); + + it('should propagate API errors', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'getResource') + .rejects(EXPECTED_ERROR); + stubs.push(stub); + return securityRules.getRuleset('foo') + .should.eventually.be.rejected.and.deep.equal(EXPECTED_ERROR); + }); + + it('should reject when API response is invalid', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'getResource') + .resolves(null); + stubs.push(stub); + return securityRules.getRuleset('foo') + .should.eventually.be.rejected.and.have.property( + 'message', 'Invalid Ruleset response: null'); + }); + + it('should reject when API response does not contain a name', () => { + const response = deepCopy(FIRESTORE_RULESET_RESPONSE); + response.name = ''; + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'getResource') + .resolves(response); + stubs.push(stub); + return securityRules.getRuleset('foo') + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid Ruleset response: ${JSON.stringify(response)}`); + }); + + it('should reject when API response does not contain a createTime', () => { + const response = deepCopy(FIRESTORE_RULESET_RESPONSE); + response.createTime = ''; + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'getResource') + .resolves(response); + stubs.push(stub); + return securityRules.getRuleset('foo') + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid Ruleset response: ${JSON.stringify(response)}`); + }); + + it('should reject when API response does not contain a source', () => { + const response = deepCopy(FIRESTORE_RULESET_RESPONSE); + response.source = null; + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'getResource') + .resolves(response); + stubs.push(stub); + return securityRules.getRuleset('foo') + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid Ruleset response: ${JSON.stringify(response)}`); + }); + + it('should resolve with Ruleset on success', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'getResource') + .resolves(FIRESTORE_RULESET_RESPONSE); + stubs.push(stub); + + return securityRules.getRuleset('foo') + .then((ruleset) => { + expect(ruleset.name).to.equal('foo'); + expect(ruleset.createTime).to.equal('Fri, 08 Mar 2019 23:45:23 GMT'); + expect(ruleset.source.length).to.equal(1); + + const file = ruleset.source[0]; + expect(file.name).equals('firestore.rules'); + expect(file.content).equals('service cloud.firestore{\n}\n'); + }); + }); + }); + + describe('getFirestoreRuleset', () => { + it('should propagate API errors', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'getResource') + .rejects(EXPECTED_ERROR); + stubs.push(stub); + return securityRules.getFirestoreRuleset() + .should.eventually.be.rejected.and.deep.equal(EXPECTED_ERROR); + }); + + it('should reject when getRelease response is invalid', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'getResource') + .resolves({}); + stubs.push(stub); + + return securityRules.getFirestoreRuleset() + .should.eventually.be.rejected.and.have.property( + 'message', 'Ruleset name not found for cloud.firestore.'); + }); + + it('should resolve with Ruleset on success', () => { + const stub = sinon.stub(SecurityRulesApiClient.prototype, 'getResource'); + stub.onCall(0).resolves({ + rulesetName: 'projects/test-project/rulesets/foo', + }); + stub.onCall(1).resolves(FIRESTORE_RULESET_RESPONSE); + stubs.push(stub); + + return securityRules.getFirestoreRuleset() + .then((ruleset) => { + expect(ruleset.name).to.equal('foo'); + expect(ruleset.createTime).to.equal('Fri, 08 Mar 2019 23:45:23 GMT'); + expect(ruleset.source.length).to.equal(1); + + const file = ruleset.source[0]; + expect(file.name).equals('firestore.rules'); + expect(file.content).equals('service cloud.firestore{\n}\n'); + }); + }); + }); +}); From c57c38230047024ccc3865c941c89f93e5a6a9f7 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Wed, 24 Jul 2019 16:43:46 -0700 Subject: [PATCH 2/7] More argument validation and assertions --- .../security-rules-api-client.ts | 6 +++++ .../security-rules-api-client.spec.ts | 23 +++++++++++++++---- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/security-rules/security-rules-api-client.ts b/src/security-rules/security-rules-api-client.ts index b2f2a2db58..1a944b8f7b 100644 --- a/src/security-rules/security-rules-api-client.ts +++ b/src/security-rules/security-rules-api-client.ts @@ -37,6 +37,12 @@ export class SecurityRulesApiClient { * @returns {Promise} A promise that fulfills with the resource. */ public getResource(name: string): Promise { + if (!name.startsWith('projects/')) { + const err = new FirebaseSecurityRulesError( + 'invalid-argument', 'Resource name must have a project ID prefix.'); + return Promise.reject(err); + } + const request: HttpRequestConfig = { method: 'GET', url: `${RULES_API_URL}${name}`, diff --git a/test/unit/security-rules/security-rules-api-client.spec.ts b/test/unit/security-rules/security-rules-api-client.spec.ts index 86c2933db6..814110d805 100644 --- a/test/unit/security-rules/security-rules-api-client.spec.ts +++ b/test/unit/security-rules/security-rules-api-client.spec.ts @@ -48,24 +48,37 @@ describe('SecurityRulesApiClient', () => { }); describe('getResource', () => { + const RESOURCE_ID = 'projects/test-project/rulesets/ruleset-id'; + it('should resolve with the requested resource on success', () => { const stub = sinon .stub(HttpClient.prototype, 'send') .resolves(utils.responseFrom({foo: 'bar'})); stubs.push(stub); - return apiClient.getResource<{foo: string}>('foo') + return apiClient.getResource<{foo: string}>(RESOURCE_ID) .then((resp) => { expect(resp.foo).to.equal('bar'); + expect(stub).to.have.been.calledOnce.and.calledWith({ + method: 'GET', + url: 'https://firebaserules.googleapis.com/v1/projects/test-project/rulesets/ruleset-id', + }); }); }); + it('should reject when the resource name is unqualified', () => { + const expected = new FirebaseSecurityRulesError( + 'invalid-argument', 'Resource name must have a project ID prefix.'); + return apiClient.getResource('rulesets/ruleset-id') + .should.eventually.be.rejected.and.deep.equal(expected); + }); + it('should throw when a full platform error response is received', () => { const stub = sinon .stub(HttpClient.prototype, 'send') .rejects(utils.errorFrom(ERROR_RESPONSE, 404)); stubs.push(stub); const expected = new FirebaseSecurityRulesError('not-found', 'Requested entity not found'); - return apiClient.getResource('foo') + return apiClient.getResource(RESOURCE_ID) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -75,7 +88,7 @@ describe('SecurityRulesApiClient', () => { .rejects(utils.errorFrom({}, 404)); stubs.push(stub); const expected = new FirebaseSecurityRulesError('unknown-error', 'Unknown server error: {}'); - return apiClient.getResource('foo') + return apiClient.getResource(RESOURCE_ID) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -86,7 +99,7 @@ describe('SecurityRulesApiClient', () => { stubs.push(stub); const expected = new FirebaseSecurityRulesError( 'unknown-error', 'Unexpected response with status: 404 and body: not json'); - return apiClient.getResource('foo') + return apiClient.getResource(RESOURCE_ID) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -96,7 +109,7 @@ describe('SecurityRulesApiClient', () => { .stub(HttpClient.prototype, 'send') .rejects(expected); stubs.push(stub); - return apiClient.getResource('foo') + return apiClient.getResource(RESOURCE_ID) .should.eventually.be.rejected.and.deep.equal(expected); }); }); From 08cb1a408f7bc6122c6963e9302bc11dcfc3d496 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Thu, 25 Jul 2019 15:06:00 -0700 Subject: [PATCH 3/7] Adding the rest of the CRUD operations for rulesets --- .../security-rules-api-client.ts | 55 ++++- src/security-rules/security-rules.ts | 111 +++++++--- .../security-rules-api-client.spec.ts | 30 ++- .../security-rules/security-rules.spec.ts | 195 +++++++++++++++++- 4 files changed, 339 insertions(+), 52 deletions(-) diff --git a/src/security-rules/security-rules-api-client.ts b/src/security-rules/security-rules-api-client.ts index 1a944b8f7b..4dcdebfc27 100644 --- a/src/security-rules/security-rules-api-client.ts +++ b/src/security-rules/security-rules-api-client.ts @@ -17,8 +17,9 @@ import { HttpRequestConfig, HttpClient, HttpError } from '../utils/api-request'; import { PrefixedFirebaseError } from '../utils/error'; import { FirebaseSecurityRulesError, SecurityRulesErrorCode } from './security-rules-utils'; +import * as validator from '../utils/validator'; -const RULES_API_URL = 'https://firebaserules.googleapis.com/v1/'; +const RULES_API_URL = 'https://firebaserules.googleapis.com/v1'; /** * Class that facilitates sending requests to the Firebase security rules backend API. @@ -27,26 +28,58 @@ const RULES_API_URL = 'https://firebaserules.googleapis.com/v1/'; */ export class SecurityRulesApiClient { - constructor(private readonly httpClient: HttpClient) { } + private readonly url: string; + + constructor(private readonly httpClient: HttpClient, projectId: string) { + if (!validator.isNonNullObject(httpClient)) { + throw new FirebaseSecurityRulesError( + 'invalid-argument', 'HttpClient must be a non-null object.'); + } + + if (!validator.isNonEmptyString(projectId)) { + throw new FirebaseSecurityRulesError( + 'invalid-argument', + 'Failed to determine project ID. Initialize the SDK with service account credentials, or ' + + 'set project ID as an app option. Alternatively, set the GOOGLE_CLOUD_PROJECT ' + + 'environment variable.'); + } + + this.url = `${RULES_API_URL}/projects/${projectId}`; + } /** - * Gets the specified resource from the rules API. Resource name must be full qualified names with project - * ID prefix (e.g. `projects/project-id/rulesets/ruleset-name`). + * Gets the specified resource from the rules API. Resource names must be the short names without project + * ID prefix (e.g. `rulesets/ruleset-name`). * * @param {string} name Full qualified name of the resource to get. * @returns {Promise} A promise that fulfills with the resource. */ public getResource(name: string): Promise { - if (!name.startsWith('projects/')) { - const err = new FirebaseSecurityRulesError( - 'invalid-argument', 'Resource name must have a project ID prefix.'); - return Promise.reject(err); - } - const request: HttpRequestConfig = { method: 'GET', - url: `${RULES_API_URL}${name}`, + url: `${this.url}/${name}`, + }; + return this.sendRequest(request); + } + + public createResource(name: string, body: object): Promise { + const request: HttpRequestConfig = { + method: 'POST', + url: `${this.url}/${name}`, + data: body, }; + return this.sendRequest(request); + } + + public deleteResource(name: string): Promise { + const request: HttpRequestConfig = { + method: 'DELETE', + url: `${this.url}/${name}`, + }; + return this.sendRequest(request); + } + + private sendRequest(request: HttpRequestConfig): Promise { return this.httpClient.send(request) .then((resp) => { return resp.data as T; diff --git a/src/security-rules/security-rules.ts b/src/security-rules/security-rules.ts index fc336c9dbb..b31aee0b21 100644 --- a/src/security-rules/security-rules.ts +++ b/src/security-rules/security-rules.ts @@ -45,14 +45,17 @@ interface Release { readonly updateTime: string; } -interface RulesetResponse { - readonly name: string; - readonly createTime: string; +interface RulesetContent { readonly source: { readonly files: RulesFile[]; }; } +interface RulesetResponse extends RulesetContent { + readonly name: string; + readonly createTime: string; +} + /** * Representa a set of Firebase security rules. */ @@ -88,7 +91,6 @@ export class SecurityRules implements FirebaseServiceInterface { public readonly INTERNAL = new SecurityRulesInternals(); private readonly client: SecurityRulesApiClient; - private readonly projectId: string; /** * @param {object} app The app for this SecurityRules service. @@ -103,16 +105,7 @@ export class SecurityRules implements FirebaseServiceInterface { } const projectId = utils.getProjectId(app); - if (!validator.isNonEmptyString(projectId)) { - throw new FirebaseSecurityRulesError( - 'invalid-argument', - 'Failed to determine project ID. Initialize the SDK with service account credentials, or ' - + 'set project ID as an app option. Alternatively, set the GOOGLE_CLOUD_PROJECT ' - + 'environment variable.'); - } - - this.projectId = projectId; - this.client = new SecurityRulesApiClient(new AuthorizedHttpClient(app)); + this.client = new SecurityRulesApiClient(new AuthorizedHttpClient(app), projectId); } /** @@ -123,20 +116,10 @@ export class SecurityRules implements FirebaseServiceInterface { * @returns {Promise} A promise that fulfills with the specified Ruleset. */ public getRuleset(name: string): Promise { - if (!validator.isNonEmptyString(name)) { - const err = new FirebaseSecurityRulesError( - 'invalid-argument', 'Ruleset name must be a non-empty string.'); - return Promise.reject(err); - } - - if (name.indexOf('/') !== -1) { - const err = new FirebaseSecurityRulesError( - 'invalid-argument', 'Ruleset name must not contain any "/" characters.'); - return Promise.reject(err); - } - - const resource = `projects/${this.projectId}/rulesets/${name}`; - return this.client.getResource(resource) + return this.getRulesetName(name) + .then((resource) => { + return this.client.getResource(resource); + }) .then((rulesetResponse) => { return new Ruleset(rulesetResponse); }); @@ -152,8 +135,78 @@ export class SecurityRules implements FirebaseServiceInterface { return this.getRulesetForService(SecurityRules.CLOUD_FIRESTORE); } + public createRulesFileFromSource(name: string, source: string | Buffer): RulesFile { + if (!validator.isNonEmptyString(name)) { + throw new FirebaseSecurityRulesError( + 'invalid-argument', 'Name must be a non-empty string.'); + } + + let content: string; + if (validator.isNonEmptyString(source)) { + content = source; + } else if (validator.isBuffer(source)) { + content = source.toString('utf-8'); + } else { + throw new FirebaseSecurityRulesError( + 'invalid-argument', 'Source must be a non-empty string or a Buffer.'); + } + + return { + name, + content, + }; + } + + public createRuleset(file: RulesFile, ...additionalFiles: RulesFile[]): Promise { + const files = [file, ...additionalFiles]; + for (const rf of files) { + if (!validator.isNonNullObject(rf) || + !validator.isNonEmptyString(rf.name) || + !validator.isNonEmptyString(rf.content)) { + + const err = new FirebaseSecurityRulesError( + 'invalid-argument', `Invalid rules file argument: ${JSON.stringify(rf)}`); + return Promise.reject(err); + } + } + + const ruleset: RulesetContent = { + source: { + files, + }, + }; + + return this.client.createResource('rulesets', ruleset) + .then((rulesetResponse) => { + return new Ruleset(rulesetResponse); + }); + } + + public deleteRuleset(name: string): Promise { + return this.getRulesetName(name) + .then((resource) => { + return this.client.deleteResource(resource); + }); + } + + private getRulesetName(name: string): Promise { + if (!validator.isNonEmptyString(name)) { + const err = new FirebaseSecurityRulesError( + 'invalid-argument', 'Ruleset name must be a non-empty string.'); + return Promise.reject(err); + } + + if (name.indexOf('/') !== -1) { + const err = new FirebaseSecurityRulesError( + 'invalid-argument', 'Ruleset name must not contain any "/" characters.'); + return Promise.reject(err); + } + + return Promise.resolve(`rulesets/${name}`); + } + private getRulesetForService(name: string): Promise { - const resource = `projects/${this.projectId}/releases/${name}`; + const resource = `releases/${name}`; return this.client.getResource(resource) .then((release) => { const rulesetName = release.rulesetName; diff --git a/test/unit/security-rules/security-rules-api-client.spec.ts b/test/unit/security-rules/security-rules-api-client.spec.ts index 814110d805..cc718670ab 100644 --- a/test/unit/security-rules/security-rules-api-client.spec.ts +++ b/test/unit/security-rules/security-rules-api-client.spec.ts @@ -29,6 +29,9 @@ const expect = chai.expect; describe('SecurityRulesApiClient', () => { + const NO_PROJECT_ID = 'Failed to determine project ID. Initialize the SDK with service ' + + 'account credentials, or set project ID as an app option. Alternatively, set the ' + + 'GOOGLE_CLOUD_PROJECT environment variable.'; const ERROR_RESPONSE = { error: { code: 404, @@ -37,7 +40,8 @@ describe('SecurityRulesApiClient', () => { }, }; - const apiClient: SecurityRulesApiClient = new SecurityRulesApiClient(new HttpClient()); + const apiClient: SecurityRulesApiClient = new SecurityRulesApiClient( + new HttpClient(), 'test-project'); // Stubs used to simulate underlying api calls. let stubs: sinon.SinonStub[] = []; @@ -47,8 +51,23 @@ describe('SecurityRulesApiClient', () => { stubs = []; }); + describe('Constructor', () => { + it('should throw when the HttpClient is null', () => { + expect(() => new SecurityRulesApiClient(null, 'test')) + .to.throw('HttpClient must be a non-null object.'); + }); + + const invalidProjectIds: any[] = [null, undefined, '', {}, [], true, 1]; + invalidProjectIds.forEach((invalidProjectId) => { + it(`should throw when the projectId is: ${invalidProjectId}`, () => { + expect(() => new SecurityRulesApiClient(new HttpClient(), invalidProjectId)) + .to.throw(NO_PROJECT_ID); + }); + }); + }); + describe('getResource', () => { - const RESOURCE_ID = 'projects/test-project/rulesets/ruleset-id'; + const RESOURCE_ID = 'rulesets/ruleset-id'; it('should resolve with the requested resource on success', () => { const stub = sinon @@ -65,13 +84,6 @@ describe('SecurityRulesApiClient', () => { }); }); - it('should reject when the resource name is unqualified', () => { - const expected = new FirebaseSecurityRulesError( - 'invalid-argument', 'Resource name must have a project ID prefix.'); - return apiClient.getResource('rulesets/ruleset-id') - .should.eventually.be.rejected.and.deep.equal(expected); - }); - it('should throw when a full platform error response is received', () => { const stub = sinon .stub(HttpClient.prototype, 'send') diff --git a/test/unit/security-rules/security-rules.spec.ts b/test/unit/security-rules/security-rules.spec.ts index 89a6a8d0ca..9b4cff4bb3 100644 --- a/test/unit/security-rules/security-rules.spec.ts +++ b/test/unit/security-rules/security-rules.spec.ts @@ -30,6 +30,7 @@ const expect = chai.expect; describe('SecurityRules', () => { + const INVALID_NAMES: any[] = [null, undefined, '', 1, true, {}, []]; const NO_PROJECT_ID = 'Failed to determine project ID. Initialize the SDK with service ' + 'account credentials, or set project ID as an app option. Alternatively, set the ' + 'GOOGLE_CLOUD_PROJECT environment variable.'; @@ -46,6 +47,7 @@ describe('SecurityRules', () => { ], }, }; + const CREATE_TIME_UTC = 'Fri, 08 Mar 2019 23:45:23 GMT'; let securityRules: SecurityRules; let mockApp: FirebaseApp; @@ -115,8 +117,7 @@ describe('SecurityRules', () => { }); describe('getRuleset', () => { - const invalidNames: any[] = [null, '', 1, true, {}, []]; - invalidNames.forEach((invalidName) => { + INVALID_NAMES.forEach((invalidName) => { it(`should reject when called with: ${JSON.stringify(invalidName)}`, () => { return securityRules.getRuleset(invalidName) .should.eventually.be.rejected.and.have.property( @@ -194,7 +195,7 @@ describe('SecurityRules', () => { return securityRules.getRuleset('foo') .then((ruleset) => { expect(ruleset.name).to.equal('foo'); - expect(ruleset.createTime).to.equal('Fri, 08 Mar 2019 23:45:23 GMT'); + expect(ruleset.createTime).to.equal(CREATE_TIME_UTC); expect(ruleset.source.length).to.equal(1); const file = ruleset.source[0]; @@ -245,4 +246,192 @@ describe('SecurityRules', () => { }); }); }); + + describe('createRulesFileFromSource', () => { + INVALID_NAMES.forEach((invalidName) => { + it(`should throw if the name is ${JSON.stringify(invalidName)}`, () => { + expect(() => securityRules.createRulesFileFromSource(invalidName, 'test')) + .to.throw('Name must be a non-empty string.'); + }); + }); + + const invalidSources = [...INVALID_NAMES]; + invalidSources.forEach((invalidSource) => { + it(`should throw if the source is ${JSON.stringify(invalidSource)}`, () => { + expect(() => securityRules.createRulesFileFromSource('test.rules', invalidSource)) + .to.throw('Source must be a non-empty string or a Buffer.'); + }); + }); + + it('should succeed when source specified as a string', () => { + const file = securityRules.createRulesFileFromSource('test.rules', 'test source {}'); + expect(file.name).to.equal('test.rules'); + expect(file.content).to.equal('test source {}'); + }); + + it('should succeed when source specified as a Buffer', () => { + const file = securityRules.createRulesFileFromSource('test.rules', Buffer.from('test source {}')); + expect(file.name).to.equal('test.rules'); + expect(file.content).to.equal('test source {}'); + }); + }); + + describe('createRuleset', () => { + const RULES_FILE = { + name: 'test.rules', + content: 'test source {}', + }; + + it(`should reject when called with no files`, () => { + return (securityRules as any).createRuleset() + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid rules file argument: ${JSON.stringify(undefined)}`); + }); + + const invalidFiles: any[] = [null, undefined, 'test', {}, {name: 'test'}, {content: 'test'}]; + invalidFiles.forEach((file) => { + it(`should reject when called with: ${JSON.stringify(file)}`, () => { + return securityRules.createRuleset(file) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid rules file argument: ${JSON.stringify(file)}`); + }); + + it(`should reject when called with extra argument: ${JSON.stringify(file)}`, () => { + return securityRules.createRuleset(RULES_FILE, file) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid rules file argument: ${JSON.stringify(file)}`); + }); + }); + + it('should propagate API errors', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'createResource') + .rejects(EXPECTED_ERROR); + stubs.push(stub); + return securityRules.createRuleset(RULES_FILE) + .should.eventually.be.rejected.and.deep.equal(EXPECTED_ERROR); + }); + + it('should reject when API response is invalid', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'createResource') + .resolves(null); + stubs.push(stub); + return securityRules.createRuleset(RULES_FILE) + .should.eventually.be.rejected.and.have.property( + 'message', 'Invalid Ruleset response: null'); + }); + + it('should reject when API response does not contain a name', () => { + const response = deepCopy(FIRESTORE_RULESET_RESPONSE); + response.name = ''; + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'createResource') + .resolves(response); + stubs.push(stub); + return securityRules.createRuleset(RULES_FILE) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid Ruleset response: ${JSON.stringify(response)}`); + }); + + it('should reject when API response does not contain a createTime', () => { + const response = deepCopy(FIRESTORE_RULESET_RESPONSE); + response.createTime = ''; + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'createResource') + .resolves(response); + stubs.push(stub); + return securityRules.createRuleset(RULES_FILE) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid Ruleset response: ${JSON.stringify(response)}`); + }); + + it('should resolve with Ruleset on success', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'createResource') + .resolves(FIRESTORE_RULESET_RESPONSE); + stubs.push(stub); + + return securityRules.createRuleset(RULES_FILE) + .then((ruleset) => { + expect(ruleset.name).to.equal('foo'); + expect(ruleset.createTime).to.equal(CREATE_TIME_UTC); + expect(ruleset.source.length).to.equal(1); + + const file = ruleset.source[0]; + expect(file.name).equals('firestore.rules'); + expect(file.content).equals('service cloud.firestore{\n}\n'); + + const request = { + source: { + files: [ + RULES_FILE, + ], + }, + }; + expect(stub).to.have.been.called.calledOnce.and.calledWith('rulesets', request); + }); + }); + + it('should resolve with Ruleset when called with multiple files', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'createResource') + .resolves(FIRESTORE_RULESET_RESPONSE); + stubs.push(stub); + + return securityRules.createRuleset(RULES_FILE, RULES_FILE) + .then((ruleset) => { + expect(ruleset.name).to.equal('foo'); + expect(ruleset.createTime).to.equal(CREATE_TIME_UTC); + expect(ruleset.source.length).to.equal(1); + + const file = ruleset.source[0]; + expect(file.name).equals('firestore.rules'); + expect(file.content).equals('service cloud.firestore{\n}\n'); + + const request = { + source: { + files: [ + RULES_FILE, + RULES_FILE, + ], + }, + }; + expect(stub).to.have.been.called.calledOnce.and.calledWith('rulesets', request); + }); + }); + }); + + describe('deleteRuleset', () => { + INVALID_NAMES.forEach((invalidName) => { + it(`should reject when called with: ${JSON.stringify(invalidName)}`, () => { + return securityRules.deleteRuleset(invalidName) + .should.eventually.be.rejected.and.have.property( + 'message', 'Ruleset name must be a non-empty string.'); + }); + }); + + it(`should reject when called with prefixed name`, () => { + return securityRules.deleteRuleset('projects/foo/rulesets/bar') + .should.eventually.be.rejected.and.have.property( + 'message', 'Ruleset name must not contain any "/" characters.'); + }); + + it('should propagate API errors', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'deleteResource') + .rejects(EXPECTED_ERROR); + stubs.push(stub); + return securityRules.deleteRuleset('foo') + .should.eventually.be.rejected.and.deep.equal(EXPECTED_ERROR); + }); + + it('should resolve on success', () => { + const stub = sinon + .stub(SecurityRulesApiClient.prototype, 'deleteResource') + .resolves(); + stubs.push(stub); + return securityRules.deleteRuleset('foo'); + }); + }); }); From 0f6fd730198bcc268a46231a8aaa3044a423bdb0 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Thu, 25 Jul 2019 15:20:13 -0700 Subject: [PATCH 4/7] Cleaning up the rules impl --- .../security-rules-api-client.ts | 29 ++++++++++++------- src/security-rules/security-rules.ts | 23 +++++---------- .../security-rules-api-client.spec.ts | 12 ++------ 3 files changed, 28 insertions(+), 36 deletions(-) diff --git a/src/security-rules/security-rules-api-client.ts b/src/security-rules/security-rules-api-client.ts index 1a944b8f7b..8beb244425 100644 --- a/src/security-rules/security-rules-api-client.ts +++ b/src/security-rules/security-rules-api-client.ts @@ -17,8 +17,9 @@ import { HttpRequestConfig, HttpClient, HttpError } from '../utils/api-request'; import { PrefixedFirebaseError } from '../utils/error'; import { FirebaseSecurityRulesError, SecurityRulesErrorCode } from './security-rules-utils'; +import * as validator from '../utils/validator'; -const RULES_API_URL = 'https://firebaserules.googleapis.com/v1/'; +const RULES_API_URL = 'https://firebaserules.googleapis.com/v1'; /** * Class that facilitates sending requests to the Firebase security rules backend API. @@ -27,25 +28,31 @@ const RULES_API_URL = 'https://firebaserules.googleapis.com/v1/'; */ export class SecurityRulesApiClient { - constructor(private readonly httpClient: HttpClient) { } + private readonly url: string; + + constructor(private readonly httpClient: HttpClient, projectId: string) { + if (!validator.isNonEmptyString(projectId)) { + throw new FirebaseSecurityRulesError( + 'invalid-argument', + 'Failed to determine project ID. Initialize the SDK with service account credentials, or ' + + 'set project ID as an app option. Alternatively, set the GOOGLE_CLOUD_PROJECT ' + + 'environment variable.'); + } + + this.url = `${RULES_API_URL}/projects/${projectId}`; + } /** - * Gets the specified resource from the rules API. Resource name must be full qualified names with project - * ID prefix (e.g. `projects/project-id/rulesets/ruleset-name`). + * Gets the specified resource from the rules API. Resource names must be the short names without project + * ID prefix (e.g. `rulesets/ruleset-name`). * * @param {string} name Full qualified name of the resource to get. * @returns {Promise} A promise that fulfills with the resource. */ public getResource(name: string): Promise { - if (!name.startsWith('projects/')) { - const err = new FirebaseSecurityRulesError( - 'invalid-argument', 'Resource name must have a project ID prefix.'); - return Promise.reject(err); - } - const request: HttpRequestConfig = { method: 'GET', - url: `${RULES_API_URL}${name}`, + url: `${this.url}/${name}`, }; return this.httpClient.send(request) .then((resp) => { diff --git a/src/security-rules/security-rules.ts b/src/security-rules/security-rules.ts index fc336c9dbb..928f455a44 100644 --- a/src/security-rules/security-rules.ts +++ b/src/security-rules/security-rules.ts @@ -88,7 +88,6 @@ export class SecurityRules implements FirebaseServiceInterface { public readonly INTERNAL = new SecurityRulesInternals(); private readonly client: SecurityRulesApiClient; - private readonly projectId: string; /** * @param {object} app The app for this SecurityRules service. @@ -103,21 +102,13 @@ export class SecurityRules implements FirebaseServiceInterface { } const projectId = utils.getProjectId(app); - if (!validator.isNonEmptyString(projectId)) { - throw new FirebaseSecurityRulesError( - 'invalid-argument', - 'Failed to determine project ID. Initialize the SDK with service account credentials, or ' - + 'set project ID as an app option. Alternatively, set the GOOGLE_CLOUD_PROJECT ' - + 'environment variable.'); - } - - this.projectId = projectId; - this.client = new SecurityRulesApiClient(new AuthorizedHttpClient(app)); + this.client = new SecurityRulesApiClient(new AuthorizedHttpClient(app), projectId); } /** * Gets the Ruleset identified by the given name. The input name should be the short name string without - * the project ID prefix. Rejects with a `not-found` error if the specified Ruleset cannot be found. + * the project ID prefix. For example, to retrieve the `projects/project-id/rulesets/my-ruleset`, pass the + * short name "my-ruleset". Rejects with a `not-found` error if the specified Ruleset cannot be found. * * @param {string} name Name of the Ruleset to retrieve. * @returns {Promise} A promise that fulfills with the specified Ruleset. @@ -135,7 +126,7 @@ export class SecurityRules implements FirebaseServiceInterface { return Promise.reject(err); } - const resource = `projects/${this.projectId}/rulesets/${name}`; + const resource = `rulesets/${name}`; return this.client.getResource(resource) .then((rulesetResponse) => { return new Ruleset(rulesetResponse); @@ -152,14 +143,14 @@ export class SecurityRules implements FirebaseServiceInterface { return this.getRulesetForService(SecurityRules.CLOUD_FIRESTORE); } - private getRulesetForService(name: string): Promise { - const resource = `projects/${this.projectId}/releases/${name}`; + private getRulesetForService(serviceName: string): Promise { + const resource = `releases/${serviceName}`; return this.client.getResource(resource) .then((release) => { const rulesetName = release.rulesetName; if (!validator.isNonEmptyString(rulesetName)) { throw new FirebaseSecurityRulesError( - 'not-found', `Ruleset name not found for ${name}.`); + 'not-found', `Ruleset name not found for ${serviceName}.`); } return this.getRuleset(stripProjectIdPrefix(rulesetName)); diff --git a/test/unit/security-rules/security-rules-api-client.spec.ts b/test/unit/security-rules/security-rules-api-client.spec.ts index 814110d805..ba222e0c6f 100644 --- a/test/unit/security-rules/security-rules-api-client.spec.ts +++ b/test/unit/security-rules/security-rules-api-client.spec.ts @@ -37,7 +37,8 @@ describe('SecurityRulesApiClient', () => { }, }; - const apiClient: SecurityRulesApiClient = new SecurityRulesApiClient(new HttpClient()); + const apiClient: SecurityRulesApiClient = new SecurityRulesApiClient( + new HttpClient(), 'test-project'); // Stubs used to simulate underlying api calls. let stubs: sinon.SinonStub[] = []; @@ -48,7 +49,7 @@ describe('SecurityRulesApiClient', () => { }); describe('getResource', () => { - const RESOURCE_ID = 'projects/test-project/rulesets/ruleset-id'; + const RESOURCE_ID = 'rulesets/ruleset-id'; it('should resolve with the requested resource on success', () => { const stub = sinon @@ -65,13 +66,6 @@ describe('SecurityRulesApiClient', () => { }); }); - it('should reject when the resource name is unqualified', () => { - const expected = new FirebaseSecurityRulesError( - 'invalid-argument', 'Resource name must have a project ID prefix.'); - return apiClient.getResource('rulesets/ruleset-id') - .should.eventually.be.rejected.and.deep.equal(expected); - }); - it('should throw when a full platform error response is received', () => { const stub = sinon .stub(HttpClient.prototype, 'send') From 1886575ad259fa8e3c47fbe2aba5c83bad2fa0f1 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Thu, 25 Jul 2019 17:04:20 -0700 Subject: [PATCH 5/7] Cleaned up tests --- .../security-rules-api-client.ts | 86 +++++++++-- src/security-rules/security-rules.ts | 70 +-------- .../security-rules-api-client.spec.ts | 133 ++++++++++++----- .../security-rules/security-rules.spec.ts | 138 +++++------------- 4 files changed, 212 insertions(+), 215 deletions(-) diff --git a/src/security-rules/security-rules-api-client.ts b/src/security-rules/security-rules-api-client.ts index ab554fe0c1..99adea198f 100644 --- a/src/security-rules/security-rules-api-client.ts +++ b/src/security-rules/security-rules-api-client.ts @@ -21,6 +21,24 @@ import * as validator from '../utils/validator'; const RULES_V1_API = 'https://firebaserules.googleapis.com/v1'; +export interface Release { + readonly name: string; + readonly rulesetName: string; + readonly createTime: string; + readonly updateTime: string; +} + +export interface RulesetContent { + readonly source: { + readonly files: Array<{name: string, content: string}>; + }; +} + +export interface RulesetResponse extends RulesetContent { + readonly name: string; + readonly createTime: string; +} + /** * Class that facilitates sending requests to the Firebase security rules backend API. * @@ -47,6 +65,45 @@ export class SecurityRulesApiClient { this.url = `${RULES_V1_API}/projects/${projectId}`; } + public getRuleset(name: string): Promise { + return this.getRulesetName(name) + .then((rulesetName) => { + return this.getResource(rulesetName); + }); + } + + public createRuleset(ruleset: RulesetContent): Promise { + if (!validator.isNonNullObject(ruleset) || + !validator.isNonNullObject(ruleset.source) || + !validator.isNonEmptyArray(ruleset.source.files)) { + + const err = new FirebaseSecurityRulesError('invalid-argument', 'Invalid rules content.'); + return Promise.reject(err); + } + + for (const rf of ruleset.source.files) { + if (!validator.isNonNullObject(rf) || + !validator.isNonEmptyString(rf.name) || + !validator.isNonEmptyString(rf.content)) { + + const err = new FirebaseSecurityRulesError( + 'invalid-argument', `Invalid rules file argument: ${JSON.stringify(rf)}`); + return Promise.reject(err); + } + } + + const request: HttpRequestConfig = { + method: 'POST', + url: `${this.url}/rulesets`, + data: ruleset, + }; + return this.sendRequest(request); + } + + public getRelease(name: string): Promise { + return this.getResource(`releases/${name}`); + } + /** * Gets the specified resource from the rules API. Resource names must be the short names without project * ID prefix (e.g. `rulesets/ruleset-name`). @@ -54,7 +111,7 @@ export class SecurityRulesApiClient { * @param {string} name Full qualified name of the resource to get. * @returns {Promise} A promise that fulfills with the resource. */ - public getResource(name: string): Promise { + private getResource(name: string): Promise { const request: HttpRequestConfig = { method: 'GET', url: `${this.url}/${name}`, @@ -62,21 +119,20 @@ export class SecurityRulesApiClient { return this.sendRequest(request); } - public createResource(name: string, body: object): Promise { - const request: HttpRequestConfig = { - method: 'POST', - url: `${this.url}/${name}`, - data: body, - }; - return this.sendRequest(request); - } + private getRulesetName(name: string): Promise { + if (!validator.isNonEmptyString(name)) { + const err = new FirebaseSecurityRulesError( + 'invalid-argument', 'Ruleset name must be a non-empty string.'); + return Promise.reject(err); + } - public deleteResource(name: string): Promise { - const request: HttpRequestConfig = { - method: 'DELETE', - url: `${this.url}/${name}`, - }; - return this.sendRequest(request); + if (name.indexOf('/') !== -1) { + const err = new FirebaseSecurityRulesError( + 'invalid-argument', 'Ruleset name must not contain any "/" characters.'); + return Promise.reject(err); + } + + return Promise.resolve(`rulesets/${name}`); } private sendRequest(request: HttpRequestConfig): Promise { diff --git a/src/security-rules/security-rules.ts b/src/security-rules/security-rules.ts index d78dce242e..825a2f4f34 100644 --- a/src/security-rules/security-rules.ts +++ b/src/security-rules/security-rules.ts @@ -18,7 +18,7 @@ import { FirebaseServiceInterface, FirebaseServiceInternalsInterface } from '../ import { FirebaseApp } from '../firebase-app'; import * as utils from '../utils/index'; import * as validator from '../utils/validator'; -import { SecurityRulesApiClient } from './security-rules-api-client'; +import { SecurityRulesApiClient, RulesetResponse, RulesetContent } from './security-rules-api-client'; import { AuthorizedHttpClient } from '../utils/api-request'; import { FirebaseSecurityRulesError } from './security-rules-utils'; @@ -38,24 +38,6 @@ export interface RulesetMetadata { readonly createTime: string; } -interface Release { - readonly name: string; - readonly rulesetName: string; - readonly createTime: string; - readonly updateTime: string; -} - -interface RulesetContent { - readonly source: { - readonly files: RulesFile[]; - }; -} - -interface RulesetResponse extends RulesetContent { - readonly name: string; - readonly createTime: string; -} - /** * Representa a set of Firebase security rules. */ @@ -117,10 +99,7 @@ export class SecurityRules implements FirebaseServiceInterface { * @returns {Promise} A promise that fulfills with the specified Ruleset. */ public getRuleset(name: string): Promise { - return this.getRulesetName(name) - .then((resource) => { - return this.client.getResource(resource); - }) + return this.client.getRuleset(name) .then((rulesetResponse) => { return new Ruleset(rulesetResponse); }); @@ -133,7 +112,7 @@ export class SecurityRules implements FirebaseServiceInterface { * @returns {Promise} A promise that fulfills with the Firestore Ruleset. */ public getFirestoreRuleset(): Promise { - return this.getRulesetForService(SecurityRules.CLOUD_FIRESTORE); + return this.getRulesetForRelease(SecurityRules.CLOUD_FIRESTORE); } public createRulesFileFromSource(name: string, source: string | Buffer): RulesFile { @@ -160,60 +139,25 @@ export class SecurityRules implements FirebaseServiceInterface { public createRuleset(file: RulesFile, ...additionalFiles: RulesFile[]): Promise { const files = [file, ...additionalFiles]; - for (const rf of files) { - if (!validator.isNonNullObject(rf) || - !validator.isNonEmptyString(rf.name) || - !validator.isNonEmptyString(rf.content)) { - - const err = new FirebaseSecurityRulesError( - 'invalid-argument', `Invalid rules file argument: ${JSON.stringify(rf)}`); - return Promise.reject(err); - } - } - const ruleset: RulesetContent = { source: { files, }, }; - return this.client.createResource('rulesets', ruleset) + return this.client.createRuleset(ruleset) .then((rulesetResponse) => { return new Ruleset(rulesetResponse); }); } - public deleteRuleset(name: string): Promise { - return this.getRulesetName(name) - .then((resource) => { - return this.client.deleteResource(resource); - }); - } - - private getRulesetName(name: string): Promise { - if (!validator.isNonEmptyString(name)) { - const err = new FirebaseSecurityRulesError( - 'invalid-argument', 'Ruleset name must be a non-empty string.'); - return Promise.reject(err); - } - - if (name.indexOf('/') !== -1) { - const err = new FirebaseSecurityRulesError( - 'invalid-argument', 'Ruleset name must not contain any "/" characters.'); - return Promise.reject(err); - } - - return Promise.resolve(`rulesets/${name}`); - } - - private getRulesetForService(serviceName: string): Promise { - const resource = `releases/${serviceName}`; - return this.client.getResource(resource) + private getRulesetForRelease(releaseName: string): Promise { + return this.client.getRelease(releaseName) .then((release) => { const rulesetName = release.rulesetName; if (!validator.isNonEmptyString(rulesetName)) { throw new FirebaseSecurityRulesError( - 'not-found', `Ruleset name not found for ${serviceName}.`); + 'not-found', `Ruleset name not found for ${releaseName}.`); } return this.getRuleset(stripProjectIdPrefix(rulesetName)); diff --git a/test/unit/security-rules/security-rules-api-client.spec.ts b/test/unit/security-rules/security-rules-api-client.spec.ts index f103808321..87063a6f9d 100644 --- a/test/unit/security-rules/security-rules-api-client.spec.ts +++ b/test/unit/security-rules/security-rules-api-client.spec.ts @@ -19,7 +19,7 @@ import * as _ from 'lodash'; import * as chai from 'chai'; import * as sinon from 'sinon'; -import { SecurityRulesApiClient } from '../../../src/security-rules/security-rules-api-client'; +import { SecurityRulesApiClient, RulesetContent } from '../../../src/security-rules/security-rules-api-client'; import { FirebaseSecurityRulesError } from '../../../src/security-rules/security-rules-utils'; import { HttpClient } from '../../../src/utils/api-request'; import * as utils from '../utils'; @@ -29,10 +29,7 @@ const expect = chai.expect; describe('SecurityRulesApiClient', () => { - const NO_PROJECT_ID = 'Failed to determine project ID. Initialize the SDK with service ' - + 'account credentials, or set project ID as an app option. Alternatively, set the ' - + 'GOOGLE_CLOUD_PROJECT environment variable.'; - const RESOURCE_ID = 'rulesets/ruleset-id'; + const RULESET_NAME = 'ruleset-id'; const ERROR_RESPONSE = { error: { code: 404, @@ -59,23 +56,41 @@ describe('SecurityRulesApiClient', () => { }); const invalidProjectIds: any[] = [null, undefined, '', {}, [], true, 1]; + const noProjectId = 'Failed to determine project ID. Initialize the SDK with service ' + + 'account credentials, or set project ID as an app option. Alternatively, set the ' + + 'GOOGLE_CLOUD_PROJECT environment variable.'; invalidProjectIds.forEach((invalidProjectId) => { it(`should throw when the projectId is: ${invalidProjectId}`, () => { expect(() => new SecurityRulesApiClient(new HttpClient(), invalidProjectId)) - .to.throw(NO_PROJECT_ID); + .to.throw(noProjectId); }); }); }); - describe('getResource', () => { - it('should resolve with the requested resource on success', () => { + describe('getRuleset', () => { + const INVALID_NAMES: any[] = [null, undefined, '', 1, true, {}, []]; + INVALID_NAMES.forEach((invalidName) => { + it(`should reject when called with: ${JSON.stringify(invalidName)}`, () => { + return apiClient.getRuleset(invalidName) + .should.eventually.be.rejected.and.have.property( + 'message', 'Ruleset name must be a non-empty string.'); + }); + }); + + it(`should reject when called with prefixed name`, () => { + return apiClient.getRuleset('projects/foo/rulesets/bar') + .should.eventually.be.rejected.and.have.property( + 'message', 'Ruleset name must not contain any "/" characters.'); + }); + + it('should resolve with the requested ruleset on success', () => { const stub = sinon .stub(HttpClient.prototype, 'send') - .resolves(utils.responseFrom({foo: 'bar'})); + .resolves(utils.responseFrom({name: 'bar'})); stubs.push(stub); - return apiClient.getResource<{foo: string}>(RESOURCE_ID) + return apiClient.getRuleset(RULESET_NAME) .then((resp) => { - expect(resp.foo).to.equal('bar'); + expect(resp.name).to.equal('bar'); expect(stub).to.have.been.calledOnce.and.calledWith({ method: 'GET', url: 'https://firebaserules.googleapis.com/v1/projects/test-project/rulesets/ruleset-id', @@ -89,7 +104,7 @@ describe('SecurityRulesApiClient', () => { .rejects(utils.errorFrom(ERROR_RESPONSE, 404)); stubs.push(stub); const expected = new FirebaseSecurityRulesError('not-found', 'Requested entity not found'); - return apiClient.getResource(RESOURCE_ID) + return apiClient.getRuleset(RULESET_NAME) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -99,7 +114,7 @@ describe('SecurityRulesApiClient', () => { .rejects(utils.errorFrom({}, 404)); stubs.push(stub); const expected = new FirebaseSecurityRulesError('unknown-error', 'Unknown server error: {}'); - return apiClient.getResource(RESOURCE_ID) + return apiClient.getRuleset(RULESET_NAME) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -110,7 +125,7 @@ describe('SecurityRulesApiClient', () => { stubs.push(stub); const expected = new FirebaseSecurityRulesError( 'unknown-error', 'Unexpected response with status: 404 and body: not json'); - return apiClient.getResource(RESOURCE_ID) + return apiClient.getRuleset(RULESET_NAME) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -120,27 +135,70 @@ describe('SecurityRulesApiClient', () => { .stub(HttpClient.prototype, 'send') .rejects(expected); stubs.push(stub); - return apiClient.getResource(RESOURCE_ID) + return apiClient.getRuleset(RULESET_NAME) .should.eventually.be.rejected.and.deep.equal(expected); }); }); - describe('createResource', () => { - const data = {foo: 'bar'}; + describe('createRuleset', () => { + const RULES_FILE = { + name: 'test.rules', + content: 'test source {}', + }; + + const RULES_CONTENT: RulesetContent = { + source: { + files: [RULES_FILE], + }, + }; + + const invalidContent: any[] = [null, undefined, {}, {source: {}}]; + invalidContent.forEach((content) => { + it(`should reject when called with: ${JSON.stringify(content)}`, () => { + return apiClient.createRuleset(content) + .should.eventually.be.rejected.and.have.property( + 'message', 'Invalid rules content.'); + }); + }); + + const invalidFiles: any[] = [null, undefined, 'test', {}, {name: 'test'}, {content: 'test'}]; + invalidFiles.forEach((file) => { + it(`should reject when called with: ${JSON.stringify(file)}`, () => { + const ruleset: RulesetContent = { + source: { + files: [file], + }, + }; + return apiClient.createRuleset(ruleset) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid rules file argument: ${JSON.stringify(file)}`); + }); + + it(`should reject when called with extra argument: ${JSON.stringify(file)}`, () => { + const ruleset: RulesetContent = { + source: { + files: [RULES_FILE, file], + }, + }; + return apiClient.createRuleset(ruleset) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid rules file argument: ${JSON.stringify(file)}`); + }); + }); it('should resolve with the created resource on success', () => { const stub = sinon .stub(HttpClient.prototype, 'send') - .resolves(utils.responseFrom({name: 'some-name', ...data})); + .resolves(utils.responseFrom({name: 'some-name', ...RULES_CONTENT})); stubs.push(stub); - return apiClient.createResource<{name: string, foo: string}>('rulesets', data) + return apiClient.createRuleset(RULES_CONTENT) .then((resp) => { expect(resp.name).to.equal('some-name'); - expect(resp.foo).to.equal('bar'); + expect(resp.source).to.not.be.undefined; expect(stub).to.have.been.calledOnce.and.calledWith({ method: 'POST', url: 'https://firebaserules.googleapis.com/v1/projects/test-project/rulesets', - data, + data: RULES_CONTENT, }); }); }); @@ -151,7 +209,7 @@ describe('SecurityRulesApiClient', () => { .rejects(utils.errorFrom(ERROR_RESPONSE, 404)); stubs.push(stub); const expected = new FirebaseSecurityRulesError('not-found', 'Requested entity not found'); - return apiClient.createResource(RESOURCE_ID, data) + return apiClient.createRuleset(RULES_CONTENT) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -161,7 +219,7 @@ describe('SecurityRulesApiClient', () => { .rejects(utils.errorFrom({}, 404)); stubs.push(stub); const expected = new FirebaseSecurityRulesError('unknown-error', 'Unknown server error: {}'); - return apiClient.createResource(RESOURCE_ID, data) + return apiClient.createRuleset(RULES_CONTENT) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -172,7 +230,7 @@ describe('SecurityRulesApiClient', () => { stubs.push(stub); const expected = new FirebaseSecurityRulesError( 'unknown-error', 'Unexpected response with status: 404 and body: not json'); - return apiClient.createResource(RESOURCE_ID, data) + return apiClient.createRuleset(RULES_CONTENT) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -182,22 +240,25 @@ describe('SecurityRulesApiClient', () => { .stub(HttpClient.prototype, 'send') .rejects(expected); stubs.push(stub); - return apiClient.createResource(RESOURCE_ID, data) + return apiClient.createRuleset(RULES_CONTENT) .should.eventually.be.rejected.and.deep.equal(expected); }); }); - describe('deleteResource', () => { - it('should resolve on success', () => { + describe('getRelease', () => { + const RELEASE_NAME = 'test.service'; + + it('should resolve with the requested release on success', () => { const stub = sinon .stub(HttpClient.prototype, 'send') - .resolves(utils.responseFrom({})); + .resolves(utils.responseFrom({name: 'bar'})); stubs.push(stub); - return apiClient.deleteResource(RESOURCE_ID) - .then(() => { + return apiClient.getRelease(RELEASE_NAME) + .then((resp) => { + expect(resp.name).to.equal('bar'); expect(stub).to.have.been.calledOnce.and.calledWith({ - method: 'DELETE', - url: 'https://firebaserules.googleapis.com/v1/projects/test-project/rulesets/ruleset-id', + method: 'GET', + url: 'https://firebaserules.googleapis.com/v1/projects/test-project/releases/test.service', }); }); }); @@ -208,7 +269,7 @@ describe('SecurityRulesApiClient', () => { .rejects(utils.errorFrom(ERROR_RESPONSE, 404)); stubs.push(stub); const expected = new FirebaseSecurityRulesError('not-found', 'Requested entity not found'); - return apiClient.deleteResource(RESOURCE_ID) + return apiClient.getRelease(RELEASE_NAME) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -218,7 +279,7 @@ describe('SecurityRulesApiClient', () => { .rejects(utils.errorFrom({}, 404)); stubs.push(stub); const expected = new FirebaseSecurityRulesError('unknown-error', 'Unknown server error: {}'); - return apiClient.deleteResource(RESOURCE_ID) + return apiClient.getRelease(RELEASE_NAME) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -229,7 +290,7 @@ describe('SecurityRulesApiClient', () => { stubs.push(stub); const expected = new FirebaseSecurityRulesError( 'unknown-error', 'Unexpected response with status: 404 and body: not json'); - return apiClient.deleteResource(RESOURCE_ID) + return apiClient.getRelease(RELEASE_NAME) .should.eventually.be.rejected.and.deep.equal(expected); }); @@ -239,7 +300,7 @@ describe('SecurityRulesApiClient', () => { .stub(HttpClient.prototype, 'send') .rejects(expected); stubs.push(stub); - return apiClient.deleteResource(RESOURCE_ID) + return apiClient.getRelease(RELEASE_NAME) .should.eventually.be.rejected.and.deep.equal(expected); }); }); diff --git a/test/unit/security-rules/security-rules.spec.ts b/test/unit/security-rules/security-rules.spec.ts index 9b4cff4bb3..acfec9b7db 100644 --- a/test/unit/security-rules/security-rules.spec.ts +++ b/test/unit/security-rules/security-rules.spec.ts @@ -22,7 +22,7 @@ import * as sinon from 'sinon'; import { SecurityRules } from '../../../src/security-rules/security-rules'; import { FirebaseApp } from '../../../src/firebase-app'; import * as mocks from '../../resources/mocks'; -import { SecurityRulesApiClient } from '../../../src/security-rules/security-rules-api-client'; +import { SecurityRulesApiClient, RulesetContent } from '../../../src/security-rules/security-rules-api-client'; import { FirebaseSecurityRulesError } from '../../../src/security-rules/security-rules-utils'; import { deepCopy } from '../../../src/utils/deep-copy'; @@ -30,10 +30,6 @@ const expect = chai.expect; describe('SecurityRules', () => { - const INVALID_NAMES: any[] = [null, undefined, '', 1, true, {}, []]; - const NO_PROJECT_ID = 'Failed to determine project ID. Initialize the SDK with service ' - + 'account credentials, or set project ID as an app option. Alternatively, set the ' - + 'GOOGLE_CLOUD_PROJECT environment variable.'; const EXPECTED_ERROR = new FirebaseSecurityRulesError('internal-error', 'message'); const FIRESTORE_RULESET_RESPONSE = { name: 'projects/test-project/rulesets/foo', @@ -97,9 +93,12 @@ describe('SecurityRules', () => { // Project ID not set in the environment. delete process.env.GOOGLE_CLOUD_PROJECT; delete process.env.GCLOUD_PROJECT; + const noProjectId = 'Failed to determine project ID. Initialize the SDK with service ' + + 'account credentials, or set project ID as an app option. Alternatively, set the ' + + 'GOOGLE_CLOUD_PROJECT environment variable.'; expect(() => { return new SecurityRules(mockCredentialApp); - }).to.throw(NO_PROJECT_ID); + }).to.throw(noProjectId); }); it('should not throw given a valid app', () => { @@ -117,23 +116,9 @@ describe('SecurityRules', () => { }); describe('getRuleset', () => { - INVALID_NAMES.forEach((invalidName) => { - it(`should reject when called with: ${JSON.stringify(invalidName)}`, () => { - return securityRules.getRuleset(invalidName) - .should.eventually.be.rejected.and.have.property( - 'message', 'Ruleset name must be a non-empty string.'); - }); - }); - - it(`should reject when called with prefixed name`, () => { - return securityRules.getRuleset('projects/foo/rulesets/bar') - .should.eventually.be.rejected.and.have.property( - 'message', 'Ruleset name must not contain any "/" characters.'); - }); - it('should propagate API errors', () => { const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'getResource') + .stub(SecurityRulesApiClient.prototype, 'getRuleset') .rejects(EXPECTED_ERROR); stubs.push(stub); return securityRules.getRuleset('foo') @@ -142,7 +127,7 @@ describe('SecurityRules', () => { it('should reject when API response is invalid', () => { const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'getResource') + .stub(SecurityRulesApiClient.prototype, 'getRuleset') .resolves(null); stubs.push(stub); return securityRules.getRuleset('foo') @@ -154,7 +139,7 @@ describe('SecurityRules', () => { const response = deepCopy(FIRESTORE_RULESET_RESPONSE); response.name = ''; const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'getResource') + .stub(SecurityRulesApiClient.prototype, 'getRuleset') .resolves(response); stubs.push(stub); return securityRules.getRuleset('foo') @@ -166,7 +151,7 @@ describe('SecurityRules', () => { const response = deepCopy(FIRESTORE_RULESET_RESPONSE); response.createTime = ''; const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'getResource') + .stub(SecurityRulesApiClient.prototype, 'getRuleset') .resolves(response); stubs.push(stub); return securityRules.getRuleset('foo') @@ -178,7 +163,7 @@ describe('SecurityRules', () => { const response = deepCopy(FIRESTORE_RULESET_RESPONSE); response.source = null; const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'getResource') + .stub(SecurityRulesApiClient.prototype, 'getRuleset') .resolves(response); stubs.push(stub); return securityRules.getRuleset('foo') @@ -188,7 +173,7 @@ describe('SecurityRules', () => { it('should resolve with Ruleset on success', () => { const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'getResource') + .stub(SecurityRulesApiClient.prototype, 'getRuleset') .resolves(FIRESTORE_RULESET_RESPONSE); stubs.push(stub); @@ -208,7 +193,7 @@ describe('SecurityRules', () => { describe('getFirestoreRuleset', () => { it('should propagate API errors', () => { const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'getResource') + .stub(SecurityRulesApiClient.prototype, 'getRelease') .rejects(EXPECTED_ERROR); stubs.push(stub); return securityRules.getFirestoreRuleset() @@ -217,7 +202,7 @@ describe('SecurityRules', () => { it('should reject when getRelease response is invalid', () => { const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'getResource') + .stub(SecurityRulesApiClient.prototype, 'getRelease') .resolves({}); stubs.push(stub); @@ -227,17 +212,20 @@ describe('SecurityRules', () => { }); it('should resolve with Ruleset on success', () => { - const stub = sinon.stub(SecurityRulesApiClient.prototype, 'getResource'); - stub.onCall(0).resolves({ - rulesetName: 'projects/test-project/rulesets/foo', - }); - stub.onCall(1).resolves(FIRESTORE_RULESET_RESPONSE); - stubs.push(stub); + const getRelease = sinon + .stub(SecurityRulesApiClient.prototype, 'getRelease') + .resolves({ + rulesetName: 'projects/test-project/rulesets/foo', + }); + const getRuleset = sinon + .stub(SecurityRulesApiClient.prototype, 'getRuleset') + .resolves(FIRESTORE_RULESET_RESPONSE); + stubs.push(getRelease, getRuleset); return securityRules.getFirestoreRuleset() .then((ruleset) => { expect(ruleset.name).to.equal('foo'); - expect(ruleset.createTime).to.equal('Fri, 08 Mar 2019 23:45:23 GMT'); + expect(ruleset.createTime).to.equal(CREATE_TIME_UTC); expect(ruleset.source.length).to.equal(1); const file = ruleset.source[0]; @@ -248,14 +236,16 @@ describe('SecurityRules', () => { }); describe('createRulesFileFromSource', () => { - INVALID_NAMES.forEach((invalidName) => { + const INVALID_STRINGS: any[] = [null, undefined, '', 1, true, {}, []]; + + INVALID_STRINGS.forEach((invalidName) => { it(`should throw if the name is ${JSON.stringify(invalidName)}`, () => { expect(() => securityRules.createRulesFileFromSource(invalidName, 'test')) .to.throw('Name must be a non-empty string.'); }); }); - const invalidSources = [...INVALID_NAMES]; + const invalidSources = [...INVALID_STRINGS]; invalidSources.forEach((invalidSource) => { it(`should throw if the source is ${JSON.stringify(invalidSource)}`, () => { expect(() => securityRules.createRulesFileFromSource('test.rules', invalidSource)) @@ -282,30 +272,9 @@ describe('SecurityRules', () => { content: 'test source {}', }; - it(`should reject when called with no files`, () => { - return (securityRules as any).createRuleset() - .should.eventually.be.rejected.and.have.property( - 'message', `Invalid rules file argument: ${JSON.stringify(undefined)}`); - }); - - const invalidFiles: any[] = [null, undefined, 'test', {}, {name: 'test'}, {content: 'test'}]; - invalidFiles.forEach((file) => { - it(`should reject when called with: ${JSON.stringify(file)}`, () => { - return securityRules.createRuleset(file) - .should.eventually.be.rejected.and.have.property( - 'message', `Invalid rules file argument: ${JSON.stringify(file)}`); - }); - - it(`should reject when called with extra argument: ${JSON.stringify(file)}`, () => { - return securityRules.createRuleset(RULES_FILE, file) - .should.eventually.be.rejected.and.have.property( - 'message', `Invalid rules file argument: ${JSON.stringify(file)}`); - }); - }); - it('should propagate API errors', () => { const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'createResource') + .stub(SecurityRulesApiClient.prototype, 'createRuleset') .rejects(EXPECTED_ERROR); stubs.push(stub); return securityRules.createRuleset(RULES_FILE) @@ -314,7 +283,7 @@ describe('SecurityRules', () => { it('should reject when API response is invalid', () => { const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'createResource') + .stub(SecurityRulesApiClient.prototype, 'createRuleset') .resolves(null); stubs.push(stub); return securityRules.createRuleset(RULES_FILE) @@ -326,7 +295,7 @@ describe('SecurityRules', () => { const response = deepCopy(FIRESTORE_RULESET_RESPONSE); response.name = ''; const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'createResource') + .stub(SecurityRulesApiClient.prototype, 'createRuleset') .resolves(response); stubs.push(stub); return securityRules.createRuleset(RULES_FILE) @@ -338,7 +307,7 @@ describe('SecurityRules', () => { const response = deepCopy(FIRESTORE_RULESET_RESPONSE); response.createTime = ''; const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'createResource') + .stub(SecurityRulesApiClient.prototype, 'createRuleset') .resolves(response); stubs.push(stub); return securityRules.createRuleset(RULES_FILE) @@ -348,7 +317,7 @@ describe('SecurityRules', () => { it('should resolve with Ruleset on success', () => { const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'createResource') + .stub(SecurityRulesApiClient.prototype, 'createRuleset') .resolves(FIRESTORE_RULESET_RESPONSE); stubs.push(stub); @@ -362,20 +331,20 @@ describe('SecurityRules', () => { expect(file.name).equals('firestore.rules'); expect(file.content).equals('service cloud.firestore{\n}\n'); - const request = { + const request: RulesetContent = { source: { files: [ RULES_FILE, ], }, }; - expect(stub).to.have.been.called.calledOnce.and.calledWith('rulesets', request); + expect(stub).to.have.been.called.calledOnce.and.calledWith(request); }); }); it('should resolve with Ruleset when called with multiple files', () => { const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'createResource') + .stub(SecurityRulesApiClient.prototype, 'createRuleset') .resolves(FIRESTORE_RULESET_RESPONSE); stubs.push(stub); @@ -389,7 +358,7 @@ describe('SecurityRules', () => { expect(file.name).equals('firestore.rules'); expect(file.content).equals('service cloud.firestore{\n}\n'); - const request = { + const request: RulesetContent = { source: { files: [ RULES_FILE, @@ -397,41 +366,8 @@ describe('SecurityRules', () => { ], }, }; - expect(stub).to.have.been.called.calledOnce.and.calledWith('rulesets', request); + expect(stub).to.have.been.called.calledOnce.and.calledWith(request); }); }); }); - - describe('deleteRuleset', () => { - INVALID_NAMES.forEach((invalidName) => { - it(`should reject when called with: ${JSON.stringify(invalidName)}`, () => { - return securityRules.deleteRuleset(invalidName) - .should.eventually.be.rejected.and.have.property( - 'message', 'Ruleset name must be a non-empty string.'); - }); - }); - - it(`should reject when called with prefixed name`, () => { - return securityRules.deleteRuleset('projects/foo/rulesets/bar') - .should.eventually.be.rejected.and.have.property( - 'message', 'Ruleset name must not contain any "/" characters.'); - }); - - it('should propagate API errors', () => { - const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'deleteResource') - .rejects(EXPECTED_ERROR); - stubs.push(stub); - return securityRules.deleteRuleset('foo') - .should.eventually.be.rejected.and.deep.equal(EXPECTED_ERROR); - }); - - it('should resolve on success', () => { - const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'deleteResource') - .resolves(); - stubs.push(stub); - return securityRules.deleteRuleset('foo'); - }); - }); }); From 608ee108a34326ae83f15553fa1749c6bd2352bc Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Mon, 29 Jul 2019 13:35:47 -0700 Subject: [PATCH 6/7] Adding some missing comments --- src/security-rules/security-rules.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/security-rules/security-rules.ts b/src/security-rules/security-rules.ts index cdaa02eccf..de73cc55e2 100644 --- a/src/security-rules/security-rules.ts +++ b/src/security-rules/security-rules.ts @@ -115,6 +115,14 @@ export class SecurityRules implements FirebaseServiceInterface { return this.getRulesetForRelease(SecurityRules.CLOUD_FIRESTORE); } + /** + * Creates a `RulesFile` with the given name and source. Throws if any of the arguments are invalid. This is a + * local operation, and does not involve any network API calls. + * + * @param {string} name Name to assign to the rules file. + * @param {string|Buffer} source Contents of the rules file. + * @returns {RulesFile} A new rules file instance. + */ public createRulesFileFromSource(name: string, source: string | Buffer): RulesFile { if (!validator.isNonEmptyString(name)) { throw new FirebaseSecurityRulesError( @@ -137,6 +145,13 @@ export class SecurityRules implements FirebaseServiceInterface { }; } + /** + * Creates a new `Ruleset` with one or more rules files. + * + * @param {RulesFile} file Rules file to include in the new Ruleset. + * @param {RulesFile[]} additionalFiles Additional rules files to be included in the new Ruleset. + * @returns {Promise} A promise that fulfills with the newly created Ruleset. + */ public createRuleset(file: RulesFile, ...additionalFiles: RulesFile[]): Promise { const files = [file, ...additionalFiles]; const ruleset: RulesetContent = { From 0990f8cc85f882ccabd58be9a93a8eef43e31961 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Mon, 29 Jul 2019 15:29:47 -0700 Subject: [PATCH 7/7] Removing support for multiple rules files in create() --- .../security-rules-api-client.ts | 15 +++++----- src/security-rules/security-rules.ts | 8 ++---- .../security-rules/security-rules.spec.ts | 28 ------------------- 3 files changed, 11 insertions(+), 40 deletions(-) diff --git a/src/security-rules/security-rules-api-client.ts b/src/security-rules/security-rules-api-client.ts index 99adea198f..ca3fd376fc 100644 --- a/src/security-rules/security-rules-api-client.ts +++ b/src/security-rules/security-rules-api-client.ts @@ -66,7 +66,10 @@ export class SecurityRulesApiClient { } public getRuleset(name: string): Promise { - return this.getRulesetName(name) + return Promise.resolve() + .then(() => { + return this.getRulesetName(name); + }) .then((rulesetName) => { return this.getResource(rulesetName); }); @@ -119,20 +122,18 @@ export class SecurityRulesApiClient { return this.sendRequest(request); } - private getRulesetName(name: string): Promise { + private getRulesetName(name: string): string { if (!validator.isNonEmptyString(name)) { - const err = new FirebaseSecurityRulesError( + throw new FirebaseSecurityRulesError( 'invalid-argument', 'Ruleset name must be a non-empty string.'); - return Promise.reject(err); } if (name.indexOf('/') !== -1) { - const err = new FirebaseSecurityRulesError( + throw new FirebaseSecurityRulesError( 'invalid-argument', 'Ruleset name must not contain any "/" characters.'); - return Promise.reject(err); } - return Promise.resolve(`rulesets/${name}`); + return `rulesets/${name}`; } private sendRequest(request: HttpRequestConfig): Promise { diff --git a/src/security-rules/security-rules.ts b/src/security-rules/security-rules.ts index de73cc55e2..2b54f40a7d 100644 --- a/src/security-rules/security-rules.ts +++ b/src/security-rules/security-rules.ts @@ -146,17 +146,15 @@ export class SecurityRules implements FirebaseServiceInterface { } /** - * Creates a new `Ruleset` with one or more rules files. + * Creates a new `Ruleset` from the given `RulesFile`. * * @param {RulesFile} file Rules file to include in the new Ruleset. - * @param {RulesFile[]} additionalFiles Additional rules files to be included in the new Ruleset. * @returns {Promise} A promise that fulfills with the newly created Ruleset. */ - public createRuleset(file: RulesFile, ...additionalFiles: RulesFile[]): Promise { - const files = [file, ...additionalFiles]; + public createRuleset(file: RulesFile): Promise { const ruleset: RulesetContent = { source: { - files, + files: [ file ], }, }; diff --git a/test/unit/security-rules/security-rules.spec.ts b/test/unit/security-rules/security-rules.spec.ts index acfec9b7db..3ead22b361 100644 --- a/test/unit/security-rules/security-rules.spec.ts +++ b/test/unit/security-rules/security-rules.spec.ts @@ -341,33 +341,5 @@ describe('SecurityRules', () => { expect(stub).to.have.been.called.calledOnce.and.calledWith(request); }); }); - - it('should resolve with Ruleset when called with multiple files', () => { - const stub = sinon - .stub(SecurityRulesApiClient.prototype, 'createRuleset') - .resolves(FIRESTORE_RULESET_RESPONSE); - stubs.push(stub); - - return securityRules.createRuleset(RULES_FILE, RULES_FILE) - .then((ruleset) => { - expect(ruleset.name).to.equal('foo'); - expect(ruleset.createTime).to.equal(CREATE_TIME_UTC); - expect(ruleset.source.length).to.equal(1); - - const file = ruleset.source[0]; - expect(file.name).equals('firestore.rules'); - expect(file.content).equals('service cloud.firestore{\n}\n'); - - const request: RulesetContent = { - source: { - files: [ - RULES_FILE, - RULES_FILE, - ], - }, - }; - expect(stub).to.have.been.called.calledOnce.and.calledWith(request); - }); - }); }); });