-
Notifications
You must be signed in to change notification settings - Fork 27
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
base: main
Are you sure you want to change the base?
Conversation
fixes TNG#157 Signed-off-by: Nageswara Rao Maridu <maridu.nageswararao@gmail.com>
681d4c6
to
e789088
Compare
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 |
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.
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() { |
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.
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"; | |||
|
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.
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; | |||
|
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.
Please remove empty line.
@Override | ||
public void handle(RoutingContext routingContext) { | ||
LOG.info( | ||
"Inside TokenIntrospectionRoute. Request body is : {}", routingContext.body().asString()); |
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.
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); |
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.
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 |
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.
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 |
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.
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) { |
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.
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) { |
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.
This is unnecessary (see comment above).
import org.slf4j.LoggerFactory; | ||
|
||
@Singleton | ||
public class TokenIntrospectionRoute implements Handler<RoutingContext> { |
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.
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).
fixes #157
Signed-off-by: Nageswara Rao Maridu <maridu.nageswararao@gmail.com