Override default request object name

[porkchop]

Hi ForbesLindesay :) I thought this might be a useful feature for others as well - the ability to choose the name of the role user object attached to the request object. Passport has a similar function, which I find useful. If you are not crazy about this change, no problem, or if you would like me to make some adjustments before acceptance, please let me know. I am happy to do so. Thank you kindly for the great lib. -porkchop

[ForbesLindesay]

Passport appears to support assignProperty (e.g. authorize method) but I can't see anywhere that lets you change the behavior of authenticate? Can you point me to the docs or code where passport does this? I'm happy to support something like this if passport does, but I don't know if I want to support this otherwise.

[porkchop]

Sure thing - check https://github.com/jaredhanson/passport/blob/master/lib/passport/index.js, line 130. An options parameter like so is supported: passport.initialize({userProperty: 'newUserPropName'})

[talamaska]

+1 for this, my problem is related to that https://github.com/ForbesLindesay/connect-roles/issues/13

[ForbesLindesay]

Cheers. I like the idea of this option, but I don't like the implementation very much. What I'd like to propose is this:

We are making a breaking change already with https://github.com/ForbesLindesay/connect-roles/commit/50b311c4e1f977c6e9569988080643e9a2c0b863 so the next release will be version 3.0.0. Given this, it's OK for this update to be a small breaking change.

I think I'd like to change the API slightly. Rather than exporting the middleware function directly as we do at the moment, lets export an object with an initialize function.

That initialize function should look like:

exports.initialize = function (options) {
  return middleware
}

That way it can accept an option of userProperty just like passport does.

[porkchop]

Cool. That would succinctify the initialization mechanism. Should this initialize function also be applied to the default user and failure handler as well then?

[ForbesLindesay]

Yes, lets add options for defaultUser and failureHandler, good point.

[porkchop]

Ok and presumably we would also remove the setFailureHandler and setDefaultUser exports too then? Or perhaps that is too breaking a change at the moment?

[talamaska]

We should keep them, to provide 2 ways of defining the failure.

On 09.07.2013, at 21:10, Aaron Caswell notifications@github.com wrote:

Ok and presumably we would also remove the setFailureHandler and setDefaultUser exports too then?

— Reply to this email directly or view it on GitHub.

[ForbesLindesay]

I'd be inclined to remove them. Why do you suggest we keep them @talamaska?

[porkchop]

Hi everyone. Please let me know if this is in line with what you were thinking. :) I can add a test in to prove initialization if you think it needs it

[talamaska]

@ForbesLindesay Because, I would like to have the old way of setting the failureHandler in my module, there I was also declare the use cases. that ways the server.js is kept clean, and I have a separate file for roles configurations only. Normally it should replace the failureHandler of initialize function. example of the module config,and usage in routing as middleware.

//in server.js
roles = require('connect-roles')
require('./config/middlewares/authorization')(roles);
//express config
require('./config/express')(app, config, passport, roles, hbs);
// Bootstrap routes
require('./config/routes')(app, passport, roles, config);

//in exapress.js
//along with the other stuff
    app.use(passport.initialize());
    app.use(passport.session());
    app.use(roles);
   app.use(roles.initialize({
       failureHandler : function(req, res, next, action){
           if(req.isAuthenticated)
              res.redirect('403.html');
           else
              next();
       }
   }));

//in authorization.js
module.exports = function (roles) {
roles.use('list user', function(req){
    if(req.user.level >= 2){
      return true;
    }
  });
//optionally controll the access denied page displayed
  var main = require('../../app/controllers/main');
  roles.setFailureHandler(function (req, res, action){

    console.log(req.isAuthenticated());

    var accept = req.headers.accept || '';

    if(action == 'isAuthenticated')
      main.notlogged(req, res); //does some redirect or return json if i'm making rest api
    else
      main.rights(req, res); //does some redirect or return json if i'm making rest api
  });
}

oh and didn't you removed 'should' library one of your previous commits?

[talamaska]

@porkchop it seems that oldUser is always falsy

var oldUser = req[userProperty];
  obj[userProperty] = req[userProperty] || Object.create(defaultUser);
  if(oldUser){
    obj[userProperty].isAuthenticated = true;
  }else{
    obj[userProperty].isAuthenticated = false;
  }

