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

Draft: Support OAuth2 token introspection #157 #158

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nrmaridu
Copy link

fixes #157

Signed-off-by: Nageswara Rao Maridu <[email protected]

fixes TNG#157

Signed-off-by: Nageswara Rao Maridu <[email protected]>
@nrmaridu
Copy link
Author

nrmaridu commented Jan 21, 2024

The Introspection Endpoint was implemented as per below guidelines. The introspection response contains a json property called 'active', which can be used to determine whether the token is active or not.

For supporting various use cases, the introspection response returns all the claims found in the provided token along with active property.

Introspection Endpoint : POST {baseUrl}/realms/{issuer}/protocol/openid-connect/token/introspect

@nrmaridu nrmaridu changed the title Support OAuth2 token introspection #157 Draft: Support OAuth2 token introspection #157 Jan 21, 2024
Copy link
Collaborator

@ostrya ostrya left a comment

Choose a reason for hiding this comment

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

Thank you for your merge request, and sorry for taking so long to review it.

@@ -132,4 +135,9 @@ public String getHostname() {
public String getRealm() {
return realm;
}

@Nonnull
public URI getTokenIntrospectionEndPoint() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe move it next to the token endpoint method? Also, please rename EndPoint to Endpoint.

@@ -16,6 +16,9 @@ public class UrlConfiguration {
private static final String OPEN_ID_JWKS_PATH = "certs";
private static final String OPEN_ID_AUTHORIZATION_PATH = "auth";
private static final String OPEN_ID_END_SESSION_PATH = "logout";

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this empty line.

@@ -11,4 +11,6 @@ public class ConfigurationResponse {
public List<String> subject_types_supported;
public List<String> id_token_signing_alg_values_supported;
public String end_session_endpoint;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove empty line.

@Override
public void handle(RoutingContext routingContext) {
LOG.info(
"Inside TokenIntrospectionRoute. Request body is : {}", routingContext.body().asString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to keep the logging limited to the bare minimum. We already log every response in the CommonHandler, I see no need to log this here.


String token = body.replaceFirst("^" + TOKEN_PREFIX, "");

LOG.debug("Received a request to introspect token : {}", token);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, we don't need to log the token here. The tool runs locally in a testing scope, so the user should know which token was passed in.

try {
claims = tokenHelper.parseToken(token);
} catch (Exception e) {
// If the token is invalid, initialize an empty claims map
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be a good point to actually add logging.

// To support various use cases, we are returning the same claims as the input token
Map<String, Object> responseClaims = new HashMap<>(claims);

if (responseClaims.get(Claims.EXPIRATION) != null
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check is unnecessary, as the JWT library will already throw an exception if the token is expired. Instead, you should add the active claim as true within the try block above, and as false in the catch block.

Map<String, Object> claims;
try {
claims = tokenHelper.parseToken(token);
} catch (Exception e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd actually differentiate here. If the exception is a ClaimJwtException, we still have access to the claims of the token, even if the token itself is expired or otherwise invalid. In this case, I'd still add all claims to the response, only with "active": "false".

.end(Json.encode(responseClaims));
}

private boolean isExpiryTimeInFuture(String expiryTime) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unnecessary (see comment above).

import org.slf4j.LoggerFactory;

@Singleton
public class TokenIntrospectionRoute implements Handler<RoutingContext> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add at least one happy test case to verify that passing in a valid token will return its claims plus the "active" field set to true. Also, please add some tests for the error cases (expired token, missing token, premature token, malformed token).

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.

Is there a reason why token introspection was not supported in this library?
2 participants