-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Protected fields pointer-permissions support #5951
Conversation
8af8de1
to
1251da3
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
1251da3
to
62b9f11
Compare
There was a problem hiding this 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.
const fieldKeys: string[] = perms[field]; | ||
|
||
if ( | ||
field === 'readUserFields' && |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
});
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
986a983
to
39ca315
Compare
There was a problem hiding this 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?
@davimacedo I think this PR should be released ASAP because of #5967 case. 👍 |
We are doing right now :) |
Yeah~ 🎉 |
* 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
This PR adds the ability to specify
readUserFields
columns to set differentprotectedFields
when 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': [] }, }