Prevent client id collison during reset

[MonkeyCanCode]

Currently only root principal can perform principal reset (which contains both client id and client secret). The problem here is there is lack of check for client ID collision. When a client ID collision happened (used principal B's client id to reset principal A's client id), this will prevent principal B from perform auth request. This also means, if an admin who accidentally used root's client id to update a low privilege principal, we will no longer be able to obtain new token for principal root.

For example:

Assuming I accidentally reset principal quickstart_user's client ID with value root (which is the default client ID for principal root), I won't be able to auth with principal root again (root can get itself locked out):

➜  polaris git:(main) ./polaris --profile dev principals list
{"name": "root", "clientId": "root", "properties": {}, "createTimestamp": 1778384520325, "lastUpdateTimestamp": 1778384520325, "entityVersion": 1}
{"name": "quickstart_user", "clientId": "a35358218e1a5458", "properties": {}, "createTimestamp": 1778384524349, "lastUpdateTimestamp": 1778384524349, "entityVersion": 1}
➜  polaris git:(main) ./polaris --profile dev principals reset quickstart_user --new-client-id root
...
  File "/Users/yong/Desktop/GitHome/polaris/client/python/apache_polaris/cli/api_client_builder.py", line 147, in _get_token
    raise Exception("Failed to get access token")

Here is the new behavior with this check:

➜  polaris git:(prevent_clientid_collison_during_reset) ✗  ./polaris --profile dev principals list
{"name": "root", "clientId": "root", "properties": {}, "createTimestamp": 1778389833416, "lastUpdateTimestamp": 1778389833416, "entityVersion": 1}
{"name": "quickstart_user", "clientId": "8c587de26b5cc0ee", "properties": {}, "createTimestamp": 1778389837767, "lastUpdateTimestamp": 1778389837767, "entityVersion": 1}
➜  polaris git:(prevent_clientid_collison_during_reset) ✗ ./polaris --profile dev principals reset quickstart_user --new-client-id root
Exception when communicating with the Polaris server. AlreadyExistsException: Client ID already in used: root
➜  polaris git:(prevent_clientid_collison_during_reset) ✗ ./polaris --profile dev principals list
{"name": "root", "clientId": "root", "properties": {}, "createTimestamp": 1778389833416, "lastUpdateTimestamp": 1778389833416, "entityVersion": 1}
{"name": "quickstart_user", "clientId": "8c587de26b5cc0ee", "properties": {}, "createTimestamp": 1778389837767, "lastUpdateTimestamp": 1778389837767, "entityVersion": 1}

Checklist

• 🛡️ Don't disclose security issues! (contact security@apache.org)
• 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
• 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
• 💡 Added comments for complex logic
• 🧾 Updated CHANGELOG.md (if needed)
• 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

[snazy]

This is a nice finding!
We should never have duplicate client-IDs.

[dimas-b]

Good point to validate client ID collisions!

[dimas-b]

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?

[snazy]

Yea, using persistence as the sole authority is fine, for the stated reasons.

[MonkeyCanCode]

I tried removing the service layer check and delegating to the persistence layer. However, the reset in the NoSQL backend already bumps the entity version as part of updateEntityPropertiesIfNotChanged. Looking into the failed tests, the full reset operation contains delete secret, update entity, then write new secret which is not atomic across backends. In this case, if the write fails due to a collision, the first two steps have already been committed, the principal will then have no secret which would get into the same state.

Should we move this to a diff PR later to chain all three operations into a single atomic commit? Or maybe I missed something here?

[dimas-b]

Ok, let's keep this check, but would it be possible to add dedicated tests for NoSQL and JDBC Persistence to validate that backends also enforce client IDs uniqueness?

[snazy]

Yea, using persistence as the sole authority is fine, for the stated reasons.

[dimas-b]

Thanks, @MonkeyCanCode !

[MonkeyCanCode]

Thanks for the review @snazy @dimas-b . Merge this one now.