[
https://issues.apache.org/jira/browse/KNOX-3424?focusedWorklogId=1037914&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1037914
]
ASF GitHub Bot logged work on KNOX-3424:
----------------------------------------
Author: ASF GitHub Bot
Created on: 25/Aug/26 22:27
Start Date: 25/Aug/26 22:27
Worklog Time Spent: 10m
Work Description: hsheinblatt commented on code in PR #1354:
URL: https://github.com/apache/knox/pull/1354#discussion_r3857874612
##########
gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/jwt/filter/TokenExchangeHandlerTest.java:
##########
@@ -113,7 +118,8 @@ public void testUnsupportedActorTokenType() throws
Exception {
filter.valid.put("subtok", jwt("alice", "KNOXSSO"));
handler.handle(request("subtok", JWT_TYPE, "acttok", SAML2_TYPE),
response, chain);
assertEquals(HttpServletResponse.SC_BAD_REQUEST, filter.errorStatus);
- assertTrue(filter.errorMessage.contains("unsupported_token_type"));
+ assertEquals("unsupported_token_type", filter.error);
Review Comment:
Shouldn't this be invalid_request? unsupported_token_type isn't allowed as
an error field, I believe: https://www.rfc-editor.org/info/rfc6749/#section-5.2
##########
gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/jwt/filter/TokenExchangeHandlerTest.java:
##########
@@ -88,23 +90,26 @@ public void testActorTokenTypeRequiredWhenActorPresent()
throws Exception {
filter.valid.put("subtok", jwt("alice", "KNOXSSO"));
handler.handle(request("subtok", JWT_TYPE, "acttok", null), response,
chain);
assertEquals(HttpServletResponse.SC_BAD_REQUEST, filter.errorStatus);
- assertTrue(filter.errorMessage.contains("actor_token_type is required"));
+ assertEquals("invalid_request", filter.error);
+ assertTrue(filter.errorDescription.contains("actor_token_type is
required"));
assertFalse(filter.continued);
}
@Test
public void testActorTokenTypeForbiddenWithoutActor() throws Exception {
handler.handle(request("subtok", JWT_TYPE, null, JWT_TYPE), response,
chain);
assertEquals(HttpServletResponse.SC_BAD_REQUEST, filter.errorStatus);
- assertTrue(filter.errorMessage.contains("must not be present"));
+ assertEquals("invalid_request", filter.error);
+ assertTrue(filter.errorDescription.contains("must not be present"));
assertFalse(filter.continued);
}
@Test
public void testUnsupportedSubjectTokenType() throws Exception {
handler.handle(request("subtok", SAML2_TYPE, null, null), response, chain);
assertEquals(HttpServletResponse.SC_BAD_REQUEST, filter.errorStatus);
- assertTrue(filter.errorMessage.contains("unsupported_token_type"));
+ assertEquals("unsupported_token_type", filter.error);
Review Comment:
Same here.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/TokenExchangeHandler.java:
##########
@@ -92,35 +92,35 @@ void handle(HttpServletRequest request, HttpServletResponse
response, FilterChai
// RFC 8693 section 2.1: subject_token and subject_token_type are REQUIRED.
if (subjectTokenValue == null || subjectTokenValue.isEmpty()) {
filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
- "invalid_request: the subject_token parameter is required");
+ "invalid_request", "the subject_token parameter is required");
return;
}
if (subjectTokenType == null || subjectTokenType.isEmpty()) {
filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
- "invalid_request: the subject_token_type parameter is required");
+ "invalid_request", "the subject_token_type parameter is required");
return;
}
// RFC 8693 section 2.1: actor_token_type is REQUIRED when actor_token is
present and MUST NOT
// be present otherwise.
if (hasActorToken && !hasActorTokenType) {
filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
- "invalid_request: actor_token_type is required when actor_token is
present");
+ "invalid_request", "actor_token_type is required when actor_token is
present");
return;
}
if (!hasActorToken && hasActorTokenType) {
filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
- "invalid_request: actor_token_type must not be present without
actor_token");
+ "invalid_request", "actor_token_type must not be present without
actor_token");
return;
}
// Only JWT-family token types are supported.
if (isNotSupportedTokenType(subjectTokenType)) {
filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
- "unsupported_token_type: unsupported subject_token_type " +
subjectTokenType);
+ "unsupported_token_type", "unsupported subject_token_type " +
subjectTokenType);
return;
}
if (hasActorToken && isNotSupportedTokenType(actorTokenType)) {
filter.handleValidationError(request, response,
HttpServletResponse.SC_BAD_REQUEST,
- "unsupported_token_type: unsupported actor_token_type " +
actorTokenType);
+ "unsupported_token_type", "unsupported actor_token_type " +
actorTokenType);
Review Comment:
Isn't this supposed to be invalid_request not unsupported_token_type
(https://www.rfc-editor.org/info/rfc6749/#section-5.2)? I realized this below,
so same comment on the test for this.
##########
gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/JWTFederationFilterTokenExchangeTest.java:
##########
@@ -324,19 +328,20 @@ public void testFutureNbfRejectedOnDynamicPath() throws
Exception {
final HttpServletRequest request = buildTokenExchangeRequest(
nbfJwt.serialize(), buildContextWithIssuerService(issuerSvc));
- final HttpServletResponse response =
EasyMock.createNiceMock(HttpServletResponse.class);
- response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Bad request: the
NotBefore check failed");
- EasyMock.expectLastCall().once();
- EasyMock.replay(request, response, issuerSvc);
+ final JsonErrorResponse response = new JsonErrorResponse();
+ EasyMock.replay(request, response.mock, issuerSvc);
final TestFilterChain chain = new TestFilterChain();
- handler.doFilter(request, response, chain);
+ handler.doFilter(request, response.mock, chain);
Assert.assertFalse(chain.doFilterCalled);
if (capturedJwt.hasCaptured()) {
Assert.assertEquals(EXTERNAL_ISSUER, capturedJwt.getValue().getIssuer());
}
- EasyMock.verify(mockAuth, issuerSvc, response);
+ Assert.assertEquals(HttpServletResponse.SC_BAD_REQUEST, response.status());
+ Assert.assertTrue(response.body(),
response.body().contains("\"error\":\"invalid_request\""));
Review Comment:
Shouldn't this be invalid_grant? The token is invalid:
`
invalid_grant
The provided authorization grant (e.g., authorization
code, resource owner credentials) or refresh token is
invalid, expired, revoked, does not match the redirection
URI used in the authorization request, or was issued to
another client.
`
nbf is kind of like a reverse expired, so I'd expect it to be that.
##########
gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/jwt/filter/TokenExchangeHandlerTest.java:
##########
@@ -113,7 +118,8 @@ public void testUnsupportedActorTokenType() throws
Exception {
filter.valid.put("subtok", jwt("alice", "KNOXSSO"));
handler.handle(request("subtok", JWT_TYPE, "acttok", SAML2_TYPE),
response, chain);
assertEquals(HttpServletResponse.SC_BAD_REQUEST, filter.errorStatus);
- assertTrue(filter.errorMessage.contains("unsupported_token_type"));
+ assertEquals("unsupported_token_type", filter.error);
+ assertTrue(filter.errorDescription.contains("actor_token_type"));
assertFalse(filter.continued);
}
Review Comment:
I think there are missing tests, at least for "Failed to parse token in
token exchange", the error path through the `catch (ParseException |
UnknownTokenException e)` block. I'd add that test as well.
##########
gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/JWTFederationFilterTokenExchangeTest.java:
##########
@@ -371,19 +376,20 @@ public void testAudienceMismatchRejectedOnDynamicPath()
throws Exception {
final HttpServletRequest request = buildTokenExchangeRequest(
subjectJwt.serialize(), buildContextWithIssuerService(issuerSvc));
- final HttpServletResponse response =
EasyMock.createNiceMock(HttpServletResponse.class);
- response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Bad request:
missing required token audience");
- EasyMock.expectLastCall().once();
- EasyMock.replay(request, response, issuerSvc);
+ final JsonErrorResponse response = new JsonErrorResponse();
+ EasyMock.replay(request, response.mock, issuerSvc);
final TestFilterChain chain = new TestFilterChain();
- handler.doFilter(request, response, chain);
+ handler.doFilter(request, response.mock, chain);
Assert.assertFalse(chain.doFilterCalled);
if (capturedJwt.hasCaptured()) {
Assert.assertEquals(EXTERNAL_ISSUER, capturedJwt.getValue().getIssuer());
}
- EasyMock.verify(mockAuth, issuerSvc, response);
+ Assert.assertEquals(HttpServletResponse.SC_BAD_REQUEST, response.status());
+ Assert.assertTrue(response.body(),
response.body().contains("\"error\":\"invalid_request\""));
Review Comment:
Same here, shouldn't it be invalid_grant?
Issue Time Tracking
-------------------
Worklog Id: (was: 1037914)
Time Spent: 0.5h (was: 20m)
> Dynamic audience handling in the KNOXTOKEN service
> --------------------------------------------------
>
> Key: KNOX-3424
> URL: https://issues.apache.org/jira/browse/KNOX-3424
> Project: Apache Knox
> Issue Type: Improvement
> Components: Server
> Affects Versions: 3.0.0
> Reporter: Tamás Hanicz
> Assignee: Tamás Hanicz
> Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
