Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scope param added to a bearer token response #800

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

chervand
Copy link

#793 scope param added to a bearer token response

@chervand chervand changed the title scope param added to a bearer token response Scope param added to a bearer token response Oct 25, 2017
Copy link
Contributor

@bretterer bretterer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Sephster
Copy link
Member

Sephster commented Nov 5, 2017

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
Copy link
Author

chervand commented Nov 6, 2017

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
Copy link
Member

Sephster commented Nov 7, 2017

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
Copy link
Author

chervand commented Nov 7, 2017

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
Copy link
Member

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
Copy link
Author

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
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants