dimas-b commented on code in PR #2197:
URL: https://github.com/apache/polaris/pull/2197#discussion_r2286458959
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1135,18 +1136,83 @@ public void deletePrincipal(String name) {
newSecrets.getPrincipalClientId(), newSecrets.getMainSecret()));
}
+ private @Nonnull PrincipalWithCredentials resetCredentialsHelper(
+ String principalName, String customClientId, String customClientSecret) {
+ PrincipalEntity currentPrincipalEntity =
+ findPrincipalByName(principalName)
+ .orElseThrow(() -> new NotFoundException("Principal %s not found",
principalName));
+
+ if (FederatedEntities.isFederated(currentPrincipalEntity)) {
+ throw new ValidationException(
+ "Cannot reset credentials for a federated principal: %s",
principalName);
+ }
+ PolarisPrincipalSecrets currentSecrets =
+ metaStoreManager
+ .loadPrincipalSecrets(getCurrentPolarisContext(),
currentPrincipalEntity.getClientId())
+ .getPrincipalSecrets();
+ if (currentSecrets == null) {
+ throw new IllegalArgumentException(
+ String.format("Failed to load current secrets for principal '%s'",
principalName));
+ }
+ PolarisPrincipalSecrets newSecrets =
+ metaStoreManager
+ .resetPrincipalSecrets(
+ getCurrentPolarisContext(),
+ currentPrincipalEntity.getClientId(),
Review Comment:
`testCreatePrincipalAndResetCredentialsWithCustomValues()` only validates
that the client ID in _credentials_ got reset properly, but it does not
validate that the client ID in the principal is in sync with that... or did I
miss it? :thinking:
##########
spec/polaris-management-service.yml:
##########
@@ -1248,6 +1283,20 @@ components:
- currentEntityVersion
- properties
+ ResetPrincipalRequest:
+ type: object
+ properties:
+ clientId:
+ type: string
+ description: >
+ Optional client ID to set for the principal.
+ Must be a valid client ID previously generated by this service.
+ clientSecret:
+ type: string
+ description: >
+ Optional client secret to set for the principal.
+ Must match the secret issued for the given client ID.
Review Comment:
note sure about the "must match" part... How about "Polaris service
implementations may impose extra requirements on what is accepted as a secret
(special chars, length, etc.)".
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -773,6 +773,73 @@ public PolarisPrincipalSecrets generateNewPrincipalSecrets(
return principalSecrets;
}
+ @Nullable
+ @Override
+ public PolarisPrincipalSecrets resetPrincipalSecrets(
+ @Nonnull PolarisCallContext callCtx,
+ @Nonnull String clientId,
+ long principalId,
+ String customClientId,
+ String customClientSecret,
+ boolean customReset) {
+ PolarisPrincipalSecrets principalSecrets = loadPrincipalSecrets(callCtx,
clientId);
+
+ // should be found
+ callCtx
+ .getDiagServices()
+ .checkNotNull(
+ principalSecrets,
+ "cannot_find_secrets",
+ "client_id={} principalId={}",
+ clientId,
+ principalId);
+
+ // ensure principal id is matching
+ callCtx
+ .getDiagServices()
+ .check(
+ principalId == principalSecrets.getPrincipalId(),
+ "principal_id_mismatch",
+ "expectedId={} id={}",
+ principalId,
+ principalSecrets.getPrincipalId());
+
+ if (customReset) {
+ principalSecrets =
+ new PolarisPrincipalSecrets(
+ principalSecrets.getPrincipalId(), customClientId,
customClientSecret, null);
+ } else {
+ principalSecrets.rotateSecrets(principalSecrets.getMainSecretHash());
Review Comment:
I'm fine with generating and returning random credentials from the reset
API. However, the API is still a bit confusing because in
`ResetPrincipalRequest` either `clientId` and `clientSecret` may be set without
the other, but in code (line 806) execution changes only when both are set. In
effect, invoking `resetCredentials` only with `clientSecret` still generates a
random secret, if I'm not mistaken.
Secondary point: calling `principalSecrets.rotateSecrets()` on the "reset"
call path is still awkward since credential "rotation" may have very specific
logic in `PolarisPrincipalSecrets`.
I propose to add a new method `PolarisPrincipalSecrets.resetSecrets(String
clientId, String secret)` returning a new `PolarisPrincipalSecrets`. The
handling of `null` values in `clientId` and `secret` will be inside that
`resetSecrets()` method. It will keep `this` immutable and return a new
`PolarisPrincipalSecrets` object possibly reusing values from the old one if
they did not get reset. WDYT?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java:
##########
@@ -892,6 +892,48 @@ private void revokeGrantRecord(
: new PrincipalSecretsResult(secrets);
}
+ @Override
+ public @Nonnull PrincipalSecretsResult resetPrincipalSecrets(
+ @Nonnull PolarisCallContext callCtx,
+ @Nonnull String clientId,
+ long principalId,
+ @Nonnull String oldSecretHash,
+ String customClientId,
+ String customClientSecret) {
+ // get metastore we should be using
+ BasePersistence ms = callCtx.getMetaStore();
+
+ // if not found, the principal must have been dropped
+ EntityResult loadEntityResult =
+ loadEntity(
+ callCtx, PolarisEntityConstants.getNullId(), principalId,
PolarisEntityType.PRINCIPAL);
+ if (loadEntityResult.getReturnStatus() != BaseResult.ReturnStatus.SUCCESS)
{
+ return new
PrincipalSecretsResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, null);
+ }
+
+ PolarisBaseEntity principal = loadEntityResult.getEntity();
+ Map<String, String> internalProps = principal.getInternalPropertiesAsMap();
+
+ boolean customReset = customClientId != null && customClientSecret != null;
+ PolarisPrincipalSecrets secrets =
+ ((IntegrationPersistence) ms)
+ .resetPrincipalSecrets(
+ callCtx, clientId, principalId, customClientId,
customClientSecret);
+
+ PolarisBaseEntity.Builder principalBuilder = new
PolarisBaseEntity.Builder(principal);
+
principalBuilder.internalProperties(PolarisObjectMapperUtil.serializeProperties(internalProps));
Review Comment:
Maybe `principalBuilder.internalPropertiesAsMap(...)`?
TBH, I'm at a loss as to the purpose of this piece of code. I do not see any
change to internal properties. Did I miss something?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -773,6 +773,73 @@ public PolarisPrincipalSecrets generateNewPrincipalSecrets(
return principalSecrets;
}
+ @Nullable
+ @Override
+ public PolarisPrincipalSecrets resetPrincipalSecrets(
+ @Nonnull PolarisCallContext callCtx,
+ @Nonnull String clientId,
+ long principalId,
+ String customClientId,
+ String customClientSecret,
+ boolean customReset) {
+ PolarisPrincipalSecrets principalSecrets = loadPrincipalSecrets(callCtx,
clientId);
+
+ // should be found
+ callCtx
+ .getDiagServices()
+ .checkNotNull(
Review Comment:
Good point... still, if the principal is found, but do not have credentials
in Polaris (i.e. it's a "federated" principal), returning 500 is not nice in
this case.
Unfortunately, I do not see well-defined existing cases for reporting nice
errors in cases like this. I'm ok with the current impl. but we'll probably
have to revisit it at some point.
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
