C0urante commented on a change in pull request #8357:
URL: https://github.com/apache/kafka/pull/8357#discussion_r421866197
##########
File path:
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/PropertyFileLoginModule.java
##########
@@ -62,35 +62,63 @@ public void initialize(Subject subject, CallbackHandler
callbackHandler, Map<Str
if (fileName == null || fileName.trim().isEmpty()) {
throw new ConfigException("Property Credentials file must be
specified");
}
+
if (!credentialPropertiesMap.containsKey(fileName)) {
+ log.trace("Opening credential properties file '{}'", fileName);
Properties credentialProperties = new Properties();
try {
try (InputStream inputStream =
Files.newInputStream(Paths.get(fileName))) {
+ log.trace("Parsing credential properties file '{}'",
fileName);
credentialProperties.load(inputStream);
}
credentialPropertiesMap.putIfAbsent(fileName,
credentialProperties);
+ if (credentialProperties.isEmpty())
+ log.warn("Credential properties file '{}' is empty; all
requests will be permitted",
+ fileName);
} catch (IOException e) {
log.error("Error loading credentials file ", e);
throw new ConfigException("Error loading Property Credentials
file");
}
+ } else {
+ log.trace(
+ "Credential properties file '{}' has already been opened and
parsed; will read from cached, in-memory store",
+ fileName);
}
}
@Override
public boolean login() throws LoginException {
Callback[] callbacks = configureCallbacks();
try {
+ log.trace("Authenticating user; invoking JAAS login callbacks");
callbackHandler.handle(callbacks);
} catch (Exception e) {
+ log.warn("Authentication failed while invoking JAAS login
callbacks");
Review comment:
I don't think this is going to get logged for auth failures (assuming
you mean things like incorrect username/password, bad auth header, etc.).
You can see the code for `callbackHandler::handle` in the
`JaasBasicAuthFilter.BasicAuthCallBackHandler` class; the only expected
exception would be an `UnsupportedCallbackException`, which probably does
warrant logging at `WARN` level.
I'd like to refactor this slightly to take into account your feedback about
the logging/exception logic in
`JaasBasicAuthFilter.BasicAuthCallBackHandler::handle`; I think we can keep
this log message here, and use it in lieu of the `WARN`-level messages in the
other file.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
