Per-request scoped scope #118

Open
coolaj86 opened this Issue Mar 15, 2013 · 5 comments

Comments

Projects
None yet
2 participants
@coolaj86

Maybe I'm just not understanding something, but it seems that you can only define the scope-to-be-used inside the strategy as an all-or-nothing deal when the strategy is created.

I'd like to be able to have my app request scopes by degrees - just getting the scope that's needed exactly when it's needed - like facebook encourages.

I'm trying to think of a way to add this feature without breaking anything. Maybe adding an options hash after the callback?

that.authenticate(req, res, cb, reqOpts);

req.authenticate(['facebook'], cb, { "scope": ["email", "birthday"] });

What would you suggest?

@coolaj86

This comment has been minimized.

Show comment Hide comment
@coolaj86

coolaj86 Mar 15, 2013

It seems like the best thing to do would probably be to change strategies into a hash or an array of hashes

req.authenticate([{ "name": "facebook", "scope": ["email", "birthday"] }], cb);

This way all of the internals could be changed to turn the string into a hash unobtrusively. It would be a new opt-in feature from the existing api user's perspective.

Would you accept such a change?

It seems like the best thing to do would probably be to change strategies into a hash or an array of hashes

req.authenticate([{ "name": "facebook", "scope": ["email", "birthday"] }], cb);

This way all of the internals could be changed to turn the string into a hash unobtrusively. It would be a new opt-in feature from the existing api user's perspective.

Would you accept such a change?

@ciaranj

This comment has been minimized.

Show comment Hide comment
@ciaranj

ciaranj Mar 15, 2013

Owner

Hmm, yes I see what you're saying, currently to pass per-request authentication settings you would need to set something on the request (or response) so that it could be used in facebook.authenticate etc.

I think that we could achieve this with minimal changes, using the existing APIs;

  1. requestMethods#authenticate already takes an optional hash or 'options' as the second argument the signature is mixed-in to become req.authenticate(strategy, options, cb) or req.authenticate(strategy, cb) (in auth_middleware.js). Currently the only thing on that opts that does 'anything' is 'scope' the idea being a user can be authenticated as a 'guest' user or 'admin' user or 'someone else'
  2. Currently these options are discarded within requestMethods#authenticate, we could pass them through to the authContext
  3. We could then in authExecutionScope.ctr() add these options to itself and provide a nice tidy method to expose them (the methods of these instances are available on 'this' within facebook#authenticate
  4. Then in facebook#authenticate could just check these options as well as my.scope to allow overriding of the requested scope?

Thoughts?

Owner

ciaranj commented Mar 15, 2013

Hmm, yes I see what you're saying, currently to pass per-request authentication settings you would need to set something on the request (or response) so that it could be used in facebook.authenticate etc.

I think that we could achieve this with minimal changes, using the existing APIs;

  1. requestMethods#authenticate already takes an optional hash or 'options' as the second argument the signature is mixed-in to become req.authenticate(strategy, options, cb) or req.authenticate(strategy, cb) (in auth_middleware.js). Currently the only thing on that opts that does 'anything' is 'scope' the idea being a user can be authenticated as a 'guest' user or 'admin' user or 'someone else'
  2. Currently these options are discarded within requestMethods#authenticate, we could pass them through to the authContext
  3. We could then in authExecutionScope.ctr() add these options to itself and provide a nice tidy method to expose them (the methods of these instances are available on 'this' within facebook#authenticate
  4. Then in facebook#authenticate could just check these options as well as my.scope to allow overriding of the requested scope?

Thoughts?

@coolaj86

This comment has been minimized.

Show comment Hide comment
@coolaj86

coolaj86 Mar 15, 2013

To help me understand a little better. The new call would look like this?

req.authenticate(['facebook'], { scopes: ["email", "birthday"] }, onAuthenticated);

To help me understand a little better. The new call would look like this?

req.authenticate(['facebook'], { scopes: ["email", "birthday"] }, onAuthenticated);
@coolaj86

This comment has been minimized.

Show comment Hide comment
@coolaj86

coolaj86 Mar 19, 2013

Bump. Did I understand you correctly or not?

From what I saw in the code I think that it would be less work and cleaner to change the signature to this, but maybe I missed something.

req.authenticate([{ "name": "facebook", "scope": ["email", "birthday"] }], cb);

Bump. Did I understand you correctly or not?

From what I saw in the code I think that it would be less work and cleaner to change the signature to this, but maybe I missed something.

req.authenticate([{ "name": "facebook", "scope": ["email", "birthday"] }], cb);
@ciaranj

This comment has been minimized.

Show comment Hide comment
@ciaranj

ciaranj Mar 19, 2013

Owner

Sorry, I meant, something like:

req.authenticate( 'facebook', { "facebook" : { scope: ["email", "birthday"] } } , cb)

That middle argument hash already exists on the API, just needs plumbing through, however it isn't as clean as your suggestion, in this scenario. (there's an unfortunate collision here between the already supported 'scope' property on that hash and facebook's scope :)

Owner

ciaranj commented Mar 19, 2013

Sorry, I meant, something like:

req.authenticate( 'facebook', { "facebook" : { scope: ["email", "birthday"] } } , cb)

That middle argument hash already exists on the API, just needs plumbing through, however it isn't as clean as your suggestion, in this scenario. (there's an unfortunate collision here between the already supported 'scope' property on that hash and facebook's scope :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment