OptimalBits / node_acl

Access control lists for node applications
2.62k stars 371 forks source link

fix the middleware first param [numPathComponents] #159

Open caosbad opened 8 years ago

caosbad commented 8 years ago

Hello,Im very appreciate you write such a wonderful moduel. I found some wrong when I use the middleware. Here is my code

// $a is the ACL moduel and $api.list is my own router function. 
router.get('/v1/courses',$a.middleware(2), $.api.list);

I need the ACL to split the resource named courses and I try to use acl.middleware(2)' to split the url, but when I debug the code . I could not get the right result ,the middleware function use that code to handle the url in line 630 ofacl/lib/acl.js`

      resource = url.split('/').slice(0,numPathComponents+1).join('/');

It return the resource is still '/v1/courses' in the code above.

I check the readme doc found this:

app.put('/blogs/:id/comments/:commentId', acl.middleware(3), function(req, res, next){…}

So , I guess the right code might be this:

      resource = url.split('/').slice(numPathComponents,numPathComponents+1).join('/');

Then I can get courses resource that I expected.

Hope it help,have a good day!

jonmassot commented 8 years ago

I don't think you're supposed to change the resource name, who's supposed to be a path. If you do it like you did, you'll come across some trouble later, like having resources with the same name in different path. And the change you're suggesting will probably break everyone's implementation of node-acl.

caosbad commented 8 years ago

@jonmassot Oh, I don't want to break everyone's implementation of node-acl ,maybe I haven't understood the doc or code yet ,I will run some test in my project and find a appropriate way to use node-acl,thanks for your reply.

jonmassot commented 8 years ago

Can you provide a bit more details on what you're trying to accomplish? That way, I might be able to help you. Cause I'm not too sure on why you want to protect only 1 part of the resource. The only I can guess is that you might have /v1/courses and /v2/courses that should have the same roles/auth, without having to duplicate entries in the datastore. I could be wrong...

caosbad commented 8 years ago

Yeah, I thought I could manage the path and source together, so that fewer code to config the auth.

If I write this config code


acl.allow(
    [
        {
            roles: ['student'],
            allows: [
                { resources: 'cards', permissions: ['get', 'put', 'post'] },
                { resources: ['courses', 'packs'], permissions: ['get'] }
            ]
        }
    ]
);

When I use middleware to split the path contain course in my REST api ,I want that url to be regard as courses resource.

jonmassot commented 8 years ago

IMO, wishing for less code regarding authentication is just another way to wish for trouble later on.

For example, Can every students get on all courses? I suppose that courses would be the list of courses. Who can post or put on courses, etc. If you want to be more flexible, you should be more granular, but that implies more code, at least for the authorization. Keep in mind that it's just my opinion.

caosbad commented 8 years ago

You are right,Thank you very much!