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

GraphQL: ACL #5957

Open
wants to merge 4 commits into
base: master
from

Conversation

@Moumouls
Copy link
Member

commented Aug 22, 2019

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
}
Spec
Fix Spec

@Moumouls Moumouls requested a review from davimacedo Aug 22, 2019

@Moumouls Moumouls force-pushed the Moumouls:gql-acl branch from fd020e4 to 852d0b6 Aug 22, 2019

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

It looks good!

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2019

@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

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

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

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

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

@Moumouls Moumouls marked this pull request as ready for review Aug 31, 2019

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2019

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

@codecov

This comment has been minimized.

Copy link

commented Aug 31, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

1 similar comment
@codecov

This comment has been minimized.

Copy link

commented Aug 31, 2019

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Copy link

commented Aug 31, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5957      +/-   ##
==========================================
- Coverage   93.75%   93.75%   -0.01%     
==========================================
  Files         156      156              
  Lines       10939    10985      +46     
==========================================
+ Hits        10256    10299      +43     
- Misses        683      686       +3
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%> (ø) ⬆️

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...b3882a9. Read the comment docs.

@davimacedo
Copy link
Member

left a comment

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.

type: new GraphQLNonNull(GraphQLID),
},
read: {
description: 'Allow the user to read the current object.',

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 2, 2019

Member

Should we also add the information that read is true by default?

},
write: {
description:
'Allow the user to write on the current object. If true, read will be set to true.',

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 2, 2019

Member

Should we also add the information that write is true by default?

},
write: {
description:
'Write is true by default. Allow users who are members of the role to write on the current object. If true, read will be set to true.',

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 2, 2019

Member

Do we really need to say "If true, read will be set to true.", since read is true by default?

This comment has been minimized.

Copy link
@Moumouls

Moumouls Sep 3, 2019

Author Member

Yes, i think here the information is important, we need to be as clear as possible about the behavior of ACL.

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 4, 2019

Member

Do you mean that we cannot have read=false and write=true?

},
write: {
description:
'Write is true by default. Allow anyone to write on the current object. If true, read will be set to true.',

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 2, 2019

Member

Do we really need to say "If true, read will be set to true.", since read is true by default?

This comment has been minimized.

Copy link
@Moumouls

Moumouls Sep 3, 2019

Author Member

Yes, i think here the information is important, we need to be as clear as possible about the behavior of ACL.

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 4, 2019

Member

Do you mean that we cannot have read=false and write=true?

},
read: {
description: 'Allow the user to read the current object.',
type: GraphQLBoolean,

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 2, 2019

Member

Shouldn't be non null?

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 2, 2019

Member

If nothing in the database, we could return false

src/GraphQL/loaders/defaultGraphQLTypes.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/defaultGraphQLTypes.js Outdated Show resolved Hide resolved
{
roleName: 'aRole',
read: true,
write: false,

This comment has been minimized.

Copy link
@davimacedo

This comment has been minimized.

Copy link
@Moumouls

Moumouls Sep 3, 2019

Author Member

Because if you only explicitly provide read to Parse it seems that write become false.
Maybe i'm wrong ?

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 4, 2019

Member

Since we've decided to make read and write defaulting to true, I think we should make it true besides Parse (I am actually not sure that Parse will set it to false as well). Otherwise it will be an unexpected behavior of the GraphQL API.

__typename: 'RoleACL',
},
],
public: { read: true, write: false, __typename: 'PublicACL' },

This comment has been minimized.

Copy link
@davimacedo

This comment has been minimized.

Copy link
@Moumouls

Moumouls Sep 3, 2019

Author Member

Because if you only explicitly provide read to Parse it seems that write become false.
Maybe i'm wrong ?

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 4, 2019

Member

Same thing here

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

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 4, 2019

Member

I am not sure that's true

if (rule.indexOf('role:') === 0) {
roles.push({
roleName: rule.replace('role:', ''),
read: p[rule].read || p[rule].write ? true : false,

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 4, 2019

Member

idem here

/* eslint-disable */
return p['*']
? {
read: p['*'].read || p['*'].write ? true : false,

This comment has been minimized.

Copy link
@davimacedo

davimacedo Sep 4, 2019

Member

and here

@davimacedo

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@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

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.