Skip to content

Adds support for read-only masterKey #4297

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

Merged
merged 4 commits into from
Oct 26, 2017

Conversation

flovilmart
Copy link
Contributor

Closes #4296

The motivation for this feature is to provide read only access to all the data, in the dashboard, the readOnlyMasterKey should be treated with as much respect as the masterKey

@codecov
Copy link

codecov bot commented Oct 26, 2017

Codecov Report

Merging #4297 into master will decrease coverage by 10.43%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4297       +/-   ##
===========================================
- Coverage   92.49%   82.06%   -10.44%     
===========================================
  Files         118      118               
  Lines        8169     8196       +27     
===========================================
- Hits         7556     6726      -830     
- Misses        613     1470      +857
Impacted Files Coverage Δ
src/Options/Definitions.js 100% <ø> (ø) ⬆️
src/Config.js 94.91% <100%> (+0.08%) ⬆️
src/rest.js 98.78% <100%> (+1.31%) ⬆️
src/RestWrite.js 93.21% <100%> (+0.05%) ⬆️
src/Auth.js 100% <100%> (ø) ⬆️
src/Routers/GlobalConfigRouter.js 100% <100%> (ø) ⬆️
src/Routers/PushRouter.js 96.77% <100%> (+0.22%) ⬆️
src/middlewares.js 98.18% <100%> (+0.05%) ⬆️
src/Routers/SchemasRouter.js 97.91% <100%> (+0.29%) ⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 3.03% <0%> (-93.48%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2fc0f5...e20a5f3. Read the comment docs.

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Looks pretty good! One question though, if we're checking for readOnly auth in schema changes should we also be checking for readOnly when submitting pushes?

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

And we'll want to get the coverage up of course.

expect(() => {
rest.del(config, readOnly, 'AnObject', {})
}).toThrow();
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we still need some more tests in here to verify that read still works, config updates are blocked & direct schema updates are blocked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I'm on it!

@montymxb
Copy link
Contributor

Oh and one more thing, we'll need a PR for this feature in the docs, otherwise nobody may know this actually exists...

@flovilmart
Copy link
Contributor Author

Push will be block at the RestWrite level, but we can block it earlier. Actually my readOnly users would likely be able to send push notificairtons, so I'll probably have to refactor after to support it.

@flovilmart
Copy link
Contributor Author

@montymxb I believe we're good with that one in terms feature + README, I'll update the docs as well.

README.md Outdated
@@ -225,6 +225,7 @@ The client keys used with Parse are no longer necessary with Parse Server. If yo
* `customPages` - A hash with urls to override email verification links, password reset links and specify frame url for masking user-facing pages. Available keys: `parseFrameURL`, `invalidLink`, `choosePassword`, `passwordResetSuccess`, `verifyEmailSuccess`.
* `middleware` - (CLI only), a module name, function that is an express middleware. When using the CLI, the express app will load it just **before** mounting parse-server on the mount path. This option is useful for injecting a monitoring middleware.
* `masterKeyIps` - The array of ip addresses where masterKey usage will be restricted to only these ips. (Default to [] which means allow all ips). If you're using this feature and have `useMasterKey: true` in cloudcode, make sure that you put your own ip in this list.
* `readOnlyMasterKey` - A masterKey that has full read access to the data, but no write access. This keys should be treated the same way as your masterKey, keeping it private.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a little typo,
This key should be treated...

});
});

it('should throw when trying to create schema', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same name as the test above ^

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Nothing else besides a couple little wording things

@flovilmart
Copy link
Contributor Author

@montymxb and those are addressed :)

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Awesome!

@flovilmart
Copy link
Contributor Author

Sweet! If @marvelm 's PR for relative dates is good, I'd like it in for 2.6.5.

@flovilmart flovilmart merged commit 1dd58b7 into master Oct 26, 2017
@flovilmart flovilmart deleted the feature/4296-readonly-masterKey branch October 26, 2017 19:35
@montymxb
Copy link
Contributor

Definitely, once that's present we can start offering it in the sdks!

@flovilmart
Copy link
Contributor Author

yep! We plan to use it purely in REST for now, but SDK support would be thrilling :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants