Graphql-shield: Add support to synchronous execution

[iagomelanias]

Feature request

Currently, all rules are executed simultaneously using Promise.all. It should be possible to run rules sequentially, it can be very useful in some cases.

Is your feature request related to a problem? Please describe

Yes, here is an example of a group of permissions to create a bank transfer.

and(isUser, canCreateBankTransfers, canUseBankAccount)

In this scenario, canUseBankAccounk can only be executed if the user is logged in and can create bank transfers. If canUseBankAccount runs without the user being logged in and before isUser is resolved, it will eventually crash, since we could be using the user context which isUser evaluates.

To fix that, we need check if context.user is set in every rule we need to get the user information. This leads to a lot of duplicate logic.

Describe the solution you'd like

Here are some ideas:

• Implement an options argument in every rule.

and(isUser, canCreateBankTransfers, canUseBankAccount, { async: false })

• Add [RuleName]sync rules (Node.js style).

andSync(isUser, canCreateBankTransfers, canUseBankAccount)
orSync(isUser, canCreateBankTransfers, canUseBankAccount)

Describe alternatives you've considered

1. Writing duplicate code.
2. Using a single rule to check all the permissions (isUserAndCanCreateBankTransfersUsingThisAccount 😅).
3. Using rule.resolve inside another rule.

Additional context

• graphql-shield version: 5.1.0

[iagomelanias]

I can work on a PR if you believe this feature can be implemented, @maticzav.

[IsaaX]

So this could be due to some recent changes as well. So if you use version 4.1.2, you can achieve this behavior. I tried upgrading to 5.1.0 and all of my specs ended up failing by what you are describing in this issue due to the context.user not being present. Thought I'd provide that info, perhaps @maticzav would know what was changed that caused this behavior changed.

This is how I've had my rules defined

and(isAuthenticated, or(isAdmin, RuleWithUserInContext))

and when I upgrade to 5.1.0 it breaks my unit tests. When I revert it back without making any changes (other than change whitelist to fallbackRule or vice versa), it goes back to functioning. It would make sense to short circuit and rules once the first condition returns a false similarly how most programming languages act. https://en.wikipedia.org/wiki/Short-circuit_evaluation That's what I was imagining when writing out my logic and made my assumption based on that semantic.

[maticzav]

@iagomelanias @IsaaX thank you for opening this issue! Could you compose a small reproduction repository where this issue can easily be traced? This way I can fix it a lot more quickly.

[IsaaX]

@maticzav @iagomelanias here's a replicated repro https://codesandbox.io/s/jv981orw4v hope it helps! Also might be worth creating your own codesandbox for people to be able to quickly fork it and make faster examples to help you out. Feel free to duplicate mine since it has some custom rules since that'll be the more common case. Think it would be quite helpful and I can reuse it for future issues as well. Cheers!

[maticzav]

@IsaaX thank you for composing the reproduction. The only problem I have with the implementation is whether we should postpone the execution of rules in a logic rule or drive them somehow side by side.

Let me explain what I have in mind. If we short circuit the logic rule, and for example, the first rule takes about 100ms to evaluate and the second about 50ms as well. With the current implementation which I believe covers more than half of the requests the execution tops at 100ms. Short-circuiting the logic rule in such a scenario wouldn't make sense because the load time would rise to 150ms.

On the other hand, we have a non-short-circuited logic rule which, as far as I know, doesn't even throw an error in production. I just noticed your suggestion about the async execution option. Pardon my ignorance. Sounds great!

If anyone of you could prepare a PR, I would be more than happy to review it. Otherwise, I believe I will be able to implement it by the end of the month.

[brunocascio]

Any progress here?

Thanks @iagomelanias for the solution suggested :clap:

[maticzav]

Hey @brunocascio 👋,

I've spent quite some time dwelling about the nucleus of this issue but didn't manage to cover it in here yet. I think we shouldn't make a distinction in the options of and rule but rather introduce a new rule as a whole. I believe such an approach could be more beneficial since there would be a clear distinction in the name itself as opposing to variations of and.

My initial idea was sequence, race or land (as lazy-and) but I haven't made a definite decision yet. What do you think?

[brunocascio]

Hi @maticzav !

I think there are a lot of options to implement this functionality, but in terms of compatibility and SoC, would be ok if the library introduces new methods instead of modifying the existing one.

Scenario:

{
   a: and(RuleA, RuleB),
   b: sequence(RuleC, RuleD),
   c: or(sequence(RuleA, RuleB), race(RuleC, RuleD)),
}

The common Example for sequence is when you have to check user roles and you need the isAuthenticated rule resolved before. So, with sequence could be:

```
const isAuthenticated = rule()((parent, args, ctx, info) => {
const user = req.headers['authorization']
if (!user) {
return false
}
ctx.user = user // injects user in context
return true
})

const isAdmin = rule()((parent, args, ctx, info) => {
// user is available in context since isAuthenticated is resolved before
return ctx.user.role === 'admin'
})

const permissions = {
test: sequence(isAuthenticated, isAdmin)
}
````

What do you think?

NOTE: I've no read the source code, I've just thought how the code looks like.

TL;DR;

There is a library for handling async operations.

[rafaelsplima]

I'm using graphql-shield and I'm going to need this feature, would it be a good way @brunocascio, any progress?

[chanlito]

Any workarounds for this issue right now?

[maticzav]

:tada: This issue has been resolved in version 5.6.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: