danielcweeks commented on code in PR #13741:
URL: https://github.com/apache/iceberg/pull/13741#discussion_r2267409898
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -273,33 +273,32 @@ protected OAuth2Util.AuthSession
newSessionFromTokenResponse(
}
private static void warnIfDeprecatedTokenEndpointUsed(Map<String, String>
properties) {
- if (usesDeprecatedTokenEndpoint(properties)) {
+ if (!properties.containsKey(OAuth2Properties.OAUTH2_SERVER_URI)) {
String credential = properties.get(OAuth2Properties.CREDENTIAL);
String initToken = properties.get(OAuth2Properties.TOKEN);
boolean hasCredential = credential != null && !credential.isEmpty();
boolean hasInitToken = initToken != null;
if (hasInitToken || hasCredential) {
LOG.warn(
"Iceberg REST client is missing the OAuth2 server URI
configuration and defaults to {}/{}. "
- + "This automatic fallback will be removed in a future Iceberg
release."
+ + "This automatic fallback will be removed in a future Iceberg
release. "
+ "It is recommended to configure the OAuth2 endpoint using
the '{}' property to be prepared. "
+ "This warning will disappear if the OAuth2 endpoint is
explicitly configured. "
+ "See https://github.com/apache/iceberg/issues/10537";,
RESTUtil.stripTrailingSlash(properties.get(CatalogProperties.URI)),
ResourcePaths.tokens(),
OAuth2Properties.OAUTH2_SERVER_URI);
}
- }
- }
-
- private static boolean usesDeprecatedTokenEndpoint(Map<String, String>
properties) {
- if (properties.containsKey(OAuth2Properties.OAUTH2_SERVER_URI)) {
+ } else {
String oauth2ServerUri =
properties.get(OAuth2Properties.OAUTH2_SERVER_URI);
- boolean relativePath = !oauth2ServerUri.startsWith("http");
- boolean sameHost =
oauth2ServerUri.startsWith(properties.get(CatalogProperties.URI));
- return relativePath || sameHost;
+ if (!oauth2ServerUri.startsWith("http") &&
oauth2ServerUri.endsWith(ResourcePaths.tokens())) {
Review Comment:
I don't think we should include this check. This implys that having an
endpoint with `v1/oauth/tokens` is incorrect, which is not the case. We're
removing the endpoint from the spec definition, but it's still a fully valid
endpoint and relative should be fine as well. Requiring the explicit URI is
good because it's clear and explicit what is being used for auth, but beyond
that we don't need to be opinionated.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
