GraphQL: ACL #5957

Moumouls · 2019-08-22T09:55:53Z

input UserACLInput {
    userId: ID!
    # If the value is true, the user can read the current object, if not provided read is set to true
    read: Boolean
    # If the value is true, the user can write on the current object, if not provided write is set to true
    write: Boolean
}

input RoleACLInput {
    roleName: String!
    # If the value is true, users who are members of the role can read the current object, if not provided read is set to true
    read: Boolean
    # If the value is true, users who are members of the role can write on the current object,  if not provided write is set to true
    write: Boolean 
}

input PublicACLInput {
    # If the value is true, anyone can read the current object, if not provided read is set to true
    read: Boolean
    # If the value is true, anyone can write on the current object,  if not provided read is set to true
    write: Boolean 
}

input ACLInput {
    # Access control level for users, if not provided object will be public
    users: [UserACLInput!]
    # Access control level for roles, if not provided object will be public
    roles: [RoleACLInput!]
    # Public access control level, if not provided object will be public
    public: PublicACLInput
}

type UserACL {
    userId: ID!
    # If the value is true, the user can read the current object
    read: Boolean
    # If the value is true, the user can write on the current object
    write: Boolean
}

type RoleACL {
    roleName: String!
    # If the value is true, users who are members of the role can read the current object
    read: Boolean
    # If the value is true, users who are members of the role can write on the current object
    write: Boolean 
}

type PublicACL {
    # If the value is true, anyone can read the current object
    read: Boolean
    # If the value is true, anyone can write on the current object
    write: Boolean 
}

type ACL {
    # Access control level for users
    users: [UserACL!]
    # Access control level for roles
    roles: [RoleACL!]
    # Public access control level
    public: PublicACL
}

Fix Spec

davimacedo · 2019-08-22T18:12:36Z

It looks good!

Moumouls · 2019-08-24T15:55:05Z

@davimacedo i think here to be more clear and avoid security risk for developers, it's better to require read and write on Inputs.

This adds a few more fields to the mutations but it will be much easier to create and maintain them

Note: ACLInput stay optional in ExampleFieldsInput

davimacedo · 2019-08-25T01:11:33Z

I am not sure I understood. Could you update the schema with the change you want?

Moumouls · 2019-08-27T22:35:08Z

I think i can finish it tomorrow then you will see in tests how it works. 👍

Moumouls · 2019-08-31T12:30:11Z

I tried to make the management of ACLs easy as possible. Tell me what you think about it @davimacedo ?

codecov · 2019-08-31T12:30:39Z

Codecov Report

Merging #5957 into master will increase coverage by <.01%.
The diff coverage is 96.07%.

@@            Coverage Diff             @@
##           master    #5957      +/-   ##
==========================================
+ Coverage   93.74%   93.75%   +<.01%     
==========================================
  Files         156      156              
  Lines       10939    10985      +46     
==========================================
+ Hits        10255    10299      +44     
- Misses        684      686       +2

Impacted Files	Coverage Δ
src/GraphQL/loaders/parseClassMutations.js	`98.57% <ø> (ø)`	⬆️
src/GraphQL/transformers/mutation.js	`97.33% <100%> (+0.96%)`	⬆️
src/GraphQL/ParseGraphQLServer.js	`93.18% <100%> (+0.15%)`	⬆️
src/GraphQL/loaders/defaultGraphQLTypes.js	`97.46% <100%> (+0.26%)`	⬆️
src/GraphQL/loaders/parseClassTypes.js	`85.65% <33.33%> (-0.07%)`	⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`92.23% <0%> (-0.71%)`	⬇️
src/RestWrite.js	`93.72% <0%> (+0.16%)`	⬆️

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 09caa8f...a0616ee. Read the comment docs.

codecov · 2019-08-31T12:30:39Z

Codecov Report

Merging #5957 into master will increase coverage by <.01%.
The diff coverage is 96.07%.

@@            Coverage Diff             @@
##           master    #5957      +/-   ##
==========================================
+ Coverage   93.74%   93.75%   +<.01%     
==========================================
  Files         156      156              
  Lines       10939    10985      +46     
==========================================
+ Hits        10255    10299      +44     
- Misses        684      686       +2

