Functional tests for the acl permission — ACL policy-management endpoints (sonador#73)

Sonador Clauderequested to merge
claude/acl-endpoint-tests into master

Summary

Functional test coverage for the acl permission specified in sonador#73 (closed) (Sonador ACL MS1: Orthanc "acl" permission functional tests) — the highest-priority remaining ACL coverage gap identified in oak-tree/medical-imaging/sonador#64.

tests/tests_sonadoracl_manage.py (SonadorAclPermissionTests, registered in tests/__init__.py; originally drafted as tests_sonadoracl_acl.py, renamed during review), covering all 11 checklist cases from #73:

Global (4) — wildcard, and patient=/study=/series= scoped acl policies. A shared fixture (one patient, study A with two sibling series, study B with one series — the second study staged only by the two cases that need it) gives every scope level a sibling/parent to assert the descend-only boundary against: patient scope authorizes both studies and all three series; study scope authorizes its own series but not the sibling study; series scope authorizes only itself, not the parent study or a sibling series.

Local (3) — the same descend-only boundary via local (Orthanc-metadata) ACL=True grants at patient/study/series level.

Negative/regression (4) — query=True, acl=False denied via the DICOMweb route; modify=True, acl=False denied via the internal route; revocation (both global and local variants) denies further policy creation; and the binding separation decision that acl does not imply view (a principal with acl=True, view=False can create/enumerate policies but is denied view-mediated retrieval of the same resource).

Status: ✅ 11/11 passing live against dev01

The enforcement fix landed across three coordinated MRs — oak-tree/medical-imaging/sonador!91, orthanc-sonador!56 (merged), orthanc-authorization!2 (merged) (deploy together). This suite caught two real bugs along the way before reaching a clean pass:

  1. The vulnerability the suite was written to catch: pre-fix, a modify=True, acl=False user's ACL-policy-creation request succeeded with no exception (testacl_negative_modify_not_acl_internal). Confirmed live, not just inferred.
  2. A live-only regression the fix's own static trace missed: local (not global) acl grants at study/series level were denied even against their own resource, due to an indentation bug in orthanc-sonador's _fetch_patient_policy (acl_traverse nested inside the wrong if block). Caught by re-running the suite live after the fix "traced clean" on paper — see sonador#73 (closed) for the full back-and-forth. Fixed in orthanc-sonador!56 (merged) (commit 917bb51); re-confirmed live afterward.

Full suite now passes 11/11, live, twice, against a rebuilt dev01.

Cleanup pass (no behavior change, re-verified 11/11 before and after)

  • DRY: deployAclFixture's ACL-clearing now reuses the existing AclBaseTestCase.clearSeriesTestAcl instead of a hand-rolled loop; the duplicated "assert client error status" helper (independently reinvented in this file and in tests_ext_worklist_procedures.py) is now one shared assertRejected/assertDenied pair on AclBaseTestCase (see companion sonador-client MR below) — both files inherit it.
  • Consolidation: new setupAclActors/setupAclScenario helpers replace ~9 lines of repeated per-test boilerplate (provisioning creator/beneficiary + baseline association) with a single call in all 11 tests.
  • Naming: per the Guru/Acorn convention (camelCase ⇒ public, no leading underscore), renamed _deployAclFixture→deployAclFixture, _cleanupAclFixture→cleanupAclFixture, and (in tests_ext_worklist_procedures.py) _runReviewArc→runReviewArc. Methods that predate this effort and were already leading-underscore (e.g. _verifySeriesLocalAcl, _dcmread_testfile) were left untouched.
  • Right-sized fixtures: the shared fixture now only stages the second study (two_studies=True) for the 2 of 11 tests that actually assert against a sibling study, instead of unconditionally uploading/indexing/clearing it in all 11 — cuts a meaningful chunk of upload/index overhead (and the very long per-test application-log volume that prompted this pass) from the other 9.

Known gap

None of the 7 primary (must-succeed) cases exercise the DICOMweb route with a positive grant — only the negative case (testacl_negative_query_not_acl_dicomweb) touches DICOMweb, and only to prove a deny. A follow-up will add a positive DICOMweb grant case.

Depends on

  • oak-tree/medical-imaging/sonador-client!74 — adds the shared assertRejected/assertDenied helpers and the uploadAndIndexDcmFile rename this suite (and the existing worklist-procedure suite) now relies on. Should merge together with, or before, this MR.

Related

Edited by Sonador Claude

Merge request reports