Feature request: Promises in ability conditions

[RedShift1]

Abilities allow using functions in the conditions of abilities, such as:

can('update', 'Post', { userId: function(id) { return id === 5 } });

Would it be possible to extend this API to also support promises? This would be handy for cases like querying databases, which return a promise.

Test example:

class Post {
    constructor(props) {
        Object.assign(this, props)
    }
}
function standardUser()
{
    return AbilityBuilder.define(
        (can, cannot) =>
        {
            can('update', 'Post', { userId: Promise.resolve(true) });
        }
    );
}

const abilities = standardUser();

console.log(abilities.can('update', new Post({userId: 5})));

Right now this evaluates to false because it doesn't recognize promises.

[stalniy]

What is the usecase for this?

Not sure whether this is a good idea because it will be hard to understand/optimize amount of queries generated by authorization library

[RedShift1]

Let's say you have users and devices, and users have access to one or more devices. The relationship between users and their devices is stored in a table in a database. You have some data about those devices stored in Redis and want to protect this function:

function doRead(data) // data is user input
{
    return redis.get(`somekey:${data.device}`);
}

It would work as follows (pseudo code):

const ability = AbilityBuilder.define(
    (can, cannot) =>
    {
        can('read', 'SomekeyRequest',
            {
                device: function(device) // Function that returns a Promise
                    {
                        return db.query(`SELECT COUNT(*) AS cnt FROM userDevices WHERE deviceId = ${device}`)
                            .then(resultSet => resultSet.cnt > 0);
                    }
            }
        );
    }
);

function doRead(data)
{
    return ability.can('read', data)
        .then(
            (authz) =>
            {
                if(!authz)
                    throw 'Unauthorized';
                else
                    return redis.get(`somekey:${data.device}`);
            }
        );
}

[stalniy]

The main issue here is that sift and mingo(libs that can do mongodb like checks) do not support async checks.

So, the starting point for PoC is to detect async conditions and resolve them, later check conditions as usually.

There are 3 downsides with this approach however:

1. You will not be able to serialize rules as soon as you define function in conditions
2. You will no longer be able to convert conditions into database query (at least without custom logic)
3. Very likely you will need to use batching logic (e.g., dataloader) to decrease amount of db queries

That’s why I tried to postpone implementation of something like this.

[JaneJeon]

I would REALLY like to see this idea dropped.

For reference, I built https://github.com/JaneJeon/objection-authorize which is built on top of a competing access control library, https://github.com/tensult/role-acl.

The developer of role-acl suddenly one day "added" the ability to have an async function in conditions, and that meant literally every method had to be changed to async.

And that broke everything needlessly. And that's actually the main reason why I'm looking to migrate my library to using casl instead.

I feel that you SHOULDN'T have async conditions to "query the database". If you need something async, you should put it OUTSIDE the actual ability condition checks - provide all of the information necessary to make a decision to the ability builder in forms of context objects.
In addition, you almost always want the frontend to share the same ability class that's used in the backend so you can limit the UI based on the ACL, but if you're "querying the database" from the frontend, you're literally BEGGING to be hacked.

You don't need async conditions, so the benefits are nil to begin with, and if you do "add" this "feature", it will break everyone's code. BAD IDEA.

@stainly I hope to dear god that you don't consider this. It's far more changes than you'd think for basically no benefit.

[stalniy]

@JaneJeon thanks for sharing your opinion!

Currently I don’t plan to change anything as I need to think about this more closely. Very likely that logic can be inverted and left to be pure and sync on Ability side.

There were some thoughts about adding functionality not changing. So, be sure, there will be always logic in Casl to check conditions synchronously.

[JaneJeon]

Very likely that logic can be inverted and left to be pure and sync on Ability side.

You're absolutely right. I'd use the factory pattern, where the context is provided beforehand:

async function defineAbilitiesFor(user, device) {
  // also @RedShift1 you're just begging for an injection attack if you're using template strings for this
  const deviceCount = await db.query(`SELECT COUNT(*) AS cnt FROM userDevices WHERE deviceId = ${device}`)

  return AbilityBuilder.define((can, cannot) => {
    if (deviceCount) can('read', 'SomekeyRequest')
  })
}

This solves RedShift's problem without having to add async ability conditions. I see no reason to even consider changing the way this library works when there's a working solution like this.

Please feel free to close this issue as it's solved. @stalniy

[stalniy]

Probably what @RedShift1 wanted is a posibility to lazily define conditions for rules.

Why is useful?

because it may be not optimal to query database for device count on each request (without caching) when you want to check permissions on action and resource which has nothing to do with user devices.

Solution:

I see 2 solutions for this issue:

1. Split rules definition into 2 function (maybe Ability instances):
   • with side effects
   • without side effects
2. Add possibility to define rules with lazy conditions. This will bring async complexity to casl but this complexity may be expressed in a separate LazyAbility class which inherits all functionality from Ability

Anyway there should be a logic which allows to associate some rules with side effects. But the 1st solution I like more as it does not introduce anything additional to the library.

[stalniy]

Something like this should work for a while:

function basicRules() {
  const { rules, can } = AbilityBuilder.extract()
  can('read', 'Post')
  return rules
}

async function devicesRules() {
  const count = await getDeviceCount()
  const { rules, can } = AbilityBuilder.extract()

  if (count > 2) {
    can('read', 'Device')
  }

  return rules // or basicRules().concat(rules)
}

const rulesProvider = {
  basic: basicRules,
  devices: devicesRules
}

function provideAbility(rulesType) {
  const defineRules = rulesProvider[rulesType]

  return async (req, res, next) => {
    const rules = await defineRules()
    req.ability = new Ability(rules)
  }
}

const app = express()

app.get('/posts', provideAbility('basic'), findPosts)
app.get('/devices', provideAbility('devices'), findDevices)

[JaneJeon]

Why is useful?

because it may be not optimal to query database for device count on each request (without caching) when you want to check permissions on action and resource which has nothing to do with user devices.

The way I see it, you are basically caching, only with ability classes, if you choose to define the "get device count and check if count is greater than $number" functionality within the ability.

Think about it - device counts can change over time, so if you really want an accurate logic, then you would need to check/create this ability class every time you get a device request. In the end, you're just moving the problem to within the ability class which is just feature bloat imo.

If you want to handle "I don't need to get the device count for every request", then your approach above works perfectly - define different abilities for every route, or if that sounds verbose, have a "central" ability factory that changes what it outputs (so that it doesn't need to call getDeviceCount() for non-device queries) based on the given context.

[stalniy]

Don't close as the recommendation should be documented in docs

[stalniy]

Will be documented as part of #204