node-oauth / node-oauth2-server

🚀 The successor to oauthjs/oauth2-server. 🔒 Complete, compliant, maintained and well tested OAuth2 Server for node.js. Includes native async await and PKCE.
https://www.npmjs.com/package/@node-oauth/oauth2-server
MIT License
317 stars 46 forks source link

`authenticate` endpoint still expects `scope` as a `string` instead of `string[]` #281

Open Valerionn opened 7 months ago

Valerionn commented 7 months ago

Specify your setup

Irrelevant

Describe the bug

According to the Documentation and the Typescript typings, OAuth2Server.authenticate expects options.scope, which is of type string[]. However, in authenticate-handler.js, this scope is passedd to parseScope [source], which expects scope to be a string:

    if (typeof requestedScope !== 'string') {
      throw new InvalidScopeError('Invalid parameter: `scope`');
    }

To Reproduce

Try to pass a scope-Array to authenticate

Expected behavior

No error thrown, but instead an error would be thrown if I wouldn't pass an array.

jankapunkt commented 7 months ago

@Valerionn thank you for reporting. From what I see this must be still wrong in the types and docs, since scope must be a string, according to the standard.

// CC @dhensby

dhensby commented 7 months ago

Ah, a good spot.

As I remember, our intention was that this should be an array of strings (my understanding was we wanted the developer's interface to deal with arrays of strings for scopes, but the HTTP interactions are a single string with space delimited scopes (as per the spec)). Implementing that intention as a "bug fix" will be quite disruptive, though...

I see three choices:

  1. Update the docs to say it's a string and leave it at that
  2. Update the code to accept an array of scopes and release a new major version
  3. Update the code to accept either a string or an array and release a minor version (with a view to remove the string support in the future / next major).
jankapunkt commented 7 months ago

This is the related PR: https://github.com/node-oauth/node-oauth2-server/pull/267

@Valerionn so the parseScope expects a string and will at the end split it into an array. Model-implementations will receive and return arrays while authenticate should indeed expect a string.

From what I see this rather calls for a fix in the docs.

Valerionn commented 7 months ago

@jankapunkt A fix in the docs and Typescript types sounds good to me as well (then I won't have to manually cast the type there). I just found it weird that the library expects a string[] for scope everywhere else except at this call (and additionally, the typing is currently wrong as well).

Btw. (regarding types), the BaseModel.getClient type defines the method as getClient(clientId: string, clientSecret: string) but should be getClient(clientId: string, clientSecret: string | null) (as correctly described in the Docs).

If you want to, I can make a PR with the doc + typing changes so they match the code (I just didn't want to make a PR yet because I didn't know the intended behavior).