dimas-b commented on code in PR #4396:
URL: https://github.com/apache/polaris/pull/4396#discussion_r3220816281
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -918,6 +919,10 @@ public PolarisPrincipalSecrets storePrincipalSecrets(
.toList(),
realmId));
} catch (SQLException e) {
+ if (datasourceOperations.isConstraintViolation(e)) {
+ throw new AlreadyExistsException(
+ String.format("Client ID already in used: %s", resolvedClientId),
e);
Review Comment:
Can we be sure the constrain violation is about the client ID?
It might be sufficient to report the JDBC error message as an
`AlreadyExistsException`... WDYT?
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -1085,6 +1085,24 @@ public void
testCreatePrincipalAndResetCredentialsWithCustomValues() {
}
}
+ @Test
+ public void testResetCredentialsClientIdCollision() {
+ PrincipalWithCredentials victimCreds =
+
managementApi.createPrincipal(client.newEntityName(("victim-principal")));
+ String victimClientId = victimCreds.getCredentials().getClientId();
+ PrincipalWithCredentials attackerCreds =
+
managementApi.createPrincipal(client.newEntityName(("attacker-principal")));
Review Comment:
let's avoid attacker/victim terms in this context. The test makes sense even
without a malicious attack.
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1077,6 +1077,16 @@ public void deletePrincipal(String name) {
throw new ValidationException(
"Cannot reset credentials for a federated principal: %s",
principalName);
}
+ if (customClientId != null) {
+ PolarisPrincipalSecrets collidingSecrets =
+ metaStoreManager
+ .loadPrincipalSecrets(getCurrentPolarisContext(), customClientId)
+ .getPrincipalSecrets();
+ if (collidingSecrets != null
+ && collidingSecrets.getPrincipalId() !=
currentPrincipalEntity.getId()) {
Review Comment:
This check makes the new test code ineffective in testing the
persistence-level changes in this PR. At the same time this check is racy:
another thread / process may create a clashing client ID after the check.
Should we just delegate client ID uniqueness checks to Persistence?
@snazy : WDYT?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapTransactionalPersistenceImpl.java:
##########
@@ -494,6 +495,12 @@ public int lookupEntityGrantRecordsVersionInCurrentTxn(
PolarisPrincipalSecrets principalSecrets =
new PolarisPrincipalSecrets(principalId, resolvedClientId,
customClientSecret);
+ // check if already exists
+ PolarisPrincipalSecrets existing =
this.store.getSlicePrincipalSecrets().read(resolvedClientId);
+ if (existing != null && existing.getPrincipalId() == principalId) {
Review Comment:
Why check for `existing.getPrincipalId() == principalId`... reusing client
ID is still an error even if the principal ID is different.
--
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]
