Copilot commented on code in PR #6534:
URL: https://github.com/apache/hive/pull/6534#discussion_r3388731767
##########
itests/hive-unit/src/test/java/org/apache/hive/service/auth/saml/TestHttpSamlAuthentication.java:
##########
@@ -554,6 +563,86 @@ public void testTokenReuse() throws Exception {
}
}
+ @Test
+ public void testValidTokenRoundTrip() throws Exception {
+ ISAMLAuthTokenGenerator tokenGenerator = createTokenGenerator("30s");
+ String token = tokenGenerator.get("alice", "relay-state-1");
+ assertEquals("alice", tokenGenerator.validate(token));
+ }
+
+ @Test
+ public void testForgedSignatureRejected() throws Exception {
+ ISAMLAuthTokenGenerator tokenGenerator = createTokenGenerator("30s");
+ String forgedPayload = "u=alice;id=1337;time=" + System.currentTimeMillis()
+ + ";rs=deadbeef;sg=bogus";
+ String forgedToken =
Base64.getEncoder().encodeToString(forgedPayload.getBytes());
+ try {
Review Comment:
Use an explicit charset when encoding the forged payload; this keeps the
test deterministic across platforms with different default charsets.
##########
itests/hive-unit/src/test/java/org/apache/hive/service/auth/saml/TestHttpSamlAuthentication.java:
##########
@@ -554,6 +563,86 @@ public void testTokenReuse() throws Exception {
}
}
+ @Test
+ public void testValidTokenRoundTrip() throws Exception {
+ ISAMLAuthTokenGenerator tokenGenerator = createTokenGenerator("30s");
+ String token = tokenGenerator.get("alice", "relay-state-1");
+ assertEquals("alice", tokenGenerator.validate(token));
+ }
+
+ @Test
+ public void testForgedSignatureRejected() throws Exception {
+ ISAMLAuthTokenGenerator tokenGenerator = createTokenGenerator("30s");
+ String forgedPayload = "u=alice;id=1337;time=" + System.currentTimeMillis()
+ + ";rs=deadbeef;sg=bogus";
+ String forgedToken =
Base64.getEncoder().encodeToString(forgedPayload.getBytes());
+ try {
+ tokenGenerator.validate(forgedToken);
+ fail("Expected forged token to be rejected");
+ } catch (HttpSamlAuthenticationException e) {
+ assertEquals("Token could not be verified", e.getMessage());
+ }
+ }
+
+ @Test
+ public void testInvalidTokenRejected() throws Exception {
+ ISAMLAuthTokenGenerator tokenGenerator = createTokenGenerator("30s");
+ try {
+ tokenGenerator.validate("notAValidToken");
+ fail("Expected malformed base64 token to be rejected");
+ } catch (HttpSamlAuthenticationException e) {
+ assertEquals("Invalid token", e.getMessage());
+ }
+ String invalidStructure =
Base64.getEncoder().encodeToString("foo".getBytes());
+ try {
+ tokenGenerator.validate(invalidStructure);
+ fail("Expected invalid token structure to be rejected");
+ } catch (HttpSamlAuthenticationException e) {
+ assertEquals("Invalid token", e.getMessage());
+ }
+ }
+
+ @Test
+ public void testExpiredTokenRejected() throws Exception {
+ ISAMLAuthTokenGenerator tokenGenerator = createTokenGenerator("1s");
+ String token = tokenGenerator.get("alice", "relay-state-1");
+ Thread.sleep(1100);
+ try {
+ tokenGenerator.validate(token);
+ fail("Expected expired token to be rejected");
+ } catch (HttpSamlAuthenticationException e) {
+ assertEquals("Token is expired", e.getMessage());
+ }
+ }
+
+ @Test
+ public void testParseHandlesBase64PaddingInSignature() {
+ Map<String, String> kv = new HashMap<>();
+ String token = "u=alice;id=1;time=1000;rs=rs1;sg=YWJjZA==";
+ assertTrue(HiveSamlAuthTokenGenerator.parse(token, kv));
+ assertEquals("alice", kv.get("u"));
+ assertEquals("YWJjZA==", kv.get("sg"));
+ }
+
+ @Test
+ public void testParseRejectsEncodedBearerToken() {
+ Map<String, String> kv = new HashMap<>();
+ String encoded = Base64.getEncoder().encodeToString(
+ "u=alice;id=1;time=1000;rs=rs1;sg=abc".getBytes());
+ assertFalse(HiveSamlAuthTokenGenerator.parse(encoded, kv));
+ }
+
+ @Test
+ public void testParseDecodedTokenFromGenerator() throws Exception {
+ ISAMLAuthTokenGenerator tokenGenerator = createTokenGenerator("30s");
+ String encoded = tokenGenerator.get("bob", "relay-42");
+ String decoded = new String(Base64.getDecoder().decode(encoded));
Review Comment:
Use an explicit charset when converting decoded Base64 bytes back into a
String to avoid platform-dependent behavior.
##########
service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java:
##########
@@ -382,7 +382,8 @@ private String doSamlAuth(HttpServletRequest request,
HttpServletResponse respon
LOG.info("Successfully validated the token for user {}", user);
// token is valid; now confirm if the client identifier matches with the
relay state.
Map<String, String> keyValues = new HashMap<>();
- if (HiveSamlAuthTokenGenerator.parse(token, keyValues)) {
+ String decodedToken = new String(Base64.getDecoder().decode(token));
Review Comment:
Avoid relying on the platform default charset when converting the decoded
bearer token bytes to a String; use an explicit charset to ensure consistent
parsing across environments.
##########
service/src/java/org/apache/hive/service/auth/saml/HiveSamlAuthTokenGenerator.java:
##########
@@ -144,7 +144,7 @@ private boolean isExpired(long currentTime, long tokenTime)
{
}
private boolean signatureMatches(String origSign, String derivedSign) {
- return !MessageDigest.isEqual(origSign.getBytes(), derivedSign.getBytes());
+ return MessageDigest.isEqual(origSign.getBytes(), derivedSign.getBytes());
}
Review Comment:
Use an explicit charset when converting signatures to bytes for comparison;
relying on the platform default charset can cause platform-dependent behavior
in signature verification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
