From ba57ae333a9d4e4c6a735b59eccd2ed16e253a7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A2=A8=E8=8F=8A?= <277378677+maiphucgiang@users.noreply.github.com> Date: Tue, 29 Sep 2026 05:39:35 +0800 Subject: [PATCH 1/6] Auto-unbind model rules when deleting a bound credential DELETE /admin/credentials/{name}?unbind=1 removes the credential's account key from every model rule binding in one control-store revision before deleting the file; without the flag the 409 guard is unchanged. The WebUI delete drawer now lists the bound rules and asks for a single confirmation instead of sending operators to unbind manually. --- app/admin_api.py | 18 +++++++-- app/control_store.py | 14 +++++++ app/gateway_management.py | 11 +++++- converter.py | 5 ++- tests/test_admin_api.py | 7 ++++ tests/test_control_store.py | 15 ++++++++ tests/test_credential_unbind.py | 67 +++++++++++++++++++++++++++++++++ web/e2e/admin.spec.ts | 21 +++++++++++ web/src/pages/Credentials.tsx | 28 ++++++++++++-- 9 files changed, 175 insertions(+), 11 deletions(-) create mode 100644 tests/test_credential_unbind.py diff --git a/app/admin_api.py b/app/admin_api.py index 181a047..24814c1 100644 --- a/app/admin_api.py +++ b/app/admin_api.py @@ -42,6 +42,15 @@ async def _body(request, maximum=65536, *, allow_empty=False): return value +def _unbind_requested(query: str) -> bool: + """Mirror the delete route's Boolean query parsing for the unbind confirmation flag.""" + for pair in (query or "").split("&"): + key, _, value = pair.partition("=") + if key == "unbind" and value.lower() in ("1", "true", "on", "yes"): + return True + return False + + def _public_credential(item): # Never pass arbitrary credential manager fields through to the browser. fields = {"id", "account_key", "name", "filename", "enabled", "label", "profile", "region", "site", @@ -173,10 +182,11 @@ async def dispatch(request): name = path.removeprefix("/admin/credentials/") if not _valid_name(name): return error_response(400, "凭证文件名无效") - try: - await run_in_threadpool(gateway.admin_delete_guard, name) - except (ValueError, HTTPException): - return error_response(409, "凭证仍被模型策略引用，请先移除绑定") + if not _unbind_requested(request.url.query): + try: + await run_in_threadpool(gateway.admin_delete_guard, name) + except (ValueError, HTTPException): + return error_response(409, "凭证仍被模型策略引用，请先移除绑定") return None # Middleware is deliberately installed only here, never on module import. diff --git a/app/control_store.py b/app/control_store.py index 6d3351d..03fa1c5 100644 --- a/app/control_store.py +++ b/app/control_store.py @@ -193,6 +193,20 @@ def set_credential(self, account_key, enabled): raise ValueError("enabled 必须为布尔值") return self._update(None, lambda state: state["credentials"].setdefault(account_key, {}).update(enabled=enabled)) + def unbind_credential(self, account_key): + """Drop an account key from every model rule binding in one revision bump.""" + _identifier(account_key, "账号指纹") + snapshot = self.snapshot() + if not any(account_key in (rule.get("credential_ids") or []) + for rule in snapshot["models"].values()): + return snapshot # Nothing references the identity; keep the revision stable. + def change(state): + for rule in state["models"].values(): + ids = rule.get("credential_ids") or [] + if account_key in ids: + rule["credential_ids"] = [identity for identity in ids if identity != account_key] + return self._update(None, change) + def set_auto_checkin(self, account_key, enabled): _identifier(account_key, "账号指纹") if type(enabled) is not bool: diff --git a/app/gateway_management.py b/app/gateway_management.py index 64f3c5b..69c653e 100644 --- a/app/gateway_management.py +++ b/app/gateway_management.py @@ -109,12 +109,19 @@ def admin_set_auto_travel(self, identity, enabled): raise HTTPException(400, "旅行仅适用于国内账号") self.CONFIG["control_store"].set_auto_travel(identity, enabled) - def admin_delete_guard(self, name): + def admin_delete_guard(self, name, *, unbind=False): + """Block deleting a bound credential unless the caller confirms auto-unbinding.""" rows = self.admin_credential_inventory() row = next((row for row in rows if row["name"] == name or row["id"] == name), None) - if row and row["bindings"]: + if row is None or not row["bindings"]: + return + if not unbind: raise HTTPException(status_code=409, detail={"message": "凭证仍被模型规则引用，请先移除绑定", "models": row["bindings"]}) + control = self.CONFIG.get("control_store") + if control is None: + raise HTTPException(status_code=500, detail="控制库不可用，无法解除绑定") + control.unbind_credential(row["id"]) def admin_model_inventory(self): pool = self.CONFIG.get("cred_pool") diff --git a/converter.py b/converter.py index cbd8fc3..84696a7 100644 --- a/converter.py +++ b/converter.py @@ -2085,13 +2085,14 @@ async def admin_add_credential(request: Request, @app.delete("/admin/credentials/{name}") def admin_del_credential(name: str, + unbind: bool = False, authorization: Optional[str] = Header(default=None), x_api_key: Optional[str] = Header(default=None, alias="X-Api-Key")): - """Delete the named .info file and remove its credential from the pool.""" + """Delete the named .info file, first unbinding model rules when confirmed.""" _check_admin_auth(authorization, x_api_key) pool = CONFIG.get("cred_pool") if CONFIG.get("management") is not None: - CONFIG["management"].admin_delete_guard(os.path.basename(name)) + CONFIG["management"].admin_delete_guard(os.path.basename(name), unbind=unbind) if pool is None or not pool.remove_file(os.path.basename(name)): raise HTTPException(status_code=404, detail={"error": {"message": f"凭据不在池中: {name}", "type": "invalid_request_error"}}) return {"removed": os.path.basename(name)} diff --git a/tests/test_admin_api.py b/tests/test_admin_api.py index 76ef1b4..7175f0c 100644 --- a/tests/test_admin_api.py +++ b/tests/test_admin_api.py @@ -511,6 +511,13 @@ def test_inventory_toggle_and_delete_guard(self): self.gateway.admin_set_credential_enabled.assert_called_once_with("fingerprint", False) self.gateway.admin_delete_guard.side_effect = ValueError("referenced") self.assertEqual(self.client.delete("/admin/credentials/first.info", headers=self.headers).status_code, 409) + # The confirmed unbind flow bypasses the middleware guard; the delete route owns it. + self.gateway.admin_delete_guard.reset_mock() + self.gateway.admin_delete_guard.side_effect = None + self.assertEqual( + self.client.delete("/admin/credentials/first.info?unbind=1", headers=self.headers).status_code, + 200) + self.gateway.admin_delete_guard.assert_not_called() def test_upload_limits_paths_and_secret_errors(self): for files in ([{"name": "../first.info", "content": "{}"}], [{"name": "db.sqlite3", "content": "{}"}], diff --git a/tests/test_control_store.py b/tests/test_control_store.py index 0bb1176..be483de 100644 --- a/tests/test_control_store.py +++ b/tests/test_control_store.py @@ -82,6 +82,21 @@ def test_legacy_combined_scopes_load_without_widening_and_require_explicit_edit( self.assertEqual(reopened.snapshot()["revision"], 1) + def test_unbind_credential_removes_only_the_target_identity(self): + self.store.update_model("real", {"public_id": "public", "credential_ids": ["fingerprint", "other"]}, 0) + self.store.update_model("second", {"credential_ids": ["fingerprint"]}, 1) + self.store.update_model("third", {"credential_ids": ["other"]}, 2) + state = self.store.unbind_credential("fingerprint") + self.assertEqual(state["revision"], 4) # Three setup bumps, then one for the unbind. + self.assertEqual(state["models"]["real"]["credential_ids"], ["other"]) + self.assertEqual(state["models"]["second"]["credential_ids"], []) + self.assertEqual(state["models"]["third"]["credential_ids"], ["other"]) + self.store.unbind_credential("unknown-identity") # Unknown identities are a no-op. + self.assertEqual(self.store.snapshot()["revision"], 4) + with self.assertRaises(ValueError): + self.store.unbind_credential("not a fingerprint") + + def test_credential_metadata_and_no_secret_settings(self): self.store.set_credential("account-fingerprint", False) self.assertEqual(self.store.snapshot()["credentials"], {"account-fingerprint": {"enabled": False}}) diff --git a/tests/test_credential_unbind.py b/tests/test_credential_unbind.py new file mode 100644 index 0000000..22d342f --- /dev/null +++ b/tests/test_credential_unbind.py @@ -0,0 +1,67 @@ +"""Deleting credentials that are still bound by model rules must auto-unbind on confirmation.""" +import sys +from pathlib import Path +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) # Allow direct execution. + +import json +import os +import tempfile +import time +import unittest +from types import SimpleNamespace +from unittest.mock import patch + +from fastapi import HTTPException + +import converter +from app.control_store import ControlStore +from app.gateway_management import Management + + +class UnbindDeleteTests(unittest.TestCase): + def setUp(self): + root = Path(self.enterContext(tempfile.TemporaryDirectory())) + self.enterContext(patch.dict(os.environ, {"CODEBUDDY_AUTH_DIR": str(root)})) + self.enterContext(patch.dict(converter.CONFIG, {"log_path": None, "cred_pool": None, "cred": None})) + self.path = root / "account.info" + self.path.write_text(json.dumps({"account": {"uid": "synthetic"}, "auth": { + "accessToken": "old-synthetic-token", "refreshToken": "synthetic-refresh", + "domain": "www.codebuddy.cn", "expiresAt": (time.time() + 86400) * 1000, + "lastRefreshTime": time.time() * 1000}}), encoding="utf-8") + self.pool = converter.CredentialPool([self.path]) + self.store = ControlStore(root / "control.sqlite3") + self.addCleanup(self.store.close) + self.config = {"cred_pool": self.pool, "control_store": self.store, + "ledger": None, "trial_ledger": None} + self.management = Management(SimpleNamespace(CONFIG=self.config)) + self.identity = self.pool.entries()[0]["account_key"] + + def bind(self): + self.store.update_model("glm-4-flash", {"credential_ids": [self.identity]}, 0) + + def test_bound_delete_is_blocked_without_the_confirmation_flag(self): + self.bind() + with self.assertRaises(HTTPException) as ctx: + self.management.admin_delete_guard("account.info") + self.assertEqual(ctx.exception.status_code, 409) + self.assertEqual(ctx.exception.detail["models"], ["glm-4-flash"]) + + def test_confirmed_delete_unbinds_rules_then_removes_the_file(self): + self.store.update_model("glm-4-flash", {"public_id": "glm4", "credential_ids": [self.identity, "other"]}, 0) + self.management.admin_delete_guard("account.info", unbind=True) + rule = self.store.snapshot()["models"]["glm-4-flash"] + self.assertEqual(rule["credential_ids"], ["other"]) + self.assertEqual(rule["public_id"], "glm4") + self.assertTrue(self.pool.remove_file("account.info")) + self.assertFalse(self.path.exists()) + + def test_unbound_and_unknown_credentials_stay_on_the_fast_path(self): + self.management.admin_delete_guard("account.info") # No bindings: nothing to unbind. + self.assertEqual(self.store.snapshot()["revision"], 0) + self.management.admin_delete_guard("missing.info", unbind=True) # Unknown name: pool reports 404. + self.assertEqual(self.store.snapshot()["revision"], 0) + + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/web/e2e/admin.spec.ts b/web/e2e/admin.spec.ts index f23be5b..b4c3dec 100644 --- a/web/e2e/admin.spec.ts +++ b/web/e2e/admin.spec.ts @@ -264,6 +264,27 @@ test("credential OAuth terminates, upload/export and safe-name deletion are wire ) .toBe(true); }); +test("deleting a bound credential confirms the automatic unbind", async ({ page }) => { + const mock = await mockAPI(page); + await page.route("**/admin/credentials", async (route) => { + if (route.request().method() !== "GET") return route.fallback(); + return route.fulfill({ json: { credentials: [{ ...credential, bindings: ["mock-model"] }] } }); + }); + await page.goto("/dashboard/credentials"); + await page.getByRole("button", { name: "删除", exact: true }).click(); + await expect(page.getByText("该凭证仍被模型规则引用：mock-model。")).toBeVisible(); + await expect(page.getByText("确认后将自动解除这些绑定，规则回退为自动选择账号。")).toBeVisible(); + await page.getByRole("button", { name: "解除绑定并删除凭证" }).click(); + await expect + .poll(() => + mock.calls.some( + (call) => + call.method === "DELETE" && + call.path === "/admin/credentials/mock-account.info?unbind=1", + ), + ) + .toBe(true); +}); test("international OAuth choices send distinct sites and show the selected official host", async ({ page, }) => { diff --git a/web/src/pages/Credentials.tsx b/web/src/pages/Credentials.tsx index 0ed78e9..3a87787 100644 --- a/web/src/pages/Credentials.tsx +++ b/web/src/pages/Credentials.tsx @@ -38,6 +38,12 @@ function expiry(value: unknown, milliseconds = false) { : text(value); } export { safeOAuthUrl } from "../OAuth"; +function boundModels(credential: Credential): string[] { + return Array.isArray(credential.bindings) + ? credential.bindings.filter((id): id is string => typeof id === "string") + : []; +} + function ImportDrawer({ onClose, onDone }: { onClose: () => void; onDone: () => void }) { const [results, setResults] = useState([]); const [error, setError] = useState(null); @@ -590,17 +596,33 @@ export function Credentials() { {deleting && ( setDeleting(null)} dismissDisabled={busy}>
- 将永久删除 {deleting.name}，不可撤销。如已绑定模型规则，请先解除绑定。 + 将永久删除 {deleting.name}，不可撤销。 + {deleting.bindings === undefined ? ( + "如已绑定模型规则，请先解除绑定。" + ) : boundModels(deleting).length ? ( + <> + 该凭证仍被模型规则引用：{boundModels(deleting).join("、")}。 + 确认后将自动解除这些绑定，规则回退为自动选择账号。 + + ) : ( + "该凭证未被模型规则引用。" + )}
)} From da2418c38ba7b5065fdbafcdf254523e74f44adf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A2=A8=E8=8F=8A?= <277378677+maiphucgiang@users.noreply.github.com> Date: Tue, 29 Sep 2026 05:43:57 +0800 Subject: [PATCH 2/6] Format the new e2e unbind test --- web/e2e/admin.spec.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/web/e2e/admin.spec.ts b/web/e2e/admin.spec.ts index b4c3dec..0d3a736 100644 --- a/web/e2e/admin.spec.ts +++ b/web/e2e/admin.spec.ts @@ -279,8 +279,7 @@ test("deleting a bound credential confirms the automatic unbind", async ({ page .poll(() => mock.calls.some( (call) => - call.method === "DELETE" && - call.path === "/admin/credentials/mock-account.info?unbind=1", + call.method === "DELETE" && call.path === "/admin/credentials/mock-account.info?unbind=1", ), ) .toBe(true); From 899d2546794ae2070077c2873b54ca618f897176 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A2=A8=E8=8F=8A?= <277378677+maiphucgiang@users.noreply.github.com> Date: Tue, 29 Sep 2026 05:52:31 +0800 Subject: [PATCH 3/6] Keep model bindings when credential removal fails admin_delete_guard now only confirms the unbind and returns the identity; the delete route strips the bindings after remove_file succeeds, so a failed removal never silently widens routing to automatic account selection. --- app/gateway_management.py | 13 ++++++++----- converter.py | 8 ++++++-- tests/test_credential_unbind.py | 33 +++++++++++++++++++++++++++------ 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/app/gateway_management.py b/app/gateway_management.py index 69c653e..8bba387 100644 --- a/app/gateway_management.py +++ b/app/gateway_management.py @@ -110,18 +110,21 @@ def admin_set_auto_travel(self, identity, enabled): self.CONFIG["control_store"].set_auto_travel(identity, enabled) def admin_delete_guard(self, name, *, unbind=False): - """Block deleting a bound credential unless the caller confirms auto-unbinding.""" + """Block a still-bound delete, or return the identity to unbind after removal.""" rows = self.admin_credential_inventory() row = next((row for row in rows if row["name"] == name or row["id"] == name), None) if row is None or not row["bindings"]: - return + return None if not unbind: raise HTTPException(status_code=409, detail={"message": "凭证仍被模型规则引用，请先移除绑定", "models": row["bindings"]}) + return row["id"] + + def admin_unbind_credential(self, identity): + """Drop the deleted credential's bindings; run only after its file is gone.""" control = self.CONFIG.get("control_store") - if control is None: - raise HTTPException(status_code=500, detail="控制库不可用，无法解除绑定") - control.unbind_credential(row["id"]) + if control is not None: + control.unbind_credential(identity) def admin_model_inventory(self): pool = self.CONFIG.get("cred_pool") diff --git a/converter.py b/converter.py index 84696a7..e2e96c6 100644 --- a/converter.py +++ b/converter.py @@ -2088,13 +2088,17 @@ def admin_del_credential(name: str, unbind: bool = False, authorization: Optional[str] = Header(default=None), x_api_key: Optional[str] = Header(default=None, alias="X-Api-Key")): - """Delete the named .info file, first unbinding model rules when confirmed.""" + """Delete the named .info file, unbinding model rules only after removal succeeds.""" _check_admin_auth(authorization, x_api_key) pool = CONFIG.get("cred_pool") + unbind_identity = None if CONFIG.get("management") is not None: - CONFIG["management"].admin_delete_guard(os.path.basename(name), unbind=unbind) + unbind_identity = CONFIG["management"].admin_delete_guard(os.path.basename(name), unbind=unbind) if pool is None or not pool.remove_file(os.path.basename(name)): + # Bindings stay untouched so a failed removal never widens routing to automatic. raise HTTPException(status_code=404, detail={"error": {"message": f"凭据不在池中: {name}", "type": "invalid_request_error"}}) + if unbind_identity is not None and CONFIG.get("management") is not None: + CONFIG["management"].admin_unbind_credential(unbind_identity) return {"removed": os.path.basename(name)} diff --git a/tests/test_credential_unbind.py b/tests/test_credential_unbind.py index 22d342f..dc8e4ec 100644 --- a/tests/test_credential_unbind.py +++ b/tests/test_credential_unbind.py @@ -11,7 +11,9 @@ from types import SimpleNamespace from unittest.mock import patch + from fastapi import HTTPException +from fastapi.testclient import TestClient import converter from app.control_store import ControlStore @@ -46,19 +48,38 @@ def test_bound_delete_is_blocked_without_the_confirmation_flag(self): self.assertEqual(ctx.exception.status_code, 409) self.assertEqual(ctx.exception.detail["models"], ["glm-4-flash"]) - def test_confirmed_delete_unbinds_rules_then_removes_the_file(self): + def test_confirmed_delete_unbinds_only_after_removal_succeeds(self): self.store.update_model("glm-4-flash", {"public_id": "glm4", "credential_ids": [self.identity, "other"]}, 0) - self.management.admin_delete_guard("account.info", unbind=True) + identity = self.management.admin_delete_guard("account.info", unbind=True) + self.assertEqual(identity, self.identity) + self.assertEqual(self.store.snapshot()["models"]["glm-4-flash"]["credential_ids"], + [self.identity, "other"]) # The guard itself must not unbind. + self.assertTrue(self.pool.remove_file("account.info")) + self.management.admin_unbind_credential(identity) rule = self.store.snapshot()["models"]["glm-4-flash"] self.assertEqual(rule["credential_ids"], ["other"]) self.assertEqual(rule["public_id"], "glm4") - self.assertTrue(self.pool.remove_file("account.info")) + self.assertFalse(self.path.exists()) + + def test_route_keeps_bindings_when_removal_fails(self): + self.bind() + config = dict(self.config, api_key="secret", management=self.management) + self.enterContext(patch.dict(converter.CONFIG, config)) + client = TestClient(converter.app) + headers = {"X-Api-Key": "secret"} + with patch.object(self.pool, "remove_file", return_value=False): + response = client.delete("/admin/credentials/account.info?unbind=1", headers=headers) + self.assertEqual(response.status_code, 404, response.text) + self.assertEqual(self.store.snapshot()["models"]["glm-4-flash"]["credential_ids"], + [self.identity]) # A failed removal never widens routing to automatic. + response = client.delete("/admin/credentials/account.info?unbind=1", headers=headers) + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(self.store.snapshot()["models"]["glm-4-flash"]["credential_ids"], []) self.assertFalse(self.path.exists()) def test_unbound_and_unknown_credentials_stay_on_the_fast_path(self): - self.management.admin_delete_guard("account.info") # No bindings: nothing to unbind. - self.assertEqual(self.store.snapshot()["revision"], 0) - self.management.admin_delete_guard("missing.info", unbind=True) # Unknown name: pool reports 404. + self.assertIsNone(self.management.admin_delete_guard("account.info")) # No bindings. + self.assertIsNone(self.management.admin_delete_guard("missing.info", unbind=True)) # Pool reports 404. self.assertEqual(self.store.snapshot()["revision"], 0) From 729567c47c884101dfcc74da264b7166227c5ed5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A2=A8=E8=8F=8A?= <277378677+maiphucgiang@users.noreply.github.com> Date: Tue, 29 Sep 2026 06:01:43 +0800 Subject: [PATCH 4/6] Roll back the unbind when credential removal fails unbind_credential now captures each rule's previous binding list and restore_bindings puts them back in one revision bump, so a delete whose file removal fails leaves routing exactly as it was; unbind failures still happen before any file is removed. --- app/control_store.py | 25 ++++++++++++++++++++----- app/gateway_management.py | 11 +++++++++-- converter.py | 12 +++++++----- tests/test_control_store.py | 19 +++++++++++++------ tests/test_credential_unbind.py | 14 +++++++------- 5 files changed, 56 insertions(+), 25 deletions(-) diff --git a/app/control_store.py b/app/control_store.py index 03fa1c5..9085a18 100644 --- a/app/control_store.py +++ b/app/control_store.py @@ -194,17 +194,32 @@ def set_credential(self, account_key, enabled): return self._update(None, lambda state: state["credentials"].setdefault(account_key, {}).update(enabled=enabled)) def unbind_credential(self, account_key): - """Drop an account key from every model rule binding in one revision bump.""" + """Drop an account key from every rule, returning {source: old_ids} for rollback.""" _identifier(account_key, "账号指纹") - snapshot = self.snapshot() if not any(account_key in (rule.get("credential_ids") or []) - for rule in snapshot["models"].values()): - return snapshot # Nothing references the identity; keep the revision stable. + for rule in self.snapshot()["models"].values()): + return {} # Nothing references the identity; keep the revision stable. + affected = {} def change(state): - for rule in state["models"].values(): + for source, rule in state["models"].items(): ids = rule.get("credential_ids") or [] if account_key in ids: + affected[source] = list(ids) rule["credential_ids"] = [identity for identity in ids if identity != account_key] + self._update(None, change) + return affected + + def restore_bindings(self, affected): + """Put back the bindings captured by unbind_credential in one revision bump.""" + pending = {source: ids for source, ids in dict(affected or {}).items() if isinstance(ids, list)} + current = self.snapshot()["models"] + if not any(current.get(source, {}).get("credential_ids") != ids for source, ids in pending.items()): + return self.snapshot() # Already restored; keep the revision stable. + def change(state): + for source, ids in pending.items(): + rule = state["models"].get(source) + if rule is not None and rule.get("credential_ids") != ids: + rule["credential_ids"] = list(ids) return self._update(None, change) def set_auto_checkin(self, account_key, enabled): diff --git a/app/gateway_management.py b/app/gateway_management.py index 8bba387..4b0ccb0 100644 --- a/app/gateway_management.py +++ b/app/gateway_management.py @@ -121,10 +121,17 @@ def admin_delete_guard(self, name, *, unbind=False): return row["id"] def admin_unbind_credential(self, identity): - """Drop the deleted credential's bindings; run only after its file is gone.""" + """Unbind now and return the rollback map used when file removal fails.""" + control = self.CONFIG.get("control_store") + if control is None: + return {} + return control.unbind_credential(identity) + + def admin_restore_bindings(self, affected): + """Put bindings back after a failed removal so routing never widens silently.""" control = self.CONFIG.get("control_store") if control is not None: - control.unbind_credential(identity) + control.restore_bindings(affected) def admin_model_inventory(self): pool = self.CONFIG.get("cred_pool") diff --git a/converter.py b/converter.py index e2e96c6..6d11572 100644 --- a/converter.py +++ b/converter.py @@ -2091,14 +2091,16 @@ def admin_del_credential(name: str, """Delete the named .info file, unbinding model rules only after removal succeeds.""" _check_admin_auth(authorization, x_api_key) pool = CONFIG.get("cred_pool") - unbind_identity = None + rollback = {} if CONFIG.get("management") is not None: - unbind_identity = CONFIG["management"].admin_delete_guard(os.path.basename(name), unbind=unbind) + identity = CONFIG["management"].admin_delete_guard(os.path.basename(name), unbind=unbind) + if identity is not None: + rollback = CONFIG["management"].admin_unbind_credential(identity) if pool is None or not pool.remove_file(os.path.basename(name)): - # Bindings stay untouched so a failed removal never widens routing to automatic. + # A failed removal rolls the unbind back so routing never widens silently. + if CONFIG.get("management") is not None and rollback: + CONFIG["management"].admin_restore_bindings(rollback) raise HTTPException(status_code=404, detail={"error": {"message": f"凭据不在池中: {name}", "type": "invalid_request_error"}}) - if unbind_identity is not None and CONFIG.get("management") is not None: - CONFIG["management"].admin_unbind_credential(unbind_identity) return {"removed": os.path.basename(name)} diff --git a/tests/test_control_store.py b/tests/test_control_store.py index be483de..b64ae50 100644 --- a/tests/test_control_store.py +++ b/tests/test_control_store.py @@ -86,13 +86,20 @@ def test_unbind_credential_removes_only_the_target_identity(self): self.store.update_model("real", {"public_id": "public", "credential_ids": ["fingerprint", "other"]}, 0) self.store.update_model("second", {"credential_ids": ["fingerprint"]}, 1) self.store.update_model("third", {"credential_ids": ["other"]}, 2) - state = self.store.unbind_credential("fingerprint") - self.assertEqual(state["revision"], 4) # Three setup bumps, then one for the unbind. - self.assertEqual(state["models"]["real"]["credential_ids"], ["other"]) - self.assertEqual(state["models"]["second"]["credential_ids"], []) - self.assertEqual(state["models"]["third"]["credential_ids"], ["other"]) - self.store.unbind_credential("unknown-identity") # Unknown identities are a no-op. + affected = self.store.unbind_credential("fingerprint") + self.assertEqual(affected, {"real": ["fingerprint", "other"], "second": ["fingerprint"]}) + self.assertEqual(self.store.snapshot()["revision"], 4) # Three setup bumps, then one for the unbind. + self.assertEqual(self.store.snapshot()["models"]["real"]["credential_ids"], ["other"]) + self.assertEqual(self.store.snapshot()["models"]["second"]["credential_ids"], []) + self.assertEqual(self.store.snapshot()["models"]["third"]["credential_ids"], ["other"]) + self.assertEqual(self.store.unbind_credential("unknown-identity"), {}) # No-op keeps the revision. self.assertEqual(self.store.snapshot()["revision"], 4) + self.store.restore_bindings(affected) # Rollback restores both rules in one bump. + self.assertEqual(self.store.snapshot()["models"]["real"]["credential_ids"], ["fingerprint", "other"]) + self.assertEqual(self.store.snapshot()["models"]["second"]["credential_ids"], ["fingerprint"]) + self.assertEqual(self.store.snapshot()["revision"], 5) + self.store.restore_bindings(affected) # An already-restored state bumps nothing. + self.assertEqual(self.store.snapshot()["revision"], 5) with self.assertRaises(ValueError): self.store.unbind_credential("not a fingerprint") diff --git a/tests/test_credential_unbind.py b/tests/test_credential_unbind.py index dc8e4ec..a3195f2 100644 --- a/tests/test_credential_unbind.py +++ b/tests/test_credential_unbind.py @@ -48,18 +48,18 @@ def test_bound_delete_is_blocked_without_the_confirmation_flag(self): self.assertEqual(ctx.exception.status_code, 409) self.assertEqual(ctx.exception.detail["models"], ["glm-4-flash"]) - def test_confirmed_delete_unbinds_only_after_removal_succeeds(self): + def test_confirmed_unbind_rolls_back_when_removal_fails(self): self.store.update_model("glm-4-flash", {"public_id": "glm4", "credential_ids": [self.identity, "other"]}, 0) identity = self.management.admin_delete_guard("account.info", unbind=True) self.assertEqual(identity, self.identity) - self.assertEqual(self.store.snapshot()["models"]["glm-4-flash"]["credential_ids"], - [self.identity, "other"]) # The guard itself must not unbind. - self.assertTrue(self.pool.remove_file("account.info")) - self.management.admin_unbind_credential(identity) + rollback = self.management.admin_unbind_credential(identity) + self.assertEqual(rollback, {"glm-4-flash": [self.identity, "other"]}) + self.assertEqual(self.store.snapshot()["models"]["glm-4-flash"]["credential_ids"], ["other"]) + self.management.admin_restore_bindings(rollback) # A failed removal undoes the unbind. rule = self.store.snapshot()["models"]["glm-4-flash"] - self.assertEqual(rule["credential_ids"], ["other"]) + self.assertEqual(rule["credential_ids"], [self.identity, "other"]) self.assertEqual(rule["public_id"], "glm4") - self.assertFalse(self.path.exists()) + self.assertTrue(self.path.exists()) def test_route_keeps_bindings_when_removal_fails(self): self.bind() From 47084ccaec96c290083d1003a826b4619be095cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A2=A8=E8=8F=8A?= <277378677+maiphucgiang@users.noreply.github.com> Date: Tue, 29 Sep 2026 06:16:54 +0800 Subject: [PATCH 5/6] Keep concurrent binding edits during unbind rollback restore_bindings re-adds only the deleted identity and only where a rule still matches its post-unbind state, so an administrator edit or rule deletion between the unbind and the rollback is preserved instead of being overwritten. --- app/control_store.py | 24 ++++++++++++++++-------- app/gateway_management.py | 8 ++++---- tests/test_control_store.py | 16 ++++++++++++++-- tests/test_credential_unbind.py | 12 +++++++++++- 4 files changed, 45 insertions(+), 15 deletions(-) diff --git a/app/control_store.py b/app/control_store.py index 9085a18..b18c619 100644 --- a/app/control_store.py +++ b/app/control_store.py @@ -209,17 +209,25 @@ def change(state): self._update(None, change) return affected - def restore_bindings(self, affected): - """Put back the bindings captured by unbind_credential in one revision bump.""" + def restore_bindings(self, account_key, affected): + """Re-add the identity where a rule still matches its post-unbind state. + + Concurrent edits that changed a rule's binding list are preserved. + """ + _identifier(account_key, "账号指纹") pending = {source: ids for source, ids in dict(affected or {}).items() if isinstance(ids, list)} - current = self.snapshot()["models"] - if not any(current.get(source, {}).get("credential_ids") != ids for source, ids in pending.items()): - return self.snapshot() # Already restored; keep the revision stable. + def restorable(rule, ids): + if rule is None: + return False # A rule deleted meanwhile stays deleted. + current = rule.get("credential_ids") or [] + return current == [identity for identity in ids if identity != account_key] + snapshot = self.snapshot() + if not any(restorable(snapshot["models"].get(source), ids) for source, ids in pending.items()): + return snapshot # Nothing to restore; concurrent edits win. def change(state): for source, ids in pending.items(): - rule = state["models"].get(source) - if rule is not None and rule.get("credential_ids") != ids: - rule["credential_ids"] = list(ids) + if restorable(state["models"].get(source), ids): + state["models"][source]["credential_ids"] = list(ids) return self._update(None, change) def set_auto_checkin(self, account_key, enabled): diff --git a/app/gateway_management.py b/app/gateway_management.py index 4b0ccb0..c21ee33 100644 --- a/app/gateway_management.py +++ b/app/gateway_management.py @@ -121,17 +121,17 @@ def admin_delete_guard(self, name, *, unbind=False): return row["id"] def admin_unbind_credential(self, identity): - """Unbind now and return the rollback map used when file removal fails.""" + """Unbind now and return the rollback record used when removal fails.""" control = self.CONFIG.get("control_store") if control is None: return {} - return control.unbind_credential(identity) + return {"identity": identity, "rules": control.unbind_credential(identity)} - def admin_restore_bindings(self, affected): + def admin_restore_bindings(self, rollback): """Put bindings back after a failed removal so routing never widens silently.""" control = self.CONFIG.get("control_store") if control is not None: - control.restore_bindings(affected) + control.restore_bindings(rollback["identity"], rollback["rules"]) def admin_model_inventory(self): pool = self.CONFIG.get("cred_pool") diff --git a/tests/test_control_store.py b/tests/test_control_store.py index b64ae50..cb2764d 100644 --- a/tests/test_control_store.py +++ b/tests/test_control_store.py @@ -94,12 +94,24 @@ def test_unbind_credential_removes_only_the_target_identity(self): self.assertEqual(self.store.snapshot()["models"]["third"]["credential_ids"], ["other"]) self.assertEqual(self.store.unbind_credential("unknown-identity"), {}) # No-op keeps the revision. self.assertEqual(self.store.snapshot()["revision"], 4) - self.store.restore_bindings(affected) # Rollback restores both rules in one bump. + self.store.restore_bindings("fingerprint", affected) # Rollback restores both rules in one bump. self.assertEqual(self.store.snapshot()["models"]["real"]["credential_ids"], ["fingerprint", "other"]) self.assertEqual(self.store.snapshot()["models"]["second"]["credential_ids"], ["fingerprint"]) self.assertEqual(self.store.snapshot()["revision"], 5) - self.store.restore_bindings(affected) # An already-restored state bumps nothing. + self.store.restore_bindings("fingerprint", affected) # An already-restored state bumps nothing. self.assertEqual(self.store.snapshot()["revision"], 5) + + def test_rollback_keeps_concurrent_edits_and_deleted_rules(self): + self.store.update_model("real", {"credential_ids": ["fingerprint", "other"]}, 0) + self.store.update_model("gone", {"custom": True, "public_id": "gone-public", "credential_ids": ["fingerprint"]}, 1) + affected = self.store.unbind_credential("fingerprint") + revision = self.store.snapshot()["revision"] + # Concurrent edits: one rule rebound elsewhere, another deleted outright. + self.store.update_model("real", {"credential_ids": ["fresh"]}, revision) + self.store.delete_model("gone", self.store.snapshot()["revision"]) + self.store.restore_bindings("fingerprint", affected) + self.assertEqual(self.store.snapshot()["models"]["real"]["credential_ids"], ["fresh"]) + self.assertNotIn("gone", self.store.snapshot()["models"]) with self.assertRaises(ValueError): self.store.unbind_credential("not a fingerprint") diff --git a/tests/test_credential_unbind.py b/tests/test_credential_unbind.py index a3195f2..0afa93c 100644 --- a/tests/test_credential_unbind.py +++ b/tests/test_credential_unbind.py @@ -53,7 +53,7 @@ def test_confirmed_unbind_rolls_back_when_removal_fails(self): identity = self.management.admin_delete_guard("account.info", unbind=True) self.assertEqual(identity, self.identity) rollback = self.management.admin_unbind_credential(identity) - self.assertEqual(rollback, {"glm-4-flash": [self.identity, "other"]}) + self.assertEqual(rollback, {"identity": self.identity, "rules": {"glm-4-flash": [self.identity, "other"]}}) self.assertEqual(self.store.snapshot()["models"]["glm-4-flash"]["credential_ids"], ["other"]) self.management.admin_restore_bindings(rollback) # A failed removal undoes the unbind. rule = self.store.snapshot()["models"]["glm-4-flash"] @@ -61,6 +61,16 @@ def test_confirmed_unbind_rolls_back_when_removal_fails(self): self.assertEqual(rule["public_id"], "glm4") self.assertTrue(self.path.exists()) + def test_rollback_preserves_concurrent_binding_edits(self): + self.store.update_model("glm-4-flash", {"credential_ids": [self.identity, "other"]}, 0) + identity = self.management.admin_delete_guard("account.info", unbind=True) + rollback = self.management.admin_unbind_credential(identity) + # Another administrator rebinds the rule between the unbind and the rollback. + revision = self.store.snapshot()["revision"] + self.store.update_model("glm-4-flash", {"credential_ids": ["fresh"]}, revision) + self.management.admin_restore_bindings(rollback) + self.assertEqual(self.store.snapshot()["models"]["glm-4-flash"]["credential_ids"], ["fresh"]) + def test_route_keeps_bindings_when_removal_fails(self): self.bind() config = dict(self.config, api_key="secret", management=self.management) From a65a77e469dbedcf748eb6521973adaa57ee26d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A2=A8=E8=8F=8A?= <277378677+maiphucgiang@users.noreply.github.com> Date: Tue, 29 Sep 2026 06:23:58 +0800 Subject: [PATCH 6/6] Serialize the confirmed delete with rule edits and sweep racing rebinds The confirmed delete now runs under the admin mutation lock shared with model-rule updates, and a second identity sweep after a successful removal catches rebinds from writers outside that lock, so no rule can keep referencing a deleted credential. --- app/admin_api.py | 2 ++ converter.py | 25 ++++++++++++++++--------- tests/test_credential_unbind.py | 20 ++++++++++++++++++++ 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/app/admin_api.py b/app/admin_api.py index 24814c1..2bc0618 100644 --- a/app/admin_api.py +++ b/app/admin_api.py @@ -82,6 +82,8 @@ def install_admin(app, config, gateway): # otherwise never clear a snapshot belonging to a superseded key. auth.reconcile() mutation_lock = threading.RLock() + # The confirmed credential delete shares this lock so rule edits cannot interleave. + config["admin_mutation_lock"] = mutation_lock oauth_lock = threading.RLock() oauth_tasks = OrderedDict() diff --git a/converter.py b/converter.py index 6d11572..257c68a 100644 --- a/converter.py +++ b/converter.py @@ -2092,15 +2092,22 @@ def admin_del_credential(name: str, _check_admin_auth(authorization, x_api_key) pool = CONFIG.get("cred_pool") rollback = {} - if CONFIG.get("management") is not None: - identity = CONFIG["management"].admin_delete_guard(os.path.basename(name), unbind=unbind) - if identity is not None: - rollback = CONFIG["management"].admin_unbind_credential(identity) - if pool is None or not pool.remove_file(os.path.basename(name)): - # A failed removal rolls the unbind back so routing never widens silently. - if CONFIG.get("management") is not None and rollback: - CONFIG["management"].admin_restore_bindings(rollback) - raise HTTPException(status_code=404, detail={"error": {"message": f"凭据不在池中: {name}", "type": "invalid_request_error"}}) + identity = None + management = CONFIG.get("management") + # Serialize with model-rule edits so a rebind cannot slip between unbind and removal. + with CONFIG.get("admin_mutation_lock") or nullcontext(): + if management is not None: + identity = management.admin_delete_guard(os.path.basename(name), unbind=unbind) + if identity is not None: + rollback = management.admin_unbind_credential(identity) + if pool is None or not pool.remove_file(os.path.basename(name)): + # A failed removal rolls the unbind back so routing never widens silently. + if management is not None and rollback: + management.admin_restore_bindings(rollback) + raise HTTPException(status_code=404, detail={"error": {"message": f"凭据不在池中: {name}", "type": "invalid_request_error"}}) + if identity is not None and management is not None: + # Sweep any rebind that raced the removal from outside the shared lock. + management.admin_unbind_credential(identity) return {"removed": os.path.basename(name)} diff --git a/tests/test_credential_unbind.py b/tests/test_credential_unbind.py index 0afa93c..082aca6 100644 --- a/tests/test_credential_unbind.py +++ b/tests/test_credential_unbind.py @@ -10,6 +10,7 @@ import unittest from types import SimpleNamespace from unittest.mock import patch +import threading from fastapi import HTTPException @@ -87,6 +88,25 @@ def test_route_keeps_bindings_when_removal_fails(self): self.assertEqual(self.store.snapshot()["models"]["glm-4-flash"]["credential_ids"], []) self.assertFalse(self.path.exists()) + def test_route_sweeps_a_rebind_that_raced_the_removal(self): + self.bind() + config = dict(self.config, api_key="secret", management=self.management, + admin_mutation_lock=threading.RLock()) + self.enterContext(patch.dict(converter.CONFIG, config)) + client = TestClient(converter.app) + headers = {"X-Api-Key": "secret"} + real_remove = self.pool.remove_file + def rebind_during_removal(file_name): + # A writer outside the shared lock rebinds while the credential is pooled. + revision = self.store.snapshot()["revision"] + self.store.update_model("glm-4-flash", {"credential_ids": [self.identity]}, revision) + return real_remove(file_name) + with patch.object(self.pool, "remove_file", side_effect=rebind_during_removal): + response = client.delete("/admin/credentials/account.info?unbind=1", headers=headers) + self.assertEqual(response.status_code, 200, response.text) + self.assertEqual(self.store.snapshot()["models"]["glm-4-flash"]["credential_ids"], []) + self.assertFalse(self.path.exists()) + def test_unbound_and_unknown_credentials_stay_on_the_fast_path(self): self.assertIsNone(self.management.admin_delete_guard("account.info")) # No bindings. self.assertIsNone(self.management.admin_delete_guard("missing.info", unbind=True)) # Pool reports 404.