dimas-b commented on code in PR #3170:
URL: https://github.com/apache/polaris/pull/3170#discussion_r2572202402
##########
runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java:
##########
@@ -88,7 +91,8 @@ public PolarisStorageIntegrationProviderImpl(
new AwsCredentialsStorageIntegration(
(AwsStorageConfigurationInfo)
polarisStorageConfigurationInfo,
stsClientProvider,
- stsCredentials);
+ stsCredentials,
+ context);
Review Comment:
This "provider" is `@ApplicationScoped`, but `SecurityContext` is
conceptually request-scoped. `AwsCredentialsStorageIntegration` is accessed via
`StorageCredentialCache`.
Current setup creates a logical inconsistency: the cache may return data
from memory, while the request-scoped context contains a different token. The
cache does not "see" this context.
I believe it would be preferably to carry the token through the method
parameter so that the cache can take it into account when it computes its key.
This will increase cache churn, of course, but it's a matter of correctness at
this point, not optimization (I think).
##########
site/content/in-dev/unreleased/command-line-interface.md:
##########
@@ -151,6 +151,7 @@ options:
--role-arn (Only for AWS S3) A role ARN to use when connecting to S3
--region (Only for S3) The region to use when connecting to S3
--external-id (Only for S3) The external ID to use when connecting to S3
+ --propagate-api-user-identity (Only for S3) Whether to use the
requesting users identity when connecting to the STS
Review Comment:
Does this belong in the CLI PR? 🤔
##########
runtime/service/build.gradle.kts:
##########
@@ -103,7 +102,7 @@ dependencies {
implementation("com.fasterxml.jackson.core:jackson-annotations")
implementation("com.fasterxml.jackson.core:jackson-core")
implementation("com.fasterxml.jackson.core:jackson-databind")
-
+ implementation(libs.apache.httpclient5)
Review Comment:
Is this required for the new Assume Role With Web Identity call?
##########
.gitignore:
##########
@@ -105,4 +105,5 @@ hs_err_pid*
.vscode/
# Python Virtual Env
-.venv
\ No newline at end of file
+.venv
+venv
Review Comment:
I agree with @cccs-cat001 on this topic, but I'd propose to make this change
in a separate PR to avoid mixing QOL changes with feature changes 😉
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java:
##########
@@ -74,4 +106,8 @@ static PolarisPrincipal of(String name, Map<String, String>
properties, Set<Stri
* as permissions, preferences, or other metadata.
*/
Map<String, String> getProperties();
+
+ /** Returns the access token of the current user. */
+ @Nullable
+ String getToken();
Review Comment:
nit: WDYT about `Optional<String>` here?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
