dimas-b commented on code in PR #2197:
URL: https://github.com/apache/polaris/pull/2197#discussion_r2289573236
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1088,6 +1089,69 @@ public void deletePrincipal(String name) {
return returnedEntity;
}
+ 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(),
+ currentPrincipalEntity.getId(),
+ currentSecrets.getMainSecretHash(),
+ customClientId,
+ customClientSecret)
+ .getPrincipalSecrets();
+ if (newSecrets == null) {
+ throw new IllegalStateException(
+ String.format("Failed to %s secrets for principal '%s'", "reset",
principalName));
+ }
+ PolarisEntity newPrincipal =
+ PolarisEntity.of(
+ metaStoreManager.loadEntity(
+ getCurrentPolarisContext(),
+ 0L,
+ currentPrincipalEntity.getId(),
+ currentPrincipalEntity.getType()));
+
+ PrincipalEntity newPrincipalEntity = PrincipalEntity.of(newPrincipal);
+ if (customClientId != null) {
+ PrincipalEntity.Builder updateBuilder = new
PrincipalEntity.Builder(newPrincipalEntity);
+ updateBuilder.setClientId(newSecrets.getPrincipalClientId());
+ PrincipalEntity updatedNewPrincipalEntity = updateBuilder.build();
+ updatedNewPrincipalEntity =
+ Optional.ofNullable(
+ PrincipalEntity.of(
+ PolarisEntity.of(
+ metaStoreManager.updateEntityPropertiesIfNotChanged(
+ getCurrentPolarisContext(), null,
updatedNewPrincipalEntity))))
+ .orElseThrow(
+ () ->
+ new CommitFailedException(
Review Comment:
I'd prefer `CommitConflictException` as noted in #2168
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java:
##########
@@ -892,6 +892,45 @@ private void revokeGrantRecord(
: new PrincipalSecretsResult(secrets);
}
+ @Override
+ public @Nonnull PrincipalSecretsResult resetPrincipalSecrets(
+ @Nonnull PolarisCallContext callCtx,
+ @Nonnull String clientId,
+ long principalId,
+ @Nonnull String oldSecretHash,
Review Comment:
`oldSecretHash` appears to be unused (in all implementations).
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java:
##########
@@ -892,6 +892,45 @@ 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();
+ PolarisPrincipalSecrets secrets =
+ ((IntegrationPersistence) ms)
+ .resetPrincipalSecrets(
+ callCtx, clientId, principalId, customClientId,
customClientSecret);
+
+ PolarisBaseEntity.Builder principalBuilder = new
PolarisBaseEntity.Builder(principal);
+ var newEntityVersion =
+ (customClientId != null) ? principal.getEntityVersion() :
principal.getEntityVersion() + 1;
+
+ principalBuilder.entityVersion(newEntityVersion);
+ // Only write if version changed
+ if (customClientId == null) {
+ ms.writeEntity(callCtx, principalBuilder.build(), true, principal);
Review Comment:
I'm sorry, but I still do not understand why this write is necessary. What
are we changing in this `PolarisBaseEntity` aside from its version?
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1088,6 +1089,69 @@ public void deletePrincipal(String name) {
return returnedEntity;
}
+ 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(),
+ currentPrincipalEntity.getId(),
+ currentSecrets.getMainSecretHash(),
+ customClientId,
+ customClientSecret)
+ .getPrincipalSecrets();
+ if (newSecrets == null) {
+ throw new IllegalStateException(
+ String.format("Failed to %s secrets for principal '%s'", "reset",
principalName));
+ }
+ PolarisEntity newPrincipal =
+ PolarisEntity.of(
+ metaStoreManager.loadEntity(
+ getCurrentPolarisContext(),
+ 0L,
+ currentPrincipalEntity.getId(),
+ currentPrincipalEntity.getType()));
+
+ PrincipalEntity newPrincipalEntity = PrincipalEntity.of(newPrincipal);
+ if (customClientId != null) {
+ PrincipalEntity.Builder updateBuilder = new
PrincipalEntity.Builder(newPrincipalEntity);
+ updateBuilder.setClientId(newSecrets.getPrincipalClientId());
+ PrincipalEntity updatedNewPrincipalEntity = updateBuilder.build();
+ updatedNewPrincipalEntity =
+ Optional.ofNullable(
+ PrincipalEntity.of(
+ PolarisEntity.of(
+ metaStoreManager.updateEntityPropertiesIfNotChanged(
Review Comment:
This is risky WRT `metaStoreManager.resetPrincipalSecrets()` (line 1112). If
the request supplies a new client ID and `resetPrincipalSecrets` succeeds, but
`updateEntityPropertiesIfNotChanged` fails, then the principal entity's client
ID will be out of sync with the `PolarisPrincipalSecrets` client ID. This will
likely break all subsequent authentication and reset credentials attempts.
I'd propose to make the inner "reset" methods robust in case the credential
is not found by client ID (i.e. create a new credential / hash).
Also, I'd update the entity first as it is more likely to hit conflicts than
the credential reset itself.
--
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