Impacted Files	Coverage Δ
src/GraphQL/loaders/parseClassMutations.js	`98.57% <ø> (ø)`	⬆️
src/GraphQL/transformers/mutation.js	`97.33% <100%> (+0.96%)`	⬆️
src/GraphQL/ParseGraphQLServer.js	`93.18% <100%> (+0.15%)`	⬆️
src/GraphQL/loaders/defaultGraphQLTypes.js	`97.46% <100%> (+0.26%)`	⬆️
src/GraphQL/loaders/parseClassTypes.js	`85.65% <33.33%> (-0.07%)`	⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`92.23% <0%> (-0.71%)`	⬇️
src/RestWrite.js	`93.72% <0%> (+0.16%)`	⬆️

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 09caa8f...a0616ee. Read the comment docs.

codecov · 2019-08-31T12:30:39Z

Codecov Report

Merging #5957 into master will increase coverage by 10.48%.
The diff coverage is 95.23%.

@@             Coverage Diff             @@
##           master    #5957       +/-   ##
===========================================
+ Coverage   83.28%   93.76%   +10.48%     
===========================================
  Files         165      165               
  Lines       11161    11199       +38     
===========================================
+ Hits         9295    10501     +1206     
+ Misses       1866      698     -1168

Impacted Files	Coverage Δ
src/GraphQL/loaders/parseClassMutations.js	`100% <ø> (ø)`	⬆️
src/GraphQL/loaders/parseClassTypes.js	`94.47% <ø> (ø)`	⬆️
src/GraphQL/transformers/inputType.js	`81.81% <0%> (ø)`	⬆️
src/GraphQL/transformers/outputType.js	`81.81% <0%> (ø)`	⬆️
src/GraphQL/transformers/mutation.js	`96.96% <100%> (+0.6%)`	⬆️
src/GraphQL/transformers/query.js	`66.66% <100%> (ø)`	⬆️
src/GraphQL/ParseGraphQLServer.js	`93.18% <100%> (+0.15%)`	⬆️
src/GraphQL/loaders/defaultGraphQLTypes.js	`96.66% <100%> (+0.35%)`	⬆️
src/Routers/UsersRouter.js	`94.19% <0%> (+0.64%)`	⬆️
src/Controllers/UserController.js	`94.39% <0%> (+0.93%)`	⬆️
... and 2 more

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 6aaebb6...226a83c. Read the comment docs.

davimacedo

thanks for tackling this one @Moumouls . It is a great job! I've just left some questions to make sure I have understood how the defaults work.

src/GraphQL/loaders/defaultGraphQLTypes.js

spec/ParseGraphQLServer.spec.js

src/GraphQL/loaders/defaultGraphQLTypes.js

davimacedo · 2019-09-04T04:26:44Z

@Moumouls I've just done some tests here. It is possible

read = false & write = false -> you can't neither GET nor PUT the object
read = false & write = true -> you can't GET but you can PUT the object
read = true & write = false -> you can GET but you can't PUT the object
read = true & write = true -> you can either GET or PUT the object

davimacedo · 2019-09-04T22:23:21Z

I was working on another issue today and I also noticed (I haven't never noticed it before) that, when using the Parse Dashboard, if you set read = true for the Public rule, all other rules become read = true automatically as well. In the same way, if you set write = true for the Public rule, all other riles become write = true automatically as well. Do you think that we should also replicate this behavior here?

Moumouls · 2019-09-10T07:44:13Z

I think it could be confusing for developers. May be it could be an esay improvement if we get some feedback on this special behavior

davimacedo · 2019-09-10T22:05:31Z

Yes. I agree with you. What about the other comments I left?

# Conflicts: # src/GraphQL/loaders/parseClassTypes.js

Moumouls · 2019-09-21T14:53:20Z

So, to avoid misunderstanding and confusion about ACL behavior through the GraphQL API. I set the write and read to Non Nullable. It's just a Boolean field and developers don't need to think about the default behavior.
It will also result into a better code understanding when developers will review other developers mutations.

davimacedo

Just one more comment. We are almost there :)

davimacedo · 2019-09-27T23:34:15Z

src/GraphQL/loaders/defaultGraphQLTypes.js

