adonis-guard icon indicating copy to clipboard operation
adonis-guard copied to clipboard

async policy method

Open wuzi opened this issue 5 years ago • 16 comments

Is it possible to create async methods in policy?

Sometimes I need to check another table to verify if an user has permission, but when adding async to policy it is not recognized anymore:

destroy (user, resource) { // works
  return true
}

async destroy (user, resource) { // doesn't
  return true
}

wuzi avatar Nov 28 '18 23:11 wuzi

Hey @Wuzi! 👋

It should work fine. Could you please give me a repository with the reproducible example?

RomainLanz avatar Nov 29 '18 07:11 RomainLanz

Hey, sure: https://github.com/Wuzi/testing

I've added two routes (/guard1, /guard2) one with async and one without both returning false, needs to authenticate first using POST /register sending username, email, password and password_confirmation.

wuzi avatar Nov 29 '18 10:11 wuzi

by looking at the code it seems you have forgotten to await the call to guard.denies().

if (await guard.denies('store', new User()))
  return response.status(403).json({ message: 'You don\'t have permission' })

RomainLanz avatar Nov 29 '18 16:11 RomainLanz

For a second I thought that could be it, but even with await guard.denies('store', new User()) i got the same result.

wuzi avatar Nov 29 '18 18:11 wuzi

I got the issue, will provide a fix soon. I simply need to figure out the best way to fix this! 😃

RomainLanz avatar Nov 29 '18 19:11 RomainLanz

@RomainLanz Hey Romain!

I saw that async/await has been removed from Guard.denies and Guard.allows in Slynova-Org/fence on that commit here: https://github.com/Slynova-Org/fence/commit/efa335127f0e45ab8839a6c8a83bcf1fe2f247b7#diff-7ea95c2e44479d89952a76cf682ac915

Any reason of why it has been changed back?

I would have also like to make async call in Policies and GateNames and to make that happen I promisified denies and allows exactly like in that commit above. But I was just wondering has why you change it back and removed async from these methods.

Cheers :)

jayrchamp avatar Apr 28 '19 18:04 jayrchamp

@RomainLanz These were the changes I made in a fork to make my async calls work:

// src/guard/index.ts

  /**
   * Check the bouncer if the gate/policy allows the user.
   *
   * @param ability  Ability to test
   * @param resource Resource to test
   * @param user     Optional. User to verify
   */
  public async allows (ability: string, resource: TResource, user: Function | object | undefined): Promise<boolean> {
    const usedUser = (user !== undefined) ? user : this.$user

    try {
      if (this.$correspondsToPolicy(resource)) {
        return (new Bouncer(usedUser)).callPolicy(ability, resource)
      }

      return (new Bouncer(usedUser)).pass(ability).for(resource)
    } catch (e) {
      return false
    }
  }
// src/guard/index.ts

  /**
   * Check the bouncer if the gate/policy denies the user.
   *
   * @param ability  Ability to test
   * @param resource Resource to test
   * @param user     Optional. User to verify
   */
  public async denies (ability: string, resource: TResource, user: Function | object | undefined): Promise<boolean> {
    return !( await this.allows(ability, resource, user))
  }

I know this is more related with slynova-org/fence than adonis-guard. But since it's kind of related with this issue, I thought posting it here would make sense.

jayrchamp avatar Apr 28 '19 19:04 jayrchamp

Yes, that's the fix.

The issue is that it doesn't work if you are using the @cannot tag because Edge cannot be async. @thetutlage is looking into it to maybe make Edge 2 async.

RomainLanz avatar Apr 28 '19 19:04 RomainLanz

@RomainLanz Roger that! Thx for your time!

jayrchamp avatar Apr 28 '19 20:04 jayrchamp

@RomainLanz

After a few tests, I realized that await guard.allows() works with async calls since it returns the promise that has been automatically resolved by the async Policy or GateName.

await guard.denies() use the logical “not” operator to return the opposite of allows which is perfectly normal, but I just noticed that this is causing the issue. await guard.denies() can't return the promise resolved by the async Policy/GateName since it tries to convert it to a boolean.

Please, correct me if I'm mistaken something.

It would be much appreciated if you could just give me your 2 cents about this below ->

Not a fan of monkey patching but as a temporary solution and since I'm not using Edge Templating, what do you think of this?

  • Use a custom GuardInit middleware
  • Overwrite denies and make it async.
  • Set default user and add guard in context
'use strict'

const { Guard } = require('@slynova/fence')

class MyCustomGuardInitMiddleware {
    async handle (ctx, next) {

        /*
        |--------------------------------------------------------------------------
        |   Monkey patching to allow async call inside `guard.denies`
        |--------------------------------------------------------------------------
        */
        Guard.prototype.denies = async function (ability, resource, user) {
            return !(await this.allows(ability, resource, user))
        }

        const guard = Guard.setDefaultUser(ctx.auth.user || null)

        /**
         * Add guard in context
         */
        ctx.guard = guard

        await next()
    }
}

module.exports = MyCustomGuardInitMiddleware

// start/kernel.js

const globalMiddleware = [
      // ...
    - 'Adonis/Middleware/GuardInit',
    + 'App/Middleware/MyCustomGuardInitMiddleware',
      // ...
]

Thx for your help! Greatly appreciated!

jayrchamp avatar Apr 28 '19 22:04 jayrchamp

Hi @jayrchamp I was having the same issue and after reading your post I changed 'denies' for 'allows' and now it works well. thanks!

woodgates avatar May 16 '19 14:05 woodgates

Sorry for the late reply @jayrchamp.

Yes, this is what should be done to fix this issue. I'm going to release a fix and add a statement in the README about denies not working with Edge.

Also, when you are monkey patching, you only want to monkey patch something once and not at every requests. Therefore, it's better to do it in a Provider or a Hook instead of using a Middleware.

RomainLanz avatar May 20 '19 07:05 RomainLanz

@RomainLanz

Also, when you are monkey patching, you only want to monkey patch something once and not at every requests. Therefore, it's better to do it in a Provider or a Hook instead of using a Middleware.

So true, didn't think of it. Thanks a lot !

jayrchamp avatar May 20 '19 12:05 jayrchamp

@woodgates

I was having the same issue and after reading your post I changed 'denies' for 'allows' and now it works well. thanks!

Glad it helped!

jayrchamp avatar May 20 '19 13:05 jayrchamp

I am also going to restructure my API to use allows instead and then simply check if it is false but this fix on async guard.denies would be greatly appreciated.

FrenchMajesty avatar Oct 30 '19 18:10 FrenchMajesty

Happy to accept a PR @FrenchMajesty!

RomainLanz avatar Oct 30 '19 19:10 RomainLanz