ForbesLindesay / connect-roles

Provides dynamic roles based authorisation for node.js connect and express servers.
749 stars 62 forks source link

OR+Array Support #51

Closed amarkumargarg closed 7 years ago

amarkumargarg commented 8 years ago

Following supports have been extended.

Support for array of action strings (logical AND)*: app.get('/', user.can( [ 'access home page', 'access private page' ] ), function (req, res) { res.render('private'); }); Logical representation of [ 'access home page', 'access private page' ]: [ 'access home page' AND 'access private page' ]

Logical OR support*: app.get('/', roles.or( [ 'Sys_User', ['Fb_User', 'Group_Member'], 'Gmail_User', ['Yahoo_User', 'Group_Member'] ), function (req, res) { res.render('private'); }); Logical representation of [ 'Sys_User', ['Fb_User', 'Group_Member'], 'Gmail_User', ['Yahoo_User', 'Group_Member']: 'Sys_User' OR ['Fb_User' AND 'Group_Member'] OR 'Gmail_User' OR ['Yahoo_User' AND 'Group_Member']

*Currently no support for async.

markstos commented 7 years ago

The idea looks useful, but the commit is harder to review than it needs to be because a lot of whitespace and formatting changes are mixed in with the logical changes. Could an updated PR be submitted that either excludes all the unrelated whitespace & formatting changes or put them into a separate commit?

ForbesLindesay commented 7 years ago

Yes, this breaks all the formatting, which makes it very hard to understand. You can write:

user.can( [ 'access home page', 'access private page' ] )

as

user.can('access home page'), user.can('access private page')

because of express middleware chaining, so I don't think we need to explicitly support it.

I think the user.or notation breaks the fluid nature of the API. It no longer reads like a sentence. I also don't really think it should be necessary. The idea behind the library is generally to use actions: user.can('delete file') rather than directly using roles: user.is('admin') OR user.is('moderator').

ForbesLindesay commented 7 years ago

Closing as it looks unlikely I'll find time to clean up and merge this.