Add StorageRulesManagerRegistry to handle rules files for multiple targets #4281

tohhsinpei · 2022-03-09T20:50:01Z

Description

Fixes #3390. Following up to #4263, this PR adds a StorageRulesManager that will live inside the Storage Emulator. The interface is implemented by:

StorageRulesManagerImplementation, which keeps track of rules for a single resource
StorageRulesManagerRegistry, which maps resource to StorageRulesManagerImplementation

In the server, all calls to the validator will pass along the resource aka bucket id in order to determine which ruleset to use.

tonyjhuang · 2022-03-09T22:08:35Z

src/emulator/storage/index.ts

+    return this._rulesManager.getRuleset(resource);
+  }
+
+  async setRules(rules: RulesType, resource: string): Promise<StorageRulesIssues> {


we already have a pattern of accessing StorageEmulator internals from within server.ts so I'm not sure we need to wrap the rules manager's interface here

Ok, I can expose the rules manager instead for the sake of consistency. Curious, is there any benefit to exposing the internals?

src/emulator/storage/index.ts

src/emulator/storage/rules/manager.ts

src/emulator/storage/index.ts

src/emulator/storage/rules/manager.ts

src/fsutils.ts

src/emulator/storage/index.ts

src/emulator/storage/rules/manager.ts

src/test/emulators/storage/rules/manager.spec.ts

sam-gc

TypeScript LGTM

tonyjhuang · 2022-03-16T15:42:26Z

CHANGELOG.md

@@ -0,0 +1 @@
+- Adds support for configuration with multiple storage targets (#4281).


We should coordinate with @puf to get the public docs updated once this is released :) https://firebase.google.com/docs/emulator-suite/install_and_configure#security_rules_configuration

There are some docs, somewhat hidden away...

That's true, the emulator-specific docs don't seem to mention this: https://firebase.google.com/docs/emulator-suite/install_and_configure#security_rules_configuration

But it's okay that we don't reiterate everything since it's already in the CLI documentation

src/emulator/storage/rules/config.ts

tonyjhuang · 2022-03-16T18:30:52Z

src/emulator/storage/rules/manager.ts

+   * Updates the source file and, correspondingly, the file watcher and ruleset for the resource.
+   * Returns an array of errors and/or warnings that arise from loading the ruleset.
+   */
+  updateSourceFile(rules: SourceFile, resource: string): Promise<StorageRulesIssues>;


Discussed offline, we may want to remove this method as we don't need to support modifying currently running StorageRulesManagers.

Yup, will do in follow up PR.

src/test/emulators/storage/rules/config.spec.ts

src/test/emulators/storage/rules/manager.spec.ts

tonyjhuang · 2022-03-16T20:13:25Z

CHANGELOG.md

@@ -0,0 +1 @@
+- Adds support for configuration with multiple storage targets (#4281).


That's true, the emulator-specific docs don't seem to mention this: https://firebase.google.com/docs/emulator-suite/install_and_configure#security_rules_configuration

But it's okay that we don't reiterate everything since it's already in the CLI documentation

scripts/storage-emulator-integration/rules/manager.test.ts

tohhsinpei force-pushed the hsinpei/storage-emulator-multiple-targets-3 branch from 40af2f8 to aa0ebeb Compare March 9, 2022 20:51

tohhsinpei requested a review from tonyjhuang March 9, 2022 20:51

tohhsinpei force-pushed the hsinpei/storage-emulator-multiple-targets-3 branch 3 times, most recently from 789252a to 4b45374 Compare March 9, 2022 22:29

tonyjhuang reviewed Mar 9, 2022

View reviewed changes

tohhsinpei requested a review from sam-gc March 10, 2022 22:55

tohhsinpei force-pushed the hsinpei/storage-emulator-multiple-targets-3 branch from 0f00d1a to 9ab16db Compare March 10, 2022 23:01

tohhsinpei requested a review from tonyjhuang March 10, 2022 23:02

tohhsinpei force-pushed the hsinpei/storage-emulator-multiple-targets-3 branch from 9ab16db to 015a865 Compare March 10, 2022 23:14

tonyjhuang reviewed Mar 14, 2022

View reviewed changes

src/emulator/storage/rules/manager.ts Outdated Show resolved Hide resolved

src/emulator/storage/rules/manager.ts Outdated Show resolved Hide resolved

tonyjhuang reviewed Mar 14, 2022

View reviewed changes

src/emulator/storage/rules/manager.ts Outdated Show resolved Hide resolved

tohhsinpei mentioned this pull request Mar 14, 2022

Add integration test for multiple storage targets #4296

Merged

sam-gc reviewed Mar 14, 2022

View reviewed changes

tohhsinpei requested review from tonyjhuang and sam-gc March 15, 2022 19:31

tonyjhuang reviewed Mar 15, 2022

View reviewed changes

tohhsinpei force-pushed the hsinpei/storage-emulator-multiple-targets-3 branch from 8e24f4c to 4575d14 Compare March 15, 2022 20:14

tohhsinpei requested a review from tonyjhuang March 15, 2022 20:14

tohhsinpei force-pushed the hsinpei/storage-emulator-multiple-targets-3 branch from 4575d14 to 9eaf5c4 Compare March 15, 2022 20:15

sam-gc approved these changes Mar 15, 2022

View reviewed changes

tonyjhuang reviewed Mar 16, 2022

View reviewed changes

tohhsinpei added 7 commits March 16, 2022 19:43

Address PR feedback; add JSDocs for new method

586e660

Make StorageRulesManager an interface; add StorageRulesManagerRegistry

fff9889

Modify RulesetProvider to take resource parameter

fc487e4

Rebase; fix lint error

edb0bd7

Change emulator arg to take SourceFile only

1ddd733

PR feedback, mostly re: API usage

6f45411

Don't delete source file or ruleset on stop(); add CHANGELOG

0594bf2

tohhsinpei and others added 8 commits March 16, 2022 19:45

Address PR feedback

8e42a13

Moved Storage rules integration tests under scripts/ (#4309)

8f3d045

Make StorageRulesManager an interface; add StorageRulesManagerRegistry

007e1f4

Modify RulesetProvider to take resource parameter

18dbd2f

Change emulator arg to take SourceFile only

fc6cdf0

PR feedback, mostly re: API usage

b7c5de5

Don't delete source file or ruleset on stop(); add CHANGELOG

f221cb8

Address PR feedback

6ed793c

tohhsinpei force-pushed the hsinpei/storage-emulator-multiple-targets-3 branch 2 times, most recently from bce98a8 to 4f53ed4 Compare March 16, 2022 19:59

Merge branch 'master' into hsinpei/storage-emulator-multiple-targets-3

0c8c165

tohhsinpei force-pushed the hsinpei/storage-emulator-multiple-targets-3 branch from 4f53ed4 to 0c8c165 Compare March 16, 2022 20:01

Fix lint

2107f90

tohhsinpei requested a review from tonyjhuang March 16, 2022 20:06

tonyjhuang approved these changes Mar 16, 2022

View reviewed changes

Add test for failing to find rules for given resource

76d7a42

tohhsinpei merged commit c69b437 into master Mar 16, 2022

tohhsinpei deleted the hsinpei/storage-emulator-multiple-targets-3 branch March 16, 2022 20:34

tohhsinpei mentioned this pull request Mar 18, 2022

Update /internal/setRules endpoint; fix ruleset file watcher #4337

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add StorageRulesManagerRegistry to handle rules files for multiple targets #4281

Add StorageRulesManagerRegistry to handle rules files for multiple targets #4281

tohhsinpei commented Mar 9, 2022

tonyjhuang Mar 9, 2022

tohhsinpei Mar 10, 2022

sam-gc left a comment

tonyjhuang Mar 16, 2022

tohhsinpei Mar 16, 2022

tonyjhuang Mar 16, 2022

tonyjhuang Mar 16, 2022

tohhsinpei Mar 16, 2022

tonyjhuang Mar 16, 2022

		@@ -0,0 +1 @@
		- Adds support for configuration with multiple storage targets (#4281).

Add StorageRulesManagerRegistry to handle rules files for multiple targets #4281

Add StorageRulesManagerRegistry to handle rules files for multiple targets #4281

Conversation

tohhsinpei commented Mar 9, 2022

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-gc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment