Skip to content

Access denied logic #731

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

Closed
wants to merge 9 commits into from
Closed

Access denied logic #731

wants to merge 9 commits into from

Conversation

Vincz
Copy link
Collaborator

@Vincz Vincz commented Aug 15, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Documented? yes
Fixed tickets #674
License MIT

This PR adds the ability to return null instead of raising an exception when the access to a field is denied.
It will not enforce the field as nullable so field with an access must be configured properly.

@mcg-web I didn't force the field as nullable as it implied to modify the user defined schema and would for example modify all a type fields as nullable if the object has a fieldsDefaultAccess. Better to let the user implement the logic himself. Don't you think?

@Vincz Vincz requested review from mcg-web and murtukov August 15, 2020 16:36
Copy link
Contributor

@mcg-web mcg-web left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When security matter I always double check, but here my review part 1.

Vincz and others added 2 commits August 16, 2020 09:28
Typehint annotation property

Co-authored-by: Timur Murtukov <[email protected]>
@Vincz
Copy link
Collaborator Author

Vincz commented Aug 16, 2020

@mcg-web Should we raise an exception when non boolean valus are returned from access callback or if the promise resolve to non boolean value ? Right now, if something other than true is return, it's an access denied and we raise an exception or a warning.

murtukov
murtukov previously approved these changes Aug 16, 2020
@murtukov
Copy link
Contributor

@Vincz squashing commits after all tests pass would be nice

@Vincz
Copy link
Collaborator Author

Vincz commented Aug 16, 2020

@murtukov I'll try ! But I'm waiting for @mcg-web to complete is review as it's security related.

@mcg-web
Copy link
Contributor

mcg-web commented Aug 16, 2020

After re-reading this PR I don't really get it.
Lets take this example:

Query:
  type: object
  config:
    fields:
        user: User!

User:
  type: object
  config:
    fields:
        nullableField:
          type: String
          access: #...
       nonNullableField:
          type: String!
          access: #...

if a user without access requests nullableField

{ user { nullableField }

the result will be null with an access warning

{ "data": { "user": { "nullableField": null } }, "extensions": {"warnings": ["..."]} }

but if this same user requests nonNullableField

{ user { nonNullableField }

the result will be an error since the field can't be nullable

{ "data": null, "errors": ["..."] }

@Vincz What is the difference with your PR ? Can you provide an example please.

@Vincz
Copy link
Collaborator Author

Vincz commented Aug 16, 2020

@mcg-web You are right. The only difference is that we don't get the warning when the option nullOnDenied is set. It doesn't make sense without the nullable part. I'm sorry. I don't think clearly this day due to too much work. I need vacation. I'll have a break and check this later if I can address the nullable problem. Sorry again.

@mcg-web
Copy link
Contributor

mcg-web commented Aug 16, 2020

All this part of the bundle is not really clear, that's why in the next version we must try to make this easier. Not a big deal sorry for the time I take before seeing that this was not totally complete 😥 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants