Skip to content

JCL-444: Improve support for RFC 9207 #1246

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.

Sign up for GitHub

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

Merged
acoburn merged 4 commits into from
May 11, 2024
Merged

JCL-444: Improve support for RFC 9207 #1246

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7273582
JCL-444: Improve support for RFC 9207
acoburn Oct 24, 2023
37ccf0b
Merge branch 'main' into JCL-444-rfc-9207-support
acoburn May 8, 2024
f00802e
Code review
acoburn May 10, 2024
0c1de1e
Simplify replacement
acoburn May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions openid/src/main/java/com/inrupt/client/openid/Metadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,9 @@ public class Metadata {
* A list of DPoP signing algorithm values supported by the given OpenID Connect provider.
*/
public List<String> dpopSigningAlgValuesSupported;

/**
* Indication of whether the OpenID Connect provider supports RFC-9207.
*/
public boolean authorizationResponseIssParameterSupported;
Comment thread
jholleran marked this conversation as resolved.
}
7 changes: 7 additions & 0 deletions openid/src/main/java/com/inrupt/client/openid/OpenIdProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,13 @@ ErrorResponse tryParseError(final InputStream input) {
}

private Request tokenRequest(final Metadata metadata, final TokenRequest request) {
if (request.getIssuer() != null) {
Copy link
Copy Markdown
Contributor

@jholleran jholleran May 10, 2024

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?

Copy link
Copy Markdown
Collaborator Author

@acoburn acoburn May 10, 2024

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

Copy link
Copy Markdown
Contributor

@jholleran jholleran May 10, 2024

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.

Copy link
Copy Markdown
Collaborator Author

@acoburn acoburn May 10, 2024

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.

Copy link
Copy Markdown
Collaborator Author

@acoburn acoburn May 10, 2024

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.

Copy link
Copy Markdown
Collaborator Author

@acoburn acoburn May 10, 2024

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

if (!request.getIssuer().equals(metadata.issuer)) {
throw new OpenIdException("Issuer mismatch. " +
"Please verify that the designated OpenID issuer is correct");
}
}

if (!metadata.grantTypesSupported.contains(request.getGrantType())) {
throw new OpenIdException("Grant type [" + request.getGrantType() + "] is not supported by this provider.");
}
Expand Down
28 changes: 26 additions & 2 deletions openid/src/main/java/com/inrupt/client/openid/TokenRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public final class TokenRequest {
private final String clientSecret;
private final String authMethod;
private final URI redirectUri;
private final URI issuer;
private final List<String> scopes;

/**
Expand All @@ -58,6 +59,15 @@ public List<String> getScopes() {
return scopes;
}

/**
* Get the issuer.
*
* @return the issuer, may be {@code null}
*/
public URI getIssuer() {
return issuer;
}

/**
* Get the authentication method.
*
Expand Down Expand Up @@ -123,7 +133,8 @@ public static Builder newBuilder() {

/* package-private */
TokenRequest(final String clientId, final String clientSecret, final URI redirectUri, final String grantType,
final String authMethod, final String code, final String codeVerifier, final List<String> scopes) {
final String authMethod, final String code, final String codeVerifier, final URI issuer,
final List<String> scopes) {
this.clientId = clientId;
this.clientSecret = clientSecret;
this.redirectUri = redirectUri;
Expand All @@ -132,6 +143,7 @@ public static Builder newBuilder() {
this.code = code;
this.codeVerifier = codeVerifier;
this.scopes = scopes;
this.issuer = issuer;
}

/**
Expand All @@ -146,6 +158,7 @@ public static class Builder {
private String builderCode;
private String builderCodeVerifier;
private URI builderRedirectUri;
private URI builderIssuer;
private List<String> builderScopes = new ArrayList<>();

/**
Expand Down Expand Up @@ -181,6 +194,17 @@ public Builder scopes(final String... scopes) {
return this;
}

/**
* Set the issuer URI.
*
* @param issuer the issuer value
* @return this builder
*/
public Builder issuer(final URI issuer) {
builderIssuer = issuer;
return this;
}

/**
* Set the authentication method for the token endpoint.
*
Expand Down Expand Up @@ -240,7 +264,7 @@ public TokenRequest build(final String grantType, final String clientId) {
}

return new TokenRequest(clientId, builderClientSecret, builderRedirectUri, grant, builderAuthMethod,
builderCode, builderCodeVerifier, builderScopes);
builderCode, builderCodeVerifier, builderIssuer, builderScopes);
}
}
}
Expand Down
37 changes: 37 additions & 0 deletions openid/src/test/java/com/inrupt/client/openid/OpenIdProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,43 @@ void tokenRequestIllegalArgumentsTest() {
() -> builder.build("myGrantType", null));
}

@Test
void tokenIssuerMismatch() {
final TokenRequest tokenReq = TokenRequest.newBuilder()
.code("someCode")
.codeVerifier("myCodeverifier")
.issuer(URI.create("https://issuer.test"))
.redirectUri(URI.create("https://example.test/redirectUri"))
.build(
"authorization_code",
"myClientId"
);

final CompletionException ex = assertThrows(CompletionException.class, openIdProvider.token(tokenReq)
.toCompletableFuture()::join);
assertTrue(ex.getCause() instanceof OpenIdException);
final OpenIdException cause = (OpenIdException) ex.getCause();
assertTrue(cause.getMessage().contains("Issuer mismatch"));
}

@Test
void tokenIssuerMatch() {
final TokenRequest tokenReq = TokenRequest.newBuilder()
.code("someCode")
.codeVerifier("myCodeverifier")
.issuer(URI.create("http://example.test"))
.redirectUri(URI.create("https://example.test/redirectUri"))
.build(
"authorization_code",
"myClientId"
);
final TokenResponse token = openIdProvider.token(tokenReq)
.toCompletableFuture().join();
assertEquals("123456", token.accessToken);
assertNotNull(token.idToken);
assertEquals("Bearer", token.tokenType);
}

@Test
void tokenNoClientSecretTest() {
final TokenRequest tokenReq = TokenRequest.newBuilder()
Expand Down
3 changes: 2 additions & 1 deletion openid/src/test/resources/metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,5 +69,6 @@
],
"dpop_signing_alg_values_supported":[
"RS256", "ES256"
]
],
"authorization_response_iss_parameter_supported": true
}