collado-mike commented on code in PR #511:
URL: https://github.com/apache/polaris/pull/511#discussion_r1876569716
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java:
##########
@@ -100,4 +113,34 @@ public PolarisPrincipalSecrets getPrincipalSecrets() {
return principalSecrets;
}
}
+
+ /** the result of load/rotate principal secrets */
+ class SecretValidationResult extends BaseResult {
+
+ private final PolarisBaseEntity principal;
+
+ public SecretValidationResult(
+ @Nonnull BaseResult.ReturnStatus errorCode, @Nullable String
extraInformation) {
+ super(errorCode, extraInformation);
+ this.principal = null;
+ }
+
+ public SecretValidationResult(@Nonnull PolarisBaseEntity principal) {
+ super(BaseResult.ReturnStatus.SUCCESS);
+ this.principal = principal;
+ }
+
+ @JsonCreator
+ private SecretValidationResult(
+ @JsonProperty("returnStatus") @Nonnull BaseResult.ReturnStatus
returnStatus,
+ @JsonProperty("extraInformation") @Nullable String extraInformation,
+ @JsonProperty("principalSecrets") @Nonnull PolarisBaseEntity
principal) {
+ super(returnStatus, extraInformation);
+ this.principal = principal;
+ }
+
+ public PolarisBaseEntity getPrincipal() {
+ return principal;
+ }
Review Comment:
Same comment on returning the principal from the secret validation command.
I can imagine storing extra metadata, such as the principal id with the client
id / secret, but the entity itself should be returned from the persistence
layer.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java:
##########
@@ -39,6 +41,17 @@ public interface PolarisSecretsManager {
PrincipalSecretsResult loadPrincipalSecrets(
@Nonnull PolarisCallContext callCtx, @Nonnull String clientId);
+ @Nonnull
+ SecretValidationResult validateSecret(
+ @Nonnull PolarisCallContext callCtx, @Nonnull String clientId, @Nonnull
String clientSecret);
+
+ @Nonnull
+ EntityResult loadPrincipal(
+ @Nonnull PolarisCallContext callCtx,
+ @Nullable String roleName,
+ @Nullable String clientId,
+ @Nullable Long principalId);
Review Comment:
Loading the principal entity should be the purview of the persistence layer,
not the secrets manager. IMO, the secrets manager should be something like
Vault or AWS Secrets manager that knows nothing about Polaris entities, but
does know how to store client id / secret
--
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]
