Feature #21910
open
Remove api_clients APIs and api_client_id field
Added by Tom Clegg about 1 month ago.
Updated 7 days ago.
Release relationship:
Auto
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.)
- Assigned To set to Tom Clegg
- Status changed from New to In Progress
- Target version changed from Development 2024-07-03 sprint to Development 2024-07-24 sprint
21910-remove-api_client_id @ d36e94985953eabdaec5f75ad9f609f39c877738 -- developer-run-tests: #4343 ![](https://ci.arvados.org/buildStatus/icon?job=developer-run-tests&build=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.
- Code is tested and passing, both automated and manual, what manual testing was done is described
- Documentation has been updated.
- ✅ api_client_id fields and .../api_clients endpoints removed from docs
- Behaves appropriately at the intended scale (describe intended scale).
- 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_uuid
from select
params. 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 ![](https://ci.arvados.org/buildStatus/icon?job=developer-run-tests-services-workbench2&build=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?
- 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.
Also available in: Atom
PDF