adutra closed pull request #10256: REST: honor OAuth config sent by the server
URL: https://github.com/apache/iceberg/pull/10256
--
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.
adutra commented on PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#issuecomment-2114629113
Hi @rdblue, thank you for your detailed answer.
I am really sorry that this PR, that I thought would be a fairly consensual
one, eventually cracked open a can of worms that I did
rdblue commented on PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#issuecomment-2113392645
@adutra, thanks for noticing and pointing out the inconsistencies. I do
think it's important to address those. In particular, I did not realize that
the table session that was introduced
danielcweeks commented on PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#issuecomment-2113348511
> Why should a (malicious) Iceberg REST endpoint do the more complex
redirect-dance, if it can get the nearly clear-text credentials due to the
`/v1/oauth/tokens` route introduced
snazy commented on PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#issuecomment-2113057306
> This change allows redirecting the auth server which should expose
sensitive information to the wrong party.
Why should a (malicious) Iceberg REST endpoint do the more complex
re
danielcweeks commented on PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#issuecomment-2112854713
> My intention was to bring the spec in
line with the current implementation by honoring credential and
oauth2-server-uri, among others, from the config
endpoint.
I don
adutra commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1601352324
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths =
adutra commented on PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#issuecomment-2112020920
@rdblue I have no other intention than the one of bringing the spec in line
with the current implementation.
There are two areas where spec and implementation differ with respect t
nastra commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1601172497
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths =
rdblue commented on PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#issuecomment-2111327877
I'm surprised by this PR because I don't think that the auth properties
should be overridden by a REST service. I'm not sure about it, but it sounds
like @snazy seems to agree when he sa
adutra commented on PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#issuecomment-2111059746
Not a fan of leaving `oauthServerUri` out of the scope of this PR, for the
reasons @snazy explained.
Also what about `optionalOAuthParams`? For now it contains only resource and
a
snazy commented on PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#issuecomment-2110994585
> If I may, I would propose to:
>
> 1. the current impl should be improved, I think @adutra change is OK
if we add scope configurable to deal with server side push
>
>
jbonofre commented on PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#issuecomment-2110987236
Maybe the confusion is due to the `config` endpoint is mixing different
semantic properties, also related to the `scope`.
For clarity, it would be better to split non authenticated c
snazy commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1600512130
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths =
snazy commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1600501374
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths =
danielcweeks commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1600446185
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.p
danielcweeks commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1600407349
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.p
snazy commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1600334750
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths =
snazy commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1600325406
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths =
danielcweeks commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1600255202
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.p
flyrain commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1591603492
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths
flyrain commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1591587764
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths
adutra commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1590651661
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths =
adutra commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1590646919
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths =
flyrain commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1590182083
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths
flyrain commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1590181761
##
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##
@@ -215,6 +215,12 @@ public void initialize(String name, Map
unresolved) {
this.paths
adutra opened a new pull request, #10256:
URL: https://github.com/apache/iceberg/pull/10256
As mandated by the spec, in case the config endpoint overrides any
auth-specific property, the overridden values should be taken into account when
creating the catalog auth session instance.
--
Th
27 matches
Mail list logo
