Compliance/fix scope

[jankapunkt]

Summary

This ensures the scope in token response is always conformant with https://datatracker.ietf.org/doc/html/rfc6749.html#section-5.1

• Scope can be sent from clients either as string or array of strings.
• Internally scope is managed as array of strings.
• scope in return values from handlers are still array of string
• scope in response body scope will always be a string

Linked issue(s)

#263

Involved parts of the project

Token Handler, Tests

Added tests?

Tests updated

OAuth2 standard

https://datatracker.ietf.org/doc/html/rfc6749.html#section-5.1

Reproduction

[dhensby]

• Scope can be sent from clients either as string or array of strings.

That is not quite my reading of the specification. 4.3.2 defines the Access Token Request, and 5.1 specifies defines a Successful Response, both references 3.3, which states "The value of the scope parameter is expressed as a list of space-delimited, case-sensitive strings". So both the request and response should deal with a string, but the internals can deal with an array.

I'd suggest removing support for scopes in requests being arrays (that never seemed to be an intended feature of this release that I can see from the test coverage, at least).

[jankapunkt]

The problem is the somewhat confusing expression

The value of the scope parameter is expressed as a list of space-delimited, case-sensitive strings.

However, the implementation with this PR allows for space-delimited strings as well as arrays. I see this as a superset to the definition of the standard and no a violation of compliance.

[dhensby]

Whilst that particular expression could be clearer, to me it's clear enough that the scopes are listed in a string with spaces as a delimiter. A bit like how a CSV row is a string with the items delimited with a comma ,.

Further, the ABNFs of the scope and scope_token is explicit that a scope is a string and it is space delimited to separate each scope_token see A.4.

Whilst I accept that support beyond the scope of the standard could be added so that an array could be provided, we then have ambiguity about how an array of scopes should be interpreted. eg:

The following scope request (url encoded) scope[]=openid&scope[]=email+user would result in an array of scope: ['openid', 'email user'], but scope_tokens aren't allowed to have spaces, so now there's a question to answer: should these scopes be split at spaces (as spaces are technically the delimiter), reject the request as invalid (but why is it invalid, we're explicitly supporting arrays now and scope can have spaces), or leave the array as-is (in contravention to the standard).

My point here isn't that supporting clients passing arrays is wrong, or that extensions to the standard shouldn't be added/supported, but instead that it needs a lot more thinking about than casually adding it into this PR.

IMO - if consumers want to be able to accept non-standard requests, then there should be an adapter model to allow that (ie: consumers can provide their own scope validator as long as it returns the scopes as an array or undefined), that way the responsibilities of the core are kept very narrow, but extensibility is provided for anyone wanting to add non-standard extensions.

[jankapunkt]

Thank you for the input. I didn't gave this too much thought, indeed.

I think this goes beyond the "scope" of this PR, haha. For now I think it's best to get a new version out to become compliant again and let's give this some more thought in the discussion section.

What do you think?

[dhensby]

I think that's right. It seems that arrays can get through the system more as a side-effect of quirky JS behaviour / missing validation, but it's not and intended behaviour so I think adding specific support for it should be intentional rather than formalising a side-effect.

[jankapunkt]

@dhensby do you think this sufficient so far? I would merge and create a new release 5.1.0 or should we first go with a release candidate?

[jorenvandeweyer]

I took some time to reread the spec and the issues that introduced the change to change the scopes internally to arrays instead of just a string.

We should implement it this way that the request only allows a string and the response only returns a string, as described in the spec.

But we should internally immediately parse it to an array for security reasons (see #104). The developer implementing this library should also only use arrays for consistency between the library and the implementation.

[jorenvandeweyer]

Lgtm

[jankapunkt]

Published as RC - feel free to test and let me know if your staging environments run stable or if there is something missing.

https://github.com/node-oauth/node-oauth2-server/releases/tag/v5.1.0-rc.0

[jankapunkt]

Did anyone get a chance to test the RC? I'd like to release 5.1.0 asap

[dhensby]

I've put it into my codebase and it works as expected, but it's not been deployed to production

[dhensby]

We've been using this in our dev/test environments now for a week and it's been performing as expected 🎉

[jankapunkt]

Hey @dhensby great to hear that! I'm sick in bed rn, but I hope to be back next week and publish 5.1.0

[dhensby]

Feel better soon!