agnes-the-ai-analyst/tests/test_marketplace_plugin_system.py
Vojtech c552bf8243
feat(api): enforce API design rules via pytest + fix DELETE/status-code violations (#338)
* feat(api): enforce API design rules via pytest + fix DELETE/status-code violations

Adds tests/test_api_design_rules.py with four forward-only design guardrails
that prevent new endpoints from accumulating REST debt:

  Rule 1 — No new verbs in URL paths (existing 28 grandfathered via allowlist)
  Rule 2 — DELETE must declare 204 No Content (zero allowlist entries)
  Rule 3 — Creator POSTs (path has GET counterpart) must declare 201/202
  Rule 4 — All protected /api/* routes must declare 401 and 403

Fixes found by running the rules:

- DELETE /api/admin/metrics/{metric_id}: return 204, drop redundant body
- DELETE /api/memory/{item_id}/dismiss (undismiss): return 204, drop body
- POST /api/memory/admin/contradictions: add status_code=201 (creates a resource)
- app/main.py: _add_auth_error_responses() injected into app.openapi() at startup;
  declares 401/403 on all protected /api/* operations centrally, fixing the 120
  routes that previously omitted these response codes from the spec.

Closes #337

* fix(api): resolve CI failures — extend 204 fixes + complete allowlists

- Fix remaining 6 DELETE endpoints to return 204: store entities,
  store entity install, marketplace curated install, marketplace plugin
  system flag, admin store submission, and observability view
- Update all affected tests to expect 204 (removed body assertions)
- Add 4 missing verb paths to _VERB_PATH_ALLOWLIST in test_api_design_rules.py
- Add 2 upsert endpoints to _CREATOR_POST_ALLOWLIST
- Update admin_marketplaces.html to not call r.json() on 204 DELETE

* fix(tests): align 2 DELETE-asserting tests with 204 contract (post-#339 rebase)

CI's test-shard (1) and (4) failures on this PR were caused by
Vojta's second commit (`fix(api): resolve CI failures — extend 204
fixes`) flipping more DELETE endpoints to status_code=204 than just
the two mentioned in the PR body. Two tests assert status_code==200
on the DELETE response and broke:

- tests/test_admin_store_submissions.py::TestQuarantineGates::test_admin_can_delete_quarantined
  (DELETE /api/store/entities/{entity_id})
- tests/test_store_api.py::TestInstallCycle::test_admin_hard_delete_cascades_installs
  (DELETE /api/store/entities/{entity_id}?hard=true)

Updated both to assert 204 with a comment pointing at
tests/test_api_design_rules.py rule 2 so future reviewers can
trace the contract. Verified via broader scan that no other test
asserts == 200 on a .delete() response directly (4 other sites do
.delete() then check 200 on a subsequent GET — those are fine).

* release: 0.54.26 — API design rules (test_api_design_rules.py) + 8 DELETE endpoints flip to 204

---------

Co-authored-by: ZdenekSrotyr <zdenek.srotyr@keboola.com>
2026-05-18 15:25:07 +02:00

481 lines
18 KiB
Python

"""End-to-end coverage for the v39 system plugin tier.
The feature reuses the existing RBAC + subscription tables — marking a
plugin as "system" simply materializes resource_grants + user_plugin_optouts
rows for every existing user_groups + users row, then locks the
corresponding admin/user controls. The resolver itself is unchanged.
Tests in this module exercise:
* mark/unmark endpoints — happy path, idempotency, audit row, fanout count
* refusal of the bypass paths — DELETE grant, unsubscribe, uninstall
* creation hooks — new user / new group inherit the mandatory tier
* sync preservation — a re-sync of the marketplace doesn't reset is_system
Mirrors the helper pattern in ``test_marketplace_api.py``.
"""
from __future__ import annotations
import json
from datetime import datetime, timezone
import pytest
from fastapi.testclient import TestClient
@pytest.fixture
def web_client(tmp_path, monkeypatch):
monkeypatch.setenv("DATA_DIR", str(tmp_path))
monkeypatch.setenv("TESTING", "1")
monkeypatch.setenv("JWT_SECRET_KEY", "test-secret-key-min-32-characters!!")
(tmp_path / "state").mkdir()
(tmp_path / "analytics").mkdir()
(tmp_path / "extracts").mkdir()
from src.db import close_system_db
close_system_db()
from app.main import create_app
app = create_app()
yield TestClient(app)
close_system_db()
def _create_user(client, email, password="UserPass1!", admin: bool = False):
"""Create a user and return (user_id, cookies). When ``admin=True``
the user is added to the seeded Admin system group so
``require_admin`` passes."""
from argon2 import PasswordHasher
from src.db import get_system_db
from src.repositories.users import UserRepository
ph = PasswordHasher()
conn = get_system_db()
user_id = email.split("@")[0]
UserRepository(conn).create(
id=user_id, email=email, name=user_id, password_hash=ph.hash(password),
)
if admin:
from tests.helpers.auth import grant_admin
grant_admin(conn, user_id)
conn.close()
r = client.post("/auth/token", json={"email": email, "password": password})
assert r.status_code == 200, r.text
return user_id, {"access_token": r.json()["access_token"]}
def _seed_marketplace_with_plugin(
*,
marketplace: str = "mkt-x",
plugin: str = "alpha",
) -> None:
"""Insert a marketplace + plugin row directly. We bypass the git
sync path here because none of the system-flag behavior depends on
plugin content — it's purely a flag + materialization story."""
from src.db import get_system_db
conn = get_system_db()
try:
existing = conn.execute(
"SELECT 1 FROM marketplace_registry WHERE id = ?", [marketplace],
).fetchone()
if not existing:
conn.execute(
"INSERT INTO marketplace_registry (id, name, url, registered_at) "
"VALUES (?, ?, ?, ?)",
[marketplace, marketplace.upper(),
f"https://example.test/{marketplace}.git",
datetime.now(timezone.utc)],
)
meta = {"name": plugin, "version": "1.0", "description": "desc"}
conn.execute(
"INSERT INTO marketplace_plugins "
"(marketplace_id, name, description, version, raw, updated_at) "
"VALUES (?, ?, ?, ?, ?, ?) "
"ON CONFLICT (marketplace_id, name) DO NOTHING",
[marketplace, plugin, meta["description"], meta["version"],
json.dumps(meta), datetime.now(timezone.utc)],
)
finally:
conn.close()
def _add_group(name: str = "engineers") -> str:
"""Create a non-system group and return its id. Mark on this group
fans out a grant; cleanup on unmark leaves it intact."""
from src.db import get_system_db
from src.repositories.user_groups import UserGroupsRepository
conn = get_system_db()
try:
return UserGroupsRepository(conn).create(name=name)["id"]
finally:
conn.close()
# ---------------------------------------------------------------------------
# Mark / Unmark endpoint behavior
# ---------------------------------------------------------------------------
class TestMarkUnmark:
def test_mark_404_when_plugin_missing(self, web_client):
_, cookies = _create_user(web_client, "admin@x.com", admin=True)
r = web_client.post(
"/api/marketplaces/missing/plugins/ghost/system",
cookies=cookies,
)
assert r.status_code == 404
def test_mark_requires_admin(self, web_client):
_seed_marketplace_with_plugin()
_, cookies = _create_user(web_client, "user@x.com", admin=False)
r = web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system",
cookies=cookies,
)
# require_admin returns 403 on non-admins
assert r.status_code in (401, 403)
def test_mark_flips_flag_and_fans_out(self, web_client):
"""After mark, every existing user has a subscription row and
every existing group has a grant row for the marked plugin."""
_seed_marketplace_with_plugin()
admin_id, admin_cookies = _create_user(
web_client, "admin@x.com", admin=True,
)
# Pre-existing non-admin user + custom group so we can observe
# both fanout dimensions.
regular_id, _ = _create_user(web_client, "regular@x.com")
gid = _add_group("engineers")
r = web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system",
cookies=admin_cookies,
)
assert r.status_code == 200, r.text
body = r.json()
assert body["is_system"] is True
# affected_users counts NEW subscription rows — both admin and
# regular start un-subscribed, so we expect at least both, plus
# the seeded scheduler service user that the app may have
# bootstrapped during create_app.
assert body["affected_users"] >= 2
from src.db import get_system_db
conn = get_system_db()
try:
row = conn.execute(
"SELECT is_system FROM marketplace_plugins "
"WHERE marketplace_id = 'mkt-x' AND name = 'alpha'"
).fetchone()
assert row[0] is True
# Subscription row exists for both users.
for uid in (admin_id, regular_id):
sub = conn.execute(
"SELECT 1 FROM user_plugin_optouts "
"WHERE user_id = ? AND marketplace_id = 'mkt-x' "
"AND plugin_name = 'alpha'",
[uid],
).fetchone()
assert sub is not None, f"subscription missing for {uid}"
# Grant row exists for engineers + Admin + Everyone (system seeded).
grant_groups = {
r[0] for r in conn.execute(
"SELECT group_id FROM resource_grants "
"WHERE resource_type = 'marketplace_plugin' "
"AND resource_id = 'mkt-x/alpha'",
).fetchall()
}
assert gid in grant_groups, "engineers group never received fanout grant"
finally:
conn.close()
def test_mark_is_idempotent(self, web_client):
_seed_marketplace_with_plugin()
_, cookies = _create_user(web_client, "admin@x.com", admin=True)
first = web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system", cookies=cookies,
)
assert first.status_code == 200
# Second call must succeed and report 0 newly affected — every
# row was already in place.
second = web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system", cookies=cookies,
)
assert second.status_code == 200
assert second.json()["affected_users"] == 0
assert second.json()["affected_groups"] == 0
def test_unmark_flips_flag_but_keeps_rows(self, web_client):
"""The agreed semantic: unmark only flips the flag. Existing
grants and subscriptions persist so a confused click doesn't
rip the plugin out of every user's stack mid-day."""
_seed_marketplace_with_plugin()
_, cookies = _create_user(web_client, "admin@x.com", admin=True)
_, _ = _create_user(web_client, "regular@x.com")
gid = _add_group("engineers")
web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system", cookies=cookies,
)
r = web_client.delete(
"/api/marketplaces/mkt-x/plugins/alpha/system", cookies=cookies,
)
assert r.status_code == 204
from src.db import get_system_db
conn = get_system_db()
try:
row = conn.execute(
"SELECT is_system FROM marketplace_plugins "
"WHERE marketplace_id = 'mkt-x' AND name = 'alpha'"
).fetchone()
assert row[0] is False
# Subscription rows survive — admin curates cleanup later.
count = conn.execute(
"SELECT COUNT(*) FROM user_plugin_optouts "
"WHERE marketplace_id = 'mkt-x' AND plugin_name = 'alpha'",
).fetchone()[0]
assert count >= 2, "subscriptions should persist past unmark"
# Grant for engineers survives too.
grant = conn.execute(
"SELECT 1 FROM resource_grants "
"WHERE group_id = ? AND resource_type = 'marketplace_plugin' "
"AND resource_id = 'mkt-x/alpha'",
[gid],
).fetchone()
assert grant is not None
finally:
conn.close()
# ---------------------------------------------------------------------------
# Bypass-path guards
# ---------------------------------------------------------------------------
class TestGuards:
def test_unsubscribe_via_my_stack_refused(self, web_client):
_seed_marketplace_with_plugin()
_, admin_cookies = _create_user(
web_client, "admin@x.com", admin=True,
)
web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system",
cookies=admin_cookies,
)
# require_admin grants the admin access via the Admin group seed,
# so they'll see the plugin in their stack and can attempt the
# toggle. The guard should refuse.
r = web_client.put(
"/api/my-stack/curated/mkt-x/alpha",
json={"enabled": False}, cookies=admin_cookies,
)
assert r.status_code == 409
assert r.json()["detail"] == "cannot_unsubscribe_system_plugin"
def test_uninstall_via_marketplace_refused(self, web_client):
_seed_marketplace_with_plugin()
_, admin_cookies = _create_user(
web_client, "admin@x.com", admin=True,
)
web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system",
cookies=admin_cookies,
)
r = web_client.delete(
"/api/marketplace/curated/mkt-x/alpha/install",
cookies=admin_cookies,
)
assert r.status_code == 409
assert r.json()["detail"] == "cannot_uninstall_system_plugin"
def test_grant_delete_refused(self, web_client):
_seed_marketplace_with_plugin()
_, admin_cookies = _create_user(
web_client, "admin@x.com", admin=True,
)
gid = _add_group("engineers")
web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system",
cookies=admin_cookies,
)
# Find the engineers group's grant for this plugin.
from src.db import get_system_db
conn = get_system_db()
try:
grant_id = conn.execute(
"SELECT id FROM resource_grants "
"WHERE group_id = ? AND resource_type = 'marketplace_plugin' "
"AND resource_id = 'mkt-x/alpha'",
[gid],
).fetchone()[0]
finally:
conn.close()
r = web_client.delete(
f"/api/admin/grants/{grant_id}", cookies=admin_cookies,
)
assert r.status_code == 409
assert r.json()["detail"] == "cannot_revoke_system_grant"
def test_subscribe_via_my_stack_still_allowed(self, web_client):
"""The guard refuses unsubscribe only — explicit subscribe must
keep working since the row already exists (idempotent)."""
_seed_marketplace_with_plugin()
_, admin_cookies = _create_user(
web_client, "admin@x.com", admin=True,
)
web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system",
cookies=admin_cookies,
)
r = web_client.put(
"/api/my-stack/curated/mkt-x/alpha",
json={"enabled": True}, cookies=admin_cookies,
)
assert r.status_code == 200
# ---------------------------------------------------------------------------
# Creation hooks
# ---------------------------------------------------------------------------
class TestCreationHooks:
def test_new_group_inherits_grant(self, web_client):
"""A group created AFTER mark gets the system grant via
ResourceGrantsRepository.fanout_system_for_group, called from
UserGroupsRepository.create()."""
_seed_marketplace_with_plugin()
_, admin_cookies = _create_user(
web_client, "admin@x.com", admin=True,
)
web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system",
cookies=admin_cookies,
)
# Create a brand-new group via the admin POST endpoint.
r = web_client.post(
"/api/admin/groups",
json={"name": "post-mark-group", "description": "test"},
cookies=admin_cookies,
)
assert r.status_code in (200, 201), r.text
new_gid = r.json()["id"]
from src.db import get_system_db
conn = get_system_db()
try:
grant = conn.execute(
"SELECT 1 FROM resource_grants "
"WHERE group_id = ? AND resource_type = 'marketplace_plugin' "
"AND resource_id = 'mkt-x/alpha'",
[new_gid],
).fetchone()
assert grant is not None, "new group did not inherit system grant"
finally:
conn.close()
def test_new_user_inherits_subscription(self, web_client):
"""A user created via the admin POST endpoint AFTER mark gets a
subscription row via fanout_system_for_user."""
_seed_marketplace_with_plugin()
_, admin_cookies = _create_user(
web_client, "admin@x.com", admin=True,
)
web_client.post(
"/api/marketplaces/mkt-x/plugins/alpha/system",
cookies=admin_cookies,
)
r = web_client.post(
"/api/users",
json={
"email": "fresh@example.com",
"name": "Fresh",
"send_invite": False,
},
cookies=admin_cookies,
)
assert r.status_code in (200, 201), r.text
new_uid = r.json()["id"]
from src.db import get_system_db
conn = get_system_db()
try:
sub = conn.execute(
"SELECT 1 FROM user_plugin_optouts "
"WHERE user_id = ? AND marketplace_id = 'mkt-x' "
"AND plugin_name = 'alpha'",
[new_uid],
).fetchone()
assert sub is not None, "new user did not inherit subscription"
finally:
conn.close()
# ---------------------------------------------------------------------------
# Sync preservation
# ---------------------------------------------------------------------------
def test_resync_preserves_is_system(tmp_path, monkeypatch):
"""``replace_for_marketplace`` re-runs every sync. The is_system
flag MUST survive — it's not in the ON CONFLICT DO UPDATE SET list
and not in the INSERT VALUES list. Test by faking a sync via the
repo with the same plugin name.
Uses ``tmp_path`` directly (no web_client) because the test only
exercises the repo, not any API surface — but we still need a
fresh DATA_DIR so we don't inherit state from a sibling test that
populated ``store_entities`` etc. through the migration ladder.
"""
monkeypatch.setenv("DATA_DIR", str(tmp_path))
monkeypatch.setenv("TESTING", "1")
(tmp_path / "state").mkdir(exist_ok=True)
(tmp_path / "analytics").mkdir(exist_ok=True)
(tmp_path / "extracts").mkdir(exist_ok=True)
from src.db import close_system_db, get_system_db
from src.repositories.marketplace_plugins import MarketplacePluginsRepository
close_system_db()
conn = get_system_db()
try:
# Set up registry + initial plugin.
conn.execute(
"INSERT INTO marketplace_registry (id, name, url, registered_at) "
"VALUES (?, ?, ?, ?) ON CONFLICT DO NOTHING",
["resync-test", "Resync", "https://example.test/r.git",
datetime.now(timezone.utc)],
)
repo = MarketplacePluginsRepository(conn)
repo.replace_for_marketplace(
"resync-test",
[{"name": "alpha", "version": "1.0", "description": "v1"}],
)
# Mark as system.
conn.execute(
"UPDATE marketplace_plugins SET is_system = TRUE "
"WHERE marketplace_id = 'resync-test' AND name = 'alpha'"
)
# Re-sync with updated description.
repo.replace_for_marketplace(
"resync-test",
[{"name": "alpha", "version": "2.0", "description": "v2-updated"}],
)
row = conn.execute(
"SELECT is_system, version, description FROM marketplace_plugins "
"WHERE marketplace_id = 'resync-test' AND name = 'alpha'"
).fetchone()
assert row[0] is True, "is_system was reset by resync"
assert row[1] == "2.0"
assert row[2] == "v2-updated"
finally:
conn.close()
close_system_db()