-
-
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
Allow protectedFields for Authenticated users and Public. Fix userField with keys/excludedKeys #6415
Allow protectedFields for Authenticated users and Public. Fix userField with keys/excludedKeys #6415
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6415 +/- ##
==========================================
- Coverage 93.94% 93.39% -0.55%
==========================================
Files 169 169
Lines 11734 11781 +47
==========================================
- Hits 11023 11003 -20
- Misses 711 778 +67
Continue to review full report at Codecov.
|
It looks good to me. I am not sure about the item below though:
I think it is not a good idea to have two names for the same thing. @acinader @dplewis thoughts? |
src/Controllers/SchemaController.js
Outdated
@@ -1405,6 +1441,8 @@ export default class SchemaController { | |||
} | |||
// requiresAuthentication passed, just move forward | |||
// probably would be wise at some point to rename to 'authenticatedUser' |
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.
Stumbled upon this old comment I though authenticated
is a better description of a group of users than requiresAuthentication
, just left the former variant for backwards compatibility. I'd propose to replace it in all the docs and may be drop completely by next major release or whenever. This also can be applied to CLP.
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 may be a better name but I am not sure if it is worth to be done since it is a breaking change.
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.
Ok just realized there is no point in supporting requiresAuthentication
for Protected Fields at all. Though for some reason it could pass validation regex before (used same regex as for CLP permissions) - it had no real effect anyway. So I'll only leave authenticated
for 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.
LGTM!
…ld with keys/excludedKeys (parse-community#6415) * fix error message and test it * protected fields fixes * clean * remove duplicate test, add some comments * no need for 'requiresAuthentication'
userField:column
were not applied if thecolumn
was not requested by client (either listed inexcludeKeys
or not included inkeys
)protectedFields
. Intersect all applicable sets of fields e.g. multiple roles / authenticated / userField will be intersected per set instead of overriding.Let's say there are
admin
andmoderator
roles (or pointers). Admin is allowed to see somefield
,moder
is not. Previously, if user had both roles he wouldn't see the field, because they were combined. But since he has at least one role allowing the field he should to be able to see the field. This is achieved by array intersection.Same applies for other means of setting
protectedFields
:each of sets will be intersected, e.g. if the role explicitly allows all fields , intersection
[fieldA,fieldB]
vs[fieldB]
vs[]
results in[]
and only users having this role will get all the fields.For authenticated users: intersection of
[fieldA, fieldB]
vs[fieldB]
results in[fieldB]
- authenticated users will not see onlyfieldB
.For not authenticated users both fields are hidden.