You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2025/08/01 13:11:14 UTC
[PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
adutra opened a new pull request, #13718:
URL: https://github.com/apache/iceberg/pull/13718
#12197 accidentally changed the other of additional params inclusion in the S3 signer properties.
This creates a regression when users have a custom scope, since custom scopes are included in the additional params. Such custom scopes are not being included anymore.
This changes fixes this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #13718:
URL: https://github.com/apache/iceberg/pull/13718
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13718:
URL: https://github.com/apache/iceberg/pull/13718#discussion_r2251171168
##########
aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3V4RestSignerClient.java:
##########
@@ -133,6 +150,18 @@ public static Stream<Arguments> validOAuth2Properties() {
"token",
OAuth2Properties.CREDENTIAL,
"user:12345"),
+ "token"),
+ // Custom scope
+ Arguments.of(
+ Map.of(
+ S3_SIGNER_URI,
+ "https://signer.com",
+ AuthProperties.AUTH_TYPE,
+ AuthProperties.AUTH_TYPE_OAUTH2,
+ OAuth2Properties.CREDENTIAL,
+ "user:12345",
+ OAuth2Properties.SCOPE,
+ "custom"),
Review Comment:
can we have a test that verifies that the default scope is `sign` when no custom scope is defined?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #13718:
URL: https://github.com/apache/iceberg/pull/13718#discussion_r2248054934
##########
aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3V4RestSignerClient.java:
##########
@@ -59,7 +59,24 @@ static void beforeAll() {
"client_secret",
"12345",
"scope",
- "sign")),
+ "catalog")),
Review Comment:
I manually validated this against pre-AuthManager code: the scope is in fact never `sign`. It's
`catalog` by default, unless overridden by the user.
This happens because there is _always_ a scope included in `optionalOAuthParams()`:
https://github.com/apache/iceberg/blob/67e181ea9b706afc8ff9d1bcd0f9e9c240f173cd/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java#L140-L142
This test is new and was wrong when I created it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #13718:
URL: https://github.com/apache/iceberg/pull/13718#discussion_r2248432549
##########
aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3V4RestSignerClient.java:
##########
@@ -59,7 +59,24 @@ static void beforeAll() {
"client_secret",
"12345",
"scope",
- "sign")),
+ "catalog")),
Review Comment:
I can confirm that the scope is never `sign` in 1.8.0 and 1.9.0, unless the user explicitly sets the scope to `sign`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #13718:
URL: https://github.com/apache/iceberg/pull/13718#discussion_r2248449305
##########
aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3V4RestSignerClient.java:
##########
@@ -59,7 +59,24 @@ static void beforeAll() {
"client_secret",
"12345",
"scope",
- "sign")),
+ "catalog")),
Review Comment:
Looking at #9839, it seems indeed that that PR introduced a regression. I will fix.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #13718:
URL: https://github.com/apache/iceberg/pull/13718#discussion_r2248457552
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -166,10 +166,10 @@ AuthSession authSession() {
ImmutableMap.Builder<String, String> properties =
ImmutableMap.<String, String>builder()
.putAll(properties())
- .putAll(optionalOAuthParams())
.put(OAuth2Properties.OAUTH2_SERVER_URI, oauth2ServerUri())
.put(OAuth2Properties.TOKEN_REFRESH_ENABLED, String.valueOf(keepTokenRefreshed()))
- .put(OAuth2Properties.SCOPE, SCOPE);
+ .put(OAuth2Properties.SCOPE, SCOPE)
Review Comment:
The call to `.put(OAuth2Properties.SCOPE, SCOPE)` here is imo useless since the scope will be overridden by `.putAll(optionalOAuthParams())` – but it doesn't hurt to keep it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13718:
URL: https://github.com/apache/iceberg/pull/13718#discussion_r2248296463
##########
aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3V4RestSignerClient.java:
##########
@@ -59,7 +59,24 @@ static void beforeAll() {
"client_secret",
"12345",
"scope",
- "sign")),
+ "catalog")),
Review Comment:
so when is the scope actually set to `sign` for the S3 signer then? Maybe this was already a regression with https://github.com/apache/iceberg/pull/9839
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13718:
URL: https://github.com/apache/iceberg/pull/13718#discussion_r2248296463
##########
aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3V4RestSignerClient.java:
##########
@@ -59,7 +59,24 @@ static void beforeAll() {
"client_secret",
"12345",
"scope",
- "sign")),
+ "catalog")),
Review Comment:
so when is the scope actually set to `sign` for the S3 signer then? Maybe this was already a regression with https://github.com/apache/iceberg/pull/9839, which we should fix. It seems a bit weird that this always defaults to the `catalog` scope even though the signer has its own default scope
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #13718:
URL: https://github.com/apache/iceberg/pull/13718#issuecomment-3144553183
@nastra FYI
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix OAuth2 additional params inclusion [iceberg]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on PR #13718:
URL: https://github.com/apache/iceberg/pull/13718#issuecomment-3144891646
@adutra thanks for the fix and @stevenzwu thanks for including it in the 1.10.0 milestone. I also tested the code manually and verified that the singer now picks up user specified scopes again.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org