Adds CLP API to Schema router by flovilmart · Pull Request #898 · parse-community/parse-server

flovilmart · 2016-03-07T19:49:39Z

No description provided.

drew-gross · 2016-03-07T19:57:38Z

Personally I'd rather have this be part of the existing route rather than a new route (ie just add more info to the existing response to GET /schemas.

flovilmart · 2016-03-07T19:59:20Z

fine by me, what parameter do you suggest
{
className: "className",
permissions: {}
}
it permissions is sent as an empty obj, that locks the object down.

drew-gross · 2016-03-07T20:05:56Z

I like long names e.g. classLevelPermissions. That aslo leaves room for pointer permissions in a separate key called pointerPermissions.

flovilmart · 2016-03-07T20:08:35Z

OK good, so on the PUT for the schema, and on the GET, I add the permissions to the current result?

drew-gross · 2016-03-07T20:54:15Z

GET sounds right, for PUT I think just replace the permission on the _SCHEMA object entirely with whatever the include in the PUT, and make no modifications if the key isn't included. POST should probably support CLPs too, which shouldn't be too hard. The dashboard won't need it but people using the schemas API for their own purposes would probably find it handy.

flovilmart · 2016-03-07T20:56:30Z

alrighty!

facebook-github-bot · 2016-03-08T02:43:52Z

@flovilmart updated the pull request.

codecov-io · 2016-03-08T02:45:24Z

Current coverage is `91.42%`

Merging #898 into master will increase coverage by +0.07% as of c9d4569

@@            master    #898   diff @@
======================================
  Files           73      73       
  Stmts         4324    4383    +59
  Branches       863     876    +13
  Methods          0       0       
======================================
+ Hit           3950    4007    +57
  Partial         10      10       
- Missed         364     366     +2

Review entire Coverage Diff as of c9d4569

Powered by Codecov. Updated on successful CI builds.

drew-gross · 2016-03-08T03:08:54Z

  _Role: ["name", "ACL"]
 }

+let CLPValidKeys = ['find', 'get', 'create', 'update', 'delete'];


What about addField?

Didn't knew it existed :) what's the full list?

btw addField is never checked anywhere.

These 6 are the full list, you can verify using dashboard.parse.com. We should probably start checking addField :p Schema.js should be able to handle it without too much trouble, since there is that 'freeze' parameter.

facebook-github-bot · 2016-03-08T03:30:20Z

@flovilmart updated the pull request.

facebook-github-bot · 2016-03-08T04:10:20Z

@flovilmart updated the pull request.

drew-gross · 2016-03-08T06:59:52Z

