Resolved fields do not get filtered by the permission system

[Discordius]

There is still a problem with how resolvers and the permission system interact, which I think I brought up a while ago.

Here for example is a schema field of mine:

{
    fieldName: 'IPs',
    fieldSchema: {
      type: Array,
      optional: true,
      group: formGroups.banUser,
      canRead: ['sunshineRegiment', 'admins'],
      resolveAs: {
        type: '[String]',
        resolver: (user, args, context) => {
          const IPs = context.LWEvents.find({userId: user._id, name: 'login'}, {fields: context.Users.getViewableFields(context.currentUser, context.LWEvents), limit: 10, sort: {createdAt: -1}}).fetch().map(event => event.properties && event.properties.ip);
          const uniqueIPs = _.uniq(IPs);
          return uniqueIPs
        },
        addOriginalField: false,
      },
    }
  },

  {
    fieldName: 'IPs.$',
    fieldSchema: {
      type: String,
      optional: true,
    }
  }

Based on how things usually work, one would expect that you can't resolve the IPs field if you are not either in the group admins or sunshineRegiment. However, any user can actually resolve this field, which has caused some security issues in my app. Changing this behavior to make it so that resolved fields by default get filtered out if the permissions are specified.

[justinr1234]

You'd want to modify the code here to respect the checks: https://github.com/VulcanJS/Vulcan/blob/c4cec6a0122c2386a5f4a22e0f9942f2d5a8fb83/packages/vulcan-lib/lib/modules/graphql.js#L175

[SachaG]

OK so this helped me clarify my understanding of how resolved fields work. It makes sense when you think about it, but I just hadn't really paid enough attention before!

So resolver will always run, and like someone said elsewhere they get added after the Vulcan code runs. So it's not something we can control.

However we can control the code of the resolver function. So for example if your IPs resolver should only return a value if the user has access to the IPs field then just wrap your resolver in a if (document.IPs) {...} block (where document is the current user, movie, post, etc. i.e. the first argument of the resolver function).

So this isn't really a bug, more of a consequence of how GraphQL works.

The reason you ran into the bug in this specific case is because your IPs resolver doesn't actually make use of the value of the IPs field, and thus keeps on returning a value even when said field was restricted.

I'll close this for now but feel free to reopen it if I missed something.

[justinr1234]

I think this is misleading. If you define the field to respect roles and it doesn’t, it isn’t immediately obvious why. I believe the Vulcan core should respect security checks in the resolver function by wrapping the user’s resolver passed in the resolveAs in a standard Vulcan check for the roles defined on the field.

[Discordius]

I agree that this makes sense given the architecture, but I think it is really surprising for someone who hasn't thought about the architecture for a long time, and the consequences are really bad if you mess up your permission system.

I think wrapping the resolver in an access check in the file that justin posted above is a much better behavior here.

[SachaG]

OK that makes sense. Let's see if this works better then.

[SachaG]

Note that the resolved field just uses the same permissions as the "original" field. I don't know if there's a use case where you'd want the two fields to have different permissions?

[eric-burel]

It sounds right to me and more intuitive. You may need stricter permissions for nested fields of the resolved objects but never lighter permissions. It still allows fully custom behaviour as you can always make your permissions less strict and enforce them by yourself or handle failure case in a specific way (like trigger an alert to an admin for very sensitive data etc.).