adutra commented on code in PR #15999:
URL: https://github.com/apache/iceberg/pull/15999#discussion_r3102061956
##########
core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Util.java:
##########
@@ -135,4 +138,113 @@ public void testCredentialFlowForSessionRefresh() throws
IOException {
any());
}
}
+
+ @Test
+ void fromTokenResponseUsesChildTokenExpiry() {
+ // parent token expires at epoch second 7200 (= 7200000 ms)
+ String parentToken = tokenWithExp(7200);
+ // child token expires at epoch second 300 (= 300000 ms)
+ String childToken = tokenWithExp(300);
+
+ AuthConfig parentConfig =
+ AuthConfig.builder()
+ .token(parentToken)
+ .tokenType(OAuth2Properties.ACCESS_TOKEN_TYPE)
+ .keepRefreshed(false)
+ .build();
+ OAuth2Util.AuthSession parent = new OAuth2Util.AuthSession(Map.of(),
parentConfig);
+
assertThat(parent.expiresAtMillis()).isEqualTo(TimeUnit.SECONDS.toMillis(7200));
+
+ OAuthTokenResponse response =
+ OAuthTokenResponse.builder()
+ .withToken(childToken)
+ .withTokenType(BEARER)
+ .withIssuedTokenType(OAuth2Properties.ACCESS_TOKEN_TYPE)
+ .setExpirationInSeconds(300)
+ .build();
+
+ // pass null executor to skip scheduling
+ OAuth2Util.AuthSession child =
+ OAuth2Util.AuthSession.fromTokenResponse(
+ null, null, response, System.currentTimeMillis(), parent);
+ assertThat(child.expiresAtMillis())
+ .as("Child session should use the child token's exp, not the parent's")
+ .isEqualTo(TimeUnit.SECONDS.toMillis(300));
+ }
+
+ @Test
+ void fromTokenResponseOpaqueTokenDoesNotInheritParentExpiry() {
+ // parent token with JWT exp claim
+ String parentToken = tokenWithExp(7200);
+ // child token with no exp claim (opaque)
+ String childToken = "opaque-access-token";
+
+ AuthConfig parentConfig =
+ AuthConfig.builder()
+ .token(parentToken)
+ .tokenType(OAuth2Properties.ACCESS_TOKEN_TYPE)
+ .keepRefreshed(false)
+ .build();
+ OAuth2Util.AuthSession parent = new OAuth2Util.AuthSession(Map.of(),
parentConfig);
+
assertThat(parent.expiresAtMillis()).isEqualTo(TimeUnit.SECONDS.toMillis(7200));
+
+ OAuthTokenResponse response =
+ OAuthTokenResponse.builder()
+ .withToken(childToken)
+ .withTokenType(BEARER)
+ .withIssuedTokenType(OAuth2Properties.ACCESS_TOKEN_TYPE)
+ .setExpirationInSeconds(600)
+ .build();
+
+ OAuth2Util.AuthSession child =
+ OAuth2Util.AuthSession.fromTokenResponse(
+ null, null, response, System.currentTimeMillis(), parent);
+
+ // child token is opaque (no exp claim), so expiresAtMillis should be null,
+ // NOT inherited from parent. The response's expiresInSeconds is used
+ // only for scheduling, not stored in the session config.
+ assertThat(child.expiresAtMillis())
+ .as("Child session with opaque token should not inherit parent's
expiresAtMillis")
+ .isNull();
+ }
+
+ @Test
+ void fromAccessTokenUsesChildTokenExpiry() {
+ // parent token expires at epoch second 7200
+ String parentToken = tokenWithExp(7200);
+ // child token expires at epoch second 300
+ String childToken = tokenWithExp(300);
+
+ AuthConfig parentConfig =
+ AuthConfig.builder()
+ .token(parentToken)
+ .tokenType(OAuth2Properties.ACCESS_TOKEN_TYPE)
+ .keepRefreshed(false)
+ .build();
+ OAuth2Util.AuthSession parent = new OAuth2Util.AuthSession(Map.of(),
parentConfig);
+
assertThat(parent.expiresAtMillis()).isEqualTo(TimeUnit.SECONDS.toMillis(7200));
+
+ // pass null executor to skip scheduling
+ OAuth2Util.AuthSession child =
+ OAuth2Util.AuthSession.fromAccessToken(null, null, childToken, null,
parent);
+ assertThat(child.expiresAtMillis())
+ .as("Child session should use the child token's exp, not the parent's")
+ .isEqualTo(TimeUnit.SECONDS.toMillis(300));
+ }
+
+ /**
+ * Creates a JWT token with the given expiration time in seconds since
epoch. The token is not
+ * cryptographically signed but is structurally valid for {@link
OAuth2Util#expiresAtMillis}
+ * parsing.
+ */
+ private static String tokenWithExp(long expSeconds) {
+ String header = "{\"alg\":\"none\",\"typ\":\"JWT\"}";
+ String payload = "{\"sub\":\"test\",\"exp\":" + expSeconds + "}";
+ Base64.Encoder encoder = Base64.getUrlEncoder().withoutPadding();
+ return encoder.encodeToString(header.getBytes(StandardCharsets.UTF_8))
+ + "."
+ + encoder.encodeToString(payload.getBytes(StandardCharsets.UTF_8))
+ + "."
+ + encoder.encodeToString("fakesig".getBytes(StandardCharsets.UTF_8));
+ }
Review Comment:
Can be replaced with Nimbus Jose (which comes transitively through
MockServer and other deps):
```suggestion
private static String tokenWithExp(long expSeconds) {
JWTClaimsSet claimsSet =
new JWTClaimsSet.Builder().subject("test").claim("exp",
expSeconds).build();
return new PlainJWT(claimsSet).serialize();
}
```
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -529,6 +529,7 @@ public Pair<Integer, TimeUnit> refresh(RESTClient client) {
.from(config())
.token(response.token())
.tokenType(response.issuedTokenType())
+ .expiresAtMillis(OAuth2Util.expiresAtMillis(response.token()))
Review Comment:
This change is good imho, but untested – it would be good to have it tested,
although I agree it's not easy to test.
--
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]
