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

3.0.1 Whitelisting is not working as expected when referencing your specs #136

Closed
1 task done
IsaaX opened this issue Aug 29, 2018 · 3 comments
Closed
1 task done

Comments

@IsaaX
Copy link

IsaaX commented Aug 29, 2018

Bug report

  • I have checked other issues to make sure this is not a duplicate.

Describe the bug

Hello, big fan of this package! Not sure if this is entirely a bug, could be an implementation detail I'm not understanding. With the latest 3.0.1 I'm trying out the whitelist option and I ran into this scenario and was wondering if this was a bug or not.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. This is my GraphQL Schema
type User {
  id: Int!
  username: String!
  role: String!
  isActive: Boolean
  email: String!
  profile: UserProfile
  auth: UserAuth
}
Query{
  currentUser: User
}
  1. Query this
{
  currentUser {
    id
    username
    isActive,
    role
  }
}
  1. Use that permissions
const isAuthenticated = rule()(
  async (parent, args, ctx, info) => {
    const result = true;

    log.info('test....', result);
    return result;
  });
const permissions = shield({
  Query: {
    currentUser: allow
  },
  Mutation: {},
  User: {
    id: isAuthenticated,
    username: isAuthenticated,
    isActive: isAuthenticated,
    role: deny
  }
}, {
  debug: true,
  whitelist: true
});
  1. See error
{
  "data": {
    "currentUser": null
  },
  "errors": [
    {
      "message": "Not Authorised!",
      "locations": [
        {
          "line": 6,
          "column": 5
        }
      ],
      "path": [
        "currentUser",
        "role"
      ]
    }
  ]
}

Expected behavior

What I expected for this interactions (I was referencing your specs when deriving my example) https://github.com/maticzav/graphql-shield/pull/119/files#diff-6b51ffc4039e2d70d0a58c36b16f55dcR77

I was expecting to get a partial object with the deny field role would be returned as null, just like in the specs.

{
  "data": {
    "currentUser": {
      "id": 1,
      "username": "admin",
      "isActive": true,
      "role": null
    }
  }
}

Actual behaviour

I receive this input

{
  "data": {
    "currentUser": null
  },
  "errors": [
    {
      "message": "Not Authorised!",
      "locations": [
        {
          "line": 6,
          "column": 5
        }
      ],
      "path": [
        "currentUser",
        "role"
      ]
    }
  ]
}

Additional context

This seems to work fine if I nest another object into the currentUser, whitelist everything about currentUser, and deny that nested object in currentUser, I obtain the currentUser correctly and the nested object is returned as null.

@maticzav
Copy link
Owner

Whoa, @IsaaX what an issue report! 🎉 I introduced templates yesterday and can see the benefits immediately. How did you like it? Does everything seem in place, anything missing? Let me know, and huge thanks for trying it out. 😄


Regarding your problem now, it's an implementation detail that is messing your code. I call it the GraphQL Schema contract - I'll also be posting an article about it, but for now you can read the idea in one of the recent issues.

#126 (comment)

Long story short, it's the role: deny and role: String! lines that are preventing your query from resolving the way you expect. Briefly, you are trying to convince the client that it will 💯 receive the data but in the next step hide it. Because of this it cannot resolve User as a whole correctly and resolves in null.

I hope this gives you some leads to your problem. Let me know if you need any help implementing it! 🙂

@IsaaX
Copy link
Author

IsaaX commented Aug 29, 2018

That makes complete sense and I had read that issue yesterday too! I didn't connect the dots, doh! I get what is happening now. So Ill close this and appreciate the quick response!

RE: Templates
Not going to lie, it made reporting easy and really helped me guide my thoughts down in a simple manner. I'll provide a bit of feedback and feel free to use/disregard it as you see fit! I would suggest the following template with tweaks that I made that would make the process a bit simpler for future candidates which should help them copy and paste their relevant info I'd hope. It's basically just rearranging your current template a bit more (the bottom part to the more relevant parts). So I hope this helps. I'll try to help with the repo soon as well once I'm setup with this. It's been awesome working with it thus far! Great job on everything you've been doing.

Describe the bug

A clear and concise description of what the bug is.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. This is my GraphQL Schema. (Delete the filler code and replace it with your own)
type Query {
  book: Book!
}

type Book {
  id: ID!
  name: String!
  content: String!
}
  1. Query that is invoked
book {
  id
  name
  content
}
  1. Use these permissions
const permissions = shield({
  Query: {
    book: allow,
  },
  Book: {
    content: deny,
  },
})
  1. See error
<insert error here>

Expected behavior

A clear and concise description of what you expected to happen.

Actual behaviour

If applicable, add screenshots to help explain your problem.

Additional context

Add any other context about the problem here.

@IsaaX IsaaX closed this as completed Aug 29, 2018
@maticzav
Copy link
Owner

Just changed! Great additions. Thank you for contributing 🎉

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

No branches or pull requests

2 participants