+          if (rule !== '*' && rule.indexOf('role:') !== 0) {
+            users.push({
+              userId: rule,
+              read: p[rule].read || p[rule].write ? true : false,


Why || p[rule].write?

good catch ! Just a miss from my part

davimacedo

LGTM!

* Spec Fix Spec * Add ACL Type + Input * Improvements * Fix

Spec

852d0b6

Fix Spec

Moumouls requested a review from davimacedo August 22, 2019 09:55

Moumouls force-pushed the gql-acl branch from fd020e4 to 852d0b6 Compare August 22, 2019 10:16

Moumouls added 2 commits August 31, 2019 10:47

Merge branch 'master-up' into gql-acl

34afc95

Add ACL Type + Input

a0616ee

Moumouls marked this pull request as ready for review August 31, 2019 12:27

davimacedo reviewed Sep 2, 2019

View reviewed changes

Improvements

b3882a9

davimacedo reviewed Sep 4, 2019

View reviewed changes

src/GraphQL/loaders/defaultGraphQLTypes.js Outdated Show resolved Hide resolved

davimacedo reviewed Sep 4, 2019

View reviewed changes

src/GraphQL/loaders/defaultGraphQLTypes.js Outdated Show resolved Hide resolved

davimacedo reviewed Sep 4, 2019

View reviewed changes

src/GraphQL/loaders/defaultGraphQLTypes.js Outdated Show resolved Hide resolved

Fix + DX Improvement

0df77c6

# Conflicts: # src/GraphQL/loaders/parseClassTypes.js

davimacedo reviewed Sep 27, 2019

View reviewed changes

Moumouls added 2 commits September 29, 2019 17:39

Merge branch 'master-up' into gql-acl

ed66f52

Fix

226a83c

davimacedo approved these changes Oct 2, 2019

View reviewed changes

davimacedo merged commit 2290145 into parse-community:master Oct 2, 2019

UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020

GraphQL: ACL (parse-community#5957)

9c6eb84

* Spec Fix Spec * Add ACL Type + Input * Improvements * Fix

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GraphQL: ACL #5957

GraphQL: ACL #5957

Moumouls commented Aug 22, 2019 •

edited

Loading

davimacedo commented Aug 22, 2019

Moumouls commented Aug 24, 2019 •

edited

Loading

davimacedo commented Aug 25, 2019

Moumouls commented Aug 27, 2019

Moumouls commented Aug 31, 2019 •

edited

Loading

codecov bot commented Aug 31, 2019

codecov bot commented Aug 31, 2019

codecov bot commented Aug 31, 2019 •

edited

Loading

davimacedo left a comment

davimacedo commented Sep 4, 2019

davimacedo commented Sep 4, 2019

Moumouls commented Sep 10, 2019

davimacedo commented Sep 10, 2019

Moumouls commented Sep 21, 2019

davimacedo left a comment

davimacedo Sep 27, 2019

Moumouls Sep 29, 2019

davimacedo left a comment

GraphQL: ACL #5957

GraphQL: ACL #5957

Conversation

Moumouls commented Aug 22, 2019 • edited Loading

davimacedo commented Aug 22, 2019

Moumouls commented Aug 24, 2019 • edited Loading

davimacedo commented Aug 25, 2019

Moumouls commented Aug 27, 2019

Moumouls commented Aug 31, 2019 • edited Loading

codecov bot commented Aug 31, 2019

Codecov Report

codecov bot commented Aug 31, 2019

Codecov Report

codecov bot commented Aug 31, 2019 • edited Loading

Codecov Report

davimacedo left a comment

Choose a reason for hiding this comment

davimacedo commented Sep 4, 2019

davimacedo commented Sep 4, 2019

Moumouls commented Sep 10, 2019

davimacedo commented Sep 10, 2019

Moumouls commented Sep 21, 2019

davimacedo left a comment

Choose a reason for hiding this comment

davimacedo Sep 27, 2019

Choose a reason for hiding this comment

Moumouls Sep 29, 2019

Choose a reason for hiding this comment

davimacedo left a comment

Choose a reason for hiding this comment

Moumouls commented Aug 22, 2019 •

edited

Loading

Moumouls commented Aug 24, 2019 •

edited

Loading

Moumouls commented Aug 31, 2019 •

edited

Loading

codecov bot commented Aug 31, 2019 •

edited

Loading