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

Allow specifying rules for both types and fields #680

Open
luhagel opened this issue Mar 1, 2020 · 6 comments
Open

Allow specifying rules for both types and fields #680

luhagel opened this issue Mar 1, 2020 · 6 comments
Labels
kind/feature A request for a new feature.

Comments

@luhagel
Copy link

luhagel commented Mar 1, 2020

Feature request

Is your feature request related to a problem? Please describe

As far as I'm aware, right now you can specify shield rules for either a type or a selection of subfields on that type, but not both.
So if I wanted to restrict access to users that have been archieved, I can do that no problem, if I want to restrict access so only the current user can see their respective address, no problem either, but I'm not aware of a way to combine the two.

Describe the solution you'd like

Implementing this might be really hard to implement without breaking anything. Ideally you'd have the option to introduce an special field into your type permissions that handles the type permissions, as that would hopefully be the least invasive.

So somethina long the lines of:

shield({
User: {
_canViewModel: rules.notArchieved,
address: rules.isCurrentUser
})

Describe alternatives you've considered

Other solutions would include changing the internal shieldRules schema to diffrentiate between subfields and the parent type, or having a diffrent naming convention for the two, along the lines of User: {...}, Usertype: rule, but both of those wouldd probably break to much existing code.

Additional context

@open-collective-bot
Copy link

Hey @luhagel 👋,

Thank you for opening an issue. We will get back to you as soon as we can. Also, check out our Open Collective and consider contributing financially.

https://opencollective.com/graphql-shield

PS.: We offer priority support for all financial contributors. Don't forget to add priority label once you start contributing 😄

@maticzav
Copy link
Owner

maticzav commented Mar 11, 2020

Hey @luhagel 👋 ,

Thank you for opening the issue. I had a similar idea a long time ago that would allow us to use and, or, and other logical patterns on dictionaries as well as rules. In the case of dictionaries, we would simply merge the rules at the lower level.

For example,

const permissions = shield({
  Query: and(isAuthenticated, {
    fruits: or(isAdmin, isEditor),
    customers: isAdmin,
  },
})

would translate to

const permissions = shield({
  Query: {
    fruits: and(isAuthenticated, or(isAdmin, isEditor)),
    customers: and(isAuthenticated, isAdmin),
  }
})

Is that what you had in mind?

It's one of the last things that I think are truly missing from the library alongside race rule (#645).

I am sad to say this, but since this library lost its funding, I am not sure when I'll find time to implement these features.

@maticzav maticzav added the kind/feature A request for a new feature. label Mar 11, 2020
@luhagel
Copy link
Author

luhagel commented Mar 20, 2020

Exactly!

I might be able to look into this as well, given I find some time off.

Just wanted to make sure this wasn't already a thing before diving into it.

@maticzav
Copy link
Owner

Awesome! I think this could be entirely implemented during a permission tree generation step. Perhaps this is of help to you 🙂

Looking forward to your PR 😄

@vinverdy
Copy link

vinverdy commented Jul 3, 2020

cmiiw, if we translate that way.

For something like this

const permissions = shield({
  User: and(isAuthenticated, {
    name: or(isAdmin, isEditor),
    email: isAdmin,
  },
})

would translate to

const permissions = shield({
  User: {
    name: and(isAuthenticated, or(isAdmin, isEditor)),
    email: and(isAuthenticated, isAdmin),
  }
})

Therefore it will return

 {"user": {"name": null, "email": null}}

instead of

 {"user": null}

is it right?

Hey @luhagel 👋 ,

Thank you for opening the issue. I had a similar idea a long time ago that would allow us to use and, or, and other logical patterns on dictionaries as well as rules. In the case of dictionaries, we would simply merge the rules at the lower level.

For example,

const permissions = shield({
  Query: and(isAuthenticated, {
    fruits: or(isAdmin, isEditor),
    customers: isAdmin,
  },
})

would translate to

const permissions = shield({
  Query: {
    fruits: and(isAuthenticated, or(isAdmin, isEditor)),
    customers: and(isAuthenticated, isAdmin),
  }
})

Is that what you had in mind?

It's one of the last things that I think are truly missing from the library alongside race rule (#645).

I am sad to say this, but since this library lost its funding, I am not sure when I'll find time to implement these features.

@maticzav
Copy link
Owner

maticzav commented Jul 3, 2020

Not exactly. I mean it depends on your schema, not your permissions.

If one of your fields (let's say email) were required in User and the email-resolver returned null, you'd get

{ "user": null }

If email were an optional field, however, you'd get

{ "user": { "name": null, "email": null } }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for a new feature.
Projects
None yet
Development

No branches or pull requests

3 participants