Skip to content

feat(worker): edit admin users on a live worker - #181

Draft
nilsmechtel wants to merge 1 commit into
mainfrom
feat/live-admin-users
Draft

feat(worker): edit admin users on a live worker#181
nilsmechtel wants to merge 1 commit into
mainfrom
feat/live-admin-users

Conversation

@nilsmechtel

Copy link
Copy Markdown
Collaborator

Closes the worker half of the "admin users and per-dataset access are fixed at startup" report. Adds list_admin_users, add_admin_user and remove_admin_user to the worker service, all admin-gated, editable on a live worker and durable across a restart.

The premise turned out to be half wrong, in a useful way

The report assumed changing the admin list needs a pod restart because it is baked in at startup. It is not baked in anywhere. check_permissions(context, self.admin_users, ...) runs inside each method body on every call, so self.admin_users is consulted live — the Hypha service registration never carries it. AppsManager and CodeExecutor are handed the same list object at complete_initialization / initialize, so a change reaches them too, provided it is made in place. What was missing was an API, not a mechanism, and this PR is correspondingly small: no re-registration, no service restart, no reconnect.

One thing genuinely is baked in at deploy time and stays that way: _inject_admin_users folds the admin list into each app's per-method authorized_users when the app is deployed. An admin added afterwards reaches the worker API immediately but not the apps already running. That asymmetry is now stated in docs/glossary.md rather than left to be discovered.

Persistence

The triage defaulted to the app_data-on-proxy round trip that PR #123's scaling state uses, and flagged the one thing that would invalidate that default: it stores per-app data while the admin list is worker-scope. That is what happened — there is no app to hang a worker-scope list on, and inventing one would mean the list disappears whenever no app is running. So this falls back to the file, as the triage pre-authorised: <workspace_dir>/admin_users.json, written via a temp file and os.replace.

The ordering constraint from the triage is respected literally. The startup --admin-users flag is a seed, the file is an overlay, the overlay wins on restart, and the divergence is logged with both directions named (added at runtime: [...], removed at runtime: [...]) instead of applied silently. A corrupt or wrong-shaped file falls back to the seed and says so at ERROR rather than starting with no admins.

Persistence happens before the in-memory list changes. A grant that exists only in memory would revert on the next restart without ever having failed — the same silent-rollback shape the triage said not to reproduce in a permission surface.

Lockout protection

remove_admin_user refuses three cases:

  • the caller, by id or by email — the invariant from the report.
  • the last admin — also from the report. Worth noting it is only reachable on a worker started with --admin-users '*': with a single named admin, that admin is necessarily the caller and the self-removal rule fires first. With the wildcard every caller is an admin, so nothing else would stop one of them from revoking the only entry and locking out everyone including the operator.
  • the worker's own Hypha identity — not in the report. _connect_to_server unconditionally re-inserts server.config.user["email"] at index 0 on every connect, so removing it would appear to succeed and then revert at the next reconnect. Refusing it is the difference between a visible error and a silent no-op.

add_admin_user refuses '*'. Starting a worker with --admin-users '*' is a deliberate operator choice; letting any one admin make the worker world-writable over RPC is a different decision, and not one this API should offer.

Tests

19 new tests in tests/worker/test_admin_users.py. Eight positive controls, each breaking one behaviour and each failing exactly the tests that name it:

control tests that fail
drop the self-removal guard test_a_caller_cannot_revoke_their_own_admin_permissions, ..._by_user_id
drop the last-admin guard test_the_last_admin_cannot_be_removed
drop the worker-identity guard test_the_workers_own_identity_cannot_be_removed
allow the wildcard test_the_wildcard_cannot_be_granted_over_rpc
rebind instead of mutating in place test_the_change_reaches_the_component_managers
persist after mutating instead of before test_an_unwritable_store_leaves_the_admin_list_unchanged
let the seed win over the overlay test_the_persisted_list_overrides_the_startup_seed, test_a_runtime_revocation_is_not_undone_by_the_seed
drop the methods from the service dict test_the_admin_methods_are_exposed_on_the_worker_service

python -m pytest tests --noconftest -q: 224 passed on the branch against 205 on main under the same invocation, with the same 55 pre-existing conftest-dependent errors on both.

Not in this PR: per-dataset authorized users

The report bundles the dataset half on the grounds that both need one persistence mechanism. Reading the data server invalidates that rationale, in two ways worth recording.

First, per-dataset access is not fixed at startup and never needed a restart. _watch_data_dir in bioengine/datasets/proxy_server.py polls the data directory every 30 s and hot-reloads the registry in place, so editing a dataset's manifest.yaml takes effect within 30 s on a running server. The gap is an API, not a restart requirement — and the persistence question is already answered for datasets, because manifest.yaml on the data directory is the durable store.

Second, the data server is a separate process in a separate image (bioengine-datasets), started independently via python -m bioengine.datasets, potentially on a different host, and the worker reaches it only over HTTP. It has no admin concept at all — no --admin-users, no write path for manifests, and per-request authorization derived solely from each dataset's own authorized_users. Adding set_dataset_authorized_users therefore means adding an authenticated write endpoint there and deciding who is allowed to call it, which is a security model rather than a plumbing detail: the data server does not know the worker's admin list, and giving it one is a new trust relationship between two processes that currently share nothing but a URL.

That decision is left open rather than defaulted. The worker half stands alone and is complete.

Permissions are checked per call against `self.admin_users`, so the list was
already editable at runtime — only the API was missing. Adds
`list_admin_users`, `add_admin_user` and `remove_admin_user` to the worker
service, all admin-gated.

The list is mutated in place because `AppsManager` and `CodeExecutor` hold the
same object, and it is persisted to `<workspace_dir>/admin_users.json` before
the in-memory change so a grant cannot exist only in memory. On restart the
persisted list overrides the replayed `--admin-users` seed and the divergence
is logged, rather than silently rolling back.

Removal refuses three cases: the caller (self-lockout), the last admin
(reachable on a worker started with `--admin-users '*'`), and the worker's own
Hypha identity (which `_connect_to_server` re-inserts, so the removal would
revert silently). `add_admin_user` refuses `'*'` — only the operator starting
the worker gets to make it world-writable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant