You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2023/01/18 10:46:30 UTC
[GitHub] [iceberg] nastra opened a new pull request, #6616: Core: Allow customizing OAuth scope
nastra opened a new pull request, #6616:
URL: https://github.com/apache/iceberg/pull/6616
In #6169 we're planning to re-use `AuthSession` but we need the ability to use a `scope`, which is different from the default `OAuth2Properties.CATALOG_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
[GitHub] [iceberg] rdblue merged pull request #6616: Core: Allow customizing OAuth scope
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #6616:
URL: https://github.com/apache/iceberg/pull/6616
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #6616: Core: Allow customizing OAuth scope
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6616:
URL: https://github.com/apache/iceberg/pull/6616#discussion_r1084268737
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -622,7 +636,11 @@ private static AuthSession fromTokenResponse(
String credential) {
AuthSession session =
new AuthSession(
- parent.headers(), response.token(), response.issuedTokenType(), credential);
+ parent.headers(),
+ response.token(),
+ response.issuedTokenType(),
+ credential,
+ parent.scope());
Review Comment:
I've simplified this to actually and removed the `scope` parameter from `fromTokenExchange`/`fromCredential` as I think we need to take the `scope` from the `parent` in those cases
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #6616: Core: Allow customizing OAuth scope
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #6616:
URL: https://github.com/apache/iceberg/pull/6616#discussion_r1084233763
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java:
##########
@@ -622,7 +636,11 @@ private static AuthSession fromTokenResponse(
String credential) {
AuthSession session =
new AuthSession(
- parent.headers(), response.token(), response.issuedTokenType(), credential);
+ parent.headers(),
+ response.token(),
+ response.issuedTokenType(),
+ credential,
+ parent.scope());
Review Comment:
When this is called in `fromTokenExchange`, the scope passed there is not passed in here. I think that this method needs to have a `scope` argument. Then the `fromTokenResponse` above can pass `parent.scope()` and `fromTokenExchange` can pass the scope it was passed.
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #6616: Core: Allow customizing OAuth scope
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6616:
URL: https://github.com/apache/iceberg/pull/6616#discussion_r1080918193
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -918,8 +919,16 @@ public void testCatalogTokenRefresh() throws Exception {
UUID.randomUUID().toString(), "user", contextCredentials, ImmutableMap.of());
RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter);
+ String scope = "custom_catalog_scope";
catalog.initialize(
- "prod", ImmutableMap.of(CatalogProperties.URI, "ignored", "credential", "catalog:secret"));
+ "prod",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ "credential",
+ "catalog:secret",
+ OAuth2Properties.SCOPE,
+ scope));
Review Comment:
done
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #6616: Core: Allow customizing OAuth scope
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6616:
URL: https://github.com/apache/iceberg/pull/6616#discussion_r1080917919
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -134,14 +134,12 @@ public void initialize(String name, Map<String, String> unresolved) {
ConfigResponse config;
OAuthTokenResponse authResponse;
String credential = props.get(OAuth2Properties.CREDENTIAL);
- // TODO: if scope can be overridden, it should be done consistently
+ String scope = props.getOrDefault(OAuth2Properties.SCOPE, OAuth2Properties.CATALOG_SCOPE);
Review Comment:
thanks for catching this, not sure how I missed that.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #6616: Core: Allow customizing OAuth scope
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6616:
URL: https://github.com/apache/iceberg/pull/6616#discussion_r1073800894
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -918,8 +919,16 @@ public void testCatalogTokenRefresh() throws Exception {
UUID.randomUUID().toString(), "user", contextCredentials, ImmutableMap.of());
RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter);
+ String scope = "custom_catalog_scope";
catalog.initialize(
- "prod", ImmutableMap.of(CatalogProperties.URI, "ignored", "credential", "catalog:secret"));
+ "prod",
+ ImmutableMap.of(
+ CatalogProperties.URI,
+ "ignored",
+ "credential",
+ "catalog:secret",
+ OAuth2Properties.SCOPE,
+ scope));
Review Comment:
Can you add a separate test case rather than repurposing an existing one?
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #6616: Core: Allow customizing OAuth scope
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6616:
URL: https://github.com/apache/iceberg/pull/6616#discussion_r1073799118
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -134,14 +134,12 @@ public void initialize(String name, Map<String, String> unresolved) {
ConfigResponse config;
OAuthTokenResponse authResponse;
String credential = props.get(OAuth2Properties.CREDENTIAL);
- // TODO: if scope can be overridden, it should be done consistently
+ String scope = props.getOrDefault(OAuth2Properties.SCOPE, OAuth2Properties.CATALOG_SCOPE);
Review Comment:
Looks like scope was missed here: https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L761-L796
--
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