Add more flexible low level GraphQL checks

EloyID commented 3 years ago

Is your feature request related to a problem? Please describe. I'm creating a multitenancy system for my application I will tag all my documents with a tenantId (user included) and I want to filter useMulti, useSingle and the queries in general with the tenantId, so I can only see the docs of my client.

Describe the solution you'd like A function or other pattern from vulcan core who let met add this checks from other packages

Describe alternatives you've considered A function like the existing addCallback, maybe addQueryFilter who let me add any check I want before showing documents. addQueryFilter({context, document}=>document.tenantId===context.currentUser.tenantId) I have also considered adding tenantId: {_eq: currentUser.tenantId} to all the inputs but it's easy to fake from client

Additional context Any other patterns are welcome I think the changings would concern these files packages/vulcan-lib/lib/server/apollo-server/context.js packages/vulcan-lib/lib/server/default_resolvers2.js

eric-burel commented 3 years ago

Hi, I'll update this comment progressively, here are my thoughts:

Easy implementation

Fork Vulcan, and focus on the following parts:

the GraphQL context creation function. You can extend it at will. It is based on the current request, and can do asynchronous calls. It is quite similar to using a middleware in Express to fetch data about the session.
the default resolvers and mutations

The context should:

If the user has not selected a tenant, fetch the default tenant for this user and put it into the context
If the user has selected a tenant, read the selected tenantId in the request cookies, check that the user can actually access this tenant, and put the tenant in the context.

The resolvers should:

Filter out document based on the current tenant, eg by altering the user filter or mongo query to guarantee that the tenantId is always respected

The limitation is that you fork Vulcan, but that's honestly not a huge deal. In particular, it is fine to copy the default resolvers and improve them in your app: they are just default, they are not meant to fit any kind of use case you can imagine.

We suppose that:

the user always has a default tenant
the user can switch tenant. This means that we store the current tenant in a Cookie client-side, so it is passed alongside all requests.

More generic implementation (for multitenancy but also: draft management, document moderation...)

Forking the context creation is a bit more annoying though. To solve this, we could simply add a callback in the context creation function to allow user to enhance the context. This opens a lot of possibilities.

We can also add callbacks in the default resolvers to be able to alter the user filter. The difference between that and modifying useMulti variables for instance, is that the callbacks are run server-side => they are secure.

Filtering at Mongo level

It is also good practices to go higher up if you want to truly ensure that data are filtered correctly, so user A of tenant X can't see data from user B or tenant Y.

Handling "singleton" currentUser in graphql?

How to handle a "singleton" in graphql? For instance, the currentUser will always be THE current user. You don't care about the _id or whatever, because you can have only one current user. However, in my Cypress tests, it messes with caching. For instance, I do 2 queries "getCurrentUser { profile }", "getCurrentUser { activeTenant }". I get a message like "instrument.js?ea14:110 Cache data may be lost when replacing the getCurrentUser field of a Query object." because Apollo has trouble merging both response into the "currentUser" object. GraphQL can't know if you may store multiple current user or just one.

To address this problem (which is not a bug in Apollo Client), either ensure all objects of type CurrentUserType have IDs, or define a custom merge function for the Query.getCurrentUser field, so InMemoryCache can safely merge these objects:**

Apollinaire commented 3 years ago

Since @EloyID asked me to implement this, here's my view on the subject:

Forking Vulcan is not an option, the cost of maintaining an ever-shifting fork is too big, and Vulcan tries to give opinionated defaults but lets the devs inject their logic where they need. The goal we're trying to reach is not too far from what Vulcan offers, so it should be possible to do without forking Vulcan.

Adding callbacks to modify some objects is probably the right way of doing things. I don't know how we are documenting these however (the debug panel seems to lack a lot of these, not to mention the docs). I'm used to just read the source code, but we can't expect that from every user.

So, I added a callback to update the context in PR #2689 (thanks for the quick followup!). We can easily handle queries and mutations that affect only one document via the permissions. The multi resolver is a bit more tricky because the "canRead" permission won't work for the totalCount. So we want a way to modify the selector and options given to the mongo query.

The legacy collection.getParameters used callbacks for that, which were not included in the new Connector.filter call.

Currently, in the multi resolver has the following:

        let { selector = {}, options = {}, filteredFields = [] } = isEmpty(terms)
          ? await Connectors.filter(collection, input, context) // can't be changed dynamically
          : await collection.getParameters(terms, {}, context); // can be changed dynamically

So in order to be able to modify selectors dynamically inside Connectors.filter too, I've created PR #2690 .

VulcanJS / Vulcan