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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
Feb 5, 2015
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0885425
pass result of scope checking to dialog middleware
Mar 4, 2014
aa1edaa
carry over ares from scope check and also dialog form body
Mar 4, 2014
d868f64
renamed res -> info
Mar 5, 2014
7404912
fixes for renaming res -> info
Mar 5, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/middleware/authorization.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,12 @@ module.exports = function(server, options, validate, immediate) {
req.oauth2.req = areq;
req.oauth2.user = req[userProperty];

function immediated(err, allow, ares) {
function immediated(err, allow, info) {
req.oauth2.info = info;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (err) { return next(err); }
if (allow) {
req.oauth2.res = ares || {};
req.oauth2.res = info || {};
req.oauth2.res.allow = true;

server._respond(req.oauth2, res, function(err) {
Expand All @@ -162,6 +164,7 @@ module.exports = function(server, options, validate, immediate) {
txn.client = obj;
txn.redirectURI = redirectURI;
txn.req = areq;
txn.info = info;
// store transaction in session
var txns = req.session[key] = req.session[key] || {};
txns[tid] = txn;
Expand Down
2 changes: 1 addition & 1 deletion lib/middleware/decision.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ module.exports = function(server, options, parse) {

var tid = req.oauth2.transactionID;
req.oauth2.user = req[userProperty];
req.oauth2.res = ares || {};
req.oauth2.res = ares || req.oauth2.info || {};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this. If you want the authorization response to be equivalent to info from the authorization request, that should be explicitly supplied by the application from the parse callback. For security reasons, oauth2orize shouldn't ever assume the application's original request was granted by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm still unclear as to what 'ares' actually is and very confused as to the purpose of 'allow' if an 'ares' without 'allow' still counts as a validated response.

But as I said before I don't care about implementation details, I just want a -> b, so I'll be happy to remove this line and make the info about what scope was accepted available in the parse function.

Anything else I should change before we call it good?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the decision middleware is reached, it means an immediate response was not allowed. In this case, the "authorization response" should be determined solely based on what the user chooses to allow. It should not be built from anything in the original "authorization request" sent by the app.

In effect, with your modifications, the results supplied by the immediate callback are interpreted based on the allow flag. If true, it is the response to be immediately sent. If false, it is some extra info the application wants to carry through the transaction. In the latter case, this info should be entirely opaque to oauth2orize. It has no meaning other than what the application chooses to associate with it, which is why the parse function should be used to parse any data from within it.

If the two noted lines can be changed, I'm happy and will get this merged. Thanks for your efforts, and the discussion. OAuth is, unfortunately, quite hard to grasp and has a lot of stuff the application is ultimately responsible for. I'm just trying to keep this module as un-assuming as possible, and keep those divisions clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this line, but if a programmer is parsing the scopes and determining what to ask the user, then when the user responds 'allow', they'll have no way to know what the user was allowing when the application gets to the grant step, which means that anyone who handles scope will need to implement a parse function that passes info as ares so that they can get it in grant.

Unless they're supposed to save the allowed scope during the parse phase, which I suppose works, grant sounds like a correct place to save granted scopes to a database whereas parse does not sound like the correct place...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow entirely, but you're right on the larger perspective: it is up to the application to present a dialog and, upon the user submitting the form, the application needs to parse out what was approved based on however they do form parameters. All of this should be delegated to the application, and handled by the parse function. If stuff is in info, that's totally cool, so long as OAuth2orize makes no such assumptions. The assumption that info contains the response is fine in your case, but not generically. So, you're app should supply that in the parse function (and other people's apps can use info how they see fit)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If somebody sets something in the fail case, it's because they need it in the decision and or grant case. Generically, it would seem a much more common case to want the data through the whole process (since it's likely in the format you already need to continue your checks) than to disregard it by default and make it a manual step to have it again.

What is it that you're using ares for in your code if you're not keeping track of permissions and scope through the decision process? I'd understand your point a lot better if I knew of any other reason a person would use it than to keep state.

Again, I'm not waiting on us to have a mutual understanding on this to resubmit the pull request (although I am for the other point), but I do want to understand the process.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is it that you're using ares for in your code if you're not keeping track of permissions and scope through the decision process?

Probably nothing. ares is entirely based on how the app chooses to present dialogs to the user (this is left completely undefined by OAuth). Tracing it through, its just routed out to the grant issue callbacks so they can handle it how they need it. Likewise, this info construct being introduced is just an opaque place to for the application to store arbitrary stuff in the transaction, probably as an optimization when later when figuring out what the response should be.

That info can be exactly what you want for ares, and that may be the best thing to use it for. But, that implies an assumption by this module on what application-specific opaque data contains. I don't like making assumptions inside of modules, and the point of the parse function is to delegate to the application. So I'd just prefer that to remain assumption-less as before.


if (req.oauth2.res.allow === undefined) {
if (!req.body[cancelField]) { req.oauth2.res.allow = true; }
Expand Down
1 change: 1 addition & 0 deletions lib/middleware/transactionLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ module.exports = function(server, options) {
req.oauth2.client = client;
req.oauth2.redirectURI = txn.redirectURI;
req.oauth2.req = txn.req;
req.oauth2.info = txn.info;
next();
});
};
Expand Down