[porkchop]

Thanks @talamaska. Just looking into this... One thing I notice in your config though is that the 'app.use(roles)' line should probably be omitted in the new regime, since the middleware is returned only by the initialize function. Ie: //in exapress.js //along with the other stuff app.use(passport.initialize()); app.use(passport.session()); // app.use(roles); <-- Right here - this is not needed with the initialize fcn below app.use(roles.initialize({ failureHandler : function(req, res, next, action){ if(req.isAuthenticated) res.redirect('403.html'); else next(); } }));

(Note that I commented out the suspect line in my sample above)

[porkchop]

Hmm @talamaska I don't seem to be having that issue when the incoming user has been authenticated. Are you sure your user is authenticated?

[talamaska]

absolutely, that's what passport is saying. I didn't yet had a chance to test, with the omitted app.use('role')

[porkchop]

that's so weird because I would expect passport to inject req[userProperty] by the time roles receives it

[porkchop]

Oh is userProperty equal for both passport and roles? Like did you override the default value for one or both? If so then definitely this will always be falsey. That is to say, if they are not equal, oldUser will be falsey always

[talamaska]

i have replaced only for connect-roles to check for another namespace, so they have to be the same?

[porkchop]

IMO they don't have to be, but I am pretty sure Forbes' intention is to make the two objects line up/use the same namespace. Why I think it's kind of ok not to is that it depends on if you need the isAuthenticated method provided by roles on the user object, or do you need the isAuthenticated method provided by passport on the request object. Respectfully, the way I see it, as it stands now, they are incompatible when they share the same name space. If they share the same name space, then req.isAuthenticated() always returns true, even if a user is not authenticated. This is because the passport isAuthenticated method basically just returns the req[userProperty] object (and roles injects a req[userProperty] object in this attachHelpers function, whether or not the user is auth'ed). However, the roles replacement for that will be correct (req[userProperty].isAuthenticated), because it sets itself up correctly in this attachHelpers function. Now, by similar-ish reasoning if you don't share namespaces, passports isAuth'ed() will behave correctly; however, roles' version will not. So (to me) it boils down to do you want to use passport's isAuthenticated function reliably (do not share namespaces) or let roles take over that function (share namespaces). Sorry so wordy, but this sort of explains one of my motivations for wanting to add this feature, aside from wanting to reserve the 'user' name for other purposes on the request object :P

[talamaska]

think I got it. I'll make some tests tomorrow. Probably I will have to end up not relying on the passport req.isAthenticated().

[porkchop]

Cool. Let me know how it goes :)

[talamaska]

ok, I rewrote some of the roles rules, seems ok, but I have returned the exports for failure redirect As I explained above, that way I can keep all the configurations in a separate file. do you want me to make pull request? but I still can't write tests.

[porkchop]

Hmm that is fine by me, but it is really up to @ForbesLindesay. What is new in your changes? About the tests, are you saying you are not comfortable writing them or that you maybe don't know how to fire them? I could be totally redundant by saying this, but you should probably make sure your pull request passes the tests, so (in case you don't know how to launch them), do cd your-connect-roles-branch npm install mocha -g mocha Maybe you already knew that. Not sure :P

[talamaska]

I have returned the exports for failure redirect. that's all in my changes

Actually I know how to fire the test, I don't know how to write them. It's not that I'm not comfortable, it's just I can't stop now, forget about everything else and start learning mocha.

https://github.com/ForbesLindesay/connect-roles/issues/16 As my comments show, I'm not sure I fully understand how it is working. Tbh, I have especially cloned his commit with additional tests, https://github.com/ForbesLindesay/connect-roles/blob/master/test/index.js#L118+L146

I see everything there like those exports we were talking about, it just misses the initialize, which I don't use, cause I don't change anything. I posted already my use case, and I still don't get it. Is it me or the docs are not clear, it's says only to return true, if no true is returned will forward to failure. It doesn't do that

