Scope param added to a bearer token response

[chervand]

#793 scope param added to a bearer token response

[bretterer]

This looks good to me... We need to make sure to also update the documentation to reflect these changes.

[Sephster]

As per the spec, the returning of scopes should be optional if there are no changes and mandatory if there have been alterations to the scopes requested.

This change would make the server always return the scope. I think it would be better to put this decision into the hands of the implementer instead of always returning a scope to avoid the server being opinionated on this matter.

Is it possible to change this so the scopes will only be returned an option has been set on the server to dictate this. That would make the returning of the scopes more in line with the RFC in my opinion

[chervand]

Since AuthorizationServer's constructor accepts ResponseTypeInterface instance, it is possible to add such an option to AbstractResponseType. But I think it should defaults to returning scopes, because they are conditionally required and will not cause any issues in most cases. If you agree, I'll update the PR. BTW, I think such change should be documented on how to disable returning scopes.

[Sephster]

Hi @chervand - I think it would be better to have them not set by default. In most situations, if we do not return a scope, the implementer can assume they are unchanged.

If we have these on by default, this adds overhead for each request. While probably negligible in most situations, I think it is better we err on the side of caution and only send back the scopes if they have been requested by the implementer, leaving them off by default.

We should aim to ensure this change is also backwards compatible. Thanks for offering help with this!

[chervand]

Please, review the updated PR. Not sure if I should add this to the ResponseTypeInterface due to BC. Maybe adding another interface for that case could be a better solution.

[Sephster]

Hi @chervand - sorry for my delayed reply. I think this is going in the right direction but I realised that we would have to set the scope policy directly on the response, where as I feel this policy should be set and controlled by the server instead. It makes sense to me that this is a server policy rather than a response type policy.

In the authenication server at the moment, we are setting up responses in the function getResponseTypes() by setting private keys and encryption keys on the bearer token.

I think this function would be the best place to also set the scope return policy based on the auth servers settings. That way, the server dictates this instead of the response itself.

One potential problem I can see with this is that we would need to call setReturnScopes from the auth server but we can pass in response types that implement the responseTypeInterface. To prevent the risk of calling setReturnScopes on a response type that doesn't have it, we might need to update the interface to ensure all response types implement this feature but this would be a BC breaking change and would likely need to be included in a future release.

If you can think of an alternative approach though, I'd very much welcome your thoughts.

[chervand]

Hi @Sephster, sorry for the delay too. I'm still interested in this feature and I agree that this is an auth server policy rather than a response type policy.

One potential problem I can see with this is that we would need to call setReturnScopes from the auth server but we can pass in response types that implement the responseTypeInterface. To prevent the risk of calling setReturnScopes on a response type that doesn't have it, we might need to update the interface to ensure all response types implement this feature but this would be a BC breaking change and would likely need to be included in a future release.

I can see 3 possible solutions now:

1. Update ResponseTypeInterface and leave it for the next possible release.
2. Add another interface declaring setReturnScopes which should extend ResponseTypeInterface and make default response type implementing this new interface instead of ResponseTypeInterface. Thus we could check whether response type instance implements it before calling setReturnScopes.
3. Add another interface which wouldn't extend ResponseTypeInterface and make default response type implementing this new interface as well as ResponseTypeInterface. With this implementation we could get rid of setReturnScopes and return scopes if response type implements such interface without breaking BC. However this implementation makes impossible to control it via the auth server.

Let me know what do you think please.

[simonhamp]

@chervand @Sephster just been looking through comments on this issue/PR and I think extending ResponseTypeInterface feels like the better option at this stage.

Having said that, bundling into a new major release wouldn't be that bad - the fact is there are not thousands of people calling for this at the moment, so maybe we don't need to go to the trouble of extending and checking which interface is being implemented.