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 2024/05/17 10:14:40 UTC
[PR] REST: disallow overriding "credential" in table sessions [iceberg]
adutra opened a new pull request, #10345:
URL: https://github.com/apache/iceberg/pull/10345
See #10256 for context.
As requested, this change disallows overriding the `credential` property in table sessions, by introducing an allow-list of auth-related properties that can be overridden in such situations.
Only the `token` property and properties used to exchange one token for another (`urn:ietf:params:oauth:token-type:*`) are now allowed.
--
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] REST: disallow overriding "credential" in table sessions [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #10345:
URL: https://github.com/apache/iceberg/pull/10345#discussion_r1605267440
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -915,7 +923,13 @@ private FileIO tableFileIO(SessionContext context, Map<String, String> config) {
}
private AuthSession tableSession(Map<String, String> tableConf, AuthSession parent) {
- Pair<String, Supplier<AuthSession>> newSession = newSession(tableConf, tableConf, parent);
+ Map<String, String> credentials = Maps.newHashMapWithExpectedSize(tableConf.size());
Review Comment:
Note: this change still allows overriding `credential` from the `SessionContext` which I believe is the desired behavior. But I'm happy to also apply the restriction to `SessionContext` as well.
--
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] REST: disallow overriding "credential" in table sessions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10345:
URL: https://github.com/apache/iceberg/pull/10345#discussion_r1625998903
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -915,7 +923,13 @@ private FileIO tableFileIO(SessionContext context, Map<String, String> config) {
}
private AuthSession tableSession(Map<String, String> tableConf, AuthSession parent) {
- Pair<String, Supplier<AuthSession>> newSession = newSession(tableConf, tableConf, parent);
+ Map<String, String> credentials = Maps.newHashMapWithExpectedSize(tableConf.size());
+ for (String prop : tableConf.keySet()) {
+ if (TABLE_SESSION_ALLOW_LIST.contains(prop)) {
+ credentials.put(prop, tableConf.get(prop));
+ }
+ }
Review Comment:
nit: newline after the closing }
--
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] REST: disallow overriding "credential" in table sessions [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #10345:
URL: https://github.com/apache/iceberg/pull/10345#issuecomment-2147204756
@nastra could you have a look at this one as well please?
--
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] REST: disallow overriding "credential" in table sessions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #10345:
URL: https://github.com/apache/iceberg/pull/10345
--
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