-
Notifications
You must be signed in to change notification settings - Fork 373
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
Adding the rules API to the public API surface #625
Conversation
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.
Thanks Hiranya!
One thing to look at globally: capitalized "Ruleset" should either be backticked to make it a literal Ruleset
object, or just left uncapitalized for more general references. Make sense?
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.
LG from a doc standpoint, thanks Hiranya!
@@ -178,6 +178,20 @@ toc: | |||
- title: "ShaCertificate" | |||
path: /docs/reference/admin/node/admin.projectManagement.ShaCertificate | |||
|
|||
- title: "admin.securityRules" |
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 and your other reviewers here, but we use FirebaseRules
in the same way we use FirebaseAuth
and FirebaseDatabase
. The extra "security" word here breaks conventions. ((Consider: what the object will be called in the Java admin SDK?))
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.
Current naming convention used is: admin.securityRules()
, SecurityRules
, (Java: FirebaseSecurityRules
).
It sounds like you're suggesting: admin.rules()
, Rules
, (Java: FirebaseRules
)
Is that correct? I don't have preference. Let's see what @rachelmyers and others think. The existing documentation uses "Firebase Security Rules" pretty much everywhere: https://firebase.google.com/docs/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.
I have a slight preference for admin.securityRules()
, but since I think people will understand either, it's not a strong preference.
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. The only thing I noticed was that we talked about createRuleset optionally taking multiple files, to match the API.
@@ -178,6 +178,20 @@ toc: | |||
- title: "ShaCertificate" | |||
path: /docs/reference/admin/node/admin.projectManagement.ShaCertificate | |||
|
|||
- title: "admin.securityRules" |
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 have a slight preference for admin.securityRules()
, but since I think people will understand either, it's not a strong preference.
* @param file Rules file to include in the new `Ruleset`. | ||
* @returns A promise that fulfills with the newly created `Ruleset`. | ||
*/ | ||
createRuleset(file: RulesFile): Promise<Ruleset>; |
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.
Was createRuleset
meant to allow multiple files, so createRuleset(files...: RulesFile): Promise<Ruleset>
, instead?
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.
In #607 we decided to only accept one RulesFile
in this API (since the backend impl currently doesn't allow multiple files). In the future we can change this into a varargs if necessary, as a non-breaking change.
I'll merge this as is. We still have time to revise the rules vs security rules argument in a separate PR. |
* 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
admin.securityRules
namespace and the getter.securityRules
getter fromFirebaseApp
.index.d.ts
file.The comments in the
d.ts
file gets compiled into our API reference docs.