feat(server): separate admin token for admin operations (#154 phase 1) - #194
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdmin token configuration is added separately from the server token. CLI and config APIs manage it, while HTTP admin routes prefer it, fall back to the server token, and bypass data-plane gating. Tests and documentation cover the new behavior. ChangesAdmin token separation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant taosmd HTTP handler
participant taosmd config
Client->>taosmd HTTP handler: Send admin or data-plane request
taosmd HTTP handler->>taosmd config: Read admin_token and server_token
taosmd HTTP handler->>taosmd HTTP handler: Authorize the selected route
taosmd HTTP handler-->>Client: Return response or 403
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| """ | ||
| if method != "POST": | ||
| return False | ||
| if path == "/shelves" or path.startswith("/shelves/"): |
There was a problem hiding this comment.
WARNING: The prefix match path.startswith("/shelves/") is broader than the actual admin routes and weakens the auth model.
This makes every POST /shelves/* path exempt from the data-plane _check_token gate, but only /shelves/{id}/archive and /shelves/{id}/unarchive reach a handler that enforces _check_admin_token. Two consequences:
- Enumeration protection regression: An unauthenticated
POST /shelves/anything-elsenow bypasses the token gate and returns404 unknown shelf action(see line 921) instead of401. Previously the pre-routing_check_tokenprotected these paths ("prevents enumeration without a token"). The/shelves/namespace is now probeable without any token even when aserver_tokenis configured. - Future auth-bypass risk: Any new non-admin
POST /shelves/{id}/...route added later would silently inherit this_check_tokenexemption. If such a handler forgot to call_check_admin_token, it would be fully unauthenticated.
Consider matching the exact admin routes (e.g. path == "/shelves" or rest.endswith("/archive")//unarchive) rather than the whole /shelves/ prefix, so the exemption tracks exactly the routes that self-enforce _check_admin_token.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (8 files)
Notes: Full review (incremental base commit Previous Review Summary (commit ebeb48d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ebeb48d)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (8 files)
Notes: The core token-separation logic ( Reviewed by hy3-20260706:free · Input: 67.8K · Output: 7.2K · Cached: 310.1K |
Admin operations were gated by the same server_token that gates every data and A2A endpoint, so on a token-less deployment the only way to authorize an admin op was to set a server token, which locked out every agent on the data plane for the duration of the admin window. Add a dedicated admin_token (config key, TAOSMD_ADMIN_TOKEN env, and taosmd config set-admin-token) that gates the admin surface independently. _check_admin_token prefers admin_token and falls back to server_token, and the admin write routes are exempt from the data-plane _check_token gate since they enforce their own fail-closed check. Existing token-secured installs are unchanged; setting only admin_token leaves data and A2A endpoints open; with both set a data-plane-only caller cannot run admin ops; with neither set the admin surface still fails closed. Phase 2 (isolating admin ops from the single service loop) is deferred and tracked separately.
ebeb48d to
e78e476
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@taosmd/cli.py`:
- Around line 224-252: Update the config command dispatch to preserve and pass
the resolved args.data_dir into _config_set_admin_token and _config_show instead
of discarding it. Ensure both config writes and reads use that directory, while
retaining existing token validation and display behavior.
In `@tests/test_config_admin_token.py`:
- Around line 15-18: Update the data_dir fixture to also remove the TAOSMD_TOKEN
environment variable, alongside TAOSMD_ADMIN_TOKEN, before returning the
temporary directory path so test_independent_of_server_token is isolated from
ambient server-token configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dcd049a-a92b-4d6d-b381-81c5bbb80129
📒 Files selected for processing (8)
CHANGELOG.mdtaosmd/cli.pytaosmd/config.pytaosmd/docs/a2a-comms.mdtaosmd/http_server.pytests/test_admin_surface.pytests/test_admin_token_separation.pytests/test_config_admin_token.py
| def _config_set_admin_token(token: str | None, clear: bool) -> int: | ||
| from . import config # noqa: PLC0415 | ||
|
|
||
| if not clear and not token: | ||
| print("error: provide <token> or --clear", file=sys.stderr) | ||
| return 2 | ||
| try: | ||
| config.set_admin_token(token or "", clear=clear) | ||
| except ValueError as exc: | ||
| print(f"error: {exc}", file=sys.stderr) | ||
| return 2 | ||
| if clear: | ||
| print("Admin token cleared.") | ||
| else: | ||
| print("Admin token stored.") | ||
| return 0 | ||
|
|
||
|
|
||
| def _config_show() -> int: | ||
| from . import config # noqa: PLC0415 | ||
|
|
||
| url = config.get_server_url() | ||
| token = config.get_server_token() | ||
| admin_token = config.get_admin_token() | ||
| model = config.get_memory_model() | ||
| print(f"server_url : {url or '(unset, local mode)'}") | ||
| print(f"server_token : {'(set)' if token else '(unset)'}") | ||
| print(f"admin_token : {'(set)' if admin_token else '(unset, falls back to server_token)'}") | ||
| print(f"memory_model : {model or '(default)'}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor --data-dir for admin-token config commands.
Lines 1712-1715 discard the already-resolved args.data_dir, so taosmd --data-dir DIR config set-admin-token ... persists to the default config location; config show reads that same wrong location.
Proposed fix
-def _config_set_admin_token(token: str | None, clear: bool) -> int:
+def _config_set_admin_token(
+ token: str | None, clear: bool, data_dir=None
+) -> int:
...
- config.set_admin_token(token or "", clear=clear)
+ config.set_admin_token(token or "", clear=clear, data_dir=data_dir)
-def _config_show() -> int:
+def _config_show(data_dir=None) -> int:
...
- url = config.get_server_url()
- token = config.get_server_token()
- admin_token = config.get_admin_token()
- model = config.get_memory_model()
+ url = config.get_server_url(data_dir)
+ token = config.get_server_token(data_dir)
+ admin_token = config.get_admin_token(data_dir)
+ model = config.get_memory_model(data_dir)
...
- return _config_set_admin_token(args.token, args.clear)
+ return _config_set_admin_token(args.token, args.clear, args.data_dir)
...
- return _config_show()
+ return _config_show(args.data_dir)Also applies to: 1712-1715
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@taosmd/cli.py` around lines 224 - 252, Update the config command dispatch to
preserve and pass the resolved args.data_dir into _config_set_admin_token and
_config_show instead of discarding it. Ensure both config writes and reads use
that directory, while retaining existing token validation and display behavior.
| @pytest.fixture | ||
| def data_dir(tmp_path, monkeypatch): | ||
| monkeypatch.delenv("TAOSMD_ADMIN_TOKEN", raising=False) | ||
| return str(tmp_path) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate TAOSMD_TOKEN too.
test_independent_of_server_token expects no server token, but an ambient TAOSMD_TOKEN overrides the empty temp config and makes the test fail. Clear it in this fixture.
def data_dir(tmp_path, monkeypatch):
monkeypatch.delenv("TAOSMD_ADMIN_TOKEN", raising=False)
+ monkeypatch.delenv("TAOSMD_TOKEN", raising=False)
return str(tmp_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.fixture | |
| def data_dir(tmp_path, monkeypatch): | |
| monkeypatch.delenv("TAOSMD_ADMIN_TOKEN", raising=False) | |
| return str(tmp_path) | |
| `@pytest.fixture` | |
| def data_dir(tmp_path, monkeypatch): | |
| monkeypatch.delenv("TAOSMD_ADMIN_TOKEN", raising=False) | |
| monkeypatch.delenv("TAOSMD_TOKEN", raising=False) | |
| return str(tmp_path) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_config_admin_token.py` around lines 15 - 18, Update the data_dir
fixture to also remove the TAOSMD_TOKEN environment variable, alongside
TAOSMD_ADMIN_TOKEN, before returning the temporary directory path so
test_independent_of_server_token is isolated from ambient server-token
configuration.
1 participant
Closes #154 (phase 1). HELD for Jay's review, do not merge.
What #154 asked
The server token (
server_token/TAOSMD_TOKEN), when set, gates every data and A2A endpoint via_check_token. The admin surface (PR #153) fails closed and required that same server token. So on a free-handle (token-less) deployment the only way to authorize an admin op was to set a server token, which simultaneously locked out every agent on the data plane for the duration of the admin window. This hit the Pi bus in production for about three minutes during a channel cleanup.The requested fix: an
admin_tokenconfig value distinct fromserver_token, checked by_check_admin_token, preferringadmin_tokenand falling back toserver_tokenfor migration. Only-admin_tokenset keeps the data plane open while the admin surface works; both set keeps current data-plane behavior.Design note: "never lock the data plane" is the AUTH lockout
Re-reading the issue, the "lock" is the auth coupling, not the service loop. Today you cannot gate admin without also gating the data plane because they share one token. A separate admin token removes that coupling. This is fully solved by token separation; it does not require moving admin ops off the single asyncio service loop.
What phase 1 builds
config.get_admin_token/set_admin_token(envTAOSMD_ADMIN_TOKEN, config keyadmin_token), exported and mirrored bytaosmd config set-admin-tokenplus aconfig showline._check_admin_tokennow usesexpected = admin_token or server_token(prefer admin, fall back to server), still fail-closed (403) when neither is set.POST /shelves,POST /shelves/{id}/archive|unarchive,POST /a2a/admin/delete-channel|rename-channel|supersede-message) are exempt from the data-plane_check_tokengate in_dispatchvia a new_is_admin_route. They already enforce the stricter, fail-closed_check_admin_tokenthemselves, so the exemption opens no hole; it is exactly what decouples admin auth from the data plane._check_token(the data plane) is unchanged.Behavior matrix:
Back-compat
Existing token-secured installs (only
server_tokenset) are unchanged: the admin surface still requires the server token and still fails closed with no token. No config migration needed.Deferred to phase 2 (not built)
Isolating admin operations from the single
_ServiceLoop: a slow or heavy admin op marshalled onto the one asyncio loop (viarunner.run) can still stall data reads/writes while it executes. #154 does not ask for this and it is a larger architectural change (a second loop/worker, ordering and SQLite-connection-ownership questions). In practice the current admin ops are short metadata updates, so the acute pain (the 3-minute lockout) is the auth coupling, which phase 1 fixes. Recommend tracking phase 2 as its own issue.Tests
tests/test_admin_token_separation.py(live server): admin token accepted when only admin_token set; data plane open (tokenless ingest succeeds) when only admin_token set; admin fails closed (401) without the admin token; with both set a server-token-only caller is rejected on admin while the admin token works; server token still gates admin when no admin token (back-compat); fails closed (403) when neither set.tests/test_config_admin_token.py: env-over-file precedence, round-trip, clear, empty-string raises, independence from server_token.tests/test_admin_surface.py: updated the 403 message assertion to the new wording.Full suite: 1104 passed.
Summary by CodeRabbit
New Features
Documentation
Tests