-
Notifications
You must be signed in to change notification settings - Fork 371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementing the Firebase Security Rules API #604
Conversation
* @returns {Promise<T>} A promise that fulfills with the resource. | ||
*/ | ||
public getResource<T>(name: string): Promise<T> { | ||
if (!name.startsWith('projects/')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be worth regexing this to catch malformed project ids (which, for instance, I think need to be lowercase).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified things a bit here by pushing the projectId to the constructor of the RulesApiClient
. Now it stores a partial URL as part of its state:
this.url = `${RULES_API_URL}/projects/${projectId}`;
Callers should pass in the resource names without project ID:
client.getResource('rulesets/my-ruleset');
As for project ID validation, we are doing the same check here as our other modules (Auth, FCM), and so it should be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to you here, but:
- there aren't that many kinds of resources
- it may be easier to explicitly spell them all out as unique methods:
client.getRuleset(<ruleset_name>)
client.listRulesets()
client.getRelease()
client.listReleases()
which directly encodes the exposed backend API. Then add another layer on top of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea. Will implement in the next PRs.
Thanks @ryanpbrewster. I've made a pass and addressed your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as-is, if you opt to refactor to match the backend API feel free to re-assign me (alternately: feel free to do this in a separate PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. There were a few interfaces that I didn't expect, and left a comment in-line for those.
🚢
readonly source: { | ||
readonly files: RulesFile[]; | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are Release
and RulesetResponse
additions to what we proposed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are part of the internal implementation. They are not exported and also won't be added to the typings.
src/security-rules/security-rules.ts
Outdated
} | ||
|
||
/** | ||
* Representa a set of Firebase security rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Representa/Represents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
* Implementing the Firebase Security Rules API (#604) * Implementing the Firebase Security Rules API * More argument validation and assertions * Cleaning up the rules impl * Internal API renamed * Fixing a typo in a comment * Implemented createRulesFileFromSource() and createRuleset() APIs (#607) * Implementing the Firebase Security Rules API * More argument validation and assertions * Adding the rest of the CRUD operations for rulesets * Cleaning up the rules impl * Cleaned up tests * Adding some missing comments * Removing support for multiple rules files in create() * Implemented the deleteRuleset() API (#609) * Added deleteRuleset API * Merged with source * Implemented the API for releasing rulesets (#610) * Implemented the API for releasing rulesets * Removed createRelease logic * Updated comment * Added the getStorageRuleset() API (#613) * Implemented the API for releasing rulesets * Removed createRelease logic * Added getStorageRules() API * Removed some redundant tests * Implementing the remaining releaseRuleset APIs (#616) * Implemented the listRulesetMetadata() API (#622) * Adding the rules API to the public API surface (#625) * Added rules API to the public admin namespace * Updated docs * Addressing comments regarding the d.ts file * Updated App typings * Rules integration tests (#633) * Rules integration tests * Refactored by adding some helper methods * Cleaned up some conditionals * Added verification for deleteRuleset test * Renamed tempRulesets * Handling ruleset limit exceeded error (#636) * Fixing alignment of an annotation * Updated comments
Added the base framework for the new security rules API (nothing new exposed in the public API --i.e.
admin
namespace -- yet). This PR specifically adds theSecurityRules
class and the following 2 methods:getRuleset()
getFirestoreRuleset()
Also defined the types
Ruleset
,RulesetMetadata
andRulesFile
which will be added to the public API in the future.go/firebase-rules-admin