docs: improve test suite documentation

tests/__init__.py

@@ -0,0 +1,4 @@
+"""Test package marker for the omnibioai-policy-engine unit-test suite.
+
+Developer: Manish Kumar <manish@omnibioai.org>
+"""

tests/conftest.py

@@ -1,3 +1,9 @@
+"""Shared pytest fixtures for the policy engine tests: stubs swagger_ui_bundle
+when the optional package is not installed, and provides a PolicyEngine wired
+to a mock Redis-backed cache so no test connects to a real Redis.
+
+Developer: Manish Kumar <manish@omnibioai.org>
+"""
 import os
 import sys
 import tempfile
@@ -18,11 +24,13 @@
 
 @pytest.fixture
 def mock_redis():
+    """Provide a MagicMock standing in for a Redis client."""
     return MagicMock()
 
 
 @pytest.fixture
 def policy_cache(mock_redis):
+    """Build a PolicyCache with its Redis client replaced by the mock."""
     with patch("app.services.cache.redis") as mock_redis_module:
         mock_redis_module.from_url.return_value = mock_redis
         from app.services.cache import PolicyCache
@@ -33,6 +41,9 @@ def policy_cache(mock_redis):
 
 @pytest.fixture
 def policy_engine(policy_cache):
+    """Build a PolicyEngine using the mocked policy cache, together with the cache and its mock
+    Redis client.
+    """
     cache, mock_redis = policy_cache
     from app.core.engine import PolicyEngine
     engine = PolicyEngine(cache=cache)

tests/test_cache.py

@@ -1,3 +1,9 @@
+"""Unit tests for app/services/cache.py::PolicyCache against a mocked Redis
+client: deterministic cache-key derivation, get/set round-tripping through JSON
+with a default TTL, and per-user cache invalidation.
+
+Developer: Manish Kumar <manish@omnibioai.org>
+"""
 import json
 import hashlib
 import pytest
@@ -6,6 +12,7 @@
 
 @pytest.fixture
 def cache_with_mock():
+    """Build a PolicyCache with its Redis client replaced by a mock."""
     mock_redis = MagicMock()
     with patch("app.services.cache.redis") as mock_redis_module:
         mock_redis_module.from_url.return_value = mock_redis
@@ -20,26 +27,31 @@ def cache_with_mock():
 # ---------------------------------------------------------------------------
 
 def test_build_key_is_deterministic(cache_with_mock):
+    """build_key produces the same key for identical user, action, resource and context arguments.
+    """
     cache, _ = cache_with_mock
     key1 = cache.build_key("u1", "read", "resource", {"env": "prod"})
     key2 = cache.build_key("u1", "read", "resource", {"env": "prod"})
     assert key1 == key2
 
 
 def test_build_key_starts_with_policy_prefix(cache_with_mock):
+    """build_key's output starts with the "policy:" prefix."""
     cache, _ = cache_with_mock
     key = cache.build_key("u1", "read", "res", {})
     assert key.startswith("policy:")
 
 
 def test_build_key_different_users_different_keys(cache_with_mock):
+    """build_key produces different keys for different users."""
     cache, _ = cache_with_mock
     key1 = cache.build_key("u1", "read", "res", {})
     key2 = cache.build_key("u2", "read", "res", {})
     assert key1 != key2
 
 
 def test_build_key_context_order_insensitive(cache_with_mock):
+    """build_key produces the same key regardless of the context dict's key order."""
     cache, _ = cache_with_mock
     key1 = cache.build_key("u1", "read", "res", {"a": 1, "b": 2})
     key2 = cache.build_key("u1", "read", "res", {"b": 2, "a": 1})
@@ -51,6 +63,7 @@ def test_build_key_context_order_insensitive(cache_with_mock):
 # ---------------------------------------------------------------------------
 
 def test_get_returns_parsed_dict_on_hit(cache_with_mock):
+    """get returns the JSON-decoded value stored under the given key."""
     cache, mock_redis = cache_with_mock
     data = {"allowed": True, "reason": "ok", "policy_source": "RBAC"}
     mock_redis.get.return_value = json.dumps(data)
