Skip to content

Commit 2d631a4

Browse files
committed
feat(auth): add migrate=False readonly dep; fix test fixtures for get_credential_manager_readonly
- Add migrate parameter to CredentialManager.__init__ (default True) so GET-only endpoints skip the machine-wide migration on read paths - Add get_credential_manager_readonly dependency in settings_v2 and github_integrations_v2 routers; wire it to list_key_status, verify_key, get_status, and get_issues - Update all test fixtures that override get_credential_manager to also override get_credential_manager_readonly, preventing test hangs caused by the unoverridden dep hitting the real keyring/CredentialStore
1 parent 8ad4495 commit 2d631a4

9 files changed

Lines changed: 58 additions & 9 deletions

codeframe/core/credentials.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -638,17 +638,26 @@ class CredentialManager:
638638
``user_id=None`` is the machine-wide store itself and never migrates.
639639
"""
640640

641-
def __init__(self, storage_dir: Optional[Path] = None, user_id: Optional[int] = None):
641+
def __init__(
642+
self,
643+
storage_dir: Optional[Path] = None,
644+
user_id: Optional[int] = None,
645+
migrate: bool = True,
646+
):
642647
"""Initialize credential manager.
643648
644649
Args:
645650
storage_dir: Directory for credential storage
646651
user_id: Scope credentials to this user; None keeps the machine-wide store
652+
migrate: Whether to run the machine-wide → per-user migration on first
653+
construction for this user. Pass ``False`` from read-only endpoints
654+
so that a plain GET cannot write credentials into a new tenant store
655+
(the admin-scoped PUT/POST paths keep the default ``True``).
647656
"""
648657
self._user_id = user_id
649658
self._storage_dir = storage_dir
650659
self._store = CredentialStore(storage_dir, user_id=user_id)
651-
if user_id is not None:
660+
if migrate and user_id is not None:
652661
if self._store.storage_dir not in _MIGRATION_COMPLETE:
653662
self._migrate_machine_wide_entries()
654663
_MIGRATION_COMPLETE.add(self._store.storage_dir)

codeframe/ui/routers/github_integrations_v2.py

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,18 @@ def get_credential_manager(auth: dict = Depends(require_auth)) -> CredentialMana
5959
6060
``user_id=None`` (auth disabled / self-hosted) yields the machine-wide
6161
store. Overridden in tests to point at an isolated temp directory.
62+
Runs the machine-wide migration — use only on write paths.
6263
"""
63-
return CredentialManager(user_id=auth.get("user_id"))
64+
return CredentialManager(user_id=auth.get("user_id"), migrate=True)
65+
66+
67+
def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> CredentialManager:
68+
"""Read-only variant: scoped to the authenticated user but skips migration.
69+
70+
Used on GET endpoints so that a plain status check cannot trigger a
71+
credential write into a new tenant's store (#790).
72+
"""
73+
return CredentialManager(user_id=auth.get("user_id"), migrate=False)
6474

6575

6676
class ConnectRequest(BaseModel):
@@ -178,7 +188,7 @@ def _issue_cache_invalidate(repo: str, user_id: Optional[int]) -> None:
178188
async def get_status(
179189
request: Request,
180190
workspace: Workspace = Depends(get_v2_workspace),
181-
manager: CredentialManager = Depends(get_credential_manager),
191+
manager: CredentialManager = Depends(get_credential_manager_readonly),
182192
) -> StatusResponse:
183193
"""Report whether a GitHub repo is connected for this workspace.
184194
@@ -339,7 +349,7 @@ async def get_issues(
339349
search: str = Query("", description="Free-text title/body search"),
340350
label: str = Query("", description="Filter by a single label name"),
341351
workspace: Workspace = Depends(get_v2_workspace),
342-
manager: CredentialManager = Depends(get_credential_manager),
352+
manager: CredentialManager = Depends(get_credential_manager_readonly),
343353
auth: dict = Depends(require_auth),
344354
) -> GitHubIssuesResponse:
345355
"""List the connected repository's **open** issues for the import browser.

codeframe/ui/routers/settings_v2.py

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,18 @@ def get_credential_manager(auth: dict = Depends(require_auth)) -> CredentialMana
8383
8484
``user_id=None`` (auth disabled / self-hosted) yields the machine-wide
8585
store. Overridden in tests to point at an isolated temp directory.
86+
Runs the machine-wide migration — use only on write (admin-scoped) paths.
8687
"""
87-
return CredentialManager(user_id=auth.get("user_id"))
88+
return CredentialManager(user_id=auth.get("user_id"), migrate=True)
89+
90+
91+
def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> CredentialManager:
92+
"""Read-only variant: scoped to the authenticated user but skips migration.
93+
94+
Used on GET endpoints so that a plain status check cannot trigger a
95+
credential write into a new tenant's store (#790).
96+
"""
97+
return CredentialManager(user_id=auth.get("user_id"), migrate=False)
8898

8999

90100
def _config_to_response(config: EnvironmentConfig) -> AgentSettingsResponse:
@@ -236,7 +246,7 @@ def _build_status(
236246
@rate_limit_standard()
237247
async def list_key_status(
238248
request: Request,
239-
manager: CredentialManager = Depends(get_credential_manager),
249+
manager: CredentialManager = Depends(get_credential_manager_readonly),
240250
) -> list[KeyStatusResponse]:
241251
"""Return status of each known API key without exposing plaintext."""
242252
return [_build_status(p, manager) for p in KEY_PROVIDERS]
@@ -392,7 +402,7 @@ def _verify_openai_sync(key: str) -> tuple[bool, str]:
392402
async def verify_key(
393403
body: VerifyKeyRequest,
394404
request: Request,
395-
manager: CredentialManager = Depends(get_credential_manager),
405+
manager: CredentialManager = Depends(get_credential_manager_readonly),
396406
) -> VerifyKeyResponse:
397407
"""Live-verify a key against its provider.
398408

