Copilot commented on code in PR #752:
URL: https://github.com/apache/atlas/pull/752#discussion_r3974007972
##########
client/common/src/main/java/org/apache/atlas/AtlasBaseClient.java:
##########
@@ -573,31 +583,71 @@ void initializeState(Configuration configuration,
String[] baseUrls, UserGroupIn
service =
client.resource(UriBuilder.fromUri(activeServiceUrl).build());
}
- private TokenRetriever<String> getJwtTokenRetriever(Configuration
configuration) {
- return new JwTokenRetrieverDefault(configuration);
+ private boolean isJwtConfigured(Configuration configuration) {
+ boolean ret = false;
+
+ if (configuration != null) {
+ String tokenSupplier =
configuration.getString(PROP_REST_AUTH_TOKEN_SUPPLIER);
+
+ ret = StringUtils.isNotBlank(tokenSupplier);
+
+ if (!ret) {
+ String jwtSource = configuration.getString(JWT_SOURCE, "");
+
+ ret = StringUtils.isNotBlank(jwtSource);
+ }
+ }
+
+ return ret;
}
- private boolean isJwtSourceConfigured(Configuration configuration) {
- if (configuration == null) {
- return false;
+ private Supplier<String> getTokenSupplier(Configuration configuration) {
+ Supplier<String> ret = null;
+
+ if (configuration != null) {
+ String clzName =
configuration.getString(PROP_REST_AUTH_TOKEN_SUPPLIER);
+
+ if (StringUtils.isBlank(clzName)) {
+ clzName = DEFAULT_REST_AUTH_TOKEN_SUPPLIER;
+ }
Review Comment:
Unit tests cover successful wiring for default/custom suppliers, but the new
configuration-driven behavior also introduces failure modes (e.g., configured
class does not implement `Supplier`, class not found, constructor throws). Add
tests asserting the expected exception type/message for these invalid
configurations to prevent regressions in error handling.
##########
client/common/src/main/java/org/apache/atlas/AtlasBaseClient.java:
##########
@@ -573,31 +583,71 @@ void initializeState(Configuration configuration,
String[] baseUrls, UserGroupIn
service =
client.resource(UriBuilder.fromUri(activeServiceUrl).build());
}
- private TokenRetriever<String> getJwtTokenRetriever(Configuration
configuration) {
- return new JwTokenRetrieverDefault(configuration);
+ private boolean isJwtConfigured(Configuration configuration) {
+ boolean ret = false;
+
+ if (configuration != null) {
+ String tokenSupplier =
configuration.getString(PROP_REST_AUTH_TOKEN_SUPPLIER);
+
+ ret = StringUtils.isNotBlank(tokenSupplier);
+
+ if (!ret) {
+ String jwtSource = configuration.getString(JWT_SOURCE, "");
+
+ ret = StringUtils.isNotBlank(jwtSource);
+ }
+ }
+
+ return ret;
}
- private boolean isJwtSourceConfigured(Configuration configuration) {
- if (configuration == null) {
- return false;
+ private Supplier<String> getTokenSupplier(Configuration configuration) {
+ Supplier<String> ret = null;
+
+ if (configuration != null) {
+ String clzName =
configuration.getString(PROP_REST_AUTH_TOKEN_SUPPLIER);
+
+ if (StringUtils.isBlank(clzName)) {
+ clzName = DEFAULT_REST_AUTH_TOKEN_SUPPLIER;
+ }
+
+ try {
+ Class<?> clz = Class.forName(clzName);
+
+ if (!Supplier.class.isAssignableFrom(clz)) {
+ throw new IllegalArgumentException(clzName + " does not
implement Supplier");
+ }
+
+ try {
+ // look for a constructor taking Configuration as its
argument
+ Constructor<?> c =
clz.getDeclaredConstructor(Configuration.class);
+
+ ret = (Supplier<String>) c.newInstance(configuration);
+ } catch (NoSuchMethodException excp) {
+ // use the default constructor
+ ret = (Supplier<String>)
clz.getDeclaredConstructor().newInstance();
+ }
+ } catch (ReflectiveOperationException excp) {
+ throw new IllegalArgumentException("Failed to instantiate
token supplier: " + clzName, excp);
+ }
Review Comment:
Reflective construction currently uses
`getDeclaredConstructor(...).newInstance(...)` without setting accessibility.
If the supplier class has a non-public constructor, instantiation will fail
with an access error. Consider either (1) using `getConstructor(...)` /
`getConstructor()` to require public constructors (clearer contract), or (2)
calling `setAccessible(true)` on the `Constructor` before instantiation if
non-public constructors are intended to be supported.
##########
client/common/src/main/java/org/apache/atlas/token/retriever/JwTokenRetrieverDefault.java:
##########
@@ -66,21 +67,33 @@ public JwTokenRetrieverDefault(Configuration config) {
}
@Override
- public synchronized Optional<String> retrieve() {
+ public synchronized String get() {
String source = StringUtils.lowerCase(jwtSource);
+
+ final Optional<String> ret;
+
switch (source) {
case SOURCE_ENV:
- return getJwtFromEnv();
+ ret = getJwtFromEnv();
+ break;
+
case SOURCE_FILE:
- return getJwtFromFile();
+ ret = getJwtFromFile();
+ break;
+
case SOURCE_CRED:
- return getJwtFromCredProvider();
+ ret = getJwtFromCredProvider();
+ break;
+
default:
if (StringUtils.isNotBlank(source)) {
LOG.warn("JwTokenRetrieverDefault.retrieve(): unsupported
source='{}'", source);
}
- return Optional.empty();
+
+ ret = Optional.empty();
}
+
+ return ret.orElse(null);
Review Comment:
`Supplier#get()` now returns `null` when no token is available. While
`Supplier` technically allows `null`, it often surprises callers and can lead
to NPEs if future usage changes. Prefer returning an empty string (and treat it
as absent), or consider using a more explicit type (e.g.,
`Supplier<Optional<String>>`) internally to represent absence.
##########
client/common/src/main/java/org/apache/atlas/token/retriever/JwTokenRetrieverDefault.java:
##########
@@ -66,21 +67,33 @@ public JwTokenRetrieverDefault(Configuration config) {
}
@Override
- public synchronized Optional<String> retrieve() {
+ public synchronized String get() {
String source = StringUtils.lowerCase(jwtSource);
+
+ final Optional<String> ret;
+
switch (source) {
case SOURCE_ENV:
- return getJwtFromEnv();
+ ret = getJwtFromEnv();
+ break;
+
case SOURCE_FILE:
- return getJwtFromFile();
+ ret = getJwtFromFile();
+ break;
+
case SOURCE_CRED:
- return getJwtFromCredProvider();
+ ret = getJwtFromCredProvider();
+ break;
+
default:
if (StringUtils.isNotBlank(source)) {
LOG.warn("JwTokenRetrieverDefault.retrieve(): unsupported
source='{}'", source);
Review Comment:
This log message still mentions `retrieve()` after the method was changed to
`get()`. Update it to avoid confusion when correlating logs with code.
##########
client/common/src/main/java/org/apache/atlas/AtlasBaseClient.java:
##########
@@ -573,31 +583,71 @@ void initializeState(Configuration configuration,
String[] baseUrls, UserGroupIn
service =
client.resource(UriBuilder.fromUri(activeServiceUrl).build());
}
- private TokenRetriever<String> getJwtTokenRetriever(Configuration
configuration) {
- return new JwTokenRetrieverDefault(configuration);
+ private boolean isJwtConfigured(Configuration configuration) {
+ boolean ret = false;
+
+ if (configuration != null) {
+ String tokenSupplier =
configuration.getString(PROP_REST_AUTH_TOKEN_SUPPLIER);
+
+ ret = StringUtils.isNotBlank(tokenSupplier);
+
+ if (!ret) {
+ String jwtSource = configuration.getString(JWT_SOURCE, "");
+
+ ret = StringUtils.isNotBlank(jwtSource);
+ }
+ }
+
+ return ret;
}
- private boolean isJwtSourceConfigured(Configuration configuration) {
- if (configuration == null) {
- return false;
+ private Supplier<String> getTokenSupplier(Configuration configuration) {
+ Supplier<String> ret = null;
+
+ if (configuration != null) {
+ String clzName =
configuration.getString(PROP_REST_AUTH_TOKEN_SUPPLIER);
+
+ if (StringUtils.isBlank(clzName)) {
+ clzName = DEFAULT_REST_AUTH_TOKEN_SUPPLIER;
+ }
+
+ try {
+ Class<?> clz = Class.forName(clzName);
+
+ if (!Supplier.class.isAssignableFrom(clz)) {
+ throw new IllegalArgumentException(clzName + " does not
implement Supplier");
+ }
+
+ try {
+ // look for a constructor taking Configuration as its
argument
+ Constructor<?> c =
clz.getDeclaredConstructor(Configuration.class);
+
+ ret = (Supplier<String>) c.newInstance(configuration);
+ } catch (NoSuchMethodException excp) {
+ // use the default constructor
+ ret = (Supplier<String>)
clz.getDeclaredConstructor().newInstance();
+ }
+ } catch (ReflectiveOperationException excp) {
+ throw new IllegalArgumentException("Failed to instantiate
token supplier: " + clzName, excp);
+ }
}
- String jwtSource = configuration.getString(JWT_SOURCE, "");
- return StringUtils.isNotBlank(jwtSource);
+ return ret;
}
private void handleJwt(com.sun.jersey.api.client.WebResource.Builder
requestBuilder) {
if (!useJwtAuth) {
return;
}
- if (tokenRetriever == null) {
+ if (tokenSupplier == null) {
LOG.warn("AtlasBaseClient.handleJwt(): tokenRetriever is null.
Skipping JWT header injection.");
return;
}
Review Comment:
The warning message refers to `tokenRetriever`, but the code now uses
`tokenSupplier`. This makes logs misleading during troubleshooting. Update the
message to reference `tokenSupplier`.
--
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]
