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

OAuth 2.0 Discovery Clients #6847

Closed
wants to merge 1 commit into from

Conversation

jzheaux
Copy link
Contributor

@jzheaux jzheaux commented May 7, 2019

A support class for configuration that is common to the various
builders that use discovery.

The following is how you would configure ClientRegistrations to use
oauth-authorization-server discovery:

ClientRegistrations.fromIssuerLocation("https://idp.example.org",
        OAuth2MetadataClientBuilder::oauth2Discovery);

Or to use OIDC Discovery as well as OAuth 2.0 Discovery do:

ClientRegistrations.fromIssuerLocation("http://idp.example.org",
        client -> client.oidcDiscovery().oauth2Discovery());

A support class for configuration that is common to the various
builders that use discovery.

The following is how you would configure ClientRegistrations to use
oauth-authorization-server discovery:

ClientRegistrations.fromIssuerLocation("https://idp.example.org",
        OAuth2MetadataClientBuilder::oauth2Discovery);

Or to use OIDC Discovery as well as OAuth 2.0 Discovery do:

ClientRegistrations.fromIssuerLocation("http://idp.example.org",
        client -> client.oidcDiscovery().oauth2Discovery());
import org.springframework.web.client.HttpClientErrorException;
import org.springframework.web.util.UriComponentsBuilder;

public class OAuth2MetadataClientBuilder {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not convinced that this is the time to introduce this as a public class. However, I see no other way to share this with ClientRegistrations, JwtDecoders, and ReactiveJwtDecoders.

Some alternatives include:

  • Making the constructor protected. I don't see this as a huge win since its still a public contract that folks simply then need to extend to use.
  • Have two copies of this class. This seems odd, really, since there is nothing Resource Server-specific or Client Application-specific about this class. It would also mean copying this into other packages where we may want to do discovery down the road (e.g. introspection configuration)
  • Narrow the class's contract down to only generating the URIs. The overlapping concerns of these three classes (ClientRegistrations, JwtDecoders, and ReactiveJwtDecoders) are the need to address a metadata endpoint using the issuer. This is the same way that Nimbus exposes this feature, and it is clearly the way that the spec outlines the contract - an issuer in exchange for metadata. While this class could more narrowly just calculate the URIs, it seems a very odd thing to expose since it seems an internal implementation detail to a metadata client (it wouldn't make sense to me to hand the fully-qualified discovery url to a discovery client since that discovery client would really prefer to calculate it itself).

For these reasons, I do lean towards keeping this class public with a URI -> Map<String, Object> contract.

Also, I'd prefer the class be final, though really this PR is in draft form and has yet some cleanup to do. My hope is to begin conversation about what is an effective consolidation of the duplicated code in these three classes, and perhaps others down the road.

@jgrandja
Copy link
Contributor

jgrandja commented May 7, 2019

Related #5543. Also see this comment that points to a branch with similar implementation.

@jzheaux
Copy link
Contributor Author

jzheaux commented May 22, 2019

Closing in favor of #6765

@jzheaux jzheaux closed this May 22, 2019
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.

2 participants