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

Protected fields pointer-permissions support #5951

Conversation

Dobbias
Copy link
Contributor

@Dobbias Dobbias commented Aug 20, 2019

This PR adds the ability to specify readUserFields columns to set different protectedFieldswhen the current user is contained in such a field.

Fixes #5884 by using the approach discussed in PR 5887.

The extended syntax can be seen in this example:
protectedFields: { Book: { '*': ['profit', 'contract'], 'readUserFields:author': [] }, }

@Dobbias Dobbias changed the title Added protected fields pointer-permissions support Protected fields pointer-permissions support Aug 20, 2019
@Dobbias Dobbias force-pushed the protected_fields-pointer-permissions-support branch from 8af8de1 to 1251da3 Compare August 21, 2019 05:18
@codecov
Copy link

codecov bot commented Aug 21, 2019

Codecov Report

Merging #5951 into master will decrease coverage by 10.68%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #5951       +/-   ##
===========================================
- Coverage   93.67%   82.99%   -10.69%     
===========================================
  Files         156      156               
  Lines       10862    10876       +14     
===========================================
- Hits        10175     9026     -1149     
- Misses        687     1850     +1163
Impacted Files Coverage Δ
src/Controllers/DatabaseController.js 95.02% <100%> (+0.22%) ⬆️
src/Controllers/SchemaController.js 96.38% <100%> (-0.04%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 2.98% <0%> (-93.7%) ⬇️
src/Adapters/Storage/Postgres/PostgresClient.js 78.57% <0%> (-7.15%) ⬇️
src/Controllers/UserController.js 93.45% <0%> (-0.94%) ⬇️
src/Routers/UsersRouter.js 93.54% <0%> (-0.65%) ⬇️
src/Routers/GlobalConfigRouter.js 100% <0%> (ø) ⬆️
src/RestWrite.js 93.56% <0%> (ø) ⬆️

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 89e8868...39ca315. Read the comment docs.

@Dobbias Dobbias force-pushed the protected_fields-pointer-permissions-support branch from 1251da3 to 62b9f11 Compare August 21, 2019 05:33
Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I've just left few questions.

spec/ProtectedFields.spec.js Outdated Show resolved Hide resolved
spec/ProtectedFields.spec.js Show resolved Hide resolved
const fieldKeys: string[] = perms[field];

if (
field === 'readUserFields' &&
Copy link
Member

Choose a reason for hiding this comment

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

Does it only work together with CLP? I mean, if the CLP is set to public, shouldn't the owner be able to see their protected fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not exactly sure if I understand your question. But as far as I understand the protectedFields were introduced to allow overriding permissions like public read. When not specifying protectedFields the owner (defined by pointer-permissions) of an object is able to read all fields on the object. If an user chooses to hide any property using the protectedFields for spefici a role or an user/user-array specified by pointer permissions (introduced by this pr), then the field should be hidden.

For the _User class the setup is different. There is code in place to allow the user reading all properties of the corresponding object in the _User collection/table. This was already in place before this pr.

Copy link
Member

Choose a reason for hiding this comment

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

That's actually not my question. Checking your code and your examples, it seems that I always need to define the readUserFields in the CLP to have the readUserFields:someUserField working in the protected fields. But I think that those two things should work independently. For example, the test below should pass:

  it('should allow access using single user pointer-permissions', async done => {
        const config = Config.get(Parse.applicationId);
        const obj = new Parse.Object('AnObject');

        obj.set('owner', user1);
        obj.set('test', 'test');
        await obj.save();

        const schema = await config.database.loadSchema();
        await schema.updateClass(
          'AnObject',
          {},
          {
            get: { '*': true },
            find: { '*': true },
            // readUserFields: ['owner'], ---> It shouldn't be required since the public read is already specified
            protectedFields: { '*': ['owner'], 'readUserFields:owner': [] },
          }
        );

        await Parse.User.logIn('user1', 'password');
        const objectAgain = await obj.fetch();
        expect(objectAgain.get('owner').id).toBe(user1.id);
        expect(objectAgain.get('test')).toBe('test');
        done();
      });

Copy link
Contributor Author

@Dobbias Dobbias Aug 21, 2019

Choose a reason for hiding this comment

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

You are correct by assuming that readUserFields currently has to be set in the CLP. I do like your approach of separating the readUserFields CLP and protectedFields.
But the name readUserFields in CLP suggests that users stored in the field are able to read all (user-) fields. I do think that we need a different prefix for the protectedFields as this is not directly related to the readUserFields anymore.
After separating the two options the only similarity is the way users a stored in the referenced column (Pointer<_User> or Array<Pointer<_User>>).

Maybe userField: this implies that the referenced field contains users.

Copy link
Member

Choose a reason for hiding this comment

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

userField:someField looks good to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decoupled readUserField CLP and renamed userField as discussed and updated tests to reflect changes.

@Dobbias Dobbias force-pushed the protected_fields-pointer-permissions-support branch from 986a983 to 39ca315 Compare August 21, 2019 21:25
Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

It looks good to me. @dplewis do you want to take a look as well?

@yomybaby
Copy link
Contributor

yomybaby commented Aug 27, 2019

@davimacedo I think this PR should be released ASAP because of #5967 case. 👍

@davimacedo
Copy link
Member

We are doing right now :)
#5978

@yomybaby
Copy link
Contributor

Yeah~ 🎉

UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* moved whitelisting of own user to remove conflict with custom classes and * permission

* added new pointer-perm regex to permissions

* added pointer-permissions support

* added tests

* fixed typo

* fixed typo 2

* added tests using find operation

* renamed protectedFields pointerPerm to userField

* decoupled readUserFields from CLP and removed readUser from protectedFields before querying

* updated tests
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.

protectedFields user white-listing not working for custom classes
3 participants