@@ -62,6 +75,7 @@ def test_get_returns_parsed_dict_on_hit(cache_with_mock):
 
 
 def test_get_returns_none_on_miss(cache_with_mock):
+    """get returns None when the key is not in Redis."""
     cache, mock_redis = cache_with_mock
     mock_redis.get.return_value = None
 
@@ -75,6 +89,7 @@ def test_get_returns_none_on_miss(cache_with_mock):
 # ---------------------------------------------------------------------------
 
 def test_set_stores_json_with_ttl(cache_with_mock):
+    """set stores the JSON-encoded value under the key with the given TTL via SETEX."""
     cache, mock_redis = cache_with_mock
     data = {"allowed": False, "reason": "denied"}
 
@@ -84,6 +99,7 @@ def test_set_stores_json_with_ttl(cache_with_mock):
 
 
 def test_set_default_ttl(cache_with_mock):
+    """set uses a default TTL of 300 seconds when none is given."""
     cache, mock_redis = cache_with_mock
     cache.set("policy:key2", {"allowed": True, "reason": "ok"})
 
@@ -96,6 +112,7 @@ def test_set_default_ttl(cache_with_mock):
 # ---------------------------------------------------------------------------
 
 def test_invalidate_user_deletes_only_matching_keys(cache_with_mock):
+    """invalidate_user scans all policy keys and deletes only the ones matching the given user."""
     cache, mock_redis = cache_with_mock
     # Keys that literally contain "u1" trigger deletion; others are skipped
     mock_redis.scan_iter.return_value = iter(["policy:u1_hash", "policy:other_user"])
@@ -107,6 +124,7 @@ def test_invalidate_user_deletes_only_matching_keys(cache_with_mock):
 
 
 def test_invalidate_user_no_matching_keys(cache_with_mock):
+    """invalidate_user deletes nothing when no scanned key matches the given user."""
     cache, mock_redis = cache_with_mock
     mock_redis.scan_iter.return_value = iter(["policy:other_user"])
 
@@ -116,6 +134,7 @@ def test_invalidate_user_no_matching_keys(cache_with_mock):
 
 
 def test_invalidate_user_no_keys(cache_with_mock):
+    """invalidate_user deletes nothing when there are no policy keys at all."""
     cache, mock_redis = cache_with_mock
     mock_redis.scan_iter.return_value = iter([])
 

tests/test_engine.py

@@ -1,3 +1,10 @@
+"""Unit tests for app/core/engine.py::PolicyEngine: RBAC, ABAC and RULES denial
+short-circuit evaluation with the right policy_source and reason, an all-pass
+request is allowed, the Redis-backed cache is read on evaluate() and written on
+a miss, and a cached decision skips policy checks entirely.
+
+Developer: Manish Kumar <manish@omnibioai.org>
+"""
 import json
 import pytest
 from unittest.mock import MagicMock
@@ -7,6 +14,9 @@
 
 
 def make_engine(mock_redis=None):
+    """Build a PolicyEngine with its cache backed by the given (or a default empty) mock Redis
+    client.
+    """
     if mock_redis is None:
         mock_redis = MagicMock()
         mock_redis.get.return_value = None
@@ -21,6 +31,7 @@ def make_engine(mock_redis=None):
 
 
 def basic_request(**kwargs):
