Added the possibility to have a wildcard in resource name? #30

Open
SeyZ opened this Issue Aug 11, 2013 · 12 comments

Comments

Projects
None yet
8 participants
@SeyZ

SeyZ commented Aug 11, 2013

What about having a wildcard management for this kind of resource name:

/api/things/*

@manast

This comment has been minimized.

Show comment Hide comment
@manast

manast Aug 14, 2013

Member

sorry for the late response.

It is an interesting feature, I will consider adding it. Can you describe a use case where you feel this is particularly useful?

regards.

Member

manast commented Aug 14, 2013

sorry for the late response.

It is an interesting feature, I will consider adding it. Can you describe a use case where you feel this is particularly useful?

regards.

@icompuiz

This comment has been minimized.

Show comment Hide comment
@icompuiz

icompuiz Feb 5, 2014

  • Granting all permissions for a particular user (i.e. isAllowed always evaluates to true)
acl.allow('admin', '*', '*')
  • Granting permissions for all resources with a particular prefix (resource name starts with)
acl.allow('user', '/api/things/*', ['get'])

A more robust solution could allow regular expression based resource names

acl.allow('xyz', /^\/api\/resource\/[0-9A-Z]$/, ['get', 'put'])

icompuiz commented Feb 5, 2014

  • Granting all permissions for a particular user (i.e. isAllowed always evaluates to true)
acl.allow('admin', '*', '*')
  • Granting permissions for all resources with a particular prefix (resource name starts with)
acl.allow('user', '/api/things/*', ['get'])

A more robust solution could allow regular expression based resource names

acl.allow('xyz', /^\/api\/resource\/[0-9A-Z]$/, ['get', 'put'])
@manast

This comment has been minimized.

Show comment Hide comment
@manast

manast Feb 6, 2014

Member

It is indeed a very nice feature, unfortunately it is not quite so easy to implement right now since resource matching is done using a "union" primitive from the backend, and there is no way to ask for a set of resources based on a regular expression or similar...

Member

manast commented Feb 6, 2014

It is indeed a very nice feature, unfortunately it is not quite so easy to implement right now since resource matching is done using a "union" primitive from the backend, and there is no way to ask for a set of resources based on a regular expression or similar...

@AnthonyMDev

This comment has been minimized.

Show comment Hide comment
@AnthonyMDev

AnthonyMDev May 1, 2014

This is a feature I would really like to see as well.

This is a feature I would really like to see as well.

@icompuiz

This comment has been minimized.

Show comment Hide comment
@icompuiz

icompuiz May 13, 2014

After working around this I have found that although it would be useful, it is not absolutely necessary. The idea of an access control list to to provide granular access control. If resources are able to be broadly defined, as they would be with wildcard definitions, you expose your application to unnecessary risk.

Although tedious, the alternative falls well within the scope of this library. That is, manually defining each resource to be controlled, which users/groups have permissions on those resources, and what permissions each users/groups has.

After working around this I have found that although it would be useful, it is not absolutely necessary. The idea of an access control list to to provide granular access control. If resources are able to be broadly defined, as they would be with wildcard definitions, you expose your application to unnecessary risk.

Although tedious, the alternative falls well within the scope of this library. That is, manually defining each resource to be controlled, which users/groups have permissions on those resources, and what permissions each users/groups has.

@thschmitt

This comment has been minimized.

Show comment Hide comment
@thschmitt

thschmitt Sep 17, 2014

I made a small change to the lib/memory-backend.js to enable regular expression based names:
in the union function add following:

for(var b in this._buckets){
      if(bucket.match("^"+b+"$")) bucket = b;
}

like this

union : function(bucket, keys, cb){    
    contract(arguments)
      .params('string', 'array', 'function')
      .end();
    for(var b in this._buckets){
      if(bucket.match("^"+b+"$")) bucket = b  
    }
    if(this._buckets[bucket]){
      var keyArrays = [];
      for(var i=0,len=keys.length;i<len;i++){
        if(this._buckets[bucket][keys[i]]){

          keyArrays.push.apply(keyArrays, this._buckets[bucket][keys[i]]);
        }
      }
      cb(undefined, _.union(keyArrays));
    }else{
      cb(undefined, []);
    }    
},

Now you can use:

acl.allow([
  {
    roles: ['user'],
    allows: [
      {
        resources: '/categories/[A-Za-z\-]+/detail',
        permissions: ['get']
      }
    ]
  }
]);

I made a small change to the lib/memory-backend.js to enable regular expression based names:
in the union function add following:

for(var b in this._buckets){
      if(bucket.match("^"+b+"$")) bucket = b;
}

like this

union : function(bucket, keys, cb){    
    contract(arguments)
      .params('string', 'array', 'function')
      .end();
    for(var b in this._buckets){
      if(bucket.match("^"+b+"$")) bucket = b  
    }
    if(this._buckets[bucket]){
      var keyArrays = [];
      for(var i=0,len=keys.length;i<len;i++){
        if(this._buckets[bucket][keys[i]]){

          keyArrays.push.apply(keyArrays, this._buckets[bucket][keys[i]]);
        }
      }
      cb(undefined, _.union(keyArrays));
    }else{
      cb(undefined, []);
    }    
},

Now you can use:

acl.allow([
  {
    roles: ['user'],
    allows: [
      {
        resources: '/categories/[A-Za-z\-]+/detail',
        permissions: ['get']
      }
    ]
  }
]);
@futurechan

This comment has been minimized.

Show comment Hide comment
@futurechan

futurechan Sep 30, 2014

@thschmitt was your solution merged?

@thschmitt was your solution merged?

@futurechan

This comment has been minimized.

Show comment Hide comment
@futurechan

futurechan Oct 1, 2014

@SeyZ if you are using memory-backend, you can use this replacement repo that supports regexes:

https://github.com/futurechan/node_acl-mem-regexp

It's basically @thschmitt 's code change.

@SeyZ if you are using memory-backend, you can use this replacement repo that supports regexes:

https://github.com/futurechan/node_acl-mem-regexp

It's basically @thschmitt 's code change.

@eightyfive

This comment has been minimized.

Show comment Hide comment
@eightyfive

eightyfive Nov 5, 2015

Contributor

There seems to be some confusion around RegExp support.

TL;DR;

  • Most catch-all RegExp resources must be declared last
  • Implement this RegExp support:
if (!this._buckets[bucket]) {
    var match;
    var re;
    Object.keys(this._buckets).some(function(b) {
      re = new RegExp("^"+b+"$");
      match = re.test(bucket);
      if (match) bucket = b;
      return match;
    });
}

Long one

It appears that the solution proposed in this thread has finally been merged:
https://github.com/OptimalBits/node_acl/blob/master/lib/memory-backend.js#L72

Related commit:
61d2d7b

However I found a few issues:

  1. Functionality is broken
  2. This is not documented
  3. Could be optimised (after documented)

1. Functionality is broken

The current implementation:

    var match;
    for(var b in this._buckets){
        var regex = new RegExp("^"+b+"$");
        if(bucket.match(regex)) bucket = b;
    }

Only works for the first RegExp test/match.

Then the bucket variable is overwritten at the first match and subsequents bucket.match tests are done against the new bucket variable value... Which is not anymore the current one we want to test all RegExp against.

UPDATE: We don't want to test against all RegExp for obvious performance reason (first match should exit the loop)

A quick solution to this, is to use the unused match variable:

//
// NOT valid example. See update above.
//
   var match;
   for(var b in this._buckets){
        var regex = new RegExp("^"+b+"$");
        if(bucket.match(regex)) match = b;
    }
    if (match) {
      bucket = match;
    }

2. This is not documented

When the above fix as been done, the documentation needs to be updated because the order in which you declare the RegExp resources matters a lot.

As per current implementation (with above fix...), the most catch-all RegExp resource needs to be added first.

An example of that is trying to implement the wildcard for resources discussed in this issue.

UPDATE: The order in which to add the RegExp ressources must be "most catch-all resource added last" for obvious performance reasons. Follows the only acceptable configuration:

    {
      "roles":["user"],
      "allows":[
        {"resources": "/users/\\d+", "permissions": "*"}
      ]
    },
    {
      "roles":["admin"],
      "allows":[
        {"resources": ".+", "permissions": "*"}
      ]
    }

Note: As per current implementation, this does not work as expected:

  • First /^\/users\/\d+$/ is matched,
  • Then bucket takes this value,
  • Then /^.+$/ is obviously matched as well.

3. Could be optimised (after documented)

Once it has been decided if to put the catch-all first or last and when this has been documented, the above piece of code can also be optimised to break on first RegExp match:

UPDATE: "Most catch-all" RegExp resources must be declared last. Here follows the optimal implementation taking this into account:

if (!this._buckets[bucket]) {
    var match;
    var re;
    Object.keys(this._buckets).some(function(b) {
      re = new RegExp("^"+b+"$");
      match = re.test(bucket);
      if (match) bucket = b;
      return match;
    });
}

This will exit as soon as a RegExp is matched against current bucket.

I'm happy to provide a pull-request if needed. Just let me know your thoughts.

Cheers.

Contributor

eightyfive commented Nov 5, 2015

There seems to be some confusion around RegExp support.

TL;DR;

  • Most catch-all RegExp resources must be declared last
  • Implement this RegExp support:
if (!this._buckets[bucket]) {
    var match;
    var re;
    Object.keys(this._buckets).some(function(b) {
      re = new RegExp("^"+b+"$");
      match = re.test(bucket);
      if (match) bucket = b;
      return match;
    });
}

Long one

It appears that the solution proposed in this thread has finally been merged:
https://github.com/OptimalBits/node_acl/blob/master/lib/memory-backend.js#L72

Related commit:
61d2d7b

However I found a few issues:

  1. Functionality is broken
  2. This is not documented
  3. Could be optimised (after documented)

1. Functionality is broken

The current implementation:

    var match;
    for(var b in this._buckets){
        var regex = new RegExp("^"+b+"$");
        if(bucket.match(regex)) bucket = b;
    }

Only works for the first RegExp test/match.

Then the bucket variable is overwritten at the first match and subsequents bucket.match tests are done against the new bucket variable value... Which is not anymore the current one we want to test all RegExp against.

UPDATE: We don't want to test against all RegExp for obvious performance reason (first match should exit the loop)

A quick solution to this, is to use the unused match variable:

//
// NOT valid example. See update above.
//
   var match;
   for(var b in this._buckets){
        var regex = new RegExp("^"+b+"$");
        if(bucket.match(regex)) match = b;
    }
    if (match) {
      bucket = match;
    }

2. This is not documented

When the above fix as been done, the documentation needs to be updated because the order in which you declare the RegExp resources matters a lot.

As per current implementation (with above fix...), the most catch-all RegExp resource needs to be added first.

An example of that is trying to implement the wildcard for resources discussed in this issue.

UPDATE: The order in which to add the RegExp ressources must be "most catch-all resource added last" for obvious performance reasons. Follows the only acceptable configuration:

    {
      "roles":["user"],
      "allows":[
        {"resources": "/users/\\d+", "permissions": "*"}
      ]
    },
    {
      "roles":["admin"],
      "allows":[
        {"resources": ".+", "permissions": "*"}
      ]
    }

Note: As per current implementation, this does not work as expected:

  • First /^\/users\/\d+$/ is matched,
  • Then bucket takes this value,
  • Then /^.+$/ is obviously matched as well.

3. Could be optimised (after documented)

Once it has been decided if to put the catch-all first or last and when this has been documented, the above piece of code can also be optimised to break on first RegExp match:

UPDATE: "Most catch-all" RegExp resources must be declared last. Here follows the optimal implementation taking this into account:

if (!this._buckets[bucket]) {
    var match;
    var re;
    Object.keys(this._buckets).some(function(b) {
      re = new RegExp("^"+b+"$");
      match = re.test(bucket);
      if (match) bucket = b;
      return match;
    });
}

This will exit as soon as a RegExp is matched against current bucket.

I'm happy to provide a pull-request if needed. Just let me know your thoughts.

Cheers.

@eightyfive

This comment has been minimized.

Show comment Hide comment
@eightyfive

eightyfive Nov 6, 2015

Contributor

After giving it some thoughts I updated/corrected my previous comment with additional information.

Contributor

eightyfive commented Nov 6, 2015

After giving it some thoughts I updated/corrected my previous comment with additional information.

@eightyfive

This comment has been minimized.

Show comment Hide comment
@eightyfive

eightyfive Nov 6, 2015

Contributor

Pull request with fix created:
#154

Contributor

eightyfive commented Nov 6, 2015

Pull request with fix created:
#154

@devourment77

This comment has been minimized.

Show comment Hide comment
@devourment77

devourment77 Jan 2, 2016

Is this feature going to be in an upcoming version?

Is this feature going to be in an upcoming version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment