Fixed #86. Carry parameters set in scope checking further along

[coolaj86]

Fix #86 by allowing results of scope checking to pass all the way through to the grant code callback by default.

Also, the callback may inspect the decision request body if needed.

[jaredhanson]

What if at, L165 you inserted:

txn.info = ares;

(The ares argument might be better named info in this case)

Essentially, this means that if it is not immediate, there is no response, but here's some extra info to carry through the transaction. As I mentioned, I think its confusing to say there's a "response" when the user has not actually made one. There should be some clear distinction between the data being supplied in the immediate vs. non-immediate case.

Would that work?

[coolaj86]

That sounds great.

[coolaj86]

So it turns out that that naming it info causes some consistency issues:

1. When the immediate response is true the extra data is referred to as res, but on false it's referred to as info. Remember, if we're using scope, we're going to operate on it and the original array of strings will no longer used because we now have a parsed object. So in both success and failure cases I want access to the parsed scope object. This is not so bad, we can also assign it to info in the true case and we have consistency. Yay.

2. But now when we get to the grant phase https://github.com/jaredhanson/oauth2orize/blob/master/lib/grant/code.js#L156 we're passing back only res, which means that we have to add another overload that also passes info (argument creep, much?). But now both of these are success cases. One of them is the success case when the user needed extra permissions and granted them and the other is the success case when the user had previously granted those permissions.

See the dilemma?

I personally think that it's very reasonable to pass around an incomplete response object as the response is being built. That's the reason that you have allow, isn't it? To distinguish between whether or not the response has been allowed? After all, node doesn't wait to hand you the http res until you're reading to res.end. As it goes through the middlewares you add to it and modify it. I don't see why an oauth response need to be different.

Let me know your thoughts and I'll move forward as you wish. I'll also go back and see if I read anything wrong and there's another way...

P.S. I didn't know what res and ares meant until we started having this discussion. I wasn't sure if it was result or something.

[coolaj86]

Actually, as far as I can tell txn.res is never set, aside from that change that I just made, so https://github.com/jaredhanson/oauth2orize/blob/master/lib/grant/code.js#L156 was effectually broken.

And I'm trying to work around the variable being named info. I'll push the changes on a separate branch so you can see, but it definitely fields a little weird - or at least it did until I realized that txn.res is never set. Now it may clean up easier since only txn.info will be passed.

UPDATED: that was stupid of me to say. I can't find anywhere where it is set, but it's obviously set because in the other issue I was console.log()ing it and said that it returns { allow: true }.

[jaredhanson]

This all sounds way too complicated, and I'll need to reread this thread again to understand completely

I do want to make one point though: it is not safe to use the requested scope when issuing a response, unless it is equal to or lesser than a previously granted authorization

So, you shouldn't be using the scope requested by the application when processing the user decision. There's a prior issue where this was discussed that I'll dig up

Sent from my iPhone

On Mar 4, 2014, at 6:09 PM, AJ ONeal [email protected] wrote:

Actually, as far as I can tell txn.res is never set, aside from that change that I just made, so https://github.com/jaredhanson/oauth2orize/blob/master/lib/grant/code.js#L156 was effectually broken.

And I'm trying to work around the variable being named info. I'll push the changes on a separate branch so you can see, but it definitely fields a little weird - or at least it did until I realized that txn.res is never set. Now it may clean up easier since only txn.info will be passed.

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

[coolaj86]

Yeah, I'm just getting confused trying to trace down where I would make info available, largely because I can't find where txn.res is ever set, which makes me think that somewhere req.oauth2 is passed to a function which then accepts it as txn, having nothing to do with the req.session[key][tid], which is how txn is referenced in other places.

I know it's not safe to use the requested scope, which is why I'm parsing the requested scope, compare it to the previously granted scopes, and then pass that object around.

For right now I'm just denying the request if the user doesn't accept the full range of scope that the application requested, but eventually I'd like to let the application know which scopes were accepted and which weren't.

[coolaj86]

I dug around a bit, figure the call stack out a little better, and made just a few simple changes that I think accomplish everything we both want.

[coveralls]

Coverage remained the same when pulling 7404912 on coolaj86:more-scope into 1a9c8ee on jaredhanson:master.

[coolaj86]

Here's the diff
coolaj86@5c7b1ca

[jaredhanson]

This seems unnecessary. Setting it at txn.info below should be sufficient. Am I missing something?

[coolaj86]

It's accessed immediately in the next middleware before deserialization has happened again.

[jaredhanson]

How's that? Any next middleware should not be invoked if the response is immediate.

[coolaj86]

Yes, the next connect / express middleware is invoked because when the allow is false then the user is directed to the dialog. There I need to be able to access req.oauth2.info so that I can show the user which scopes were requested so that they can choose to accept them or not.

Trust me, I've tried it both ways.

[jaredhanson]

Yes, I get that, and that's what setting info on the transaction accomplishes at L167, which will be available in the next middleware at req.oauth2.info. My issue is with L140, which seems unnecessary. There is no need to set info when allow is true, because the response is sent immediately and no further middleware gets invoked.

[coolaj86]

Setting it at L167 doesn't make it available to the next middleware because req.oauth2 !== txn when the response is deserialized certain keys of txn are copied to req.oauth2 and when it's serialized certain keys of req.oauth2 is copied to txn.

I don't think there's any harm in moving the line down into the allow !== true condition, but it has to exist.

[jaredhanson]

Gotcha on the serialization bit. Just move this line to L158 then. There may be no harm in setting it in both cases, but I also can't think of a good reason for doing it, so therefore it shouldn't be.

[jaredhanson]

@coolaj86 I merged this (along with some modifications) - the diff can be seen in #110.

The only breaking thing I did (relative to your patch) was remove the req.oauth2.info assumption in the decision middleware. If your app expects that, you'll have to pass it from the parse callback:

server.decision(function(req, done) {
    return done(null, req.oauth2.info)
});

I also added support for passing data to the next callback that is not serialized into the transaction. Comments in the diff should explain the intent clearly. Take a look at this and let me know what you think. If all looks good to you, I'll get this up on npm shortly.