So I'm not sure I'm able to contribute to this task, since I'm not fully capable of understanding everything. I was just trying to use an already made module, tried to report some of the issues I have, but it seems that my expectations from the module are very different from what it is. So I already wrote very simplified routing module relying on passport req object, which exactly what I want, with no additional fancies and no app.use(middleware).

[porkchop]

Fair enough Talamaska. I am super familiar with the learn vs produce trade-off we have to make very often. All the best with your project.

[talamaska]

btw do you know good tutorial for writing routing middlewares. My solution is used just like an external module library, using for example https://github.com/madhums/node-express-mongoose-demo/blob/master/config/middlewares/authorization.js

[porkchop]

Sorry I missed your comment @talamaska - did you figure out a good solution? Fwiw, your solution seems reasonable to me. I'm not sure of a great tutorial honestly, but I would recommend looking at anything ever written by visionmedia, particularly his express examples for this kind of thing.

[talamaska]

Well, I kinda liked the connect-roles middleware. I made some tweaks to make it work as I expect. I didn't make pull request. But i have it in separate branch. after the explanations given, I finally understood how to deal with it.

https://github.com/talamaska/connect-roles/tree/stop-on-first-failed-rule

My other tool for now stays as backup possibility, if things don't go well with connect-roles. I am soloing my current project so I don't have to argue with colleagues.

[mrlucmorin]

I would like to bring my two cents to this discussion.

I also have been looking at the issue revolving around using Passport with Connect-roles, and the fact that as it is now, Connect-roles is creating a "dummy" user in the attachHelpers function (index.js line 122+). As others have found out, this dummy user fools Passport into thinking that it is authenticated.

Connect-roles should never be concerned with authentication, as it is an authorization library. I think we should always delegate the authentication to a 3rd party, and simply check to see if req.user is defined before doing any testing.

As long as we're not authenticated, and so req.user is undefined, it serves no purpose to try to authorize.

Regards

[porkchop]

Good point @mrlucmorin and I agree with the idea of authorization tasks only being being connect-roles' job. I've read @ForbesLindesay's reasoning on this somewhere before, and as I understand it, conceptually there is always a user, whether or not they have been authenticated. Like a visitor is still a user, but without an account yet, or maybe they have a mega-low privilege account generated. So from that point of view, it kinda makes sense as well to have the user object there, and I guess the reasoning is that your app should recognize that it's a not yet authed/low privilege user via connect-roles. You are right though, I originally offered this feature partly with the idea of being able to get connect-roles 'out of the way' of passport. Another reason is that almost every important module for passport has a defaultUser override capability - oauth2orize for example, so it seemed a natural fit to this lib as well given that it seems to strive to go along side passport.

[mrlucmorin]

@porkchop I'm not too sure that even the "guest" user scenario should warrant the authz library to create a user.

I think if the web site/app designer wants to have a guest user, he should still use the auth library to define req.user, and then the authz library will know which resources to allow to this user.

My understanding is that the middleware chaining should call auth before authz, so if authz creates a user, it messes with the proper ordering of concerns. What do you think ?

Auth and authz are really orthogonal concerns, and should not be mixed IMHO. Or else we will need some higher level of abstraction built into Connect/Express that defines the proper way to communicate between the two layers.

Even is we override the "user" property name, what I see in Connect-roles is an additional req.user.isAutehnticated property that doesn't belong there, so there is a real potential for errors when working with different developpers. Some might decide to use Passport's req.isAuthenticated(), while other would use req.user.isAuthenticated, and as I've seen, both sometimes return contradicting values.

I suggest Connect-roles should use Passport's req.isAuthenticated(), and if not defined then fallback to its own internal variable.

Now to bring all the different auth/authz libraries' authors to speak the same language...

[ForbesLindesay]

The next version will not set user if there is not already one defined. It will call req.isAuthenticated if it is already defined by another library (e.g. passport). It also lets you override the user property via userProperty. Thanks to everyone for their input. I apologise for taking so long to get round to this.

[porkchop]

Thanks @ForbesLindesay and apologies @mrlucmorin - I missed responding to your thoughts above. FWIW I have to agree with your assessment of concerns regarding authentication vs authorization.

[mrlucmorin]

Heh, no worries, I've been so absorbed in other work that I've completely put this project aside. I can't wait to get back to it though :-)