+    """Build a PolicyRequest with sensible defaults, overridable by keyword."""
     defaults = {
         "user_id": "u1",
         "email": "u1@test.com",
@@ -39,6 +50,9 @@ def basic_request(**kwargs):
 # ---------------------------------------------------------------------------
 
 def test_rbac_deny_stops_evaluation():
+    """A request missing the researcher role is denied at the RBAC stage with a reason naming the
+    role.
+    """
     engine, _ = make_engine()
     req = basic_request(roles=[], action="tes.submit")  # no researcher role
 
@@ -54,6 +68,9 @@ def test_rbac_deny_stops_evaluation():
 # ---------------------------------------------------------------------------
 
 def test_abac_deny_gpu_access():
+    """A GPU-required request from a caller without the gpu_user role is denied at the ABAC stage
+    with a reason mentioning GPU.
+    """
     engine, _ = make_engine()
     req = basic_request(
         roles=["researcher"],
@@ -68,6 +85,9 @@ def test_abac_deny_gpu_access():
 
 
 def test_abac_deny_hpc_access():
+    """An HPC-node request from a caller without the hpc_user role is denied at the ABAC stage with
+    a reason mentioning HPC.
+    """
     engine, _ = make_engine()
     req = basic_request(
         roles=["researcher"],
@@ -86,6 +106,7 @@ def test_abac_deny_hpc_access():
 # ---------------------------------------------------------------------------
 
 def test_rules_deny_protected_dataset_delete():
+    """Deleting a protected dataset is denied at the RULES stage even after RBAC and ABAC pass."""
     engine, _ = make_engine()
     req = basic_request(
         roles=["data_scientist"],
@@ -100,6 +121,9 @@ def test_rules_deny_protected_dataset_delete():
 
 
 def test_rules_deny_model_registry_delete():
+    """Deleting from the model registry is denied at the RULES stage even for an admin who passes
+    RBAC and ABAC.
+    """
     engine, _ = make_engine()
     req = basic_request(
         roles=["admin"],
@@ -118,6 +142,9 @@ def test_rules_deny_model_registry_delete():
 # ---------------------------------------------------------------------------
 
 def test_all_checks_pass_returns_allowed():
+    """A request that passes every stage is allowed with policy_source ALL_PASSED and reason "access
+    granted".
+    """
     engine, _ = make_engine()
     req = basic_request()
 
@@ -133,6 +160,9 @@ def test_all_checks_pass_returns_allowed():
 # ---------------------------------------------------------------------------
 
 def test_evaluate_returns_cached_decision():
+    """evaluate() returns a decision reconstructed from a cached, previously stored decision
+    payload, reporting policy_source CACHE and never calling setex.
+    """
     mock_redis = MagicMock()
     # A real cached entry is always a previous decision.dict(), which always
     # includes policy_source (RBAC/ABAC/RULES/ALL_PASSED) -- a payload
@@ -163,12 +193,15 @@ class _FakeRedis:
     used to, before it was corrected above)."""
 
     def __init__(self):
+        """Start with an empty in-memory store."""
         self.store: dict = {}
 
     def get(self, key):
+        """Return the stored value for a key, or None."""
         return self.store.get(key)
 
     def setex(self, key, ttl, value):
+        """Store a value under a key with a TTL."""
         self.store[key] = value
 
 
@@ -196,6 +229,7 @@ def test_evaluate_cache_hit_after_miss_does_not_raise():
 
 
 def test_evaluate_cache_miss_computes_and_stores():
+    """On a cache miss, evaluate() computes an allowed decision and stores it once via setex."""
     mock_redis = MagicMock()
     mock_redis.get.return_value = None
     engine, _ = make_engine(mock_redis)
@@ -208,6 +242,7 @@ def test_evaluate_cache_miss_computes_and_stores():
 
 
 def test_evaluate_cache_miss_rbac_deny_stores_denial():
+    """On a cache miss, evaluate() computes and stores a denied RBAC decision once via setex."""
     mock_redis = MagicMock()
     mock_redis.get.return_value = None
     engine, _ = make_engine(mock_redis)
@@ -224,6 +259,7 @@ def test_evaluate_cache_miss_rbac_deny_stores_denial():
 # ---------------------------------------------------------------------------
 
 def test_abac_gpu_with_gpu_user_role_passes():
+    """A GPU-required request from a caller with the gpu_user role passes the ABAC stage."""
     engine, _ = make_engine()
     req = basic_request(
         roles=["researcher", "gpu_user"],
@@ -234,6 +270,7 @@ def test_abac_gpu_with_gpu_user_role_passes():
 
 
 def test_abac_hpc_with_hpc_user_role_passes():
+    """An HPC-node request from a caller with the hpc_user role passes the ABAC stage."""
     engine, _ = make_engine()
     req = basic_request(
         roles=["researcher", "hpc_user"],

tests/test_engine_permission_tenancy.py

@@ -1,3 +1,12 @@
+"""Unit tests for PolicyEngine's permission and tenancy checks: a required
+permission's absence denies at the PERMISSION stage (unless the caller is admin
+or held no permissions at all, the legacy role-only shape), a request whose
+resource organization differs from the caller's is denied at the TENANCY stage,
+and PolicyCache.build_key's cache key varies with organization id and
+permissions.
+
+Developer: Manish Kumar <manish@omnibioai.org>
+"""
 import json
 from unittest.mock import MagicMock, patch
 
@@ -7,6 +16,7 @@
 
 
 def make_engine():
+    """Build a PolicyEngine with its cache backed by a mock Redis client that always misses."""
     mock_redis = MagicMock()
     mock_redis.get.return_value = None
     with patch("app.services.cache.redis") as mock_redis_module:
@@ -17,6 +27,7 @@ def make_engine():
 
 
 def basic_request(**kwargs):
+    """Build a PolicyRequest with sensible defaults, overridable by keyword."""
     defaults = {
         "user_id": "u1",
         "email": "u1@test.com",
@@ -36,6 +47,9 @@ def basic_request(**kwargs):
 # ---------------------------------------------------------------------------
 
 def test_permission_deny_stops_before_abac():
+    """A request lacking the required workflow.execute permission is denied at the PERMISSION stage
+    with a reason naming it.
+    """
     engine, _ = make_engine()
     req = basic_request(
         roles=["researcher"],  # passes RBAC
@@ -52,6 +66,7 @@ def test_permission_deny_stops_before_abac():
 
 
 def test_permission_allow_falls_through_to_all_passed():
+    """A request holding the required workflow.execute permission passes through to ALL_PASSED."""
     engine, _ = make_engine()
     req = basic_request(
         roles=["researcher"],
@@ -87,6 +102,9 @@ def test_permission_deny_for_real_gateway_action_shape_workflow_execute():
 
 
 def test_permission_allow_for_real_gateway_action_shape_model_use():
+    """A request for the real model.use action against the model registry, holding no roles but the
+    model.use permission, is allowed.
+    """
     engine, _ = make_engine()
     req = basic_request(
         roles=[], permissions=["model.use"], action="model.use", resource="model-registry",
@@ -115,6 +133,9 @@ def test_permission_check_is_noop_for_legacy_role_only_traffic():
 # ---------------------------------------------------------------------------
 
 def test_tenancy_deny_cross_org_access():
+    """A request whose resource belongs to a different organization than the caller is denied at the
+    TENANCY stage with reason "cross-tenant access denied".
+    """
     engine, _ = make_engine()
     req = basic_request(
         roles=["researcher"],
@@ -132,6 +153,8 @@ def test_tenancy_deny_cross_org_access():
 
 
 def test_tenancy_allow_same_org():
+    """A request whose resource belongs to the caller's own organization passes the TENANCY stage.
+    """
     engine, _ = make_engine()
     req = basic_request(
         roles=["researcher"],
@@ -172,6 +195,9 @@ def test_admin_bypasses_permission_but_not_tenancy():
 # ---------------------------------------------------------------------------
 
 def test_cache_key_differs_by_org_id():
+    """build_key produces different cache keys for the same request under different organization
+    ids.
+    """
     _, mock_redis = make_engine()
     cache = PolicyCache(redis_url="redis://localhost")
     key_org1 = cache.build_key("u1", "tes.submit", "job", {}, org_id="org-1", permissions=[])
@@ -180,6 +206,8 @@ def test_cache_key_differs_by_org_id():
 
 
 def test_cache_key_differs_by_permissions():
+    """build_key produces different cache keys for the same request with different permission sets.
+    """
     cache = PolicyCache(redis_url="redis://localhost")
     key_a = cache.build_key("u1", "tes.submit", "job", {}, org_id=None, permissions=["workflow.execute"])
     key_b = cache.build_key("u1", "tes.submit", "job", {}, org_id=None, permissions=[])