-
Notifications
You must be signed in to change notification settings - Fork 5
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
JCL-444: Improve support for RFC 9207 #1246
Conversation
@@ -211,6 +211,13 @@ ErrorResponse tryParseError(final InputStream input) { | |||
} | |||
|
|||
private Request tokenRequest(final Metadata metadata, final TokenRequest request) { | |||
if (request.getIssuer() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the Issuer required to be in the request? Is it ok to ignore if null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the metadata.authorizationResponseIssParameterSupported
is true
(from the OpenID Provider), then we should expect an iss
parameter in the response. I'll re-read the relevant part of the spec to make sure we're doing the proper validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case is there a missing if
statement missing here. It would also be nice to have a test covering both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In short, we need that additional if
statement you refer to.
Now I recall how all this fits together in the context of a web application:
// Intialize the OAuth2 authorization code flow
@GET
@Path("/login")
public CompletionStage<Response> login() {
var client = new OpenIdProvider(issuer, dpop);
var request = AuthorizationRequest.newBuilder()
.scope("openid")
.scope("webid")
.build(config.clientId, config.redirectUri);
// Redirect the client to the authorization endpoint
return client.authorize(request).thenApply(Response::seeOther);
}
Then, the response gets processed in the following way:
// Continue the OAuth2 authorization code flow
// The client will receive a URL such as /callback?code=123456&iss=https://op.example
@GET
@Path("/callback")
public CompletionStage<Response> callback(@QueryParam("code") String code, @QueryParam("iss") String issuer) {
var client = new OpenIdProvider(issuer, dpop);
var request = TokenRequest.newBuilder()
.code(code)
.issuer(issuer)
.build("authorization_code", config.clientId);
return client.token(request)
.thenApply(token -> {
// store or process token.idToken
// set a session cookie for the application
// redirect the user to a landing page (e.g. /profile)
return Response.seeOther(URI.create("/profile"));
});
}
In this flow, setting TokenRequest.Builder.issuer()
with a null value is equivalent to not setting it at all. And so, if the designated OP supports RFC 9207 (via the Metadata response), then we should expect that the value not only matches the OP's own issuer URI but also that it is non-null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec gives some guidance as a SHOULD:
Clients SHOULD discard authorization responses with the iss parameter from authorization servers that do not indicate their support for the parameter. However, there might be legitimate authorization servers that provide the iss parameter without indicating their support in their metadata. Local policy or configuration can determine whether to accept such responses, and specific guidance is out of scope for this specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f00802e adjusts the conditional logic. The existing integration tests cover the case where authorization_response_iss_parameter_supported
is undefined (i.e. false
) so the inverse case is also covered in the tests
This improves support for RFC 9270 for anyone building an authorization code flow client with the JCL API.