AWS: Fix OAuth2 additional params inclusion #13718
Conversation
apache#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. Compare before: https://github.com/apache/iceberg/blame/57ec405a651b99d5fce3f3b4bec217d24bc98d20/aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java#L221-L227 With after: https://github.com/apache/iceberg/blame/071d5606bc6199a0be9b3f274ec7fbf111d88821/aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java#L163-L166
|
@nastra FYI |
| @@ -59,7 +59,24 @@ static void beforeAll() { | |||
| "client_secret", | |||
| "12345", | |||
| "scope", | |||
| "sign")), | |||
| "catalog")), | |||
There was a problem hiding this 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():
iceberg/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java
Lines 140 to 142 in 67e181e
This test is new and was wrong when I created it.
There was a problem hiding this comment.
so when is the scope actually set to sign for the S3 signer then? Maybe this was already a regression with #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
There was a problem hiding this 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.
There was a problem hiding this comment.
Looking at #9839, it seems indeed that that PR introduced a regression. I will fix.
|
@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. |
| .put(OAuth2Properties.OAUTH2_SERVER_URI, oauth2ServerUri()) | ||
| .put(OAuth2Properties.TOKEN_REFRESH_ENABLED, String.valueOf(keepTokenRefreshed())) | ||
| .put(OAuth2Properties.SCOPE, SCOPE); | ||
| .put(OAuth2Properties.SCOPE, SCOPE) |
There was a problem hiding this 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.
#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 change fixes this.