fix: close task 3 resolution findings
This commit is contained in:
@@ -71,3 +71,61 @@ The warnings are existing third-party test-environment warnings from `pytest_asy
|
|||||||
|
|
||||||
- Live PostgreSQL and agent execution were not exercised; verification used the repository's offline SQLite test fixtures.
|
- Live PostgreSQL and agent execution were not exercised; verification used the repository's offline SQLite test fixtures.
|
||||||
- The blocker remains open until the agent's normal reprocess completion path reports resolution; this prevents marking it resolved before the bounded job succeeds.
|
- The blocker remains open until the agent's normal reprocess completion path reports resolution; this prevents marking it resolved before the bounded job succeeds.
|
||||||
|
|
||||||
|
## Review Fixes
|
||||||
|
|
||||||
|
### RED
|
||||||
|
|
||||||
|
Added regression tests before changing production code for the four review findings:
|
||||||
|
|
||||||
|
- Legacy blockers with `installation_id=NULL` must fail closed without selecting an Agent or creating work.
|
||||||
|
- Cross-tenant installation and Agent pointers must fail before context, job, or audit mutation.
|
||||||
|
- Oversized values/reasons and non-string fields must be rejected cleanly.
|
||||||
|
- Successful web resolution must redirect to the blocker detail page.
|
||||||
|
|
||||||
|
Focused command before the fixes:
|
||||||
|
|
||||||
|
```text
|
||||||
|
python -m pytest central/tests/test_blocker_resolution_panel.py -q
|
||||||
|
```
|
||||||
|
|
||||||
|
RED result:
|
||||||
|
|
||||||
|
```text
|
||||||
|
4 failed, 5 passed
|
||||||
|
```
|
||||||
|
|
||||||
|
The failures reproduced the legacy Agent fallback, cross-tenant acceptance, unbounded/malformed input, and `/blockers` redirect behavior.
|
||||||
|
|
||||||
|
### GREEN
|
||||||
|
|
||||||
|
- Resolution now requires a non-null exact installation and verifies both installation and Agent tenant ownership before mutation.
|
||||||
|
- `value` is required to be a string of at most 512 characters; `reason` is required to be a string of at most 2000 characters.
|
||||||
|
- Boundary-sized audit data retains all required audit fields without whole-context truncation.
|
||||||
|
- Web success redirects to `/blockers/{blocker_id}`.
|
||||||
|
|
||||||
|
Focused command:
|
||||||
|
|
||||||
|
```text
|
||||||
|
python -m pytest central/tests/test_blocker_resolution_panel.py -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Result:
|
||||||
|
|
||||||
|
```text
|
||||||
|
9 passed, 1 warning
|
||||||
|
```
|
||||||
|
|
||||||
|
Full central command:
|
||||||
|
|
||||||
|
```text
|
||||||
|
python -m pytest central/tests -q
|
||||||
|
```
|
||||||
|
|
||||||
|
Result:
|
||||||
|
|
||||||
|
```text
|
||||||
|
265 passed, 1 warning
|
||||||
|
```
|
||||||
|
|
||||||
|
The existing `pytest_asyncio` configuration and `openpyxl` print-area warnings remain unchanged.
|
||||||
|
|||||||
@@ -13,6 +13,8 @@ from .models import Agent, BlockedProduct, FiscalContextEntry, Installation, Ten
|
|||||||
|
|
||||||
WORKFLOW_SCOPES = {"PRODUCT", "GROUP", "COMPANY"}
|
WORKFLOW_SCOPES = {"PRODUCT", "GROUP", "COMPANY"}
|
||||||
ENTRY_FIELDS = {"scope", "scope_key", "key", "value", "reason"}
|
ENTRY_FIELDS = {"scope", "scope_key", "key", "value", "reason"}
|
||||||
|
MAX_VALUE_LENGTH = 512
|
||||||
|
MAX_REASON_LENGTH = 2000
|
||||||
|
|
||||||
|
|
||||||
def validate_resolution_input(payload):
|
def validate_resolution_input(payload):
|
||||||
@@ -22,6 +24,14 @@ def validate_resolution_input(payload):
|
|||||||
unexpected = set(payload) - ENTRY_FIELDS
|
unexpected = set(payload) - ENTRY_FIELDS
|
||||||
if unexpected:
|
if unexpected:
|
||||||
return None, f"unsupported resolution fields: {sorted(unexpected)}"
|
return None, f"unsupported resolution fields: {sorted(unexpected)}"
|
||||||
|
if not isinstance(payload.get("value"), str):
|
||||||
|
return None, "value must be a string"
|
||||||
|
if not isinstance(payload.get("reason"), str):
|
||||||
|
return None, "reason must be a string"
|
||||||
|
if len(payload["value"]) > MAX_VALUE_LENGTH:
|
||||||
|
return None, f"value must be at most {MAX_VALUE_LENGTH} characters"
|
||||||
|
if len(payload["reason"]) > MAX_REASON_LENGTH:
|
||||||
|
return None, f"reason must be at most {MAX_REASON_LENGTH} characters"
|
||||||
|
|
||||||
normalized, error = validate_entry(payload)
|
normalized, error = validate_entry(payload)
|
||||||
if error:
|
if error:
|
||||||
@@ -69,15 +79,15 @@ def resolve_blocker_with_context(session, blocker_id, admin, payload, request_ip
|
|||||||
installation = session.get(Installation, blocker.installation_id)
|
installation = session.get(Installation, blocker.installation_id)
|
||||||
if installation is None:
|
if installation is None:
|
||||||
raise LookupError("blocker installation not found")
|
raise LookupError("blocker installation not found")
|
||||||
|
if installation.tenant_id != blocker.tenant_id:
|
||||||
|
raise LookupError("blocker installation tenant mismatch")
|
||||||
agent = session.get(Agent, installation.agent_id)
|
agent = session.get(Agent, installation.agent_id)
|
||||||
|
if agent is None:
|
||||||
|
raise LookupError("blocker installation agent not found")
|
||||||
|
if agent.tenant_id != blocker.tenant_id:
|
||||||
|
raise LookupError("blocker agent tenant mismatch")
|
||||||
else:
|
else:
|
||||||
agent = session.scalar(
|
raise LookupError("blocker installation is required")
|
||||||
select(Agent)
|
|
||||||
.where(Agent.tenant_id == blocker.tenant_id, Agent.enabled.is_(True))
|
|
||||||
.order_by(Agent.id.asc())
|
|
||||||
)
|
|
||||||
if agent is None:
|
|
||||||
raise LookupError("blocker agent not found")
|
|
||||||
|
|
||||||
tenant = session.get(Tenant, blocker.tenant_id)
|
tenant = session.get(Tenant, blocker.tenant_id)
|
||||||
company_key = _company_key(blocker, installation, tenant)
|
company_key = _company_key(blocker, installation, tenant)
|
||||||
|
|||||||
+1
-1
@@ -149,7 +149,7 @@ def router(session_factory):
|
|||||||
except ValueError as error:
|
except ValueError as error:
|
||||||
detail = blocker_detail(session, blocker_id)
|
detail = blocker_detail(session, blocker_id)
|
||||||
return render(request, "blocker_detail.html", {"detail": detail, "auth": auth_data, "context_entries": [], "context_keys": sorted(ALLOWED_KEYS), "value_choices": sorted({value for values in ENUMS.values() for value in values}), "error": str(error)}, 422)
|
return render(request, "blocker_detail.html", {"detail": detail, "auth": auth_data, "context_entries": [], "context_keys": sorted(ALLOWED_KEYS), "value_choices": sorted({value for values in ENUMS.values() for value in values}), "error": str(error)}, 422)
|
||||||
return RedirectResponse("/blockers", status_code=303)
|
return RedirectResponse(f"/blockers/{blocker_id}", status_code=303)
|
||||||
finally:
|
finally:
|
||||||
session.close()
|
session.close()
|
||||||
|
|
||||||
|
|||||||
@@ -1,10 +1,11 @@
|
|||||||
from fastapi.testclient import TestClient
|
from fastapi.testclient import TestClient
|
||||||
|
import pytest
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select
|
||||||
|
|
||||||
from central.app.blocked_products import upsert_blocker
|
from central.app.blocked_products import upsert_blocker
|
||||||
from central.app.blocker_resolution import resolve_blocker_with_context, validate_resolution_input
|
from central.app.blocker_resolution import resolve_blocker_with_context, validate_resolution_input
|
||||||
from central.app.main import create_app
|
from central.app.main import create_app
|
||||||
from central.app.models import AdminUser, AuditEvent, FiscalContextEntry, Job, Installation
|
from central.app.models import AdminUser, Agent, AuditEvent, BlockedProduct, FiscalContextEntry, Job, Installation, Tenant
|
||||||
from central.tests.test_control_plane import seed, setup_db
|
from central.tests.test_control_plane import seed, setup_db
|
||||||
|
|
||||||
|
|
||||||
@@ -106,6 +107,78 @@ def test_resolve_blocker_with_context_persists_one_context_row_job_and_sanitized
|
|||||||
assert session.query(FiscalContextEntry).count() == 1
|
assert session.query(FiscalContextEntry).count() == 1
|
||||||
|
|
||||||
|
|
||||||
|
def test_legacy_blocker_without_installation_fails_closed_without_job_or_audit():
|
||||||
|
session_factory = setup_db()
|
||||||
|
blocker_id, admin_id, _ = _seed_blocker(session_factory)
|
||||||
|
|
||||||
|
with session_factory() as session:
|
||||||
|
blocker = session.get(BlockedProduct, blocker_id)
|
||||||
|
blocker.installation_id = None
|
||||||
|
session.commit()
|
||||||
|
admin = session.get(AdminUser, admin_id)
|
||||||
|
|
||||||
|
with pytest.raises(LookupError, match="installation"):
|
||||||
|
resolve_blocker_with_context(session, blocker_id, admin, {"scope": "PRODUCT", "scope_key": "P-001", "key": "product_role", "value": "RESALE", "reason": "reviewed"})
|
||||||
|
|
||||||
|
assert session.query(FiscalContextEntry).count() == 0
|
||||||
|
assert session.query(Job).count() == 0
|
||||||
|
assert session.query(AuditEvent).count() == 0
|
||||||
|
|
||||||
|
|
||||||
|
def test_cross_tenant_installation_pointer_fails_before_mutation():
|
||||||
|
session_factory = setup_db()
|
||||||
|
blocker_id, admin_id, _ = _seed_blocker(session_factory)
|
||||||
|
|
||||||
|
with session_factory() as session:
|
||||||
|
blocker = session.get(BlockedProduct, blocker_id)
|
||||||
|
other_tenant = Tenant(name="Other Client")
|
||||||
|
session.add(other_tenant)
|
||||||
|
session.flush()
|
||||||
|
other_agent = Agent(tenant_id=other_tenant.id, name="other-agent", token_hash="other-agent-token".ljust(64, "x"))
|
||||||
|
session.add(other_agent)
|
||||||
|
session.flush()
|
||||||
|
other_installation = Installation(agent_id=other_agent.id, tenant_id=other_tenant.id, installation_id="22222222-2222-4222-8222-222222222222", company_code="OTHER-COMPANY")
|
||||||
|
session.add(other_installation)
|
||||||
|
session.flush()
|
||||||
|
blocker.installation_id = other_installation.id
|
||||||
|
session.commit()
|
||||||
|
admin = session.get(AdminUser, admin_id)
|
||||||
|
|
||||||
|
with pytest.raises(LookupError, match="tenant"):
|
||||||
|
resolve_blocker_with_context(session, blocker_id, admin, {"scope": "PRODUCT", "scope_key": "P-001", "key": "product_role", "value": "RESALE", "reason": "reviewed"})
|
||||||
|
|
||||||
|
assert session.query(FiscalContextEntry).count() == 0
|
||||||
|
assert session.query(Job).count() == 0
|
||||||
|
assert session.query(AuditEvent).count() == 0
|
||||||
|
|
||||||
|
|
||||||
|
def test_resolution_input_rejects_oversized_and_non_string_fields_cleanly():
|
||||||
|
base = {"scope": "PRODUCT", "scope_key": "P-001", "key": "service_cfop", "value": "5102", "reason": "reviewed"}
|
||||||
|
for field, value in (("value", "v" * 513), ("reason", "r" * 2001), ("reason", 123)):
|
||||||
|
payload = {**base, field: value}
|
||||||
|
normalized, error = validate_resolution_input(payload)
|
||||||
|
assert normalized is None
|
||||||
|
assert error
|
||||||
|
|
||||||
|
|
||||||
|
def test_boundary_length_audit_keeps_required_fields_instead_of_truncating_context():
|
||||||
|
session_factory = setup_db()
|
||||||
|
blocker_id, admin_id, _ = _seed_blocker(session_factory)
|
||||||
|
|
||||||
|
with session_factory() as session:
|
||||||
|
admin = session.get(AdminUser, admin_id)
|
||||||
|
result = resolve_blocker_with_context(
|
||||||
|
session,
|
||||||
|
blocker_id,
|
||||||
|
admin,
|
||||||
|
{"scope": "PRODUCT", "scope_key": "P-001", "key": "service_cfop", "value": "v" * 512, "reason": "r" * 2000},
|
||||||
|
)
|
||||||
|
audit = session.get(AuditEvent, result["audit_id"])
|
||||||
|
required = {"admin_user_id", "tenant_id", "client", "company_key", "installation_id", "product_code", "blocker_id", "key", "scope", "scope_key", "value", "reason", "before", "after", "reprocess_job_id", "request_ip"}
|
||||||
|
assert required <= audit.context.keys()
|
||||||
|
assert "truncated" not in audit.context
|
||||||
|
|
||||||
|
|
||||||
def test_human_resolution_detail_requires_auth_csrf_and_renders_safe_form():
|
def test_human_resolution_detail_requires_auth_csrf_and_renders_safe_form():
|
||||||
session_factory = setup_db()
|
session_factory = setup_db()
|
||||||
blocker_id, _, _ = _seed_blocker(session_factory)
|
blocker_id, _, _ = _seed_blocker(session_factory)
|
||||||
@@ -127,6 +200,14 @@ def test_human_resolution_detail_requires_auth_csrf_and_renders_safe_form():
|
|||||||
)
|
)
|
||||||
assert missing_csrf.status_code == 403
|
assert missing_csrf.status_code == 403
|
||||||
|
|
||||||
|
success = client.post(
|
||||||
|
f"/blockers/{blocker_id}/resolve-context",
|
||||||
|
data={"csrf_token": client.cookies.get("gti_csrf"), "scope": "PRODUCT", "scope_key": "P-001", "key": "product_role", "value": "RESALE", "reason": "reviewed"},
|
||||||
|
follow_redirects=False,
|
||||||
|
)
|
||||||
|
assert success.status_code == 303
|
||||||
|
assert success.headers["location"] == f"/blockers/{blocker_id}"
|
||||||
|
|
||||||
|
|
||||||
def test_human_and_json_resolution_routes_use_the_same_validated_workflow():
|
def test_human_and_json_resolution_routes_use_the_same_validated_workflow():
|
||||||
session_factory = setup_db()
|
session_factory = setup_db()
|
||||||
|
|||||||
Reference in New Issue
Block a user