tests/ui/test_credential_tenant_isolation.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,9 @@ def _workspace_dep(auth: dict = Depends(require_auth)):
8787
return workspaces[uid]
8888

8989
app.dependency_overrides[settings_v2.get_credential_manager] = _manager_dep
90+
app.dependency_overrides[settings_v2.get_credential_manager_readonly] = _manager_dep
9091
app.dependency_overrides[github_integrations_v2.get_credential_manager] = _manager_dep
92+
app.dependency_overrides[github_integrations_v2.get_credential_manager_readonly] = _manager_dep
9193
app.dependency_overrides[get_v2_workspace] = _workspace_dep
9294

9395
client = TestClient(app)
@@ -124,7 +126,7 @@ class TestManagerDependencyScoping:
124126
"""The production dependency builds the manager from auth["user_id"]."""
125127

126128
class _FakeCM:
127-
def __init__(self, storage_dir=None, user_id=None):
129+
def __init__(self, storage_dir=None, user_id=None, migrate=True):
128130
self.user_id = user_id
129131

130132
def test_settings_manager_dep_scopes_to_auth_user(self, monkeypatch):

tests/ui/test_github_integrations_v2.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,9 @@ def client(workspace, manager):
6363
app.dependency_overrides[github_integrations_v2.get_credential_manager] = (
6464
lambda: manager
6565
)
66+
app.dependency_overrides[github_integrations_v2.get_credential_manager_readonly] = (
67+
lambda: manager
68+
)
6669
return TestClient(app)
6770

6871

tests/ui/test_hosted_credential_block.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,17 +62,23 @@ def hosted_client(tmp_path, monkeypatch):
6262

6363
app = server.app
6464
app.dependency_overrides[settings_v2.get_credential_manager] = lambda: manager
65+
app.dependency_overrides[settings_v2.get_credential_manager_readonly] = lambda: manager
6566
app.dependency_overrides[github_integrations_v2.get_credential_manager] = (
6667
lambda: manager
6768
)
69+
app.dependency_overrides[github_integrations_v2.get_credential_manager_readonly] = (
70+
lambda: manager
71+
)
6872
app.dependency_overrides[get_v2_workspace] = lambda: workspace
6973

7074
yield TestClient(app)
7175

7276
# Restore the shared app for other tests in this process.
7377
for dep in (
7478
settings_v2.get_credential_manager,
79+
settings_v2.get_credential_manager_readonly,
7580
github_integrations_v2.get_credential_manager,
81+
github_integrations_v2.get_credential_manager_readonly,
7682
get_v2_workspace,
7783
):
7884
app.dependency_overrides.pop(dep, None)

tests/ui/test_settings_v2.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,9 @@ def keys_client(temp_credentials_dir):
8787
app.dependency_overrides[settings_v2.get_credential_manager] = (
8888
lambda: temp_credentials_dir
8989
)
90+
app.dependency_overrides[settings_v2.get_credential_manager_readonly] = (
91+
lambda: temp_credentials_dir
92+
)
9093
yield TestClient(app)
9194

9295

tests/ui/test_v2_auth_enforcement.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,9 @@ def _build_auth_app(tmp_path, monkeypatch, *, enable_test_endpoints):
103103
cred_manager._store = cred_store
104104
for dep in (
105105
settings_v2.get_credential_manager,
106+
settings_v2.get_credential_manager_readonly,
106107
github_integrations_v2.get_credential_manager,
108+
github_integrations_v2.get_credential_manager_readonly,
107109
):
108110
server.app.dependency_overrides[dep] = lambda: cred_manager
109111

@@ -115,7 +117,9 @@ def _pop_credential_overrides(app) -> None:
115117

116118
for dep in (
117119
settings_v2.get_credential_manager,
120+
settings_v2.get_credential_manager_readonly,
118121
github_integrations_v2.get_credential_manager,
122+
github_integrations_v2.get_credential_manager_readonly,
119123
):
120124
app.dependency_overrides.pop(dep, None)
121125

tests/ui/test_v2_scope_enforcement.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,9 @@ def scoped_app(tmp_path, monkeypatch):
6969
cred_manager._store = cred_store
7070
cred_deps = (
7171
settings_v2.get_credential_manager,
72+
settings_v2.get_credential_manager_readonly,
7273
github_integrations_v2.get_credential_manager,
74+
github_integrations_v2.get_credential_manager_readonly,
7375
)
7476
for dep in cred_deps:
7577
server.app.dependency_overrides[dep] = lambda: cred_manager

0 commit comments

Comments
 (0)