Skip to content
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

feat: Security Rules Management API #645

Merged
merged 15 commits into from
Sep 18, 2019
Merged

feat: Security Rules Management API #645

merged 15 commits into from
Sep 18, 2019

Conversation

hiranya911
Copy link
Contributor

@hiranya911 hiranya911 commented Sep 9, 2019

RELEASE NOTE: Added a new admin.securityRules() API for managing Firebase security rules applied on services like Cloud Firestore and Cloud Storage.
RELEASE NOTE: Added getFirestoreRuleset() and getStorageRuleset() methods for retrieving rulesets that are currently in effect.
RELEASE NOTE: Added releaseFirestoreRuleset() and releaseStorageRuleset() methods for creating new rulesets and applying them to Firestore and Storage.
RELEASE NOTE: Added getRuleset(), createRuleset() and deleteRuleset() methods for managing the lifecycle of a ruleset.

* 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
* 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()
* Added deleteRuleset API

* Merged with source
* Implemented the API for releasing rulesets

* Removed createRelease logic

* Updated comment
* Implemented the API for releasing rulesets

* Removed createRelease logic

* Added getStorageRules() API

* Removed some redundant tests
* Added rules API to the public admin namespace

* Updated docs

* Addressing comments regarding the d.ts file

* Updated App typings
* Rules integration tests

* Refactored by adding some helper methods

* Cleaned up some conditionals

* Added verification for deleteRuleset test

* Renamed tempRulesets
*
* @example
* ```javascript
* // Get the SecurityRules service for the default app
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest backticks like you have below: "the SecurityRules service"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it matter here? This is a comment inside an example. It would be rendered as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's right :)

We can skip this one and the others inside comments for sure.

src/index.d.ts Outdated
*
* @param app Optional app to return the `SecurityRules` service
* for. If not provided, the default `SecurityRules` service will
* be returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

A good case for just "is returned."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/index.d.ts Outdated
* limit.
* @param nextPageToken The next page token. If not specified, returns rulesets
* starting without any offset.
* @return A promise that fulfills a page of rulesets.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "fulfills with" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

A few formatting things to look at. Thanks Hiranya!

@hiranya911
Copy link
Contributor Author

Thanks @egilmorez. Made some of the changes suggested. As for adding backticks, I don't think it's needed there since they are rendered as comments in an example snippet. WDYT?

Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Thanks Hiranya!

@hiranya911 hiranya911 assigned hiranya911 and unassigned egilmorez Sep 18, 2019
@hiranya911 hiranya911 merged commit 0b2082f into master Sep 18, 2019
@hiranya911 hiranya911 deleted the rules branch October 17, 2019 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants