How to pass data to allow/deny dialog?

[coolaj86]

I'm looking at this function here:
https://github.com/jaredhanson/oauth2orize/blob/master/lib/middleware/authorization.js#L139

And you only assign oauth2.req in when allow is true. Shouldn't it be assigned in both cases?

In my use case I check to see if the scope has new permissions or not.

If the scope does not required new permissions, I pass allow as true.

If the scope does require permissions that are being requested for the first time, I set allow as false and pass in an object containing information necessary for nexted function, which is the one that renders the allow / deny permission dialog.

I'd like to patch it so that req.oauth2.res is set even in the false condition, or perhaps another variable name would be better?

I'd be happy to submit a patch, I just need to know your thoughts on how this should be handled.

[jaredhanson]

Is this covered by #82 (see discussion in #81)? I plan on merging that patch.

[coolaj86]

No, it's a few lines down: res, not areq.

https://github.com/jaredhanson/oauth2orize/blob/master/lib/middleware/authorization.js#L142

needs to happen somewhere around here too:

https://github.com/jaredhanson/oauth2orize/blob/master/lib/middleware/authorization.js#L160

[jaredhanson]

The next function, that renders the dialog has access to the whole req and req.oauth2 structure. What is missing there that is relevant?

[jaredhanson]

Basically, you're comment:

pass in an object containing information necessary for nexted function

Why are you passing in something for the nexted function? If you pass true (for immediate mode), you're supplying the response. If you pass false, you shouldn't supply further options, and the next function should pick out whatever it needs from the request (which should have everything that is passed as arguments to the immediate mode check). I think.

[coolaj86]

I didn't explain clearly. Here's a snippet:

exports.authorization = [
  login.ensureLoggedIn('/login.html')
, server.authorization(
    function checkAppIdAndUrl(appId, redirectURI, scope, type, done) {
        // for brevity, I cut out the code that actually checks
        done(null, consumer, redirectURI);
    }
  , function checkAuthorizationAndScope(consumer, user, scope, done) {
      utils.permissions.delta(user.id, consumer.appId, { scope: scope }, function (err, newScope, invalids, reqScope) {
        var ares = {
              newScope: newScope
            , invalids: invalids
            , rawScope: scope
            , requestedScope: reqScope
            }
          ;

        // just a nasty hack until this issue is solved
        // https://github.com/jaredhanson/oauth2orize/issues/86
        scope.ares = ares;

        // this third argument should be for options that get passed to the dialog
        // for example, showing new scope to allow
        if (0 === Object.keys(newScope).length) {
          // nothing new to ask for
          done(null, true, ares);
        } else {
          // there are new things to ask for
          done(null, false, ares);
        }
      });
    }
  )
, function askUserToAllowScope(req, res /*, next*/) {
    console.log('[render dialog]');
    var client = req.oauth2.client
      , authReq = req.oauth2.req
      , scope = authReq.scope
      ;

    client.logo = client.logo || '/images/app.png';
    res.render('dialog', {
      transactionId: req.oauth2.transactionID
    , user: {
        name: req.user.name
      , photo: req.user.photo
      }
    , scope: scope
    , permissions: utils.permissions.stringify(scope.ares)
    , client: req.oauth2.client
    , url: req.url
    , body: req.body
    });
  }
];

Since I'm already doing all of the work to parse the scope and lookup the permissions, I don't want to do it again.

[jaredhanson]

Ah, OK, gotcha. Yes, I'd be fine with a patch that lets you pass your results on to the next middleware.

[coolaj86]

In testing my little 2-line change I ran into another problem that I'll want to fix in the same patch.
(my change: coolaj86@0885425)

After the user accepts the permissions dialog the next interaction is the grant code. This seems to be the logical place to save the scope that the user accepted would be in this function, however, ares here seems to be disconnected from the ares I added in the change. Can you tell me where to look for the connection?

server.grant(oauth2orize.grant.code(function (consumer, redirectURI, user, ares, done) {
  console.log('[grant code]', consumer.appId, user.id);
  console.log(consumer);
  var code = consumer.appId + ':' + utils.uid(16)
    , values = {
        userId: user.id
      , scope: ares.scope
      , appId: consumer.appId
      , redirectURI: redirectURI
      , ts: Date.now()
      }
    ;

  console.log('[grant] values', consumer.appId, user.id);
  console.log(values);
  console.log('ares.keys()');
  console.log(Object.keys(ares)); // only ['allow']
  console.log('ares');  // { allow: true }
  console.log(ares);

  utils.permissions.set(user.id, consumer.appId, { scope: ares.scope });
  db.authorizationCodes.save(code, values, function (err) {
    if (err) { return done(err); }
    done(null, code);
  });
}));

Eventually I may also need access to the form body of the permission form response, as I may add checkboxes for optional permissions.

[coolaj86]

I think I found it here:

https://github.com/jaredhanson/oauth2orize/blob/master/lib/middleware/decision.js#L74

parse = parse || function(req, done) { return done(null, req.oauth2.res); };

testing...

[coolaj86]

nope... that wasn't it.

[jaredhanson]

Yes, ares is the "authorization response" and should only be filled out when there is a definitive response (either immediately or after the user has made a decision). In the dialog case, there's no response until the user has made one, so that structure is only filled out at that time.

I need to digest this a bit further, but I'd be disinclined against anything that attempted to carry a "response" through the transaction before it has ended.

[jaredhanson]

If you need to attach something during the "fail, not immediate" case, I would attach it to oauth2.req.extra or something to that effect. That data would carry through the transaction in the session.

[coolaj86]

I don't care what it's named as long as I can access it.

Would you mind doing the honor if naming things as you see fit and I'll just pull from you until it lands in npm?

[jaredhanson]

I will, but you've got the immediate use case. If you want the patched merged soonish, I'm going to depend on you to get it to release quality. Otherwise it'll sit in my queue until I have time, which is not likely going to be soon.

[coolaj86]

Alright then, I'll rename it to info and republish once I get back home.

:-)

[coolaj86]

The discussion has been had. The pull request is in. Closing this since it's linked to in the Pull Request.

[aferlim]

You need to put it in decision flow.
After authorization, that defines decision flow.. before in decision UI:

...

<button class="btn btn-danger" type="submit" name="cancel" value="Deny">Deny
Allow
...

Then in Post:

app.route('/oauth/authorize')
.get(isLoggedIn, authorization)
.post(isLoggedIn, require('oauth-config').decision)

oauth-config:

...

module.exports = {
decision: [
server.decision({ cancelField: 'cancel' }, (req, done) => (
done(null, { scope: req.body.scope })
))
]}