+    return;
+  }
+  Object.keys(perms).forEach((key) => {
+    if (CLPValidKeys.indexOf(key) == -1) {


I think this function doesn't go far enough - it should also validate the contents of the keys to make sure they contain only roles and users. Also my preference would be to have it return true/false rather than throw.

drew-gross · 2016-03-08T07:04:11Z

I think since this is such a security critical feature we should do much more thorough testing than usual - verify that users/roles don't gain permissions after a failed attempt to change permissions, verify that they don't gain one permission after a different permission was modified, etc. At the very least we need to do some tests for users as well as for roles, which I don't currently see. Other than that and the stricter validation in validateCLP this looks good :)

flovilmart · 2016-03-08T12:53:07Z

Agreed with all that!

facebook-github-bot · 2016-03-10T00:35:22Z

@flovilmart updated the pull request.

…velPermissions: null

facebook-github-bot · 2016-03-10T16:33:34Z

@flovilmart updated the pull request.

drew-gross · 2016-03-10T16:38:09Z

+    })
+  });
+
+  it('should not be able to add a field', done => {


drew-gross · 2016-03-10T17:30:57Z

I see lots of tests for public permissions and for permissions on roles, but still none for users. Also we should have some tests for setting a permission to false explicitly (that really should have been done in the original CLP tests but now is as good a time to add them as any)

facebook-github-bot · 2016-03-10T23:03:14Z

@flovilmart updated the pull request.

flovilmart · 2016-03-10T23:04:02Z

@drew-gross I updated, the setting permission to false blacklist, I can revert that if we don't want it now. Added 2 test cases with multiple users, as well as role and public

facebook-github-bot · 2016-03-11T00:21:13Z

@flovilmart updated the pull request.

drew-gross · 2016-03-11T02:55:22Z

    fields: mongoSchemaAPIResponseFields(schema),
  };
+  if (schema._metadata && schema._metadata.class_permissions) {
+    result.classLevelPermissions = schema._metadata.class_permissions;


One last request: Can you have the response always include the classLevelPermissions key? Have it return something equivalent to how the server behaves if there is actually no _metadata or _metadata.class_permissions, which appears to be the same as public everything.

No problem!

drew-gross · 2016-03-11T02:56:22Z

One more small request. Sorry for having to go back and forth so many times on this.

flovilmart · 2016-03-11T03:34:56Z

Don't be sorry, that's part of the process. In that case should we store it as the default?

drew-gross · 2016-03-11T04:02:56Z

We shouldn't be making any database writes as part of a GET query, but I would be cool with saving in POST/PUT if you want to do that. Thats definitely optional though.

drew-gross · 2016-03-11T04:04:36Z

Feel free to merge assuming tests pass.

facebook-github-bot · 2016-03-11T04:05:17Z

@flovilmart updated the pull request.

Adds CLP API to Schema router

facebook-github-bot added the GH Review: review-needed label Mar 7, 2016

flovilmart force-pushed the flovilmart.CLPAPI branch from 10b0ee3 to 609661e Compare March 8, 2016 02:38

drew-gross reviewed Mar 8, 2016
View reviewed changes

flovilmart assigned drew-gross Mar 8, 2016

drew-gross reviewed Mar 8, 2016
View reviewed changes

flovilmart added GH Review: needs-revision and removed GH Review: review-needed labels Mar 8, 2016

flovilmart assigned flovilmart and unassigned drew-gross Mar 8, 2016

flovilmart force-pushed the flovilmart.CLPAPI branch from 4a229ee to 7607bbc Compare March 10, 2016 00:30

flovilmart added 4 commits March 9, 2016 19:33

Adds CLP API to Schema router

d4fd731

Merges CLP endpoints with POST, PUT and GET

5780c1e

Adds addField in CLP valid keys

64f9fad

Adds validation of addFields

e75d233

flovilmart force-pushed the flovilmart.CLPAPI branch from 7607bbc to e75d233 Compare March 10, 2016 00:34

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Mar 10, 2016

flovilmart assigned drew-gross Mar 10, 2016

flovilmart closed this Mar 10, 2016

flovilmart reopened this Mar 10, 2016

flovilmart force-pushed the flovilmart.CLPAPI branch from 5f300ca to 20aebe2 Compare March 10, 2016 16:01

Adds tests, improve coverage, adds ability to delete CLP with classLe…

d71a58c

…velPermissions: null

flovilmart force-pushed the flovilmart.CLPAPI branch from 20aebe2 to d71a58c Compare March 10, 2016 16:31

drew-gross reviewed Mar 10, 2016
View reviewed changes

Comment thread spec/schemas.spec.js

})

});

it('should not be able to add a field', done => {

drew-gross Mar 10, 2016

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

wrong name

Adds blacklist permission, more test scenarios

b1d399b

Removes blacklisting, *-but test case

16e3529

drew-gross reviewed Mar 11, 2016
View reviewed changes

Always return default public permissions

c935ed8

drew-gross added GH Review: accepted and removed GH Review: review-needed labels Mar 11, 2016

flovilmart added a commit that referenced this pull request Mar 11, 2016

Merge pull request #898 from ParsePlatform/flovilmart.CLPAPI

1e7e4fe

Adds CLP API to Schema router

flovilmart merged commit 1e7e4fe into master Mar 11, 2016

flovilmart deleted the flovilmart.CLPAPI branch March 11, 2016 05:32

Uh oh!

Uh oh!

Conversation

flovilmart commented Mar 7, 2016

Uh oh!

drew-gross commented Mar 7, 2016

Uh oh!

flovilmart commented Mar 7, 2016

Uh oh!

drew-gross commented Mar 7, 2016

Uh oh!

flovilmart commented Mar 7, 2016

Uh oh!

drew-gross commented Mar 7, 2016

Uh oh!

flovilmart commented Mar 7, 2016

Uh oh!

facebook-github-bot commented Mar 8, 2016

Uh oh!

codecov-io commented Mar 8, 2016

Current coverage is 91.42%

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Mar 8, 2016

Uh oh!

facebook-github-bot commented Mar 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drew-gross commented Mar 8, 2016

Uh oh!

flovilmart commented Mar 8, 2016

Uh oh!

facebook-github-bot commented Mar 10, 2016

Uh oh!

facebook-github-bot commented Mar 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drew-gross commented Mar 10, 2016

Uh oh!

facebook-github-bot commented Mar 10, 2016

Uh oh!

flovilmart commented Mar 10, 2016

Uh oh!

facebook-github-bot commented Mar 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drew-gross commented Mar 11, 2016

Uh oh!

flovilmart commented Mar 11, 2016

Uh oh!

drew-gross commented Mar 11, 2016

Uh oh!

drew-gross commented Mar 11, 2016

Uh oh!

facebook-github-bot commented Mar 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Current coverage is `91.42%`