diff --git a/codeframe/core/credentials.py b/codeframe/core/credentials.py index d08256a4..7f8ce26f 100644 --- a/codeframe/core/credentials.py +++ b/codeframe/core/credentials.py @@ -26,7 +26,9 @@ import logging import os import platform +import threading import uuid +from contextlib import ExitStack from dataclasses import dataclass, field from datetime import datetime, timezone from enum import Enum @@ -42,6 +44,16 @@ keyring = None KeyringError = Exception +try: + from keyring.errors import PasswordDeleteError +except ImportError: # pragma: no cover (older keyring without this subclass) + PasswordDeleteError = KeyringError + +try: + from filelock import FileLock +except ImportError: # pragma: no cover (filelock is a declared dependency) + FileLock = None # type: ignore[misc,assignment] + from cryptography.fernet import Fernet, InvalidToken from cryptography.hazmat.primitives import hashes from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC @@ -55,6 +67,35 @@ SALT_FILE_NAME = "salt" DEFAULT_STORAGE_DIR = Path.home() / ".codeframe" +# Per-process memo of completed machine-wide → per-user migrations (#790). +# CredentialManager is constructed per request, but migration is idempotent +# (copy-only), so one run per user storage_dir per process is enough — the +# next process retries anything that failed. Tests that build managers via +# ``CredentialManager.__new__`` bypass __init__ and thus this memo entirely. +# Tests reset it with ``_MIGRATION_COMPLETE.clear()``. +_MIGRATION_COMPLETE: set[Path] = set() + +# Thread locks serialize concurrent migrations for the same storage root within +# a process. The optional file lock (requires ``filelock``) extends that +# serialization across processes. +_MIGRATION_THREAD_LOCKS: dict[Path, threading.Lock] = {} + + +def _get_migration_locks(storage_dir: Path) -> tuple[threading.Lock, Any | None]: + """Return the migration locks for ``storage_dir``. + + The returned tuple is ``(thread_lock, file_lock_or_none)``. ``file_lock`` + is ``None`` when ``filelock`` is not installed; callers fall back to the + thread lock only and log a warning that cross-process serialization is + disabled. + """ + lock_path = storage_dir / ".migration.lock" + thread_lock = _MIGRATION_THREAD_LOCKS.setdefault(lock_path, threading.Lock()) + file_lock: Any | None = None + if FileLock is not None: + file_lock = FileLock(str(lock_path)) + return thread_lock, file_lock + class CredentialSource(str, Enum): """Source of a credential.""" @@ -329,18 +370,42 @@ class CredentialStore: """Low-level credential storage. Uses platform keyring as primary storage with encrypted file fallback. + + Storage is machine-wide by default (``user_id=None``). With a ``user_id`` + the store is scoped to that user (#790): keyring entries live under a + per-user service name and the encrypted-file fallback lives under + ``/users//`` with its own salt — the per-directory salt + yields per-user encryption keys from the unchanged key derivation. """ - def __init__(self, storage_dir: Optional[Path] = None): + def __init__(self, storage_dir: Optional[Path] = None, user_id: Optional[int] = None): """Initialize credential store. Args: storage_dir: Directory for encrypted file storage + user_id: Scope storage to this user; None keeps machine-wide storage """ - self.storage_dir = storage_dir or DEFAULT_STORAGE_DIR + self.user_id = user_id + self.storage_dir = self._resolve_storage_dir(storage_dir, user_id) + self._keyring_service_name = self._resolve_keyring_service_name(user_id) self._keyring_available = self._check_keyring() self._fernet: Optional[Fernet] = None + @staticmethod + def _resolve_storage_dir(storage_dir: Optional[Path], user_id: Optional[int]) -> Path: + """Per-user stores live in ``/users//``.""" + base = storage_dir or DEFAULT_STORAGE_DIR + if user_id is None: + return base + return base / "users" / str(user_id) + + @staticmethod + def _resolve_keyring_service_name(user_id: Optional[int]) -> str: + """Per-user keyring entries are addressed under their own service.""" + if user_id is None: + return KEYRING_SERVICE_NAME + return f"{KEYRING_SERVICE_NAME}-user-{user_id}" + def _check_keyring(self) -> bool: """Check if keyring is available and working.""" if not KEYRING_AVAILABLE: @@ -411,6 +476,15 @@ def _load_encrypted_store(self) -> dict[str, dict]: def _save_encrypted_store(self, store: dict[str, dict]) -> None: """Save all credentials to encrypted file.""" self.storage_dir.mkdir(parents=True, exist_ok=True) + if self.user_id is not None: + # Per-user dirs are 0700 so other local accounts cannot enumerate + # tenant ids (#790). The machine-wide base dir keeps its default + # perms. Best-effort — Windows does not honor POSIX modes. + for directory in (self.storage_dir, self.storage_dir.parent): + try: + directory.chmod(0o700) + except OSError: # pragma: no cover (chmod may fail on Windows) + pass file_path = self._get_encrypted_file_path() fernet = self._get_fernet() @@ -446,13 +520,13 @@ def store(self, credential: Credential) -> None: # Try keyring first if self._keyring_available: try: - keyring.set_password(KEYRING_SERVICE_NAME, key, data) + keyring.set_password(self._keyring_service_name, key, data) logger.debug(f"Stored {key} in keyring") return except Exception as e: logger.warning(f"Keyring storage failed, using encrypted file: {e}") try: - keyring.delete_password(KEYRING_SERVICE_NAME, key) + keyring.delete_password(self._keyring_service_name, key) except Exception: pass self._keyring_available = False @@ -479,7 +553,7 @@ def retrieve(self, provider: CredentialProvider) -> Optional[Credential]: # Try keyring first if self._keyring_available: try: - data = keyring.get_password(KEYRING_SERVICE_NAME, key) + data = keyring.get_password(self._keyring_service_name, key) if data: return Credential.from_dict(json.loads(data)) except (KeyError, TypeError, ValueError, json.JSONDecodeError) as e: @@ -509,8 +583,12 @@ def delete(self, provider: CredentialProvider) -> None: # Try keyring if self._keyring_available: try: - keyring.delete_password(KEYRING_SERVICE_NAME, key) + keyring.delete_password(self._keyring_service_name, key) logger.debug(f"Deleted {key} from keyring") + except PasswordDeleteError: + # Not in the keyring (e.g. a file-only entry) — that is + # "nothing to do", not a failure. Continue to file cleanup. + logger.debug(f"{key} not present in keyring; nothing to delete there") except Exception as e: logger.warning(f"Keyring deletion failed: {e}") raise @@ -553,15 +631,80 @@ class CredentialManager: Provides environment variable override, storage abstraction, and credential lifecycle management. + + With ``user_id`` the underlying store is scoped to that user (#790); the + first per-user manager also copies any legacy machine-wide entries into + the user's store while leaving the machine-wide source intact. + ``user_id=None`` is the machine-wide store itself and never migrates. """ - def __init__(self, storage_dir: Optional[Path] = None): + def __init__( + self, + storage_dir: Optional[Path] = None, + user_id: Optional[int] = None, + migrate: bool = True, + ): """Initialize credential manager. Args: storage_dir: Directory for credential storage + user_id: Scope credentials to this user; None keeps the machine-wide store + migrate: Whether to run the machine-wide → per-user migration on first + construction for this user. Pass ``False`` from read-only endpoints + so that a plain GET cannot write credentials into a new tenant store + (the admin-scoped PUT/POST paths keep the default ``True``). + """ + self._user_id = user_id + self._storage_dir = storage_dir + self._store = CredentialStore(storage_dir, user_id=user_id) + if migrate and user_id is not None: + if self._store.storage_dir not in _MIGRATION_COMPLETE: + self._migrate_machine_wide_entries() + _MIGRATION_COMPLETE.add(self._store.storage_dir) + + def _migrate_machine_wide_entries(self) -> None: + """Copy legacy machine-wide credentials into this user's store (#790). + + Probes every known provider (the keyring backend cannot enumerate, so + the enum is the probe list — same trick as ``list_credentials``), copies + entries the user's store lacks, and leaves the machine-wide entries in + place. Idempotent; first-come-first-served for the per-user copy only. + The machine-wide file + salt are left in place so legacy managers + (CLI/background, ``user_id=None``) continue to work and autoclose keeps + seeing the source credential. + + A thread lock and an optional file lock (when ``filelock`` is installed) + serialize concurrent migrations for the same storage root, protecting + the per-user store's read-modify-write save from concurrent first-time + requests for the same user. """ - self._store = CredentialStore(storage_dir) + machine_store = CredentialStore(self._storage_dir) + if FileLock is None: + logger.warning( + "filelock is not installed; cross-process migration serialization " + "is disabled. Install filelock for multi-process deployments." + ) + + thread_lock, file_lock = _get_migration_locks(machine_store.storage_dir) + with ExitStack() as stack: + stack.enter_context(thread_lock) + if file_lock is not None: + stack.enter_context(file_lock) + + copied: list[str] = [] + for provider in CredentialProvider: + if self._store.retrieve(provider) is not None: + continue + credential = machine_store.retrieve(provider) + if credential is None: + continue + self._store.store(credential) + copied.append(provider.name) + if copied: + logger.info( + f"Copied {len(copied)} legacy credential(s) into user " + f"{self._user_id}'s store: {', '.join(copied)}" + ) def get_credential( self, diff --git a/codeframe/core/github_integration_config.py b/codeframe/core/github_integration_config.py index 278930c3..b2ab0248 100644 --- a/codeframe/core/github_integration_config.py +++ b/codeframe/core/github_integration_config.py @@ -2,7 +2,8 @@ Stores only **non-secret** repo metadata for a connected GitHub repository under ``.codeframe/github_integration.json``. The PAT itself is stored in the -machine-wide ``CredentialManager`` (``CredentialProvider.GIT_GITHUB``) — never +caller-scoped ``CredentialManager`` (``CredentialProvider.GIT_GITHUB``; issue +#790 — per-user when authenticated, machine-wide when auth is disabled) — never in this file. Headless — no FastAPI or HTTP imports (architecture rule #1). Mirrors the diff --git a/codeframe/core/github_issues_service.py b/codeframe/core/github_issues_service.py index 6dc837d1..37e61191 100644 --- a/codeframe/core/github_issues_service.py +++ b/codeframe/core/github_issues_service.py @@ -2,8 +2,9 @@ Headless service used by the Integrations issues endpoint to fetch a connected repository's **open** issues for the import browser UI. Builds on the connection -established in #563: the PAT comes from the machine-wide ``CredentialManager`` -and the ``owner/repo`` from per-workspace ``.codeframe/github_integration.json`` +established in #563: the PAT comes from the caller-scoped ``CredentialManager`` +(issue #790; per-user in hosted mode, machine-wide when auth is disabled) and +the ``owner/repo`` from per-workspace ``.codeframe/github_integration.json`` — this module only performs the GitHub API call given those values. No FastAPI / HTTP-framework imports (architecture rule #1 — core is headless). diff --git a/codeframe/core/tasks.py b/codeframe/core/tasks.py index 5b8cac92..c0967223 100644 --- a/codeframe/core/tasks.py +++ b/codeframe/core/tasks.py @@ -523,8 +523,12 @@ def _dispatch_github_autoclose(workspace: Workspace, task: Task) -> None: task transition. The repo is taken from the task's own ``external_url`` (its source repo) — NOT the workspace's current connection — so completing an older imported task always closes the right issue even after the workspace - is reconnected to a different repository. The PAT comes from the machine-wide - credential store. + is reconnected to a different repository. + + The PAT is resolved from the machine-wide credential store / ``GITHUB_TOKEN`` + environment variable. Per-user stored PATs are **not** used by autoclose + (this is a known limitation of the current headless/background task path; + hosted tenants should supply ``GITHUB_TOKEN`` via environment for autoclose). """ if not task.auto_close_github_issue or task.github_issue_number is None: return diff --git a/codeframe/ui/dependencies.py b/codeframe/ui/dependencies.py index dfc77694..e766b72c 100644 --- a/codeframe/ui/dependencies.py +++ b/codeframe/ui/dependencies.py @@ -99,32 +99,6 @@ def revalidate_workspace_path(workspace_path: str, user_id: Optional[int]) -> Op return None -def forbid_shared_credentials_in_hosted_mode() -> None: - """Block shared machine-wide credential mutation in hosted mode (#718). - - CodeFRAME's credential store (``CredentialManager``) is machine-wide — one - set of LLM keys and one GitHub PAT per host. That is fine for a *self-hosted* - deployment, which is a single trust domain (one operator/team). In *hosted* - (multi-tenant) mode it would let any tenant view (last-4), overwrite, or - delete another tenant's secrets, so credential storage/deletion and GitHub - connect/disconnect are disabled there — hosted tenants supply credentials via - per-instance environment variables instead. Per-user credential scoping is a - tracked follow-up; until then hosted mode fails closed. - """ - # Deferred import: codeframe.ui.server imports from this module at module - # level, so importing it at the top would be circular. - from codeframe.ui.server import is_hosted_mode - - if is_hosted_mode(): - raise HTTPException( - status_code=403, - detail=( - "Shared credential storage is disabled in hosted mode. Provide " - "credentials via environment variables per deployment (#718)." - ), - ) - - def get_workspace_manager(request: Request) -> WorkspaceManager: """Get workspace manager from application state. diff --git a/codeframe/ui/routers/github_integrations_v2.py b/codeframe/ui/routers/github_integrations_v2.py index 0d6f1b6d..e98fe4e1 100644 --- a/codeframe/ui/routers/github_integrations_v2.py +++ b/codeframe/ui/routers/github_integrations_v2.py @@ -6,7 +6,8 @@ GET /status - Report connection status (never exposes the PAT) GET /issues - List the connected repo's open issues (#564) -The PAT is stored machine-wide via ``CredentialManager`` under +The PAT is stored via ``CredentialManager`` scoped to the authenticated user +(issue #790; machine-wide when auth is disabled) under ``CredentialProvider.GIT_GITHUB`` — the same slot the API Keys settings tab (#555) uses. Repo metadata (non-secret) is persisted per-workspace under ``.codeframe/github_integration.json``. The PAT is never returned in any @@ -21,6 +22,8 @@ from fastapi import APIRouter, Depends, HTTPException, Query, Request, Response from pydantic import BaseModel, Field +from codeframe.auth.api_keys import SCOPE_ADMIN +from codeframe.auth.dependencies import require_auth, require_scope from codeframe.core.credentials import CredentialManager, CredentialProvider from codeframe.core.github_connect_service import ( GitHubConnectError, @@ -44,10 +47,7 @@ from codeframe.core import tasks from codeframe.core.workspace import Workspace from codeframe.lib.rate_limiter import rate_limit_ai, rate_limit_standard -from codeframe.ui.dependencies import ( - forbid_shared_credentials_in_hosted_mode, - get_v2_workspace, -) +from codeframe.ui.dependencies import get_v2_workspace from codeframe.ui.response_models import ErrorCodes, api_error logger = logging.getLogger(__name__) @@ -55,12 +55,23 @@ router = APIRouter(prefix="/api/v2/integrations/github", tags=["integrations"]) -def get_credential_manager() -> CredentialManager: - """Dependency: machine-wide CredentialManager. +def get_credential_manager(auth: dict = Depends(require_auth)) -> CredentialManager: + """Dependency: CredentialManager scoped to the authenticated user (#790). + + ``user_id=None`` (auth disabled / self-hosted) yields the machine-wide + store. Overridden in tests to point at an isolated temp directory. + Runs the machine-wide migration — use only on write paths. + """ + return CredentialManager(user_id=auth.get("user_id"), migrate=True) + + +def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> CredentialManager: + """Read-only variant: scoped to the authenticated user but skips migration. - Overridden in tests to point at an isolated temp directory. + Used on GET endpoints so that a plain status check cannot trigger a + credential write into a new tenant's store (#790). """ - return CredentialManager() + return CredentialManager(user_id=auth.get("user_id"), migrate=False) class ConnectRequest(BaseModel): @@ -117,9 +128,11 @@ class ImportResponse(BaseModel): # In-process TTL cache for issue listings (#564). Keyed by the full query -# (repo + page + per_page + search + label); entries expire after 60s to avoid -# hammering GitHub's rate limit on repeated browses. Module-level so it is -# shared across requests within the same server process. Bounded to +# (repo + page + per_page + search + label) plus the calling user — two +# tenants may browse the same repo slug with different PATs and must never +# share payloads (#790). Entries expire after 60s to avoid hammering GitHub's +# rate limit on repeated browses. Module-level so it is shared across requests +# within the same server process. Bounded to # _ISSUE_CACHE_MAX_SIZE entries and swept on every set so search text (an # unbounded key space) can't accumulate stale payloads forever (#761). _ISSUE_CACHE_TTL_SECONDS = 60.0 @@ -156,14 +169,18 @@ def _issue_cache_set(key: str, payload: Any) -> None: _evict_issue_cache() -def _issue_cache_invalidate(repo: str) -> None: - """Drop all cached issue listings for ``repo``. +def _issue_cache_invalidate(repo: str, user_id: Optional[int]) -> None: + """Drop cached issue listings for ``repo`` scoped to ``user_id``. Called after an import so reopening the browse modal doesn't keep offering just-imported issues as selectable (they would now be skipped as dupes). + Keys are ``repo|page|per_page|search|label|user_id``; only drop entries + belonging to the calling user so one tenant's import does not wipe another's + cache for the same repo (#790). """ prefix = f"{repo}|" - for key in [k for k in _ISSUE_CACHE if k.startswith(prefix)]: + suffix = f"|{user_id}" + for key in [k for k in _ISSUE_CACHE if k.startswith(prefix) and k.endswith(suffix)]: _ISSUE_CACHE.pop(key, None) @@ -172,12 +189,12 @@ def _issue_cache_invalidate(repo: str) -> None: async def get_status( request: Request, workspace: Workspace = Depends(get_v2_workspace), - manager: CredentialManager = Depends(get_credential_manager), + manager: CredentialManager = Depends(get_credential_manager_readonly), ) -> StatusResponse: """Report whether a GitHub repo is connected for this workspace. Connected means BOTH the per-workspace repo metadata exists AND the - machine-wide GitHub PAT is present (env var or stored). + calling user's GitHub PAT is present (env var or stored). """ cfg = load_github_integration_config(workspace) has_pat = manager.get_credential(CredentialProvider.GIT_GITHUB) is not None @@ -194,7 +211,6 @@ async def get_status( @router.post( "/connect", response_model=ConnectResponse, - dependencies=[Depends(forbid_shared_credentials_in_hosted_mode)], # shared PAT, not cross-tenant (#718) ) @rate_limit_ai() async def connect( @@ -202,6 +218,7 @@ async def connect( body: ConnectRequest, workspace: Workspace = Depends(get_v2_workspace), manager: CredentialManager = Depends(get_credential_manager), + _auth: dict = Depends(require_scope(SCOPE_ADMIN)), # PAT storage is admin-only (#717/#790) ) -> ConnectResponse: """Validate a PAT against the target repo, then store the PAT + repo metadata. @@ -249,10 +266,10 @@ async def connect( detail=api_error(str(e), ErrorCodes.EXECUTION_FAILED), ) - # Validation passed — store the PAT (machine-wide) and repo metadata. - # The GIT_GITHUB slot is shared machine-wide with the API Keys tab, so - # capture any prior token first to restore it if the config write fails — - # never blindly delete an unrelated, previously working credential. + # Validation passed — store the PAT (per user, #790) and repo metadata. + # The GIT_GITHUB slot is shared with the API Keys tab, so capture any + # prior token first to restore it if the config write fails — never + # blindly delete an unrelated, previously working credential. prior_pat = manager.get_credential(CredentialProvider.GIT_GITHUB) try: manager.set_credential(CredentialProvider.GIT_GITHUB, body.pat) @@ -306,13 +323,13 @@ async def connect( @router.delete( "/disconnect", status_code=204, - dependencies=[Depends(forbid_shared_credentials_in_hosted_mode)], # shared PAT, not cross-tenant (#718) ) @rate_limit_standard() async def disconnect( request: Request, workspace: Workspace = Depends(get_v2_workspace), manager: CredentialManager = Depends(get_credential_manager), + _auth: dict = Depends(require_scope(SCOPE_ADMIN)), # PAT deletion is admin-only (#717/#790) ) -> Response: """Clear stored repo metadata and delete the GitHub PAT. Idempotent.""" clear_github_integration_config(workspace) @@ -335,14 +352,16 @@ async def get_issues( search: str = Query("", description="Free-text title/body search"), label: str = Query("", description="Filter by a single label name"), workspace: Workspace = Depends(get_v2_workspace), - manager: CredentialManager = Depends(get_credential_manager), + manager: CredentialManager = Depends(get_credential_manager_readonly), + auth: dict = Depends(require_auth), ) -> GitHubIssuesResponse: """List the connected repository's **open** issues for the import browser. Requires an established connection (#563): repo metadata in ``.codeframe/github_integration.json`` AND a stored GitHub PAT. Responses - are cached in-process for 60s keyed by the full query to avoid GitHub - rate-limit pressure during paging/searching. The PAT is never returned. + are cached in-process for 60s keyed by the full query plus the calling + user (#790) to avoid GitHub rate-limit pressure during paging/searching. + The PAT is never returned. """ cfg = load_github_integration_config(workspace) pat = manager.get_credential(CredentialProvider.GIT_GITHUB) @@ -360,7 +379,9 @@ async def get_issues( per_page = max(1, min(per_page, 100)) repo = cfg["repo"] - cache_key = f"{repo}|{page}|{per_page}|{search}|{label}" + # The user component keeps the repo|-prefixed invalidation in + # _issue_cache_invalidate working while separating tenants (#790). + cache_key = f"{repo}|{page}|{per_page}|{search}|{label}|{auth.get('user_id')}" cached = _issue_cache_get(cache_key) if cached is not None: return cached @@ -452,7 +473,8 @@ async def import_issues( request: Request, body: ImportRequest, workspace: Workspace = Depends(get_v2_workspace), - manager: CredentialManager = Depends(get_credential_manager), + manager: CredentialManager = Depends(get_credential_manager_readonly), # read-only: uses PAT, doesn't store (#790) + auth: dict = Depends(require_auth), ) -> ImportResponse: """Import selected GitHub issues as CodeFRAME tasks (issue #565). @@ -558,7 +580,8 @@ async def import_issues( # Invalidate the browse cache so a re-open reflects current duplicate state. # Always — even a skipped-only import (issues created by another tab/process) # must drop the stale listing that still offers them as selectable. - _issue_cache_invalidate(repo) + # Scoped to the caller so one tenant's import does not flush another's cache. + _issue_cache_invalidate(repo, auth.get("user_id")) return ImportResponse( created=created, skipped=skipped, total_created=len(created) diff --git a/codeframe/ui/routers/settings_v2.py b/codeframe/ui/routers/settings_v2.py index a122158f..246f3eaa 100644 --- a/codeframe/ui/routers/settings_v2.py +++ b/codeframe/ui/routers/settings_v2.py @@ -11,9 +11,11 @@ PUT /api/v2/settings/notifications - Save outbound webhook config POST /api/v2/settings/notifications/test - Fire a test payload and return HTTP status -Key management is machine-wide (CredentialManager / keyring) and does not -require a workspace. Env vars take precedence at read time. Notifications -config is per-workspace and persisted under .codeframe/notifications_config.json. +Key management is scoped per authenticated user (CredentialManager keyed by +``auth["user_id"]``; issue #790) and does not require a workspace. With auth +disabled (self-hosted/no-auth) the store is machine-wide. Env vars take +precedence at read time. Notifications config is per-workspace and persisted +under .codeframe/notifications_config.json. """ import logging @@ -26,8 +28,7 @@ from fastapi.concurrency import run_in_threadpool from codeframe.auth.api_keys import SCOPE_ADMIN -from codeframe.auth.dependencies import require_scope -from codeframe.ui.dependencies import forbid_shared_credentials_in_hosted_mode +from codeframe.auth.dependencies import require_auth, require_scope from openai import AuthenticationError as _OpenAIAuthError from openai import OpenAI as _OpenAIClient from pydantic import BaseModel, Field @@ -77,12 +78,23 @@ router = APIRouter(prefix="/api/v2/settings", tags=["settings"]) -def get_credential_manager() -> CredentialManager: - """Dependency: machine-wide CredentialManager. +def get_credential_manager(auth: dict = Depends(require_auth)) -> CredentialManager: + """Dependency: CredentialManager scoped to the authenticated user (#790). - Overridden in tests to point at an isolated temp directory. + ``user_id=None`` (auth disabled / self-hosted) yields the machine-wide + store. Overridden in tests to point at an isolated temp directory. + Runs the machine-wide migration — use only on write (admin-scoped) paths. """ - return CredentialManager() + return CredentialManager(user_id=auth.get("user_id"), migrate=True) + + +def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> CredentialManager: + """Read-only variant: scoped to the authenticated user but skips migration. + + Used on GET endpoints so that a plain status check cannot trigger a + credential write into a new tenant's store (#790). + """ + return CredentialManager(user_id=auth.get("user_id"), migrate=False) def _config_to_response(config: EnvironmentConfig) -> AgentSettingsResponse: @@ -170,9 +182,10 @@ async def update_settings( # ============================================================================ # API Key Management (issue #555) # -# These endpoints are machine-wide: they do not require a workspace. Keys are -# stored via CredentialManager (platform keyring or encrypted file fallback). -# Plaintext key values are NEVER returned in any response. +# These endpoints do not require a workspace. Keys are stored via +# CredentialManager scoped to the authenticated user (#790) — machine-wide +# when auth is disabled. Plaintext key values are NEVER returned in any +# response. # ============================================================================ # Map the public provider name to the internal CredentialProvider enum. @@ -233,7 +246,7 @@ def _build_status( @rate_limit_standard() async def list_key_status( request: Request, - manager: CredentialManager = Depends(get_credential_manager), + manager: CredentialManager = Depends(get_credential_manager_readonly), ) -> list[KeyStatusResponse]: """Return status of each known API key without exposing plaintext.""" return [_build_status(p, manager) for p in KEY_PROVIDERS] @@ -242,16 +255,13 @@ async def list_key_status( @router.put( "/keys/{provider}", response_model=KeyStatusResponse, - dependencies=[ - Depends(require_scope(SCOPE_ADMIN)), # credential storage is admin-only (#717) - Depends(forbid_shared_credentials_in_hosted_mode), # not shareable across tenants (#718) - ], ) @rate_limit_standard() async def store_key( provider: str, body: StoreKeyRequest, request: Request, + auth: dict = Depends(require_scope(SCOPE_ADMIN)), # credential storage is admin-only (#717) manager: CredentialManager = Depends(get_credential_manager), ) -> KeyStatusResponse: """Store an API key for the given provider after validating its format.""" @@ -282,15 +292,12 @@ async def store_key( @router.delete( "/keys/{provider}", status_code=204, - dependencies=[ - Depends(require_scope(SCOPE_ADMIN)), # credential deletion is admin-only (#717) - Depends(forbid_shared_credentials_in_hosted_mode), # not shareable across tenants (#718) - ], ) @rate_limit_standard() async def delete_key( provider: str, request: Request, + auth: dict = Depends(require_scope(SCOPE_ADMIN)), # credential deletion is admin-only (#717) manager: CredentialManager = Depends(get_credential_manager), ) -> Response: """Delete a stored credential. Idempotent — non-existent keys are a no-op.""" @@ -395,7 +402,7 @@ def _verify_openai_sync(key: str) -> tuple[bool, str]: async def verify_key( body: VerifyKeyRequest, request: Request, - manager: CredentialManager = Depends(get_credential_manager), + manager: CredentialManager = Depends(get_credential_manager_readonly), ) -> VerifyKeyResponse: """Live-verify a key against its provider. diff --git a/codeframe/ui/server.py b/codeframe/ui/server.py index 68574f94..afdda59d 100644 --- a/codeframe/ui/server.py +++ b/codeframe/ui/server.py @@ -124,26 +124,37 @@ def _validate_security_config(): Raises: RuntimeError: If auth is enabled (or hosted mode) while the default - secret is in use and no escape hatch applies. + secret is in use and no escape hatch applies, or if hosted mode is + requested with authentication disabled. """ from codeframe.auth.manager import SECRET, DEFAULT_SECRET from codeframe.auth.dependencies import auth_required - if SECRET != DEFAULT_SECRET: - logger.info("🔐 AUTH_SECRET configured (custom secret in use)") - return - deployment_mode = get_deployment_mode() - # Hosted/production mode: never tolerate the default secret. The dev escape - # hatch is intentionally NOT honored here. + # Hosted/production mode checks must run regardless of whether the secret is + # custom, because hosted mode must never run with auth disabled. if deployment_mode == DeploymentMode.HOSTED: - raise RuntimeError( - "🚨 SECURITY: AUTH_SECRET must be set in hosted/production mode. " - "Using the default secret compromises all JWT tokens. " - "Set the AUTH_SECRET environment variable to a secure random value " - "(e.g. `openssl rand -hex 32`)." - ) + # Never tolerate the default secret. The dev escape hatch is + # intentionally NOT honored here. + if SECRET == DEFAULT_SECRET: + raise RuntimeError( + "🚨 SECURITY: AUTH_SECRET must be set in hosted/production mode. " + "Using the default secret compromises all JWT tokens. " + "Set the AUTH_SECRET environment variable to a secure random value " + "(e.g. `openssl rand -hex 32`)." + ) + if not auth_required(): + raise RuntimeError( + "Hosted mode requires authentication. " + "Set CODEFRAME_AUTH_REQUIRED=true (or equivalent) and configure a secure AUTH_SECRET." + ) + # Hosted mode with a real secret and auth enabled is safe to start. + return + + if SECRET != DEFAULT_SECRET: + logger.info("🔐 AUTH_SECRET configured (custom secret in use)") + return # Self-hosted with auth enabled: forging a JWT is trivial with the known # default secret, so refuse to start unless the operator has explicitly diff --git a/pyproject.toml b/pyproject.toml index 45f7a596..9297246c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -55,6 +55,7 @@ dependencies = [ "pytest-json-report>=1.5.0", "hypothesis>=6.148.0", "fastapi-users[sqlalchemy]>=15.0.2", + "filelock>=3.0", "python-jose[cryptography]>=3.4.0", "passlib[argon2]>=1.7.4", "keyring>=24.0.0", diff --git a/tests/core/test_credentials_per_user.py b/tests/core/test_credentials_per_user.py new file mode 100644 index 00000000..b2c17fb4 --- /dev/null +++ b/tests/core/test_credentials_per_user.py @@ -0,0 +1,500 @@ +"""Per-user credential scoping (issue #790). + +In hosted multi-tenant mode every user gets an isolated credential store: +- keyring entries are addressed under a per-user service name + (``codeframe-credentials-user-``), and +- the encrypted-file fallback lives under ``/users//`` with its + own salt + credentials.encrypted (per-user encryption keys fall out of the + per-directory salt). + +``user_id=None`` keeps the legacy machine-wide behavior byte-for-byte — that is +the no-auth/self-hosted/CLI path. + +The first ``CredentialManager(user_id=)`` also migrates legacy machine-wide +entries into that user's store (copy what the user lacks, leaving the machine- +wide entries in place), so a self-hosted install that turns on multi-user auth +does not strand previously configured credentials. +""" + +import logging +import threading +from unittest.mock import MagicMock, patch + +import pytest + +from codeframe.core.credentials import ( + KEYRING_SERVICE_NAME, + Credential, + CredentialManager, + CredentialProvider, + CredentialSource, + CredentialStore, +) + +pytestmark = pytest.mark.v2 + +ANTHROPIC_A = "test-anthropic-key-" + "a" * 20 +ANTHROPIC_B = "test-anthropic-key-" + "b" * 20 +GITHUB_A = "test-github-token-" + "a" * 20 + + +@pytest.fixture(autouse=True) +def _clear_provider_env(monkeypatch): + """Deterministic source detection: no ambient provider env vars.""" + for provider in CredentialProvider: + monkeypatch.delenv(provider.env_var, raising=False) + + +@pytest.fixture(autouse=True) +def _clear_migration_memo(): + """Reset the per-process migration memo so each test migrates fresh. + + Clears the module-level set in place (rebinding would break the memo the + CredentialManager holds). getattr-guarded: a no-op until the memo exists. + """ + import codeframe.core.credentials as credentials_module + + getattr(credentials_module, "_MIGRATION_COMPLETE", set()).clear() + + +@pytest.fixture +def file_backed(monkeypatch): + """Force every CredentialStore onto the encrypted-file backend.""" + monkeypatch.setattr(CredentialStore, "_check_keyring", lambda self: False) + + +class TestPerUserStorageLayout: + def test_user_storage_dir_is_users_subdir(self, tmp_path, file_backed): + store = CredentialStore(storage_dir=tmp_path, user_id=7) + assert store.storage_dir == tmp_path / "users" / "7" + + def test_machine_wide_storage_dir_unchanged(self, tmp_path, file_backed): + store = CredentialStore(storage_dir=tmp_path) + assert store.storage_dir == tmp_path + + def test_user_store_writes_own_encrypted_file_and_salt(self, tmp_path, file_backed): + store = CredentialStore(storage_dir=tmp_path, user_id=1) + store.store( + Credential(provider=CredentialProvider.LLM_ANTHROPIC, value=ANTHROPIC_A) + ) + user_dir = tmp_path / "users" / "1" + assert (user_dir / "credentials.encrypted").exists() + assert (user_dir / "salt").exists() + # Nothing leaked into the machine-wide root. + assert not (tmp_path / "credentials.encrypted").exists() + + def test_user_files_have_owner_only_permissions(self, tmp_path, file_backed): + store = CredentialStore(storage_dir=tmp_path, user_id=1) + store.store( + Credential(provider=CredentialProvider.LLM_ANTHROPIC, value=ANTHROPIC_A) + ) + user_dir = tmp_path / "users" / "1" + assert ((user_dir / "credentials.encrypted").stat().st_mode & 0o777) == 0o600 + assert ((user_dir / "salt").stat().st_mode & 0o777) == 0o600 + # Tenant ids must not be enumerable by other local accounts. + assert (user_dir.stat().st_mode & 0o777) == 0o700 + assert ((tmp_path / "users").stat().st_mode & 0o777) == 0o700 + + def test_different_users_use_different_encryption_keys(self, tmp_path, file_backed): + """Each user dir has its own salt, so ciphertext is not interchangeable.""" + store_a = CredentialStore(storage_dir=tmp_path, user_id=1) + store_b = CredentialStore(storage_dir=tmp_path, user_id=2) + store_a.store( + Credential(provider=CredentialProvider.LLM_ANTHROPIC, value=ANTHROPIC_A) + ) + salt_a = (tmp_path / "users" / "1" / "salt").read_bytes() + store_b.store( + Credential(provider=CredentialProvider.LLM_ANTHROPIC, value=ANTHROPIC_A) + ) + salt_b = (tmp_path / "users" / "2" / "salt").read_bytes() + assert salt_a != salt_b + + +class TestPerUserKeyringService: + def test_machine_wide_service_name_unchanged(self, tmp_path): + with patch("codeframe.core.credentials.keyring") as mock_keyring: + mock_keyring.get_keyring.return_value = MagicMock() + store = CredentialStore(storage_dir=tmp_path) + store.store( + Credential(provider=CredentialProvider.LLM_ANTHROPIC, value=ANTHROPIC_A) + ) + assert mock_keyring.set_password.call_args[0][0] == KEYRING_SERVICE_NAME + + def test_per_user_service_name(self, tmp_path): + with patch("codeframe.core.credentials.keyring") as mock_keyring: + mock_keyring.get_keyring.return_value = MagicMock() + store = CredentialStore(storage_dir=tmp_path, user_id=7) + expected = f"{KEYRING_SERVICE_NAME}-user-7" + + store.store( + Credential(provider=CredentialProvider.LLM_ANTHROPIC, value=ANTHROPIC_A) + ) + assert mock_keyring.set_password.call_args[0][0] == expected + + store.retrieve(CredentialProvider.LLM_ANTHROPIC) + assert mock_keyring.get_password.call_args[0][0] == expected + + store.delete(CredentialProvider.LLM_ANTHROPIC) + assert mock_keyring.delete_password.call_args[0][0] == expected + + def test_per_user_service_names_differ_between_users(self, tmp_path): + with patch("codeframe.core.credentials.keyring") as mock_keyring: + mock_keyring.get_keyring.return_value = MagicMock() + store_1 = CredentialStore(storage_dir=tmp_path, user_id=1) + store_2 = CredentialStore(storage_dir=tmp_path, user_id=2) + cred = Credential( + provider=CredentialProvider.LLM_ANTHROPIC, value=ANTHROPIC_A + ) + store_1.store(cred) + service_1 = mock_keyring.set_password.call_args[0][0] + store_2.store(cred) + service_2 = mock_keyring.set_password.call_args[0][0] + assert service_1 != service_2 + + +class TestPerUserIsolation: + def test_two_users_same_storage_dir_are_isolated(self, tmp_path, file_backed): + user_a = CredentialManager(storage_dir=tmp_path, user_id=1) + user_b = CredentialManager(storage_dir=tmp_path, user_id=2) + + user_a.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + + assert user_a.get_credential(CredentialProvider.LLM_ANTHROPIC) == ANTHROPIC_A + assert user_b.get_credential(CredentialProvider.LLM_ANTHROPIC) is None + assert ( + user_b.get_credential_source(CredentialProvider.LLM_ANTHROPIC) + == CredentialSource.NOT_FOUND + ) + + def test_user_delete_does_not_touch_other_user(self, tmp_path, file_backed): + user_a = CredentialManager(storage_dir=tmp_path, user_id=1) + user_b = CredentialManager(storage_dir=tmp_path, user_id=2) + user_a.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + + user_b.delete_credential(CredentialProvider.LLM_ANTHROPIC) # no-op for B + + assert user_a.get_credential(CredentialProvider.LLM_ANTHROPIC) == ANTHROPIC_A + + def test_machine_wide_and_user_stores_are_isolated(self, tmp_path, file_backed): + machine = CredentialManager(storage_dir=tmp_path) + machine.set_credential(CredentialProvider.GIT_GITHUB, GITHUB_A) + + # Copy-only migration gives every new user a copy of the legacy entries + # while leaving the machine-wide source intact. + first = CredentialManager(storage_dir=tmp_path, user_id=1) + assert first.get_credential(CredentialProvider.GIT_GITHUB) == GITHUB_A + second = CredentialManager(storage_dir=tmp_path, user_id=2) + assert second.get_credential(CredentialProvider.GIT_GITHUB) == GITHUB_A + + # Writes to a user store never appear machine-wide. + second.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_B) + assert machine.get_credential(CredentialProvider.LLM_ANTHROPIC) is None + + def test_env_var_precedence_unchanged_for_per_user_manager( + self, tmp_path, file_backed, monkeypatch + ): + env_value = "test-anthropic-env-override-value-0000" + monkeypatch.setenv("ANTHROPIC_API_KEY", env_value) + manager = CredentialManager(storage_dir=tmp_path, user_id=1) + manager.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + + assert manager.get_credential(CredentialProvider.LLM_ANTHROPIC) == env_value + assert ( + manager.get_credential_source(CredentialProvider.LLM_ANTHROPIC) + == CredentialSource.ENVIRONMENT + ) + + +class TestMachineWideMigration: + """Legacy machine-wide credentials are copied into each per-user store.""" + + def test_first_user_migrates_machine_wide_entries(self, tmp_path, file_backed): + machine = CredentialManager(storage_dir=tmp_path) + machine.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + machine.set_credential(CredentialProvider.GIT_GITHUB, GITHUB_A) + + user1 = CredentialManager(storage_dir=tmp_path, user_id=1) + + # Copied into the user's store... + assert user1.get_credential(CredentialProvider.LLM_ANTHROPIC) == ANTHROPIC_A + assert user1.get_credential(CredentialProvider.GIT_GITHUB) == GITHUB_A + # ...and the machine-wide source is left in place. + machine_store = CredentialStore(storage_dir=tmp_path) + assert machine_store.retrieve(CredentialProvider.LLM_ANTHROPIC).value == ANTHROPIC_A + assert machine_store.retrieve(CredentialProvider.GIT_GITHUB).value == GITHUB_A + # The machine-wide file + salt are left in place. + assert (tmp_path / "credentials.encrypted").exists() + assert (tmp_path / "salt").exists() + + def test_second_user_also_gets_legacy_entries(self, tmp_path, file_backed): + machine = CredentialManager(storage_dir=tmp_path) + machine.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + + CredentialManager(storage_dir=tmp_path, user_id=1) + user2 = CredentialManager(storage_dir=tmp_path, user_id=2) + + # Copy-only migration leaves the legacy source intact, so every user gets + # their own copy. + assert user2.get_credential(CredentialProvider.LLM_ANTHROPIC) == ANTHROPIC_A + assert ( + user2.get_credential_source(CredentialProvider.LLM_ANTHROPIC) + == CredentialSource.STORED + ) + + def test_migration_is_idempotent(self, tmp_path, file_backed): + machine = CredentialManager(storage_dir=tmp_path) + machine.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + + user1 = CredentialManager(storage_dir=tmp_path, user_id=1) + # The user's own subsequent writes survive a repeat migration pass. + user1.set_credential(CredentialProvider.LLM_OPENAI, "test-openai-key-migration-fake-0000") + + again = CredentialManager(storage_dir=tmp_path, user_id=1) + + assert again.get_credential(CredentialProvider.LLM_ANTHROPIC) == ANTHROPIC_A + assert ( + again.get_credential(CredentialProvider.LLM_OPENAI) == "test-openai-key-migration-fake-0000" + ) + # Machine-wide store keeps the legacy source; nothing is duplicated. + machine_store = CredentialStore(storage_dir=tmp_path) + assert machine_store.retrieve(CredentialProvider.LLM_ANTHROPIC).value == ANTHROPIC_A + assert machine_store.list_providers() == [CredentialProvider.LLM_ANTHROPIC] + + def test_migration_skips_entries_the_user_already_has(self, tmp_path, file_backed): + # Seed the user's store directly (bypassing migration) so the user already + # holds their own Anthropic key before the first per-user manager runs. + pre_store = CredentialStore(storage_dir=tmp_path, user_id=1) + pre_store.store( + Credential(provider=CredentialProvider.LLM_ANTHROPIC, value=ANTHROPIC_B) + ) + + machine = CredentialManager(storage_dir=tmp_path) + machine.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + machine.set_credential(CredentialProvider.GIT_GITHUB, GITHUB_A) + + user1 = CredentialManager(storage_dir=tmp_path, user_id=1) + + # The user's own key wins; only the lacking GitHub entry migrates. + assert user1.get_credential(CredentialProvider.LLM_ANTHROPIC) == ANTHROPIC_B + assert user1.get_credential(CredentialProvider.GIT_GITHUB) == GITHUB_A + machine_store = CredentialStore(storage_dir=tmp_path) + # Copy-only: both the skipped Anthropic entry and the copied GitHub entry + # remain machine-wide. + assert machine_store.retrieve(CredentialProvider.LLM_ANTHROPIC).value == ANTHROPIC_A + assert machine_store.retrieve(CredentialProvider.GIT_GITHUB).value == GITHUB_A + + def test_user_id_none_never_migrates(self, tmp_path, file_backed): + machine = CredentialManager(storage_dir=tmp_path) + machine.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + + # Re-constructing machine-wide managers must not move anything. + CredentialManager(storage_dir=tmp_path) + CredentialManager(storage_dir=tmp_path) + + machine_store = CredentialStore(storage_dir=tmp_path) + assert machine_store.retrieve(CredentialProvider.LLM_ANTHROPIC) is not None + assert not (tmp_path / "users").exists() + + def test_migration_logs_at_info(self, tmp_path, file_backed, caplog): + machine = CredentialManager(storage_dir=tmp_path) + machine.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + + with caplog.at_level(logging.INFO, logger="codeframe.core.credentials"): + CredentialManager(storage_dir=tmp_path, user_id=1) + + assert any( + record.levelno == logging.INFO and "copied" in record.message.lower() + for record in caplog.records + ) + + def test_no_log_when_nothing_to_migrate(self, tmp_path, file_backed, caplog): + with caplog.at_level(logging.INFO, logger="codeframe.core.credentials"): + CredentialManager(storage_dir=tmp_path, user_id=1) + + assert not any("copied" in record.message.lower() for record in caplog.records) + + def test_concurrent_migrations_for_different_users(self, tmp_path, file_backed): + """Two first-time users hitting the same legacy secret at once must both + end up with a copy and must not raise. + """ + machine = CredentialManager(storage_dir=tmp_path) + machine.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + machine.set_credential(CredentialProvider.GIT_GITHUB, GITHUB_A) + + managers: dict[int, CredentialManager] = {} + errors: list[Exception] = [] + + def make_user(uid: int) -> None: + try: + managers[uid] = CredentialManager(storage_dir=tmp_path, user_id=uid) + except Exception as exc: + errors.append(exc) + + barrier = threading.Barrier(2) + threads = [ + threading.Thread( + target=lambda uid=uid: (barrier.wait(), make_user(uid)) + ) + for uid in (1, 2) + ] + for t in threads: + t.start() + for t in threads: + t.join() + + assert not errors + for uid in (1, 2): + assert ( + managers[uid].get_credential(CredentialProvider.LLM_ANTHROPIC) + == ANTHROPIC_A + ) + assert ( + managers[uid].get_credential(CredentialProvider.GIT_GITHUB) + == GITHUB_A + ) + + machine_store = CredentialStore(storage_dir=tmp_path) + assert machine_store.retrieve(CredentialProvider.LLM_ANTHROPIC).value == ANTHROPIC_A + assert machine_store.retrieve(CredentialProvider.GIT_GITHUB).value == GITHUB_A + + def test_keyring_backend_migration_copies_and_preserves_source(self, tmp_path): + """Legacy keyring entries under the machine-wide service are copied to + the per-user service, and the machine-wide service keeps them. + """ + passwords: dict[tuple[str, str], str] = {} + + def set_pw(service: str, key: str, value: str) -> None: + passwords[(service, key)] = value + + def get_pw(service: str, key: str) -> str | None: + return passwords.get((service, key)) + + def del_pw(service: str, key: str) -> None: + passwords.pop((service, key), None) + + with patch("codeframe.core.credentials.keyring") as mock_keyring: + mock_keyring.get_keyring.return_value = MagicMock() + mock_keyring.set_password.side_effect = set_pw + mock_keyring.get_password.side_effect = get_pw + mock_keyring.delete_password.side_effect = del_pw + + machine = CredentialManager(storage_dir=tmp_path) + machine.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + machine.set_credential(CredentialProvider.GIT_GITHUB, GITHUB_A) + + user1 = CredentialManager(storage_dir=tmp_path, user_id=1) + + assert ( + user1.get_credential(CredentialProvider.LLM_ANTHROPIC) == ANTHROPIC_A + ) + assert user1.get_credential(CredentialProvider.GIT_GITHUB) == GITHUB_A + + user_service = f"{KEYRING_SERVICE_NAME}-user-1" + assert (user_service, "LLM_ANTHROPIC") in passwords + assert (user_service, "GIT_GITHUB") in passwords + assert (KEYRING_SERVICE_NAME, "LLM_ANTHROPIC") in passwords + assert (KEYRING_SERVICE_NAME, "GIT_GITHUB") in passwords + + +class TestDeleteToleratesMissingKeyringEntry: + """delete() must clean the file even when the entry isn't in the keyring. + + keyring.delete_password raises PasswordDeleteError for an absent entry — + that is "nothing to do", not a backend failure. Re-raising it skipped the + encrypted-file cleanup, which is what stranded machine-wide leftovers that + migration then redistributed to every subsequent user. + """ + + def test_password_delete_error_still_cleans_file(self, tmp_path, monkeypatch): + from keyring.errors import PasswordDeleteError + + monkeypatch.setattr(CredentialStore, "_check_keyring", lambda self: False) + store = CredentialStore(storage_dir=tmp_path) + store.store( + Credential(provider=CredentialProvider.LLM_ANTHROPIC, value=ANTHROPIC_A) + ) + assert store.retrieve(CredentialProvider.LLM_ANTHROPIC) is not None + + # Keyring backend present but the entry is file-only. + store._keyring_available = True + monkeypatch.setattr( + "codeframe.core.credentials.keyring.get_password", + lambda service, key: None, + ) + + def raise_not_found(service, key): + raise PasswordDeleteError("No such password!") + + monkeypatch.setattr( + "codeframe.core.credentials.keyring.delete_password", raise_not_found + ) + + store.delete(CredentialProvider.LLM_ANTHROPIC) # must not raise + store._keyring_available = False + assert store.retrieve(CredentialProvider.LLM_ANTHROPIC) is None + + def test_real_keyring_failure_still_raises(self, tmp_path, monkeypatch): + monkeypatch.setattr(CredentialStore, "_check_keyring", lambda self: False) + store = CredentialStore(storage_dir=tmp_path) + store._keyring_available = True + + def explode(service, key): + raise RuntimeError("secret service died") + + monkeypatch.setattr( + "codeframe.core.credentials.keyring.delete_password", explode + ) + + with pytest.raises(RuntimeError): + store.delete(CredentialProvider.LLM_ANTHROPIC) + + +class TestMigrationMemo: + """Migration runs once per (storage_dir, user_id) per process (#790). + + CredentialManager is built per request; probing 6 providers machine-side + plus 2 PBKDF2 derivations on every GET /keys poll is wasteful. The memo + keeps it to a single idempotent run per process. + """ + + def test_migration_runs_once_per_user_per_process( + self, tmp_path, file_backed, monkeypatch + ): + calls: list = [] + real = CredentialManager._migrate_machine_wide_entries + + def spy(self): + calls.append(self._user_id) + return real(self) + + monkeypatch.setattr( + CredentialManager, "_migrate_machine_wide_entries", spy + ) + + CredentialManager(storage_dir=tmp_path) # None never migrates + CredentialManager(storage_dir=tmp_path, user_id=1) + CredentialManager(storage_dir=tmp_path, user_id=1) # memo hit + CredentialManager(storage_dir=tmp_path, user_id=2) + + assert calls == [1, 2] + + +class TestMigrationFileLockWarning: + """Migration warns when the optional filelock dependency is unavailable.""" + + def test_warns_when_filelock_unavailable( + self, tmp_path, monkeypatch, caplog + ): + monkeypatch.setattr( + "codeframe.core.credentials.KEYRING_AVAILABLE", False + ) + monkeypatch.setattr("codeframe.core.credentials.FileLock", None) + + machine = CredentialManager(storage_dir=tmp_path) + machine.set_credential(CredentialProvider.LLM_ANTHROPIC, ANTHROPIC_A) + + with caplog.at_level(logging.WARNING, logger="codeframe.core.credentials"): + user = CredentialManager(storage_dir=tmp_path, user_id=1) + + assert user.get_credential(CredentialProvider.LLM_ANTHROPIC) == ANTHROPIC_A + assert "filelock is not installed" in caplog.text diff --git a/tests/core/test_task_github_traceability.py b/tests/core/test_task_github_traceability.py index 71b6f2df..43ae411b 100644 --- a/tests/core/test_task_github_traceability.py +++ b/tests/core/test_task_github_traceability.py @@ -236,6 +236,62 @@ def test_done_skips_when_no_pat(self, workspace, monkeypatch): tasks.update_status(workspace, task.id, TaskStatus.DONE) assert calls == [] + def test_done_ignores_per_user_stored_pat(self, workspace, tmp_path, monkeypatch): + """Autoclose uses the machine-wide store / env; per-user stored PATs + are not picked up (#790 known limitation). + """ + from codeframe.core import credentials as creds_mod + from codeframe.core.credentials import ( + CredentialManager, + CredentialProvider, + CredentialStore, + ) + + # Keep this test off the real keyring/secret-service backend. + monkeypatch.setattr(CredentialStore, "_check_keyring", lambda self: False) + + calls = [] + monkeypatch.setattr( + tasks, + "_close_issue_background", + lambda pat, repo, number: calls.append((pat, repo, number)), + ) + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + + # Seed a per-user store with a valid PAT. + per_user = CredentialManager(storage_dir=tmp_path, user_id=1) + per_user.set_credential(CredentialProvider.GIT_GITHUB, "test-github-token-per-user-only-fake-0000") + + # The dispatch uses a machine-wide manager; make it an empty one so a + # real per-user credential cannot accidentally leak through. + machine_wide = CredentialManager(storage_dir=tmp_path) + + real_manager = creds_mod.CredentialManager + created_user_ids = [] + + def fake_manager(*, storage_dir=None, user_id=None): + created_user_ids.append(user_id) + if user_id is None: + return machine_wide + return real_manager(storage_dir=storage_dir, user_id=user_id) + + monkeypatch.setattr(creds_mod, "CredentialManager", fake_manager) + + task = tasks.create( + workspace, + title="Imported", + status=TaskStatus.IN_PROGRESS, + github_issue_number=99, + external_url="https://github.com/acme/app/issues/99", + auto_close_github_issue=True, + ) + tasks.update_status(workspace, task.id, TaskStatus.DONE) + + # Autoclose resolved to the machine-wide (user_id=None) manager, which + # had no PAT, so nothing was dispatched despite the per-user store. + assert created_user_ids == [None] + assert calls == [] + class TestUpdateAutoClose: def test_toggle_on_and_off(self, workspace): @@ -253,3 +309,5 @@ def test_toggle_on_and_off(self, workspace): def test_raises_for_missing_task(self, workspace): with pytest.raises(ValueError, match="not found"): tasks.update_auto_close(workspace, "does-not-exist", True) + + diff --git a/tests/ui/test_credential_tenant_isolation.py b/tests/ui/test_credential_tenant_isolation.py new file mode 100644 index 00000000..6c58657d --- /dev/null +++ b/tests/ui/test_credential_tenant_isolation.py @@ -0,0 +1,424 @@ +"""Cross-tenant isolation for per-user credentials (issue #790). + +With per-user credential scoping, each authenticated principal operates on its +own credential store. These tests wire the routers exactly as production does — +``get_credential_manager(auth)`` builds ``CredentialManager(user_id=...)`` — +and assert that tenant A's keys/PAT are invisible to tenant B across every +credential surface: key status, store/delete, verify-key fallback, GitHub +connect/status/disconnect, and the issue-list cache. +""" + +from datetime import datetime, timezone +from types import SimpleNamespace +from typing import Optional + +import pytest +from fastapi import Depends, FastAPI +from fastapi.testclient import TestClient + +from codeframe.auth.dependencies import require_auth + +pytestmark = pytest.mark.v2 + +VALID_ANTHROPIC_A = "sk-ant-test-" + "a" * 20 +VALID_ANTHROPIC_B = "sk-ant-test-" + "b" * 20 +VALID_GITHUB_A = "ghp_test" + "a" * 20 +VALID_GITHUB_B = "ghp_test" + "b" * 20 + + +def _make_file_backed_manager(storage_dir, user_id: Optional[int]): + """CredentialManager over an isolated per-user file store (no keyring).""" + from codeframe.core.credentials import CredentialManager, CredentialStore + + store = CredentialStore(storage_dir=storage_dir, user_id=user_id) + store._keyring_available = False + mgr = CredentialManager.__new__(CredentialManager) + mgr._store = store + return mgr + + +@pytest.fixture +def tenants(tmp_path, monkeypatch): + """Two-tenant app: per-user managers + per-user workspaces. + + ``as_user(uid)`` points the auth override at that principal; the manager + and workspace overrides derive from the same auth dict, mirroring the + production dependency wiring. + """ + from codeframe.core.credentials import CredentialProvider + + for provider in CredentialProvider: + monkeypatch.delenv(provider.env_var, raising=False) + + from codeframe.ui.dependencies import get_v2_workspace + from codeframe.ui.routers import github_integrations_v2, settings_v2 + + github_integrations_v2._ISSUE_CACHE.clear() + + app = FastAPI() + app.include_router(settings_v2.router) + app.include_router(github_integrations_v2.router) + + managers: dict = {} + workspaces: dict = {} + + def _manager_dep(auth: dict = Depends(require_auth)): + uid = auth.get("user_id") + if uid not in managers: + managers[uid] = _make_file_backed_manager(tmp_path / "creds", uid) + return managers[uid] + + def _workspace_dep(auth: dict = Depends(require_auth)): + from codeframe.core.workspace import Workspace + + uid = auth.get("user_id") + if uid not in workspaces: + ws_path = tmp_path / "ws" / str(uid) + state_dir = ws_path / ".codeframe" + state_dir.mkdir(parents=True, exist_ok=True) + # Lightweight Workspace: the integration config is a JSON file + # under state_dir; full workspace init is unneeded here. + workspaces[uid] = Workspace( + id=f"test-ws-{uid}", + repo_path=ws_path, + state_dir=state_dir, + created_at=datetime.now(timezone.utc), + ) + return workspaces[uid] + + app.dependency_overrides[settings_v2.get_credential_manager] = _manager_dep + app.dependency_overrides[settings_v2.get_credential_manager_readonly] = _manager_dep + app.dependency_overrides[github_integrations_v2.get_credential_manager] = _manager_dep + app.dependency_overrides[github_integrations_v2.get_credential_manager_readonly] = _manager_dep + app.dependency_overrides[get_v2_workspace] = _workspace_dep + + client = TestClient(app) + + def as_user(user_id: Optional[int]) -> TestClient: + app.dependency_overrides[require_auth] = lambda: { + "type": "jwt", + "user_id": user_id, + "scopes": ["read", "write", "admin"], + } + return client + + yield SimpleNamespace( + client=client, as_user=as_user, managers=managers, workspaces=workspaces + ) + + github_integrations_v2._ISSUE_CACHE.clear() + + +def _mock_validate_connection(monkeypatch): + from codeframe.ui.routers import github_integrations_v2 + + async def fake(pat, repo, **kwargs): + return { + "repo_full_name": repo, + "owner_login": repo.split("/")[0], + "owner_avatar_url": "https://avatars/1", + } + + monkeypatch.setattr(github_integrations_v2, "validate_connection", fake) + + +class TestManagerDependencyScoping: + """The production dependency builds the manager from auth["user_id"].""" + + class _FakeCM: + def __init__(self, storage_dir=None, user_id=None, migrate=True): + self.user_id = user_id + + def test_settings_manager_dep_scopes_to_auth_user(self, monkeypatch): + from codeframe.ui.routers import settings_v2 + + monkeypatch.setattr(settings_v2, "CredentialManager", self._FakeCM) + manager = settings_v2.get_credential_manager(auth={"user_id": 7}) + assert manager.user_id == 7 + + def test_settings_manager_dep_machine_wide_when_no_user(self, monkeypatch): + from codeframe.ui.routers import settings_v2 + + monkeypatch.setattr(settings_v2, "CredentialManager", self._FakeCM) + manager = settings_v2.get_credential_manager(auth={"user_id": None}) + assert manager.user_id is None + + def test_github_manager_dep_scopes_to_auth_user(self, monkeypatch): + from codeframe.ui.routers import github_integrations_v2 + + monkeypatch.setattr(github_integrations_v2, "CredentialManager", self._FakeCM) + manager = github_integrations_v2.get_credential_manager(auth={"user_id": 7}) + assert manager.user_id == 7 + + +class TestKeyStatusIsolation: + def test_tenant_b_sees_no_stored_keys_after_a_stores(self, tenants): + r = tenants.as_user(1).put( + "/api/v2/settings/keys/LLM_ANTHROPIC", json={"value": VALID_ANTHROPIC_A} + ) + assert r.status_code == 200 + assert r.json()["stored"] is True + + for entry in tenants.as_user(2).get("/api/v2/settings/keys").json(): + assert entry["stored"] is False + assert entry["source"] == "none" + assert entry["last_four"] is None + + # Tenant A still sees its own key. + a_keys = tenants.as_user(1).get("/api/v2/settings/keys").json() + anth = next(e for e in a_keys if e["provider"] == "LLM_ANTHROPIC") + assert anth["stored"] is True + assert anth["source"] == "stored" + assert anth["last_four"] == VALID_ANTHROPIC_A[-4:] + + def test_tenant_b_put_does_not_overwrite_a(self, tenants): + tenants.as_user(1).put( + "/api/v2/settings/keys/LLM_ANTHROPIC", json={"value": VALID_ANTHROPIC_A} + ) + r = tenants.as_user(2).put( + "/api/v2/settings/keys/LLM_ANTHROPIC", json={"value": VALID_ANTHROPIC_B} + ) + assert r.status_code == 200 + + a_keys = tenants.as_user(1).get("/api/v2/settings/keys").json() + b_keys = tenants.as_user(2).get("/api/v2/settings/keys").json() + a_anth = next(e for e in a_keys if e["provider"] == "LLM_ANTHROPIC") + b_anth = next(e for e in b_keys if e["provider"] == "LLM_ANTHROPIC") + assert a_anth["last_four"] == VALID_ANTHROPIC_A[-4:] + assert b_anth["last_four"] == VALID_ANTHROPIC_B[-4:] + + def test_tenant_b_delete_does_not_touch_a(self, tenants): + tenants.as_user(1).put( + "/api/v2/settings/keys/LLM_ANTHROPIC", json={"value": VALID_ANTHROPIC_A} + ) + + # Idempotent 204 against B's own (empty) store. + r = tenants.as_user(2).delete("/api/v2/settings/keys/LLM_ANTHROPIC") + assert r.status_code == 204 + + a_keys = tenants.as_user(1).get("/api/v2/settings/keys").json() + anth = next(e for e in a_keys if e["provider"] == "LLM_ANTHROPIC") + assert anth["stored"] is True + + def test_verify_key_fallback_does_not_read_other_tenant( + self, tenants, monkeypatch + ): + from codeframe.ui.routers import settings_v2 + + calls: list[str] = [] + + async def fake_check(token: str): + calls.append(token) + return True, "ok" + + monkeypatch.setattr(settings_v2, "_check_github_token", fake_check) + + tenants.as_user(1).put( + "/api/v2/settings/keys/GIT_GITHUB", json={"value": VALID_GITHUB_A} + ) + + # B has no stored key and no env var — the fallback must NOT find A's. + r = tenants.as_user(2).post( + "/api/v2/settings/verify-key", + json={"provider": "GIT_GITHUB", "value": None}, + ) + assert r.status_code == 200 + assert r.json()["valid"] is False + assert "no key" in r.json()["message"].lower() + assert calls == [] + + # A's fallback resolves A's own stored key. + r = tenants.as_user(1).post( + "/api/v2/settings/verify-key", + json={"provider": "GIT_GITHUB", "value": None}, + ) + assert r.status_code == 200 + assert r.json()["valid"] is True + assert calls == [VALID_GITHUB_A] + + +class TestGitHubIntegrationIsolation: + def test_connect_status_disconnect_are_per_user(self, tenants, monkeypatch): + _mock_validate_connection(monkeypatch) + + r = tenants.as_user(1).post( + "/api/v2/integrations/github/connect", + json={"pat": VALID_GITHUB_A, "repo": "acme/app"}, + ) + assert r.status_code == 200 + + a_status = tenants.as_user(1).get("/api/v2/integrations/github/status").json() + assert a_status["connected"] is True + # B's own workspace + own (empty) credential store → not connected. + b_status = tenants.as_user(2).get("/api/v2/integrations/github/status").json() + assert b_status["connected"] is False + + # B disconnects (idempotent on B's side); A's connection is untouched. + r = tenants.as_user(2).delete("/api/v2/integrations/github/disconnect") + assert r.status_code == 204 + a_status = tenants.as_user(1).get("/api/v2/integrations/github/status").json() + assert a_status["connected"] is True + + def test_b_connect_does_not_clobber_a_pat(self, tenants, monkeypatch): + _mock_validate_connection(monkeypatch) + + tenants.as_user(1).post( + "/api/v2/integrations/github/connect", + json={"pat": VALID_GITHUB_A, "repo": "acme/app"}, + ) + tenants.as_user(2).post( + "/api/v2/integrations/github/connect", + json={"pat": VALID_GITHUB_B, "repo": "acme/app"}, + ) + + from codeframe.core.credentials import CredentialProvider + + assert ( + tenants.managers[1].get_credential(CredentialProvider.GIT_GITHUB) + == VALID_GITHUB_A + ) + assert ( + tenants.managers[2].get_credential(CredentialProvider.GIT_GITHUB) + == VALID_GITHUB_B + ) + + def test_b_disconnect_leaves_a_pat(self, tenants, monkeypatch): + _mock_validate_connection(monkeypatch) + + tenants.as_user(1).post( + "/api/v2/integrations/github/connect", + json={"pat": VALID_GITHUB_A, "repo": "acme/app"}, + ) + tenants.as_user(2).post( + "/api/v2/integrations/github/connect", + json={"pat": VALID_GITHUB_B, "repo": "acme/app"}, + ) + + r = tenants.as_user(2).delete("/api/v2/integrations/github/disconnect") + assert r.status_code == 204 + + from codeframe.core.credentials import CredentialProvider + + assert ( + tenants.managers[1].get_credential(CredentialProvider.GIT_GITHUB) + == VALID_GITHUB_A + ) + assert tenants.managers[2].get_credential(CredentialProvider.GIT_GITHUB) is None + + +class TestIssueCacheTenantSeparation: + def test_issues_cache_is_separated_by_user(self, tenants, monkeypatch): + """Two tenants browsing the same repo slug with different PATs must not + share cached issue listings — the cache key carries the user.""" + from codeframe.ui.routers import github_integrations_v2 + + _mock_validate_connection(monkeypatch) + tenants.as_user(1).post( + "/api/v2/integrations/github/connect", + json={"pat": VALID_GITHUB_A, "repo": "acme/app"}, + ) + tenants.as_user(2).post( + "/api/v2/integrations/github/connect", + json={"pat": VALID_GITHUB_B, "repo": "acme/app"}, + ) + + calls: list[str] = [] + + async def fake_list_issues(pat, repo, **kwargs): + calls.append(pat) + number = 1 if pat == VALID_GITHUB_A else 2 + return ( + [ + { + "number": number, + "title": f"issue seen by {pat[-4:]}", + "labels": [], + "assignee": None, + "created_at": "2026-01-01T00:00:00Z", + "html_url": f"https://github.com/{repo}/issues/{number}", + } + ], + 1, + ) + + monkeypatch.setattr(github_integrations_v2, "list_issues", fake_list_issues) + + a_resp = tenants.as_user(1).get("/api/v2/integrations/github/issues") + assert a_resp.status_code == 200 + assert a_resp.json()["issues"][0]["number"] == 1 + + # Same query as B: must hit the service with B's PAT, not A's cache. + b_resp = tenants.as_user(2).get("/api/v2/integrations/github/issues") + assert b_resp.status_code == 200 + assert b_resp.json()["issues"][0]["number"] == 2 + + assert calls == [VALID_GITHUB_A, VALID_GITHUB_B] + + +class TestIssueCacheInvalidationScoping: + """Import-time cache invalidation is scoped to the calling user (#790). + + Cache keys end with ``|{user_id}``; A's import must not wipe B's cached + listings for the same repo. + """ + + def test_invalidate_drops_only_calling_users_entries(self, tenants): + from codeframe.ui.routers import github_integrations_v2 as gi + + gi._ISSUE_CACHE.clear() + gi._issue_cache_set("acme/app|1|25|||1", "A-payload") + gi._issue_cache_set("acme/app|1|25|||2", "B-payload") + + gi._issue_cache_invalidate("acme/app", 1) + + assert gi._issue_cache_get("acme/app|1|25|||1") is None + assert gi._issue_cache_get("acme/app|1|25|||2") == "B-payload" + + def test_import_invalidates_own_cache_not_other_tenants( + self, tenants, monkeypatch, tmp_path + ): + from codeframe.core.workspace import create_or_load_workspace + from codeframe.ui.routers import github_integrations_v2 as gi + + # A's workspace needs a real task DB: import creates tasks in it. + ws_path = tmp_path / "ws-a-real" + ws_path.mkdir() + tenants.workspaces[1] = create_or_load_workspace(ws_path) + + _mock_validate_connection(monkeypatch) + tenants.as_user(1).post( + "/api/v2/integrations/github/connect", + json={"pat": VALID_GITHUB_A, "repo": "acme/app"}, + ) + tenants.as_user(2).post( + "/api/v2/integrations/github/connect", + json={"pat": VALID_GITHUB_B, "repo": "acme/app"}, + ) + + async def fake_list_issues(pat, repo, **kwargs): + return ([], 0) + + monkeypatch.setattr(gi, "list_issues", fake_list_issues) + gi._ISSUE_CACHE.clear() + tenants.as_user(1).get("/api/v2/integrations/github/issues") + tenants.as_user(2).get("/api/v2/integrations/github/issues") + assert any(k.endswith("|1") for k in gi._ISSUE_CACHE) + assert any(k.endswith("|2") for k in gi._ISSUE_CACHE) + + async def fake_get_issue(pat, repo, number, **kwargs): + return { + "number": number, + "title": "One", + "body": "x", + "labels": [], + "html_url": f"https://github.com/{repo}/issues/{number}", + } + + monkeypatch.setattr(gi, "get_issue", fake_get_issue) + r = tenants.as_user(1).post( + "/api/v2/integrations/github/import", json={"issue_numbers": [1]} + ) + assert r.json()["total_created"] == 1 + + assert not any(k.endswith("|1") for k in gi._ISSUE_CACHE) + assert any(k.endswith("|2") for k in gi._ISSUE_CACHE) diff --git a/tests/ui/test_github_integrations_v2.py b/tests/ui/test_github_integrations_v2.py index 958260b8..4c370578 100644 --- a/tests/ui/test_github_integrations_v2.py +++ b/tests/ui/test_github_integrations_v2.py @@ -63,6 +63,9 @@ def client(workspace, manager): app.dependency_overrides[github_integrations_v2.get_credential_manager] = ( lambda: manager ) + app.dependency_overrides[github_integrations_v2.get_credential_manager_readonly] = ( + lambda: manager + ) return TestClient(app) @@ -305,6 +308,20 @@ def test_size_capped_evicting_oldest(self, monkeypatch): assert "repo|new" in gi._ISSUE_CACHE assert "repo|0" not in gi._ISSUE_CACHE + def test_invalidation_scoped_to_calling_user(self, monkeypatch): + """User A's import drops A's cache entries but leaves user B's alone (#790).""" + from codeframe.ui.routers import github_integrations_v2 as gi + + _clear_issue_cache() + base = gi.time.monotonic() + gi._ISSUE_CACHE["acme/app|1|25|||1"] = (base + 60.0, "a-payload") + gi._ISSUE_CACHE["acme/app|1|25|||2"] = (base + 60.0, "b-payload") + + gi._issue_cache_invalidate("acme/app", 1) + + assert "acme/app|1|25|||1" not in gi._ISSUE_CACHE + assert "acme/app|1|25|||2" in gi._ISSUE_CACHE + class TestListIssues: def test_requires_connection(self, client, monkeypatch): diff --git a/tests/ui/test_hosted_credential_block.py b/tests/ui/test_hosted_credential_block.py index 90e65515..3c67546c 100644 --- a/tests/ui/test_hosted_credential_block.py +++ b/tests/ui/test_hosted_credential_block.py @@ -1,77 +1,140 @@ -"""Single-trust-domain enforcement (issue #718 / P0.7). +"""Per-user credentials in hosted mode (issue #790; supersedes the #718 block). -CodeFRAME's credential store is machine-wide (one LLM key set + one GitHub PAT -per host). That's fine for self-hosted (a single trust domain), but in hosted -multi-tenant mode it would let any tenant view/overwrite/delete another's -secrets. So credential + GitHub-PAT *mutation* endpoints fail closed in hosted -mode; tenants supply credentials via per-instance env vars instead. +The #718 stopgap blocked credential + GitHub-PAT *mutation* endpoints in hosted +multi-tenant mode because the credential store was machine-wide. With per-user +credential scoping (#790) each tenant operates on their own store, so hosted +mode now ALLOWS these endpoints — the old 403 is gone. These tests pin that +behavior against the real server app with an isolated credential store. -Also verifies workspaces persist owner_user_id from the authenticated principal. +Also verifies workspaces persist owner_user_id from the authenticated principal +(#718 workspace ownership — unrelated to the credential guard; kept intact). """ +from datetime import datetime, timezone + import pytest -from fastapi import HTTPException from fastapi.testclient import TestClient pytestmark = pytest.mark.v2 +VALID_OPENAI = "sk-test-" + "x" * 20 +VALID_GITHUB = "ghp_test" + "x" * 20 + + +@pytest.fixture +def hosted_client(tmp_path, monkeypatch): + """Real server app in hosted mode, backed by an isolated credential store. + + conftest leaves auth disabled (synthetic admin principal), so these tests + isolate the hosted-mode credential behavior from the auth/scope layer. + """ + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "hosted") + + from codeframe.core.credentials import ( + CredentialManager, + CredentialProvider, + CredentialStore, + ) + + for provider in CredentialProvider: + monkeypatch.delenv(provider.env_var, raising=False) + + from codeframe.core.workspace import Workspace + from codeframe.ui import server + from codeframe.ui.dependencies import get_v2_workspace + from codeframe.ui.routers import github_integrations_v2, settings_v2 + + store = CredentialStore(storage_dir=tmp_path / "creds") + store._keyring_available = False + manager = CredentialManager.__new__(CredentialManager) + manager._store = store + + # Lightweight Workspace: connect only writes the JSON config under state_dir. + ws_path = tmp_path / "ws" + state_dir = ws_path / ".codeframe" + state_dir.mkdir(parents=True) + workspace = Workspace( + id="test-ws-hosted", + repo_path=ws_path, + state_dir=state_dir, + created_at=datetime.now(timezone.utc), + ) + + app = server.app + app.dependency_overrides[settings_v2.get_credential_manager] = lambda: manager + app.dependency_overrides[settings_v2.get_credential_manager_readonly] = lambda: manager + app.dependency_overrides[github_integrations_v2.get_credential_manager] = ( + lambda: manager + ) + app.dependency_overrides[github_integrations_v2.get_credential_manager_readonly] = ( + lambda: manager + ) + app.dependency_overrides[get_v2_workspace] = lambda: workspace + + yield TestClient(app) + + # Restore the shared app for other tests in this process. + for dep in ( + settings_v2.get_credential_manager, + settings_v2.get_credential_manager_readonly, + github_integrations_v2.get_credential_manager, + github_integrations_v2.get_credential_manager_readonly, + get_v2_workspace, + ): + app.dependency_overrides.pop(dep, None) + + +class TestCredentialMutationAllowedInHostedMode: + """Hosted mode no longer blocks per-user credential mutation (#790).""" + + def test_store_key_allowed_in_hosted_mode(self, hosted_client): + r = hosted_client.put( + "/api/v2/settings/keys/LLM_OPENAI", json={"value": VALID_OPENAI} + ) + assert r.status_code == 200, r.text + assert r.json()["stored"] is True + assert r.json()["source"] == "stored" -class TestForbidGuard: - def test_raises_in_hosted_mode(self, monkeypatch): - monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "hosted") - from codeframe.ui.dependencies import forbid_shared_credentials_in_hosted_mode - - with pytest.raises(HTTPException) as exc: - forbid_shared_credentials_in_hosted_mode() - assert exc.value.status_code == 403 - - def test_noop_in_self_hosted(self, monkeypatch): - monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "self_hosted") - from codeframe.ui.dependencies import forbid_shared_credentials_in_hosted_mode - - assert forbid_shared_credentials_in_hosted_mode() is None # no raise - - -class TestCredentialEndpointHostedBlock: - """conftest defaults auth OFF → synthetic admin principal, so these isolate - the hosted-mode credential guard (not the scope/auth layer).""" + def test_delete_key_allowed_in_hosted_mode(self, hosted_client): + hosted_client.put( + "/api/v2/settings/keys/LLM_OPENAI", json={"value": VALID_OPENAI} + ) + r = hosted_client.delete("/api/v2/settings/keys/LLM_OPENAI") + assert r.status_code == 204, r.text - def _client(self): - from codeframe.ui import server + keys = hosted_client.get("/api/v2/settings/keys").json() + openai = next(e for e in keys if e["provider"] == "LLM_OPENAI") + assert openai["stored"] is False - return TestClient(server.app) + def test_github_connect_allowed_in_hosted_mode(self, hosted_client, monkeypatch): + from codeframe.ui.routers import github_integrations_v2 - def test_store_key_blocked_in_hosted_mode(self, monkeypatch): - monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "hosted") - r = self._client().put("/api/v2/settings/keys/openai", json={"value": "sk-x"}) - assert r.status_code == 403 - assert "hosted mode" in r.json().get("detail", "").lower() + async def fake_validate(pat, repo, **kwargs): + return { + "repo_full_name": repo, + "owner_login": repo.split("/")[0], + "owner_avatar_url": "https://avatars/1", + } - def test_delete_key_blocked_in_hosted_mode(self, monkeypatch): - monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "hosted") - r = self._client().delete("/api/v2/settings/keys/openai") - assert r.status_code == 403 + monkeypatch.setattr(github_integrations_v2, "validate_connection", fake_validate) - def test_github_connect_blocked_in_hosted_mode(self, monkeypatch): - monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "hosted") - # The guard fires before workspace-allowlist resolution, so no - # WORKSPACE_ROOT setup is needed. - r = self._client().post( - "/api/v2/integrations/github/connect", json={"repo": "o/r", "pat": "ghp_x"} + r = hosted_client.post( + "/api/v2/integrations/github/connect", + json={"pat": VALID_GITHUB, "repo": "acme/app"}, ) - assert r.status_code == 403 + assert r.status_code == 200, r.text + assert r.json()["connected"] is True - def test_github_disconnect_blocked_in_hosted_mode(self, monkeypatch): - monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "hosted") - r = self._client().delete("/api/v2/integrations/github/disconnect") - assert r.status_code == 403 + def test_github_disconnect_allowed_in_hosted_mode(self, hosted_client): + r = hosted_client.delete("/api/v2/integrations/github/disconnect") + assert r.status_code == 204, r.text - def test_store_key_not_hosted_blocked_in_self_hosted(self, monkeypatch): + def test_self_hosted_mutation_still_allowed(self, hosted_client, monkeypatch): monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "self_hosted") - r = self._client().put("/api/v2/settings/keys/openai", json={"value": "sk-x"}) - # self-hosted: the hosted guard is a no-op; the request passes it and hits - # value-format validation (400). It must never be the hosted-mode 403. - assert r.status_code != 403, f"unexpected hosted-mode 403 in self-hosted: {r.json()}" + r = hosted_client.put( + "/api/v2/settings/keys/LLM_OPENAI", json={"value": VALID_OPENAI} + ) + assert r.status_code == 200, r.text class TestOwnerPersistence: diff --git a/tests/ui/test_security_config.py b/tests/ui/test_security_config.py index 71f52f42..4e255c46 100644 --- a/tests/ui/test_security_config.py +++ b/tests/ui/test_security_config.py @@ -107,6 +107,16 @@ def test_hosted_mode_escape_hatch_still_fails(monkeypatch): server._validate_security_config() +def test_hosted_mode_with_auth_disabled_fails_even_with_real_secret(monkeypatch): + """Hosted mode must never run with synthetic user_id=None principals.""" + _set_secret(monkeypatch, "a-real-and-very-secret-value") + monkeypatch.setenv("CODEFRAME_DEPLOYMENT_MODE", "hosted") + monkeypatch.setenv("CODEFRAME_AUTH_REQUIRED", "false") + + with pytest.raises(RuntimeError, match="authentication"): + server._validate_security_config() + + @pytest.mark.parametrize( "raw", ["", " ", DEFAULT_SECRET + " ", " " + DEFAULT_SECRET, f"\t{DEFAULT_SECRET}\n"], diff --git a/tests/ui/test_settings_v2.py b/tests/ui/test_settings_v2.py index 9db86fa8..7fe37847 100644 --- a/tests/ui/test_settings_v2.py +++ b/tests/ui/test_settings_v2.py @@ -87,6 +87,9 @@ def keys_client(temp_credentials_dir): app.dependency_overrides[settings_v2.get_credential_manager] = ( lambda: temp_credentials_dir ) + app.dependency_overrides[settings_v2.get_credential_manager_readonly] = ( + lambda: temp_credentials_dir + ) yield TestClient(app) diff --git a/tests/ui/test_v2_auth_enforcement.py b/tests/ui/test_v2_auth_enforcement.py index a74d2c10..a907ab29 100644 --- a/tests/ui/test_v2_auth_enforcement.py +++ b/tests/ui/test_v2_auth_enforcement.py @@ -90,14 +90,46 @@ def _build_auth_app(tmp_path, monkeypatch, *, enable_test_endpoints): from codeframe.ui import server importlib.reload(server) + + # Isolate the credential store: authenticated requests carry user_id=1 and + # the per-user CredentialManager (#790) would otherwise probe — and migrate + # entries out of — the developer's real ~/.codeframe store. + from codeframe.core.credentials import CredentialManager, CredentialStore + from codeframe.ui.routers import github_integrations_v2, settings_v2 + + cred_store = CredentialStore(storage_dir=tmp_path / "creds") + cred_store._keyring_available = False + cred_manager = CredentialManager.__new__(CredentialManager) + cred_manager._store = cred_store + for dep in ( + settings_v2.get_credential_manager, + settings_v2.get_credential_manager_readonly, + github_integrations_v2.get_credential_manager, + github_integrations_v2.get_credential_manager_readonly, + ): + server.app.dependency_overrides[dep] = lambda: cred_manager + return server.app +def _pop_credential_overrides(app) -> None: + from codeframe.ui.routers import github_integrations_v2, settings_v2 + + for dep in ( + settings_v2.get_credential_manager, + settings_v2.get_credential_manager_readonly, + github_integrations_v2.get_credential_manager, + github_integrations_v2.get_credential_manager_readonly, + ): + app.dependency_overrides.pop(dep, None) + + @pytest.fixture def auth_app(tmp_path, monkeypatch): """Real server app with auth enforcement and test endpoints enabled.""" app = _build_auth_app(tmp_path, monkeypatch, enable_test_endpoints=True) yield app + _pop_credential_overrides(app) reset_auth_engine() @@ -106,6 +138,7 @@ def auth_app_no_test_endpoints(tmp_path, monkeypatch): """Real server app with auth enforcement but NO test endpoints (#753).""" app = _build_auth_app(tmp_path, monkeypatch, enable_test_endpoints=False) yield app + _pop_credential_overrides(app) reset_auth_engine() diff --git a/tests/ui/test_v2_scope_enforcement.py b/tests/ui/test_v2_scope_enforcement.py index 9ff6e04e..743c97e9 100644 --- a/tests/ui/test_v2_scope_enforcement.py +++ b/tests/ui/test_v2_scope_enforcement.py @@ -56,7 +56,29 @@ def scoped_app(tmp_path, monkeypatch): from codeframe.ui import server importlib.reload(server) + + # Isolate the credential store: with auth ON, requests carry user_id=1 and + # the per-user CredentialManager (#790) would otherwise probe — and migrate + # entries out of — the developer's real ~/.codeframe store. + from codeframe.core.credentials import CredentialManager, CredentialStore + from codeframe.ui.routers import github_integrations_v2, settings_v2 + + cred_store = CredentialStore(storage_dir=tmp_path / "creds") + cred_store._keyring_available = False + cred_manager = CredentialManager.__new__(CredentialManager) + cred_manager._store = cred_store + cred_deps = ( + settings_v2.get_credential_manager, + settings_v2.get_credential_manager_readonly, + github_integrations_v2.get_credential_manager, + github_integrations_v2.get_credential_manager_readonly, + ) + for dep in cred_deps: + server.app.dependency_overrides[dep] = lambda: cred_manager + yield server.app, keys + for dep in cred_deps: + server.app.dependency_overrides.pop(dep, None) reset_auth_engine() diff --git a/uv.lock b/uv.lock index 511cb3de..49d683d9 100644 --- a/uv.lock +++ b/uv.lock @@ -601,6 +601,7 @@ dependencies = [ { name = "cryptography" }, { name = "fastapi" }, { name = "fastapi-users", extra = ["sqlalchemy"] }, + { name = "filelock" }, { name = "gitpython" }, { name = "httpx" }, { name = "hypothesis" }, @@ -678,6 +679,7 @@ requires-dist = [ { name = "e2b", marker = "extra == 'dev'", specifier = ">=2.0.0" }, { name = "fastapi", specifier = ">=0.109.0" }, { name = "fastapi-users", extras = ["sqlalchemy"], specifier = ">=15.0.2" }, + { name = "filelock", specifier = ">=3.0" }, { name = "gitpython", specifier = ">=3.1.50" }, { name = "httpx", specifier = ">=0.27.0" }, { name = "hypothesis", specifier = ">=6.148.0" },