Feature #21910
open
Remove api_clients APIs and api_client_id field
Description
Background
The api_clients table and api_client_id field were originally intended to restrict certain operations to more-trusted clients, like the cluster's own Workbench. However, in practice this mechanism has just made initial cluster setup more finicky, and made some token checking queries less efficient.
The TrustedClients config has made the api_clients.is_trusted mechanism redundant.
Proposed improvement
Delete the api_clients table and api_client_authorizations.api_client_id column.
(This is made awkward by the fact that tests and install docs still rely on api_client_id to decide whether to add a new token or use an existing one, etc.)
Related issues
Updated by Tom Clegg about 1 month ago
- Related to Feature #15397: Declutter the API added
Updated by Peter Amstutz 19 days ago
- Target version changed from Development 2024-07-03 sprint to Development 2024-07-24 sprint
Updated by Tom Clegg 14 days ago
21910-remove-api_client_id @ d36e94985953eabdaec5f75ad9f609f39c877738 -- developer-run-tests: #4343
21910-remove-api_client_id @ 451af2f0f399080133d225ee30572e38a7e9ec8a -- developer-run-tests: #4345 (same code as d36e94985, upgrade notes updated)
- All agreed upon points are implemented / addressed.
- ✅ api_client_id fields removed, arvados/v1/api_clients endpoints removed
- Anything not implemented (discovered or discussed during work) has a follow-up story.
- see below
- Code is tested and passing, both automated and manual, what manual testing was done is described
- tests updated, see below
- Documentation has been updated.
- ✅ api_client_id fields and .../api_clients endpoints removed from docs
- Behaves appropriately at the intended scale (describe intended scale).
- N/A
- Considered backwards and forwards compatibility issues between client and server.
- To address compatibility with earlier workbench2 versions in a federation scenario (and other older clients that might have the same issue), we now silently drop
modified_by_client_uuidfromselectparams. To check whether the missing field makes workbench2 crash, I made a test branch that reverts services/workbench2 to main (except some test setup code): 21910-remove-api_client_id-oldworkbench @ 8c233c045333592b079b77830252e1bccdf0ed35 -- developer-run-tests-services-workbench2: #981 - Should we also silently ignore the other removed fields (
api_client_id)? Or is it enough that we already advertised them as deprecated in previous versions?
- To address compatibility with earlier workbench2 versions in a federation scenario (and other older clients that might have the same issue), we now silently drop
- Follows our coding standards and GUI style guidelines.
- ✅
I removed a test ("saving an unchanged client still makes a log" in source:services/api/test/unit/log_test.rb) without fully understanding why it existed. It tested that saving an api_client record after a.is_trusted = a.is_trusted resulted in an "update" log entry but didn't update the modified_at timestamp -- but I don't know why that was true or desirable. I tried updating the test to use a collection (a.name = a.name) but that does not generate a log entry, which seems fine. Possibly Rails behavior changed since the test was first written, but the test continued to pass because by then is_trusted had become a computed field, and therefore behaved differently than regular attributes? The test was added in 54978ac2108a5b1913f641d480143356659a413a for #2375.