Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 24 additions & 3 deletions src/security-rules/security-rules-api-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ const RULES_V1_API = 'https://firebaserules.googleapis.com/v1';
export interface Release {
readonly name: string;
readonly rulesetName: string;
readonly createTime: string;
readonly updateTime: string;
readonly createTime?: string;
readonly updateTime?: string;

Choose a reason for hiding this comment

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

Is there a reason to expect that we won't get the create and updateTimes back, or you're just signaling that we don't rely on them for anything?

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 use the same interface as an input argument when updating/patching a release. In this case I wanted the flexibility to not specify these 2 attributes. And you're right -- we also don't rely on them for anything else.

}

export interface RulesetContent {
Expand All @@ -46,6 +46,7 @@ export interface RulesetResponse extends RulesetContent {
*/
export class SecurityRulesApiClient {

private readonly projectIdPrefix: string;
private readonly url: string;

constructor(private readonly httpClient: HttpClient, projectId: string) {
Expand All @@ -62,7 +63,8 @@ export class SecurityRulesApiClient {
+ 'environment variable.');
}

this.url = `${RULES_V1_API}/projects/${projectId}`;
this.projectIdPrefix = `projects/${projectId}`;
this.url = `${RULES_V1_API}/${this.projectIdPrefix}`;
}

public getRuleset(name: string): Promise<RulesetResponse> {
Expand Down Expand Up @@ -121,6 +123,18 @@ export class SecurityRulesApiClient {
return this.getResource<Release>(`releases/${name}`);
}

public updateRelease(name: string, rulesetName: string): Promise<Release> {
const data = {
release: this.getReleaseDescription(name, rulesetName),
};
const request: HttpRequestConfig = {
method: 'PATCH',
url: `${this.url}/releases/${name}`,
data,
};
return this.sendRequest<Release>(request);
}

/**
* Gets the specified resource from the rules API. Resource names must be the short names without project
* ID prefix (e.g. `rulesets/ruleset-name`).
Expand All @@ -136,6 +150,13 @@ export class SecurityRulesApiClient {
return this.sendRequest<T>(request);
}

private getReleaseDescription(name: string, rulesetName: string): Release {
return {
name: `${this.projectIdPrefix}/releases/${name}`,
rulesetName: `${this.projectIdPrefix}/${this.getRulesetName(rulesetName)}`,
};
}

private getRulesetName(name: string): string {
if (!validator.isNonEmptyString(name)) {
throw new FirebaseSecurityRulesError(
Expand Down
28 changes: 27 additions & 1 deletion src/security-rules/security-rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, RulesetResponse, RulesetContent } from './security-rules-api-client';
import { SecurityRulesApiClient, RulesetResponse, RulesetContent, Release } from './security-rules-api-client';
import { AuthorizedHttpClient } from '../utils/api-request';
import { FirebaseSecurityRulesError } from './security-rules-utils';

Expand Down Expand Up @@ -115,6 +115,17 @@ export class SecurityRules implements FirebaseServiceInterface {
return this.getRulesetForRelease(SecurityRules.CLOUD_FIRESTORE);
}

/**
* Makes the specified ruleset the currently applied ruleset for Cloud Firestore.
*
* @param {string|RulesetMetadata} ruleset Name of the ruleset to release or a RulesetMetadata object containing
* the name.
* @returns {Promise<void>} A promise that fulfills when the ruleset is released.
*/
public releaseFirestoreRuleset(ruleset: string | RulesetMetadata): Promise<void> {
return this.releaseRuleset(ruleset, 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.
Expand Down Expand Up @@ -188,6 +199,21 @@ export class SecurityRules implements FirebaseServiceInterface {
return this.getRuleset(stripProjectIdPrefix(rulesetName));
});
}

private releaseRuleset(ruleset: string | RulesetMetadata, releaseName: string): Promise<void> {
if (!validator.isNonEmptyString(ruleset) &&
(!validator.isNonNullObject(ruleset) || !validator.isNonEmptyString(ruleset.name))) {

Choose a reason for hiding this comment

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

It's interesting that the name could come in either as a String or as part of the RulesetMetadata Object. Why did you opt for this and not the simpler signature of only passing in the name as a String, making it the caller's job to pluck out the name if it had an Object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use the same private method for releasing storage rulesets. By doing it here we save a few lines from being duplicated.

const err = new FirebaseSecurityRulesError(
'invalid-argument', 'ruleset must be a non-empty name or a RulesetMetadata object.');
return Promise.reject(err);
}

const rulesetName = validator.isString(ruleset) ? ruleset : ruleset.name;
return this.client.updateRelease(releaseName, rulesetName)
.then(() => {
return;
});
}
}

class SecurityRulesInternals implements FirebaseServiceInternalsInterface {
Expand Down
67 changes: 65 additions & 2 deletions test/unit/security-rules/security-rules-api-client.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const expect = chai.expect;
describe('SecurityRulesApiClient', () => {

const RULESET_NAME = 'ruleset-id';
const RELEASE_NAME = 'test.service';
const ERROR_RESPONSE = {
error: {
code: 404,
Expand Down Expand Up @@ -246,8 +247,6 @@ describe('SecurityRulesApiClient', () => {
});

describe('getRelease', () => {
const RELEASE_NAME = 'test.service';

it('should resolve with the requested release on success', () => {
const stub = sinon
.stub(HttpClient.prototype, 'send')
Expand Down Expand Up @@ -305,6 +304,70 @@ describe('SecurityRulesApiClient', () => {
});
});

describe('updateRelease', () => {
it('should resolve with the updated release on success', () => {
const stub = sinon
.stub(HttpClient.prototype, 'send')
.resolves(utils.responseFrom({name: 'bar'}));
stubs.push(stub);
return apiClient.updateRelease(RELEASE_NAME, RULESET_NAME)
.then((resp) => {
expect(resp.name).to.equal('bar');
expect(stub).to.have.been.calledOnce.and.calledWith({
method: 'PATCH',
url: 'https://firebaserules.googleapis.com/v1/projects/test-project/releases/test.service',
data: {
release: {
name: 'projects/test-project/releases/test.service',
rulesetName: 'projects/test-project/rulesets/ruleset-id',
},
},
});
});
});

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.updateRelease(RELEASE_NAME, RULESET_NAME)
.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.updateRelease(RELEASE_NAME, RULESET_NAME)
.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.updateRelease(RELEASE_NAME, RULESET_NAME)
.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.updateRelease(RELEASE_NAME, RULESET_NAME)
.should.eventually.be.rejected.and.deep.equal(expected);
});
});

describe('deleteRuleset', () => {
const INVALID_NAMES: any[] = [null, undefined, '', 1, true, {}, []];
INVALID_NAMES.forEach((invalidName) => {
Expand Down
51 changes: 51 additions & 0 deletions test/unit/security-rules/security-rules.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,57 @@ describe('SecurityRules', () => {
});
});

describe('releaseFirestoreRuleset', () => {
const invalidRulesetError = new FirebaseSecurityRulesError(
'invalid-argument',
'ruleset must be a non-empty name or a RulesetMetadata object.',
);
const invalidRulesets: any[] = [null, undefined, '', 1, true, {}, [], {name: ''}];
invalidRulesets.forEach((invalidRuleset) => {
it(`should reject when called with: ${JSON.stringify(invalidRuleset)}`, () => {
return securityRules.releaseFirestoreRuleset(invalidRuleset)
.should.eventually.be.rejected.and.deep.equal(invalidRulesetError);
});
});

it('should propagate API errors', () => {
const stub = sinon
.stub(SecurityRulesApiClient.prototype, 'updateRelease')
.rejects(EXPECTED_ERROR);
stubs.push(stub);
return securityRules.releaseFirestoreRuleset('foo')
.should.eventually.be.rejected.and.deep.equal(EXPECTED_ERROR);
});

it('should resolve on success when the ruleset specified by name', () => {
const stub = sinon
.stub(SecurityRulesApiClient.prototype, 'updateRelease')
.resolves({
rulesetName: 'projects/test-project/rulesets/foo',
});
stubs.push(stub);

return securityRules.releaseFirestoreRuleset('foo')
.then(() => {
expect(stub).to.have.been.calledOnce.and.calledWith('cloud.firestore', 'foo');
});
});

it('should resolve on success when the ruleset specified as an object', () => {
const stub = sinon
.stub(SecurityRulesApiClient.prototype, 'updateRelease')
.resolves({
rulesetName: 'projects/test-project/rulesets/foo',
});
stubs.push(stub);

return securityRules.releaseFirestoreRuleset({name: 'foo', createTime: 'time'})
.then(() => {
expect(stub).to.have.been.calledOnce.and.calledWith('cloud.firestore', 'foo');
});
});
});

describe('createRulesFileFromSource', () => {
const INVALID_STRINGS: any[] = [null, undefined, '', 1, true, {}, []];

Expand Down