feat(forge): per-user forge connections — keyring, host-keyed resolution, project pin (#2778)
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / TypeScript typecheck (push) Successful in 41s
CI & Build / integration (push) Successful in 37s
CI & Build / Python tests (push) Successful in 1m4s
CI & Build / Build & push image (push) Successful in 40s
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / TypeScript typecheck (push) Successful in 41s
CI & Build / integration (push) Successful in 37s
CI & Build / Python tests (push) Successful in 1m4s
CI & Build / Build & push image (push) Successful in 40s
A forge token is a user's credential, not an instance's. The single admin-settings config is replaced by per-user keyring rows (one per forge host), and every server-side forge read runs on the PROJECT OWNER's keyring: - forge_connections table + projects.forge_connection_id pin (migration 0078, which also carries the existing admin config into the first admin's row and deletes the old setting keys — no legacy dual-read) - get_forge() replaced by get_forges(owner_id, project_id) -> ForgeSelector; resolve(repo) picks the connection whose host serves the repo. A pinned project uses ONLY its pinned connection; a stale pin (ownership moved) is ignored, never honored across users - env FORGE_* config survives as an implicit entry for admin owners only; a stored row for the same host beats it - consumers threaded: pull-time freshness (owner of the note), coverage (owner of the project), coverage routes' configured flag - routes: /api/settings/forge-connections CRUD + per-connection test (own-rows only, tokens never returned); /api/admin/forge shrinks to /api/admin/forge-webhook (secret only); PUT /api/projects/<id>/forge pins, owner-or-admin asking, owner's connections only - UI: Git Forges card moves to Settings -> Integrations as a connection list; webhook secret stays in the admin Config tab; owner-only forge select on the project coverage card - backups exclude forge_connections (credentials, api_keys precedent) and the pin, so restores fall back to keyring resolution Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -7,12 +7,13 @@ decoding, error taxonomy) is testable with no live server and no new
|
||||
dependency. These are the reference behaviors step 8's GitHub adapter must
|
||||
reproduce.
|
||||
|
||||
The most load-bearing tests are the OFF ones: an unconfigured instance must
|
||||
get None from get_forge(), because every consumer treats None as "behave as if
|
||||
the module didn't exist" (rule #115 — the baseline install has no forge).
|
||||
The most load-bearing tests are the OFF ones: a user with no usable connection
|
||||
must get an empty selector, because every consumer treats that as "behave as
|
||||
if the module didn't exist" (rule #115 — the baseline user has no forge).
|
||||
DB-backed keyring behavior (get_forges: rows, the project pin, the admin-only
|
||||
env entry) lives in tests/test_integration_forge_keyring.py.
|
||||
"""
|
||||
import base64
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
@@ -20,8 +21,9 @@ import pytest
|
||||
from scribe.services.forge import (
|
||||
ForgeError,
|
||||
ForgeNotFound,
|
||||
ForgeSelector,
|
||||
GiteaForge,
|
||||
get_forge,
|
||||
build_adapter,
|
||||
)
|
||||
|
||||
BASE = "https://git.example.com"
|
||||
@@ -140,63 +142,59 @@ async def test_check_reports_version_and_identity():
|
||||
assert result == {"ok": True, "version": "1.23.1", "username": "scribe-bot"}
|
||||
|
||||
|
||||
# --- the configuration gate --------------------------------------------------
|
||||
# --- the configuration gate (build_adapter) ----------------------------------
|
||||
|
||||
def _settings(values: dict):
|
||||
async def fake(key, default=""):
|
||||
return values.get(key, default)
|
||||
return patch("scribe.services.forge.get_admin_setting", AsyncMock(side_effect=fake))
|
||||
|
||||
|
||||
async def test_unconfigured_instance_gets_none():
|
||||
with _settings({}), patch("scribe.services.forge.Config") as cfg:
|
||||
cfg.FORGE_KIND = cfg.FORGE_BASE_URL = cfg.FORGE_TOKEN = ""
|
||||
assert await get_forge() is None
|
||||
|
||||
|
||||
async def test_partial_config_is_still_off():
|
||||
def test_empty_or_partial_values_build_nothing():
|
||||
# A base URL with no token (or vice versa) must not half-enable anything.
|
||||
for values in (
|
||||
{"forge_kind": "gitea", "forge_base_url": BASE},
|
||||
{"forge_kind": "gitea", "forge_token": "tok"},
|
||||
{"forge_base_url": BASE, "forge_token": "tok"}, # no kind selected
|
||||
):
|
||||
with _settings(values), patch("scribe.services.forge.Config") as cfg:
|
||||
cfg.FORGE_KIND = cfg.FORGE_BASE_URL = cfg.FORGE_TOKEN = ""
|
||||
assert await get_forge() is None
|
||||
assert build_adapter("", "", "") is None
|
||||
assert build_adapter("gitea", BASE, "") is None
|
||||
assert build_adapter("gitea", "", "tok") is None
|
||||
assert build_adapter("", BASE, "tok") is None # no kind selected
|
||||
|
||||
|
||||
async def test_unknown_kind_disables_with_a_warning_not_a_crash():
|
||||
with _settings({
|
||||
"forge_kind": "sourcehut", "forge_base_url": BASE, "forge_token": "tok",
|
||||
}), patch("scribe.services.forge.Config") as cfg:
|
||||
cfg.FORGE_KIND = cfg.FORGE_BASE_URL = cfg.FORGE_TOKEN = ""
|
||||
assert await get_forge() is None
|
||||
def test_unknown_kind_disables_with_a_warning_not_a_crash():
|
||||
assert build_adapter("sourcehut", BASE, "tok") is None
|
||||
|
||||
|
||||
async def test_full_config_builds_a_gitea_adapter():
|
||||
with _settings({
|
||||
"forge_kind": "gitea",
|
||||
"forge_base_url": BASE + "/", # trailing slash normalized away
|
||||
"forge_token": "tok",
|
||||
}), patch("scribe.services.forge.Config") as cfg:
|
||||
cfg.FORGE_KIND = cfg.FORGE_BASE_URL = cfg.FORGE_TOKEN = ""
|
||||
forge = await get_forge()
|
||||
def test_schemeless_base_url_disables():
|
||||
assert build_adapter("gitea", "git.example.com", "tok") is None
|
||||
|
||||
|
||||
def test_full_values_build_a_gitea_adapter():
|
||||
forge = build_adapter("Gitea", BASE + "/", "tok") # case + slash normalized
|
||||
assert isinstance(forge, GiteaForge)
|
||||
assert forge.base_url == BASE
|
||||
assert forge.host == "git.example.com"
|
||||
|
||||
|
||||
async def test_env_channel_fills_gaps_but_db_wins():
|
||||
# Docker-secret deployments set FORGE_* env; an admin-UI value overrides.
|
||||
with _settings({"forge_base_url": "https://db.example.com"}), \
|
||||
patch("scribe.services.forge.Config") as cfg:
|
||||
cfg.FORGE_KIND = "gitea"
|
||||
cfg.FORGE_BASE_URL = "https://env.example.com"
|
||||
cfg.FORGE_TOKEN = "env-tok"
|
||||
forge = await get_forge()
|
||||
assert isinstance(forge, GiteaForge)
|
||||
assert forge.host == "db.example.com"
|
||||
def test_full_values_build_a_github_adapter():
|
||||
from scribe.services.forge import GitHubForge
|
||||
|
||||
assert isinstance(
|
||||
build_adapter("github", "https://github.com", "tok"), GitHubForge
|
||||
)
|
||||
|
||||
|
||||
# --- the selector (#2778): host-keyed resolution over a keyring ---------------
|
||||
|
||||
def test_empty_selector_is_the_rule_115_baseline():
|
||||
selector = ForgeSelector()
|
||||
assert not selector.configured
|
||||
assert selector.resolve(f"{BASE}/alice/widget") is None
|
||||
|
||||
|
||||
def test_selector_resolves_by_host_across_the_keyring():
|
||||
gitea = build_adapter("gitea", BASE, "tok")
|
||||
github = build_adapter("github", "https://github.com", "tok2")
|
||||
selector = ForgeSelector((gitea, github))
|
||||
assert selector.configured
|
||||
|
||||
hit = selector.resolve("https://git.example.com/alice/widget")
|
||||
assert hit == (gitea, "alice/widget")
|
||||
hit = selector.resolve("git@github.com:alice/Widget.git")
|
||||
assert hit == (github, "alice/widget")
|
||||
# A host neither connection serves is a NORMAL miss, not an error.
|
||||
assert selector.resolve("https://elsewhere.example.org/a/b") is None
|
||||
|
||||
|
||||
def test_forge_error_taxonomy_is_catchable_as_one_family():
|
||||
@@ -220,30 +218,50 @@ def test_adapter_contract_surface():
|
||||
assert set(FORGE_KINDS) == {"gitea", "github"}
|
||||
|
||||
|
||||
def test_admin_routes_registered():
|
||||
def test_forge_routes_registered_at_their_post_2778_homes():
|
||||
"""Connections are USER settings; only the webhook secret stays admin.
|
||||
The old instance-wide /api/admin/forge endpoints must be GONE, not
|
||||
coexisting — a legacy config surface that still wrote admin settings
|
||||
would silently configure nothing (rule #22: no dual path)."""
|
||||
from scribe.app import create_app
|
||||
from scribe.routes import admin as admin_routes
|
||||
|
||||
for name in ("get_forge_settings", "update_forge_settings", "test_forge"):
|
||||
for name in ("get_forge_webhook_settings", "update_forge_webhook_settings"):
|
||||
assert callable(getattr(admin_routes, name))
|
||||
rules = {r.rule for r in create_app().url_map.iter_rules()}
|
||||
assert "/api/admin/forge" in rules
|
||||
assert "/api/admin/forge/test" in rules
|
||||
assert "/api/admin/forge-webhook" in rules
|
||||
assert "/api/admin/forge" not in rules
|
||||
assert "/api/admin/forge/test" not in rules
|
||||
assert "/api/settings/forge-connections" in rules
|
||||
assert "/api/settings/forge-connections/<int:connection_id>" in rules
|
||||
assert "/api/settings/forge-connections/<int:connection_id>/test" in rules
|
||||
assert "/api/projects/<int:project_id>/forge" in rules
|
||||
|
||||
|
||||
def test_settings_kv_surface_masks_the_forge_token():
|
||||
def test_settings_kv_surface_masks_the_webhook_secret():
|
||||
"""The generic /api/settings dump masked nothing — the admin endpoints'
|
||||
masking was bypassable by reading the raw KV rows (found while wiring the
|
||||
forge token; smtp_password had the same exposure)."""
|
||||
forge token; smtp_password had the same exposure). forge_token left the
|
||||
KV with #2778 (keyring rows carry it now), so it is deliberately no
|
||||
longer a secret KEY here."""
|
||||
from scribe.routes.settings import _SECRET_KEYS, _masked
|
||||
|
||||
out = _masked({"forge_token": "tok-123", "smtp_password": "pw", "theme": "dark"})
|
||||
assert out["forge_token"] == "********"
|
||||
out = _masked({"forge_webhook_secret": "sec", "smtp_password": "pw", "theme": "dark"})
|
||||
assert out["forge_webhook_secret"] == "********"
|
||||
assert out["smtp_password"] == "********"
|
||||
assert out["theme"] == "dark"
|
||||
assert {"forge_token", "smtp_password"} <= set(_SECRET_KEYS)
|
||||
assert {"forge_webhook_secret", "smtp_password"} <= set(_SECRET_KEYS)
|
||||
assert "forge_token" not in _SECRET_KEYS
|
||||
# An unset secret stays empty rather than reading as a set-but-masked one.
|
||||
assert _masked({"forge_token": ""})["forge_token"] == ""
|
||||
assert _masked({"forge_webhook_secret": ""})["forge_webhook_secret"] == ""
|
||||
|
||||
|
||||
def test_connection_to_dict_never_carries_the_token():
|
||||
"""The model is the last line: every list/create/update route returns
|
||||
to_dict(), so a field added here is a field leaked there."""
|
||||
from scribe.models.forge_connection import ForgeConnection
|
||||
|
||||
assert "token" not in ForgeConnection.to_dict.__code__.co_consts
|
||||
|
||||
|
||||
def test_config_has_the_docker_secret_channel():
|
||||
@@ -358,14 +376,3 @@ async def test_latest_commit_parses_tolerantly_on_both_adapters():
|
||||
assert await _forge(lambda r: _json(200, [])).latest_commit("a/w", "x.py") == ""
|
||||
|
||||
|
||||
async def test_full_config_builds_a_github_adapter():
|
||||
from scribe.services.forge import GitHubForge
|
||||
|
||||
with _settings({
|
||||
"forge_kind": "github",
|
||||
"forge_base_url": "https://github.com",
|
||||
"forge_token": "tok",
|
||||
}), patch("scribe.services.forge.Config") as cfg:
|
||||
cfg.FORGE_KIND = cfg.FORGE_BASE_URL = cfg.FORGE_TOKEN = ""
|
||||
forge = await get_forge()
|
||||
assert isinstance(forge, GitHubForge)
|
||||
|
||||
Reference in New Issue
Block a user