Closed jankapunkt closed 9 months ago
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.
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.
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?
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.
@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?
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
Did anyone get a chance to test the RC? I'd like to release 5.1.0 asap
I've put it into my codebase and it works as expected, but it's not been deployed to production
We've been using this in our dev/test environments now for a week and it's been performing as expected 🎉
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
Feel better soon!
Summary
This ensures the scope in token response is always conformant with https://datatracker.ietf.org/doc/html/rfc6749.html#section-5.1
Linked issue(s)
https://github.com/node-oauth/node-oauth2-server/discussions/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