-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
#5301 sensitive fields acl #5334
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
#5301 sensitive fields acl #5334
Conversation
- Public read ACL should never expose PII to authenticated and non-authenticated - Explicit ACL like custom user Role should be able to read PII
…s CLP\nAdd defaults for protectedFields CLP\nFix tests
Codecov Report
@@ Coverage Diff @@
## master #5334 +/- ##
==========================================
+ Coverage 93.89% 93.96% +0.06%
==========================================
Files 123 127 +4
Lines 8972 9091 +119
==========================================
+ Hits 8424 8542 +118
- Misses 548 549 +1
Continue to review full report at Codecov.
|
Closes: #5301 |
Nice one! I’ll review shortly! Sent with GitHawk |
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.
This is a good PR and oddly timely given what I'm working on. :P Thanks! Hope it gets merged. Now I've just gotta upgrade to parse 3...
magnifique ! |
yes, I will. |
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.
@awgeorge this is really nice. Thanks!
I found a typo or two.
It would be nice to add a ProtectedFields.spec
to exercise this feature on non-user objects, which has the added benefit of being some documentation.
I think that protectedFields
can be set not only in a CLP, but also in an objects ACL (even though the sdk's don't support yet). If so, this would be good to exercise in a protectedFields
spec?
I know it has been a while since you first submitted. Is adding a spec something you can bite off? If not, I can give it a try...
would be good to get the docs updated to include this too, but I don't see that ads a merge dependency.
@acinader Thanks for reviewing, I should be able to sort those specs out, but I won't be able to get to it for a little while. Maybe if someone in the community who wanted this feature could lend a hand with the last tests that would be amazing. I'll pick it back up when my storm blows over if the work is still outstanding. |
@awgeorge there's a few failing tests for pii now. hope it wasn't one of my suggestions :0. we don't need to wait for the tests i'm asking for to merge this into master and I can try my hand at it but not until next week. if someone can take a look at the failed test, otherwise I'll try to later. |
No description provided.