Copilot commented on code in PR #7817:
URL: https://github.com/apache/ignite-3/pull/7817#discussion_r2959699536
##########
modules/eventlog/src/integrationTest/java/org/apache/ignite/internal/eventlog/ItEventLogTest.java:
##########
@@ -121,6 +153,32 @@ void logsToFile() throws Exception {
+ "}";
assertThat(readEventLog(),
hasItem(matchesRegex(expectedEventJsonPatternAfterInvalidAuth)));
+
+ // And the client handler log contains authentication failure details.
+ assertThat(clientLogInspector.events(), hasItem(allOf(
+ containsString("Client authentication failed"),
+ containsString("remoteAddress="),
+ containsString("identity=UNKNOWN]")
+ )));
+
+ // When try to authenticate with invalid credentials via REST API.
+ HttpClient httpClient = HttpClient.newBuilder().build();
+ String endpoint = "/management/v1/configuration/cluster/";
+ HttpRequest request = HttpRequest.newBuilder(URI.create(NODE_URL +
endpoint))
+ .header("Authorization", basicAuthHeader("UNKNOWN", "SECRET"))
+ .build();
+
+ assertThat(httpClient.send(request, BodyHandlers.ofString()),
hasStatusCode(401));
+
+ // Then the REST log contains authentication failure details.
+ assertThat(restLogInspector.events(), hasItem(allOf(
+ containsString("REST authentication failed [uri=" + endpoint +
", remoteAddress="),
+ containsString("identity=UNKNOWN]")
+ )));
Review Comment:
These assertions read the accumulating log inspector immediately after
triggering authentication failures. Log emission can be asynchronous, so this
can be flaky. Use Awaitility (`untilAsserted`/`until(...)`) to wait until the
expected log line appears (similar to other log-inspector-based tests in the
repo).
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -516,6 +517,11 @@ private void handshake(ChannelHandlerContext ctx,
ClientMessageUnpacker unpacker
.thenCompose(unused ->
authenticationManager.authenticateAsync(authReq))
.whenCompleteAsync((user, err) -> {
if (err != null) {
+ if (isAuthenticationException(err)) {
+ LOG.warn("Client authentication failed
[connectionId={}, remoteAddress={}, identity={}]: {}",
+ connectionId,
ctx.channel().remoteAddress(), authReq.getIdentity(), err.getMessage());
Review Comment:
The authentication-failure log uses `err.getMessage()`, but `err` can be a
`CompletionException`/`ExecutionException` wrapper. This tends to log a noisy
wrapper message instead of the underlying auth failure. Consider logging
`ExceptionUtils.unwrapCause(err).getMessage()` (and/or the unwrapped exception
class) to match the intent of unwrapping authentication exceptions.
##########
modules/eventlog/src/integrationTest/java/org/apache/ignite/internal/eventlog/ItEventLogTest.java:
##########
@@ -121,6 +153,32 @@ void logsToFile() throws Exception {
+ "}";
assertThat(readEventLog(),
hasItem(matchesRegex(expectedEventJsonPatternAfterInvalidAuth)));
+
+ // And the client handler log contains authentication failure details.
+ assertThat(clientLogInspector.events(), hasItem(allOf(
+ containsString("Client authentication failed"),
+ containsString("remoteAddress="),
+ containsString("identity=UNKNOWN]")
+ )));
+
+ // When try to authenticate with invalid credentials via REST API.
+ HttpClient httpClient = HttpClient.newBuilder().build();
+ String endpoint = "/management/v1/configuration/cluster/";
+ HttpRequest request = HttpRequest.newBuilder(URI.create(NODE_URL +
endpoint))
+ .header("Authorization", basicAuthHeader("UNKNOWN", "SECRET"))
+ .build();
+
+ assertThat(httpClient.send(request, BodyHandlers.ofString()),
hasStatusCode(401));
+
+ // Then the REST log contains authentication failure details.
+ assertThat(restLogInspector.events(), hasItem(allOf(
+ containsString("REST authentication failed [uri=" + endpoint +
", remoteAddress="),
+ containsString("identity=UNKNOWN]")
+ )));
+ }
+
+ private static String basicAuthHeader(String username, String password) {
+ return "Basic " + Base64.getEncoder().encodeToString((username + ":" +
password).getBytes());
}
Review Comment:
`(username + ":" + password).getBytes()` uses the platform default charset,
which can make the Basic auth header non-deterministic across environments. Use
an explicit charset (RFC 7617 defaults to ISO-8859-1; UTF-8 is also commonly
used if the server expects it).
##########
modules/rest/src/main/java/org/apache/ignite/internal/rest/authentication/IgniteAuthenticationProvider.java:
##########
@@ -60,11 +65,22 @@ public Publisher<AuthenticationResponse>
authenticate(HttpRequest<?> httpRequest
return null;
});
} catch (InvalidCredentialsException ex) {
+ logAuthenticationFailure(httpRequest, authenticationRequest,
ex);
emitter.error(AuthenticationResponse.exception(ex.getMessage()));
}
}, FluxSink.OverflowStrategy.ERROR);
}
+ private static void logAuthenticationFailure(
+ HttpRequest<?> httpRequest,
+ AuthenticationRequest<?, ?> authenticationRequest,
+ Throwable throwable
+ ) {
+ LOG.warn("REST authentication failed [uri={}, remoteAddress={},
identity={}]: {}",
+ httpRequest.getUri(), httpRequest.getRemoteAddress(),
authenticationRequest.getIdentity(),
+ throwable.getMessage());
+ }
Review Comment:
The REST auth failure log (and response message) uses
`throwable.getMessage()`, which can be a wrapper message (e.g.,
`CompletionException`) rather than the real authentication exception. Consider
unwrapping via `ExceptionUtils.unwrapCause(throwable)` before logging/returning
the message so operators see the actual failure reason.
--
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]
