-
+
)}
{open && isDrilldown(metric.key) && (
@@ -281,8 +197,6 @@ interface DataQualityProps {
windowDays: number;
minRecords: number;
lateCteLabel: string;
- /** Called after the agreed sending time is saved, to reload the measures. */
- onSendingHoursSaved?: () => void;
}
/** Four separate data quality measures for one facility. No combined score. */
@@ -293,7 +207,6 @@ export function DataQuality({
windowDays,
minRecords,
lateCteLabel,
- onSendingHoursSaved,
}: DataQualityProps) {
if (unavailable) {
return
Data quality unavailable
;
@@ -314,7 +227,6 @@ export function DataQuality({
minRecords={minRecords}
lateCteLabel={lateCteLabel}
agreedSendingHours={summary.agreed_sending_hours ?? null}
- onSendingHoursSaved={onSendingHoursSaved}
/>
))}
diff --git a/frontend/src/app/dashboard/suppliers/page.tsx b/frontend/src/app/dashboard/suppliers/page.tsx
index aace5472a..058bd71bb 100644
--- a/frontend/src/app/dashboard/suppliers/page.tsx
+++ b/frontend/src/app/dashboard/suppliers/page.tsx
@@ -910,7 +910,6 @@ export default function SupplierDashboardPage() {
windowDays={dataQuality?.window_days ?? 30}
minRecords={dataQuality?.min_records ?? 20}
lateCteLabel={dataQuality?.late_cte_label ?? ''}
- onSendingHoursSaved={loadDataQuality}
/>
)}
{comp.gaps.length > 0 && (
diff --git a/frontend/src/lib/api-client.ts b/frontend/src/lib/api-client.ts
index e0b20fbf9..2cac34272 100644
--- a/frontend/src/lib/api-client.ts
+++ b/frontend/src/lib/api-client.ts
@@ -893,7 +893,7 @@ class APIClient {
agreedSendingHours: number | null,
): Promise {
const { data } = await this.adminClient.put(
- `/v1/supplier/facilities/${encodeURIComponent(facilityId)}/agreed-sending-hours`,
+ `/v1/supplier/admin/facilities/${encodeURIComponent(facilityId)}/agreed-sending-hours`,
{ agreed_sending_hours: agreedSendingHours },
);
return data;
diff --git a/services/admin/app/supplier_compliance_routes.py b/services/admin/app/supplier_compliance_routes.py
index 41341f2ec..594301a7a 100644
--- a/services/admin/app/supplier_compliance_routes.py
+++ b/services/admin/app/supplier_compliance_routes.py
@@ -40,6 +40,7 @@
from .dependencies import get_current_user
from .models import TenantContext
from .sqlalchemy_models import SupplierFacilityModel, UserModel
+from .tenant_settings_routes import _ADMIN_ROLE_NAMES, _get_user_role_name
from .supplier_onboarding_routes import (
SupplierComplianceGap,
SupplierComplianceGapsResponse,
@@ -356,14 +357,38 @@ async def get_supplier_data_quality_records(
)
-@router.put("/facilities/{facility_id}/agreed-sending-hours", response_model=AgreedSendingHoursResponse)
+class TenantAdminChecker:
+ """Allow only an Owner or Admin membership in the bound tenant.
+
+ Reuses the tenant settings role gate (#1386). ``required_permission``
+ is read by the route security manifest.
+ """
+
+ required_permission = "tenant.admin"
+
+ def __call__(self, user: UserModel = Depends(get_current_user), db: Session = Depends(get_session)) -> UserModel:
+ tenant_id = TenantContext.get_tenant_context(db)
+ if not tenant_id:
+ raise HTTPException(status_code=403, detail="E_NO_TENANT_CONTEXT: tenant-scoped endpoint requires X-Tenant-ID")
+ if _get_user_role_name(tenant_id, user, db) not in _ADMIN_ROLE_NAMES:
+ raise HTTPException(status_code=403, detail="Only tenant Owner or Admin roles may set agreed sending hours")
+ return user
+
+
+require_tenant_admin = TenantAdminChecker()
+
+
+@router.put("/admin/facilities/{facility_id}/agreed-sending-hours", response_model=AgreedSendingHoursResponse)
async def set_agreed_sending_hours(
facility_id: str,
request: AgreedSendingHoursRequest,
- current_user: UserModel = Depends(get_current_user),
+ _admin: UserModel = Depends(require_tenant_admin),
db: Session = Depends(get_session),
) -> AgreedSendingHoursResponse:
- """Set or clear (null) the agreed sending time for one owned facility."""
+ """Tenant admin sets or clears (null) the agreed sending time for a facility in the tenant.
+
+ Suppliers can read it in their data quality summary but cannot change it.
+ """
tenant_id = TenantContext.get_tenant_context(db)
if not tenant_id:
raise HTTPException(status_code=400, detail="Tenant context required")
@@ -371,12 +396,15 @@ async def set_agreed_sending_hours(
scoped_facility_id = uuid_module.UUID(facility_id)
except ValueError as exc:
raise HTTPException(status_code=400, detail="Invalid facility id") from exc
- facility = _get_supplier_facility_or_404(
- db,
- tenant_id=tenant_id,
- supplier_user_id=current_user.id,
- facility_id=scoped_facility_id,
- )
+ # Tenant scoping lives in the query: a facility in another tenant is never loaded.
+ facility = db.execute(
+ select(SupplierFacilityModel).where(
+ SupplierFacilityModel.id == scoped_facility_id,
+ SupplierFacilityModel.tenant_id == tenant_id,
+ )
+ ).scalar_one_or_none()
+ if facility is None:
+ raise HTTPException(status_code=404, detail="Facility not found")
facility.agreed_sending_hours = request.agreed_sending_hours
db.commit()
diff --git a/services/admin/tests/test_supplier_data_quality_routes.py b/services/admin/tests/test_supplier_data_quality_routes.py
index 29796fe8e..bfede37fc 100644
--- a/services/admin/tests/test_supplier_data_quality_routes.py
+++ b/services/admin/tests/test_supplier_data_quality_routes.py
@@ -81,6 +81,12 @@ def commit(self) -> None:
def get(self, _model, key):
return self.facilities.get(key)
+ def execute(self, stmt):
+ # The admin write filters by id and tenant in SQL; match on the bound values.
+ bound = set(stmt.compile().params.values())
+ found = [f for f in self.facilities.values() if f.id in bound and f.tenant_id in bound]
+ return SimpleNamespace(scalar_one_or_none=lambda: found[0] if found else None)
+
def scalars(self, _stmt):
# The route filters by tenant and user in SQL; the fake returns what that
# filter yields for TENANT and USER.
@@ -92,9 +98,12 @@ def calls():
return []
-def _client(monkeypatch, calls, tenant=TENANT, session=None):
+def _client(monkeypatch, calls, tenant=TENANT, session=None, role=None):
import app.supplier_compliance_routes as routes
+ # Membership role of the caller in the bound tenant; None means no membership.
+ monkeypatch.setattr(routes, "_get_user_role_name", lambda _tenant, _user, _db: role)
+
session = session if session is not None else FakeSession()
def fake_records(_db, tenant_id, user_id, facility_ids, since):
@@ -252,38 +261,65 @@ def test_drilldown_rejects_bad_facility_id_and_limit(monkeypatch, calls):
def _hours_url(facility) -> str:
- return f"/v1/supplier/facilities/{facility}/agreed-sending-hours"
+ return f"/v1/supplier/admin/facilities/{facility}/agreed-sending-hours"
+
+
+def _admin_role() -> str:
+ from app.tenant_settings_routes import _ADMIN_ROLE_NAMES
+
+ return sorted(_ADMIN_ROLE_NAMES)[0]
+
+def _admin(monkeypatch, calls, **kwargs):
+ return _client(monkeypatch, calls, role=_admin_role(), **kwargs)
-def test_owner_sets_agreed_sending_hours(monkeypatch, calls):
+
+@pytest.mark.parametrize("facility", [OWN_FACILITY, FOREIGN_FACILITY])
+def test_tenant_admin_sets_agreed_sending_hours_for_any_facility_in_tenant(monkeypatch, calls, facility):
session = FakeSession()
- resp = _client(monkeypatch, calls, session=session).put(
- _hours_url(OWN_FACILITY), json={"agreed_sending_hours": AGREED_HOURS}
- )
+ resp = _admin(monkeypatch, calls, session=session).put(_hours_url(facility), json={"agreed_sending_hours": AGREED_HOURS})
assert resp.status_code == 200
- assert resp.json() == {"facility_id": str(OWN_FACILITY), "agreed_sending_hours": AGREED_HOURS}
- assert session.facilities[OWN_FACILITY].agreed_sending_hours == AGREED_HOURS
+ assert resp.json() == {"facility_id": str(facility), "agreed_sending_hours": AGREED_HOURS}
+ assert session.facilities[facility].agreed_sending_hours == AGREED_HOURS
assert session.commits == len([resp])
-def test_owner_clears_agreed_sending_hours_with_null(monkeypatch, calls):
+def test_tenant_admin_clears_agreed_sending_hours_with_null(monkeypatch, calls):
session = FakeSession(own_hours=AGREED_HOURS)
- resp = _client(monkeypatch, calls, session=session).put(
- _hours_url(OWN_FACILITY), json={"agreed_sending_hours": None}
- )
+ resp = _admin(monkeypatch, calls, session=session).put(_hours_url(OWN_FACILITY), json={"agreed_sending_hours": None})
assert resp.status_code == 200
assert resp.json()["agreed_sending_hours"] is None
assert session.facilities[OWN_FACILITY].agreed_sending_hours is None
-@pytest.mark.parametrize("facility", [FOREIGN_FACILITY, OTHER_TENANT_FACILITY])
-def test_agreed_sending_hours_refuses_facility_not_owned_in_tenant(monkeypatch, calls, facility):
+@pytest.mark.parametrize("role", [None, "Member", "Viewer", "supplier"])
+def test_supplier_user_cannot_set_agreed_sending_hours(monkeypatch, calls, role):
+ # The supplier owns OWN_FACILITY and is still refused: the setting is the tenant's.
session = FakeSession()
- resp = _client(monkeypatch, calls, session=session).put(
- _hours_url(facility), json={"agreed_sending_hours": AGREED_HOURS}
+ resp = _client(monkeypatch, calls, session=session, role=role).put(
+ _hours_url(OWN_FACILITY), json={"agreed_sending_hours": AGREED_HOURS}
+ )
+ assert resp.status_code == 403
+ assert session.facilities[OWN_FACILITY].agreed_sending_hours is None
+ assert session.commits == 0
+
+
+def test_old_supplier_write_path_is_gone(monkeypatch, calls):
+ session = FakeSession()
+ resp = _client(monkeypatch, calls, session=session, role=_admin_role()).put(
+ f"/v1/supplier/facilities/{OWN_FACILITY}/agreed-sending-hours", json={"agreed_sending_hours": AGREED_HOURS}
+ )
+ assert resp.status_code in (404, 405)
+ assert session.commits == 0
+
+
+def test_admin_of_another_tenant_gets_404(monkeypatch, calls):
+ session = FakeSession()
+ resp = _admin(monkeypatch, calls, session=session).put(
+ _hours_url(OTHER_TENANT_FACILITY), json={"agreed_sending_hours": AGREED_HOURS}
)
assert resp.status_code == 404
- assert session.facilities[facility].agreed_sending_hours is None
+ assert session.facilities[OTHER_TENANT_FACILITY].agreed_sending_hours is None
assert session.commits == 0
@@ -303,7 +339,7 @@ def test_agreed_sending_hours_refuses_facility_not_owned_in_tenant(monkeypatch,
)
def test_agreed_sending_hours_must_be_positive_int_or_null(monkeypatch, calls, body):
session = FakeSession()
- resp = _client(monkeypatch, calls, session=session).put(_hours_url(OWN_FACILITY), json=body)
+ resp = _admin(monkeypatch, calls, session=session).put(_hours_url(OWN_FACILITY), json=body)
assert resp.status_code == 422
assert session.facilities[OWN_FACILITY].agreed_sending_hours is None
assert session.commits == 0
@@ -311,5 +347,82 @@ def test_agreed_sending_hours_must_be_positive_int_or_null(monkeypatch, calls, b
def test_agreed_sending_hours_rejects_bad_id_and_missing_tenant(monkeypatch, calls):
body = {"agreed_sending_hours": AGREED_HOURS}
- assert _client(monkeypatch, calls).put(_hours_url("not-a-uuid"), json=body).status_code == 400
- assert _client(monkeypatch, calls, tenant=None).put(_hours_url(OWN_FACILITY), json=body).status_code == 400
+ assert _admin(monkeypatch, calls).put(_hours_url("not-a-uuid"), json=body).status_code == 400
+ assert _admin(monkeypatch, calls, tenant=None).put(_hours_url(OWN_FACILITY), json=body).status_code == 403
+
+
+# --- real TenantAdminChecker membership path ----------------------------------
+# No stubbed role lookup: memberships, roles and the facility live in an
+# in-memory SQLite database built from the real models.
+
+
+def _membership_client(monkeypatch, *, member_tenant, is_active):
+ import app.supplier_compliance_routes as routes
+ from app.sqlalchemy_models import (
+ MembershipModel,
+ RoleModel,
+ SupplierFacilityModel,
+ TenantModel,
+ UserModel,
+ )
+ from sqlalchemy import create_engine
+ from sqlalchemy.orm import sessionmaker
+ from sqlalchemy.pool import StaticPool
+
+ engine = create_engine(
+ "sqlite:///:memory:", connect_args={"check_same_thread": False}, poolclass=StaticPool, future=True
+ )
+ for model in (TenantModel, UserModel, RoleModel, MembershipModel, SupplierFacilityModel):
+ model.__table__.create(bind=engine)
+ session = sessionmaker(bind=engine, expire_on_commit=False, future=True)()
+
+ bound_tenant, other_tenant = uuid4(), uuid4()
+ tenant_ids = {"bound": bound_tenant, "other": other_tenant}
+ for tenant_id in tenant_ids.values():
+ session.add(TenantModel(id=tenant_id, name=str(tenant_id), slug=str(tenant_id), settings={}))
+ owner = UserModel(id=uuid4(), email=f"{uuid4()}@example.com")
+ supplier = UserModel(id=uuid4(), email=f"{uuid4()}@example.com")
+ role = RoleModel(id=uuid4(), tenant_id=None, name="Owner", permissions=[])
+ session.add_all([owner, supplier, role])
+ session.flush()
+ session.add(
+ MembershipModel(user_id=owner.id, tenant_id=tenant_ids[member_tenant], role_id=role.id, is_active=is_active)
+ )
+ facility = SupplierFacilityModel(
+ id=uuid4(),
+ tenant_id=bound_tenant,
+ supplier_user_id=supplier.id,
+ name=str(uuid4()),
+ street=str(uuid4()),
+ city=str(uuid4()),
+ state=str(uuid4()),
+ postal_code=str(uuid4()),
+ roles=[],
+ )
+ session.add(facility)
+ session.commit()
+
+ monkeypatch.setattr(routes.TenantContext, "get_tenant_context", lambda _db: bound_tenant)
+ app = FastAPI()
+ app.include_router(routes.router, prefix="/v1")
+ app.dependency_overrides[routes.get_session] = lambda: session
+ app.dependency_overrides[routes.get_current_user] = lambda: owner
+ return TestClient(app), session, facility
+
+
+@pytest.mark.parametrize(
+ ("member_tenant", "is_active", "expected"),
+ [
+ ("bound", True, 200),
+ ("bound", False, 403),
+ ("other", True, 403),
+ ],
+ ids=["owner-of-tenant", "inactive-owner", "owner-of-other-tenant"],
+)
+def test_tenant_admin_checker_uses_real_membership(monkeypatch, member_tenant, is_active, expected):
+ client, session, facility = _membership_client(monkeypatch, member_tenant=member_tenant, is_active=is_active)
+ resp = client.put(_hours_url(facility.id), json={"agreed_sending_hours": AGREED_HOURS})
+ assert resp.status_code == expected
+ session.expire_all()
+ stored = session.get(type(facility), facility.id).agreed_sending_hours
+ assert stored == (AGREED_HOURS if expected == 200 else None)
diff --git a/tests/security/test_route_security_manifest.py b/tests/security/test_route_security_manifest.py
index d7622fe8d..e4617e669 100644
--- a/tests/security/test_route_security_manifest.py
+++ b/tests/security/test_route_security_manifest.py
@@ -73,8 +73,8 @@ def test_checked_in_manifest_matches_live_route_and_dependency_graph(manifest):
# +1 tenant-scoped (#3572): admin:GET:/v1/supplier/gap-time-to-fix.
# +2 tenant-scoped: admin:GET:/v1/supplier/data-quality and
# admin:GET:/v1/supplier/data-quality/{facility_id}/records, owner-only, tenant-filtered.
- # +1 tenant-scoped: admin:PUT:/v1/supplier/facilities/{facility_id}/agreed-sending-hours,
- # owner-only, tenant-filtered.
+ # +1 tenant-scoped: admin:PUT:/v1/supplier/admin/facilities/{facility_id}/agreed-sending-hours,
+ # tenant Owner or Admin only (tenant.admin), tenant-filtered.
"enabled_operation_count": 258,
# 178/161 includes graph:GET:/api/v1/provisions/by-request: it is added
# to the graph v1_router AFTER router.include_router(v1_router), so the