[5.0.0] scope as array in responses

[dhensby]

I've been doing some work to upgrade a current implementation of v4 to v5 and noticed that the scope property is now treated as an array of strings.

Logically this makes sense within the internals of the OAuth server and the logic therein; often scope is actually a list of scopes that need to be validated against a token/client so treating these as a list makes a lot of sense.

However, changing the response format of the Token response to also have scope as an array has a much wider reaching impact than a breaking change within the implementation details of the library, it impacts and forces a breaking change on every single consumer of the OAuth API being served by the library, making a graceful upgrade of clients "in the field" basically impossible.

We have clients that are expecting scope to be a string, so changing it to a list is simply not feasible if we want to maintain access of deployed clients. This means we have to do some manual patching of responses to ensure the list of scopes is actually returned as a string instead of an array to ensure clients in the wild are not impacted.

My personal preference would be that this change is reverted and scope is returned as a string in response bodies, but understand that may be difficult now that v5 is "stable"; alternatively could we provide either a scope formatter function for the request so we can easily format the scope property (allowing implementors to keep the legacy behaviour) or add a config option to opt-in to scope-as-string behaviour?

[jorenvandeweyer]

Hi @dhensby

Can you provide us with a bit more context of your implementation? Which function are you invoking?

I assume you are returning the token response directly to the user with an API call?

[dhensby]

Yes, so this is for a very basic client_credentials grant / token response. Simplified code below:

const server = new OAuth2Server({
  model: new AuthModel(),
});

const token = await server.token(request, response);

// return the HTTP Response
return response;

In v4 response.body would have been: { access_token: string; token_type: string; expires_in: number; scope: string; }. In v5 it is { access_token: string; token_type: string; expires_in: number; scope: string[]; }.

The scope prop being an array of strings here means that clients in the wild that may be a bit too tightly coupled to the the scope being a string and not an array of strings now break. As such I have to do this to ensure backward compatibility:

const server = new OAuth2Server({
  model: new AuthModel(),
});

const token = await server.token(request, response);

response.body = {
  ...response.body,
  scope: token.scope.join(' '),
};

// return the HTTP Resonse
return response;

This is pretty tightly coupled to my expectation that scope will always be present in the response (which it may not be in other applications) and also is just a bit nasty to have to re-write the response body in this way. If we could either set a config on the server or provide a scope-formatter, that would mean the internals can worry about whether there is even a scope prop to format.

[jankapunkt]

@jorenvandeweyer how was it before in 4.x? IIRC it was like so:

• string in; string out
• array in; array out

Is that correct?

I would like to not do a hard breaking change after a major update (we had lots of users in the RC releases and I think they expect no further breaking change now being on 5.x).

Therefore I would propose a solution that by default keeps the array in; array out signature but that also allows to have string in; string out;

[dhensby]

The library claims to be standards compliant but, as pointed out, the standard states the scope prop should be a string.

I'm not really bothered about that aspect so much, but I am bothered that the library now no longer supports the standard and provides no way to be compliant.

Using arrays is a very real problem for clients that are deployed in the wild. Updating clients to accept an array of scopes instead of strings would be much more of an inconvenience than fixing the server implementation because it was acknowledged that it was intended for a non-complaint change to have made it into a release. There appears to be no graceful way to make this upgrade.

[jankapunkt]

@jorenvandeweyer can you take on this? I can review, merge and release but not implement, as I'm fully occupied the next two weeks.

This also shows that we should improve on our Compliance test suite and make it much closer to the RFC

@dhensby would you help out with review, too?

[jankapunkt]

@dhensby please review #267

[jorenvandeweyer]

4.x.x was not compliant with the spec and didn't allow a scope change in the refresh_token grant type.

5.x.x returns accidentally an array as type for the scope parameter in the response. It think this behaviour should be classified as a bug. We can fix this is in 5.x.x and we should not see this as breaking compatibility but a minor update (5.1.x).