Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix the middleware first param [numPathComponents] #159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

caosbad
Copy link

@caosbad caosbad commented Nov 27, 2015

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
Copy link
Contributor

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
Copy link
Author

caosbad commented Nov 28, 2015

@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
Copy link
Contributor

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
Copy link
Author

caosbad commented Nov 28, 2015

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
Copy link
Contributor

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
Copy link
Author

caosbad commented Nov 29, 2015

You are right,Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants