From 90f8fa87e80bcb9c087aeaf1deb72ddc89e9bbc7 Mon Sep 17 00:00:00 2001 From: Marcus Cruz Date: Mon, 13 Jul 2026 16:56:30 -0300 Subject: [PATCH 1/4] SEP-1432: Gate apps on cross-app requires_apps dependencies Add a requires_apps primitive to BaseApp so an app's effective-enabled state ANDs its own AppState with every app it depends on. Resolution is centralised in AppRegistry.resolve_effective_enabled, the single source of truth shared by the mount gate, sidebar filter, and GET /api/apps. Dependencies are validated at registry-build time (unknown key, self-dep, cycle all fail fast). atw now declares requires_apps=(snippets,), so it hides and returns 503 when snippets is disabled. --- app/sep/api/router.py | 2 +- app/sep/api/routes/apps.py | 20 +- app/sep/apps/atw/app.py | 1 + app/sep/apps/framework/base.py | 8 + app/sep/apps/framework/registry.py | 124 ++++++++++- app/sep/deps.py | 39 ++-- app/sep/main.py | 2 +- changelog.d/SEP-1432.added.md | 1 + frontend/packages/api/specs/sep.json | 2 +- tests/app/sep/api/routes/test_apps.py | 28 +++ .../sep/apps/framework/test_conformance.py | 11 + tests/app/sep/apps/framework/test_registry.py | 207 +++++++++++++++++- tests/app/sep/test_deps.py | 132 ++++++++++- tests/app/sep/test_main.py | 32 ++- 14 files changed, 572 insertions(+), 37 deletions(-) create mode 100644 changelog.d/SEP-1432.added.md diff --git a/app/sep/api/router.py b/app/sep/api/router.py index 6936e3e9b3..3eb90e3c12 100644 --- a/app/sep/api/router.py +++ b/app/sep/api/router.py @@ -79,7 +79,7 @@ def build_apps_router(registry: AppRegistry) -> APIRouter: plugin_deps = ( [] if app.state_key in PROTECTED_APP_KEYS - else [Depends(require_app_enabled(app.state_key))] + else [Depends(require_app_enabled(app.key))] ) apps_router.include_router( app.api_router, diff --git a/app/sep/api/routes/apps.py b/app/sep/api/routes/apps.py index 45671c1938..5172d976a6 100644 --- a/app/sep/api/routes/apps.py +++ b/app/sep/api/routes/apps.py @@ -30,8 +30,7 @@ from app.sep.apps.framework.registry import get_app_registry from app.sep.apps.nav_icons import NavIcon from app.sep.crud import AppStateManager -from app.sep.deps import PROTECTED_APP_KEYS, SessionDep -from app.sep.models import AppLifecycleEnum +from app.sep.deps import SessionDep router = APIRouter(tags=["apps"]) APPS_ROUTE_PREFIX = "/apps" @@ -88,21 +87,22 @@ async def list_apps_for_navigation(session: SessionDep) -> list[AppKeyResponse]: """Return per-app state for the current user's navigation. Protected apps are always reported ``enabled=True``. Every other app reflects - the DB state of the row governing it (a missing row -> ``enabled=True``: a - configured plugin is active until explicitly disabled). A child app owns no - row, so it resolves through its parent via - :attr:`~app.sep.apps.framework.base.BaseApp.state_key`. + its *effective* state via :meth:`AppRegistry.resolve_effective_enabled`: its + own row must be ``ENABLED`` (a missing row -> ``enabled=True``: a configured + plugin is active until explicitly disabled) **and** every app it declares in + ``requires_apps`` must be effectively enabled, so the shell hides an app when + a dependency is off. A child app owns no row, so it resolves through its + parent via :attr:`~app.sep.apps.framework.base.BaseApp.state_key`. :param session: The database session. :return: The per-app navigation list. """ states = await AppStateManager.all_lifecycle_states(session) + registry = get_app_registry() return [ AppKeyResponse( app_key=app.key, - enabled=app.state_key in PROTECTED_APP_KEYS - or states.get(app.state_key, AppLifecycleEnum.ENABLED) - == AppLifecycleEnum.ENABLED, + enabled=registry.resolve_effective_enabled(app.key, states), sidebar=app.sidebar, uri_path=app.uri_path, display_name=app.display_name, @@ -112,5 +112,5 @@ async def list_apps_for_navigation(session: SessionDep) -> list[AppKeyResponse]: react_route=build_navigation_react_route(app.key, app.react_route), nav_icon=app.nav_icon, ) - for app in get_app_registry() + for app in registry ] diff --git a/app/sep/apps/atw/app.py b/app/sep/apps/atw/app.py index d9f970daad..dc4adb82e0 100644 --- a/app/sep/apps/atw/app.py +++ b/app/sep/apps/atw/app.py @@ -40,4 +40,5 @@ nav_icon=NavIcon.SUPPORT_AGENT, api_router=api_router, schema=atw_schema, + requires_apps=("snippets",), ) diff --git a/app/sep/apps/framework/base.py b/app/sep/apps/framework/base.py index ac0ea99951..209d63fc1b 100644 --- a/app/sep/apps/framework/base.py +++ b/app/sep/apps/framework/base.py @@ -67,6 +67,13 @@ class BaseApp(BaseModel): registry appends each right after this app and stamps its ``enabled`` from this app's, so a child is mounted and snapshotted exactly when its parent is. Each child must set ``parent_key`` to this app's key. + :param requires_apps: App **keys** (not display names) this app functionally + depends on. An app's *effective* enabled state ANDs its own ``AppState`` + with the effective state of every app named here, so a disabled dependency + hides and gates this app too. Depending on a protected app (one that can + never be disabled) is a harmless no-op. Keys are validated at + registry-build time: an unknown key, a self-dependency, or a cycle fails + fast. See :meth:`AppRegistry.resolve_effective_enabled`. """ model_config = ConfigDict(arbitrary_types_allowed=True, populate_by_name=True) @@ -88,6 +95,7 @@ class BaseApp(BaseModel): app_schema: AppSchema | None = Field(default=None, alias="schema") parent_key: str | None = None child_apps: tuple["BaseApp", ...] = () + requires_apps: tuple[str, ...] = () @model_validator(mode="before") @classmethod diff --git a/app/sep/apps/framework/registry.py b/app/sep/apps/framework/registry.py index b935d50518..20334ed3e8 100644 --- a/app/sep/apps/framework/registry.py +++ b/app/sep/apps/framework/registry.py @@ -31,7 +31,7 @@ imports through plugin ``__init__`` modules. """ -from collections.abc import Iterable, Iterator +from collections.abc import Iterable, Iterator, Mapping from functools import lru_cache from importlib import import_module @@ -46,6 +46,7 @@ from app.sep.config import App, sep_settings from app.sep.crud import AppStateManager from app.sep.deps import PROTECTED_APP_KEYS +from app.sep.models import AppLifecycleEnum class AppRegistry: @@ -56,11 +57,130 @@ class AppRegistry: :param apps: The mounted apps, in activation order. :type apps: list[BaseApp] + :raises ValueError: When two apps share a key, or a ``requires_apps`` entry + names an unknown app, itself, or forms a dependency cycle. """ def __init__(self, apps: list[BaseApp]) -> None: self._apps = apps - self._by_key = {app.key: app for app in apps} + # Explicit loop, not a comprehension: a duplicate key must raise, not + # silently overwrite, or a ``requires_apps`` reference turns ambiguous. + self._by_key: dict[str, BaseApp] = {} + for app in apps: + if app.key in self._by_key: + raise ValueError( + f"Duplicate app key {app.key!r}: two apps cannot share a" + " key, or a requires_apps reference would be ambiguous.", + ) + self._by_key[app.key] = app + self._validate_dependencies() + + def _validate_dependencies(self) -> None: + """Reject self-dependencies, dangling deps, and cycles at build time. + + :raises ValueError: When a ``requires_apps`` entry names the app itself, + names an unregistered key, or participates in a dependency cycle. + """ + for app in self._apps: + for dep_key in app.requires_apps: + if dep_key == app.key: + raise ValueError( + f"App {app.key!r} cannot depend on itself.", + ) + if dep_key not in self._by_key: + raise ValueError( + f"App {app.key!r} requires unknown app key {dep_key!r}.", + ) + self._assert_acyclic() + + def _assert_acyclic(self) -> None: + """Raise when the ``requires_apps`` graph contains a cycle. + + :raises ValueError: When a dependency cycle is detected. + """ + white, gray, black = 0, 1, 2 + color = {app.key: white for app in self._apps} + + def visit(key: str, path: list[str]) -> None: + color[key] = gray + for dep_key in self._by_key[key].requires_apps: + if color[dep_key] == gray: + chain = " -> ".join([*path, key, dep_key]) + raise ValueError(f"App dependency cycle detected: {chain}.") + if color[dep_key] == white: + visit(dep_key, [*path, key]) + color[key] = black + + for app in self._apps: + if color[app.key] == white: + visit(app.key, []) + + def resolve_effective_enabled( + self, + key: str, + states: Mapping[str, AppLifecycleEnum], + ) -> bool: + """Return whether ``key``'s app is effectively enabled. + + An app is effective-enabled when its own ``AppState`` is ``ENABLED`` + **and** every app in its ``requires_apps`` is itself effective-enabled + (resolved transitively). This is the single resolver the mount gate, the + sidebar filter, and the ``GET /api/apps`` projection all share, so they + cannot drift. A protected app (or dependency) is always treated as + enabled; a missing ``AppState`` row defaults to ``ENABLED``. + + :param key: The registry key of the app to resolve. + :param states: A ``{state_key: lifecycle}`` map (e.g. from + :meth:`AppStateManager.all_lifecycle_states`). + :return: ``True`` when the app and every dependency are enabled. + """ + app = self._by_key.get(key) + if app is None: + return False + return self._effective_enabled(app, states, frozenset()) + + def _effective_enabled( + self, + app: BaseApp, + states: Mapping[str, AppLifecycleEnum], + stack: frozenset[str], + ) -> bool: + """Resolve ``app``'s effective-enabled state along a DFS path. + + :param app: The app being resolved. + :param states: The ``{state_key: lifecycle}`` map. + :param stack: The keys on the current recursion path (cycle guard). + :return: ``True`` when the app and every dependency are enabled. + """ + if app.state_key in PROTECTED_APP_KEYS: + # Can never be disabled, so return True without recursing into its + # own ``requires_apps``. + return True + if not self._own_enabled(app, states): + return False + if app.key in stack: + # Defensive: the build rejects cycles, so this is unreachable. + return True + stack = stack | {app.key} + for dep_key in app.requires_apps: + dep = self._by_key.get(dep_key) + if dep is None or not self._effective_enabled(dep, states, stack): + return False + return True + + @staticmethod + def _own_enabled(app: BaseApp, states: Mapping[str, AppLifecycleEnum]) -> bool: + """Return whether ``app``'s own ``AppState`` is enabled (ignoring deps). + + :param app: The app whose own state is inspected. + :param states: The ``{state_key: lifecycle}`` map. + :return: ``True`` when protected or the row is ``ENABLED`` (or absent). + """ + return ( + app.state_key in PROTECTED_APP_KEYS + or states.get(app.state_key, AppLifecycleEnum.ENABLED) + == AppLifecycleEnum.ENABLED + ) def __iter__(self) -> Iterator[BaseApp]: return iter(self._apps) diff --git a/app/sep/deps.py b/app/sep/deps.py index e45a6bd73d..b3e0b33619 100644 --- a/app/sep/deps.py +++ b/app/sep/deps.py @@ -76,7 +76,7 @@ CSRF_FORM_FIELD, request_has_bearer_authorization, ) -from app.sep.models import AppLifecycleEnum, SyncInventoryEntityTypeEnum +from app.sep.models import SyncInventoryEntityTypeEnum from app.tasks.config import tasks_settings from app.tasks.models import ( Task, @@ -483,7 +483,12 @@ def require_app_enabled(app_key: str) -> Callable[[AsyncSession], Awaitable[None Used as ``dependencies=[Depends(require_app_enabled())]`` on each non-protected app's router at mount time. Raises :class:`app.core.exceptions.HTTPServiceUnavailableException` (HTTP 503) when - the app is disabled in :class:`app.sep.models.AppState`. + the app is not *effectively* enabled -- that is, when its own + :class:`app.sep.models.AppState` is disabled **or** any app it declares in + ``requires_apps`` is disabled. Resolution is delegated to + :meth:`AppRegistry.resolve_effective_enabled`, the single source of truth + shared with the sidebar filter and the ``GET /api/apps`` projection, so the + gate is passed the app's ``key`` (not ``state_key``) to resolve dependencies. The factory closure-captures ``app_key`` at router-mount time; the returned coroutine is invoked per request and queries the DB via the standard @@ -503,8 +508,12 @@ def require_app_enabled(app_key: str) -> Callable[[AsyncSession], Awaitable[None """ async def _gate(session: SessionDep) -> None: + # Deferred: the framework package __init__ imports back into this module, + # so a top-level import here would cycle. + from app.sep.apps.framework.registry import get_app_registry + try: - enabled = await AppStateManager.is_enabled(session, app_key) + states = await AppStateManager.all_lifecycle_states(session) except SQLAlchemyError: logger.warning( "Could not read app state for '%s'; allowing the request.", @@ -512,7 +521,7 @@ async def _gate(session: SessionDep) -> None: exc_info=True, ) return - if not enabled: + if not get_app_registry().resolve_effective_enabled(app_key, states): raise HTTPServiceUnavailableException( detail=f"App '{app_key}' is currently disabled.", ) @@ -575,13 +584,14 @@ async def get_default_context( ) -> dict[str, Any]: """Return the default context for templates. - The sidebar ``plugins`` list is filtered by runtime app state: protected - apps always pass through; every other app is shown unless the - :class:`app.sep.models.AppState` row governing it has - ``lifecycle_state != ENABLED`` (a missing row is treated as enabled). A child - app owns no row, so it resolves through its parent via - :attr:`~app.sep.apps.framework.base.BaseApp.state_key`. This is the single - source of truth that drives sidebar visibility. + The sidebar ``plugins`` list is filtered by *effective* app state via + :meth:`AppRegistry.resolve_effective_enabled`: protected apps always pass + through; every other app is shown only when its own + :class:`app.sep.models.AppState` row is ``ENABLED`` (a missing row is treated + as enabled) **and** every app it declares in ``requires_apps`` is itself + effectively enabled. A child app owns no row, so it resolves through its + parent via :attr:`~app.sep.apps.framework.base.BaseApp.state_key`. This + shares the one resolver used by the mount gate and the JSON app listing. :param request: The HTTP request object. :param user: The authenticated user. @@ -603,12 +613,9 @@ async def get_default_context( # so a top-level import here would cycle. from app.sep.apps.framework.registry import get_app_registry + registry = get_app_registry() plugins = [ - app - for app in get_app_registry() - if app.state_key in PROTECTED_APP_KEYS - or states.get(app.state_key, AppLifecycleEnum.ENABLED) - == AppLifecycleEnum.ENABLED + app for app in registry if registry.resolve_effective_enabled(app.key, states) ] return { "user": user, diff --git a/app/sep/main.py b/app/sep/main.py index c482fe9bf6..bd9108001f 100644 --- a/app/sep/main.py +++ b/app/sep/main.py @@ -402,7 +402,7 @@ async def sep_lifespan(app: FastAPI) -> AsyncGenerator[None, None]: plugin_deps = ( [] if app.state_key in PROTECTED_APP_KEYS - else [Depends(require_app_enabled(app.state_key))] + else [Depends(require_app_enabled(app.key))] ) sep_app.include_router( app.jinja_router, prefix=app.uri_path, dependencies=plugin_deps diff --git a/changelog.d/SEP-1432.added.md b/changelog.d/SEP-1432.added.md new file mode 100644 index 0000000000..7347bc2e9b --- /dev/null +++ b/changelog.d/SEP-1432.added.md @@ -0,0 +1 @@ +atw is hidden and returns 503 when the snippets app it depends on is disabled, via a new requires_apps cross-app dependency primitive diff --git a/frontend/packages/api/specs/sep.json b/frontend/packages/api/specs/sep.json index a9fa5966d6..bc844aaa5b 100644 --- a/frontend/packages/api/specs/sep.json +++ b/frontend/packages/api/specs/sep.json @@ -11675,7 +11675,7 @@ }, "/api/apps/": { "get": { - "description": "Return per-app state for the current user's navigation.\n\nProtected apps are always reported ``enabled=True``. Every other app reflects\nthe DB state of the row governing it (a missing row -> ``enabled=True``: a\nconfigured plugin is active until explicitly disabled). A child app owns no\nrow, so it resolves through its parent via\n:attr:`~app.sep.apps.framework.base.BaseApp.state_key`.\n\n:param session: The database session.\n:return: The per-app navigation list.", + "description": "Return per-app state for the current user's navigation.\n\nProtected apps are always reported ``enabled=True``. Every other app reflects\nits *effective* state via :meth:`AppRegistry.resolve_effective_enabled`: its\nown row must be ``ENABLED`` (a missing row -> ``enabled=True``: a configured\nplugin is active until explicitly disabled) **and** every app it declares in\n``requires_apps`` must be effectively enabled, so the shell hides an app when\na dependency is off. A child app owns no row, so it resolves through its\nparent via :attr:`~app.sep.apps.framework.base.BaseApp.state_key`.\n\n:param session: The database session.\n:return: The per-app navigation list.", "operationId": "apps_list_apps_for_navigation_api_apps__get", "responses": { "200": { diff --git a/tests/app/sep/api/routes/test_apps.py b/tests/app/sep/api/routes/test_apps.py index c2bd5388e4..7ead195557 100644 --- a/tests/app/sep/api/routes/test_apps.py +++ b/tests/app/sep/api/routes/test_apps.py @@ -226,6 +226,34 @@ async def test_non_enabled_plugin_reported_disabled( assert snippets["enabled"] is False assert "lifecycle_state" not in snippets + async def test_atw_reported_disabled_when_snippets_disabled( + self, api_user_client: TestClient, override_session: AsyncSession + ) -> None: + """Atw reports ``enabled=False`` when the ``snippets`` app it requires is disabled. + + This pins the cross-app dependency on the nav surface: atw owns an + ``ENABLED`` row (or none) yet is projected disabled because a required + app is off, so the shell hides it. + """ + override_session.add( + AppState(app_key="snippets", lifecycle_state=AppLifecycleEnum.DISABLED) + ) + await override_session.commit() + + response = api_user_client.get("/api/apps/") + entries = {e["app_key"]: e for e in response.json()} + assert entries["snippets"]["enabled"] is False + assert entries["atw"]["enabled"] is False + + async def test_atw_reported_enabled_when_snippets_enabled( + self, api_user_client: TestClient + ) -> None: + """Atw reports ``enabled=True`` when snippets is enabled (no regression).""" + response = api_user_client.get("/api/apps/") + entries = {e["app_key"]: e for e in response.json()} + assert entries["snippets"]["enabled"] is True + assert entries["atw"]["enabled"] is True + async def test_unauthenticated_returns_json_401( self, api_unauthenticated_client: TestClient ) -> None: diff --git a/tests/app/sep/apps/framework/test_conformance.py b/tests/app/sep/apps/framework/test_conformance.py index ba9ccee2a6..5cb37a3828 100644 --- a/tests/app/sep/apps/framework/test_conformance.py +++ b/tests/app/sep/apps/framework/test_conformance.py @@ -654,6 +654,17 @@ def test_registry_migrated_app_structural_checks(registry_app): assert check_schema_derivation_succeeds(registry_app) == [] +@pytest.mark.parametrize("registry_app", _APPS, ids=lambda app: app.key) +def test_registry_app_requires_apps_resolve(registry_app): + """Assert every app's ``requires_apps`` is a tuple of registered app keys.""" + assert isinstance(registry_app.requires_apps, tuple) + for dep_key in registry_app.requires_apps: + assert isinstance(dep_key, str) + assert _REGISTRY.get(dep_key) is not None, ( + f"{registry_app.key} requires unregistered app {dep_key!r}" + ) + + def test_registry_has_no_route_collisions(): """Assert no two registry routes share a ``(path, method)`` signature.""" assert check_route_collisions(_REGISTRY) == [] diff --git a/tests/app/sep/apps/framework/test_registry.py b/tests/app/sep/apps/framework/test_registry.py index aa2ec5c075..32fa1dd28a 100644 --- a/tests/app/sep/apps/framework/test_registry.py +++ b/tests/app/sep/apps/framework/test_registry.py @@ -561,11 +561,39 @@ def test_skips_plugins_without_declaration(self) -> None: entries = collect_app_owned_settings_classes([App(module_name="checksums")]) assert entries == [] - def test_rejects_duplicate_setting_class(self) -> None: - """Fail when the same settings class is declared twice.""" + def test_rejects_duplicate_setting_class(self, mocker: MockerFixture) -> None: + """Fail when the same settings class is declared by two distinct apps. + + Two entries share ``ALERT_SETTINGS`` across two distinctly-keyed apps + (``alerts`` and ``checksums``), so the duplicate-setting-class guard -- + not the duplicate-app-key guard -- is what trips. + """ + dup_entry = AppOwnedClassEntry( + setting_class=SettingClassEnum.ALERT_SETTINGS, + settings_cls=AlertSettings, + proxy=alert_settings, + app_key="checksums", + ) + fake_module = mocker.MagicMock() + fake_module.APP_OWNED_SETTINGS_CLASSES = [dup_entry] + real_checksums = importlib.import_module("app.sep.apps.checksums") + import_calls = {"count": 0} + + def import_side_effect(name: str): + if name == "app.sep.apps.checksums": + import_calls["count"] += 1 + if import_calls["count"] == 1: + return real_checksums + return fake_module + return importlib.import_module(name) + + mocker.patch( + "app.sep.apps.framework.registry.import_module", + side_effect=import_side_effect, + ) with pytest.raises(ValueError, match="more than one app-owned"): collect_app_owned_settings_classes( - [App(module_name="alerts"), App(module_name="alerts")], + [App(module_name="alerts"), App(module_name="checksums")], ) def test_rejects_unknown_app_key(self, mocker: MockerFixture) -> None: @@ -727,6 +755,179 @@ def _parent_plugin(*, enabled: bool = True) -> App: return App.model_construct(name="Parent", module_name="parent_app", enabled=enabled) +def _dep_app(key: str, *, requires_apps: tuple[str, ...] = ()) -> BaseApp: + """Build a minimal top-level ``BaseApp`` carrying ``requires_apps``.""" + return BaseApp( + key=key, + name=key, + display_name=key, + uri_path=f"/{key}", + requires_apps=requires_apps, + ) + + +class TestRequiresAppsValidation: + """Cover build-time validation of ``requires_apps`` and app keys.""" + + def test_duplicate_app_key_raises(self) -> None: + """Reject two apps sharing a key rather than silently collapsing them.""" + with pytest.raises(ValueError, match="duplicate|Duplicate"): + AppRegistry([_dep_app("dup"), _dep_app("dup")]) + + def test_dangling_dependency_raises(self) -> None: + """Reject a ``requires_apps`` key that resolves to no registered app.""" + with pytest.raises(ValueError, match="ghost"): + AppRegistry([_dep_app("a", requires_apps=("ghost",))]) + + def test_self_dependency_raises(self) -> None: + """Reject an app that depends on itself.""" + with pytest.raises(ValueError, match="itself|self"): + AppRegistry([_dep_app("a", requires_apps=("a",))]) + + def test_dependency_cycle_raises(self) -> None: + """Reject a dependency cycle across apps.""" + with pytest.raises(ValueError, match="cycle|Cycle"): + AppRegistry( + [ + _dep_app("a", requires_apps=("b",)), + _dep_app("b", requires_apps=("a",)), + ] + ) + + def test_valid_dependency_graph_builds(self) -> None: + """Build a well-formed dependency graph without error.""" + registry = AppRegistry([_dep_app("a", requires_apps=("b",)), _dep_app("b")]) + assert registry.keys() == ["a", "b"] + + +class TestEffectiveEnabled: + """Cover the centralized effective-enabled resolver.""" + + def test_enabled_when_own_and_dep_enabled(self) -> None: + """Resolve effective-enabled when an app and its dependency are enabled.""" + registry = AppRegistry([_dep_app("a", requires_apps=("b",)), _dep_app("b")]) + assert registry.resolve_effective_enabled("a", {}) is True + + def test_disabled_when_own_disabled(self) -> None: + """Gate an app when its own state is disabled.""" + registry = AppRegistry([_dep_app("a", requires_apps=("b",)), _dep_app("b")]) + states = {"a": AppLifecycleEnum.DISABLED} + assert registry.resolve_effective_enabled("a", states) is False + + def test_disabled_when_dependency_disabled(self) -> None: + """Gate an app when a dependency is disabled.""" + registry = AppRegistry([_dep_app("a", requires_apps=("b",)), _dep_app("b")]) + states = {"b": AppLifecycleEnum.DISABLED} + assert registry.resolve_effective_enabled("a", states) is False + + @pytest.mark.parametrize( + "state", + [ + AppLifecycleEnum.DISABLING, + AppLifecycleEnum.ENABLING, + ], + ) + def test_disabled_when_dependency_not_fully_enabled( + self, state: AppLifecycleEnum + ) -> None: + """Gate the dependent app for any dependency state other than ENABLED.""" + registry = AppRegistry([_dep_app("a", requires_apps=("b",)), _dep_app("b")]) + assert registry.resolve_effective_enabled("a", {"b": state}) is False + + def test_missing_row_treated_as_enabled(self) -> None: + """Treat a missing ``AppState`` row as ENABLED for every node.""" + registry = AppRegistry([_dep_app("a", requires_apps=("b",)), _dep_app("b")]) + assert registry.resolve_effective_enabled("a", {}) is True + + def test_unknown_key_resolves_disabled(self) -> None: + """Resolve an unregistered key to ``False`` rather than raising.""" + registry = AppRegistry([_dep_app("a")]) + assert registry.resolve_effective_enabled("ghost", {}) is False + + @pytest.mark.parametrize( + "state", + [ + AppLifecycleEnum.DISABLING, + AppLifecycleEnum.ENABLING, + ], + ) + def test_disabled_when_own_state_not_fully_enabled( + self, state: AppLifecycleEnum + ) -> None: + """Gate an app for any of its own states other than ENABLED.""" + registry = AppRegistry([_dep_app("a", requires_apps=("b",)), _dep_app("b")]) + assert registry.resolve_effective_enabled("a", {"a": state}) is False + + def test_disabled_when_one_of_several_deps_disabled(self) -> None: + """Gate an app when any one of its multiple dependencies is disabled.""" + registry = AppRegistry( + [ + _dep_app("a", requires_apps=("b", "c")), + _dep_app("b"), + _dep_app("c"), + ] + ) + assert registry.resolve_effective_enabled("a", {}) is True + states = {"c": AppLifecycleEnum.DISABLED} + assert registry.resolve_effective_enabled("a", states) is False + + def test_child_app_gated_through_parent_state(self) -> None: + """Gate a child app via its parent's ``AppState`` (child owns no row). + + A child resolves its own enabled state through ``state_key`` (its + ``parent_key``), so disabling the parent gates the child. + """ + registry = AppRegistry( + [ + _dep_app("parent_app"), + _child_app("parent_app/restore", parent_key="parent_app"), + ] + ) + assert registry.resolve_effective_enabled("parent_app/restore", {}) is True + states = {"parent_app": AppLifecycleEnum.DISABLED} + assert registry.resolve_effective_enabled("parent_app/restore", states) is False + + def test_protected_dependency_is_always_enabled(self) -> None: + """Never gate on a protected dependency, even when its row says disabled.""" + registry = AppRegistry( + [_dep_app("a", requires_apps=("inventory",)), _dep_app("inventory")] + ) + states = {"inventory": AppLifecycleEnum.DISABLED} + assert registry.resolve_effective_enabled("a", states) is True + + def test_protected_app_ignores_its_own_disabled_dependency(self) -> None: + """Keep a dependent enabled when a protected middle node has a disabled dep. + + ``inventory`` is protected, so ``consumer -> inventory -> snippets`` must + not gate ``consumer`` when ``snippets`` is disabled: a protected node is + unconditionally enabled and its own ``requires_apps`` are never walked. + """ + registry = AppRegistry( + [ + _dep_app("consumer", requires_apps=("inventory",)), + _dep_app("inventory", requires_apps=("snippets",)), + _dep_app("snippets"), + ] + ) + states = {"snippets": AppLifecycleEnum.DISABLED} + assert registry.resolve_effective_enabled("consumer", states) is True + + def test_transitive_dependency_gates(self) -> None: + """Gate the whole chain on a disabled transitive dependency.""" + registry = AppRegistry( + [ + _dep_app("a", requires_apps=("b",)), + _dep_app("b", requires_apps=("c",)), + _dep_app("c"), + ] + ) + assert ( + registry.resolve_effective_enabled("a", {"c": AppLifecycleEnum.DISABLED}) + is False + ) + assert registry.resolve_effective_enabled("a", {}) is True + + class TestChildApps: """Cover ``child_apps`` structural registration in ``build_app_registry``.""" diff --git a/tests/app/sep/test_deps.py b/tests/app/sep/test_deps.py index af8c50a611..e68e67e028 100644 --- a/tests/app/sep/test_deps.py +++ b/tests/app/sep/test_deps.py @@ -41,7 +41,8 @@ ) from app.core.pagination import MAX_PAGINATION_LIMIT from app.inventory.models import ServiceTypeEnum -from app.sep.apps.framework.registry import build_app_registry +from app.sep.apps.framework.base import BaseApp +from app.sep.apps.framework.registry import AppRegistry, build_app_registry from app.sep.clients.pmm import PMMRemoteAPI from app.sep.config import App, sep_settings from app.sep.crud import AppStateManager @@ -1646,6 +1647,62 @@ def test_inventory_is_protected(self) -> None: """``inventory`` is the protected key the mount loops must skip.""" assert "inventory" in PROTECTED_APP_KEYS + @staticmethod + def _dependent_registry() -> AppRegistry: + """Build a two-app registry where ``dependent`` requires ``dep``.""" + return AppRegistry( + [ + BaseApp( + key="dependent", + name="dependent", + display_name="dependent", + uri_path="/dependent", + requires_apps=("dep",), + ), + BaseApp( + key="dep", + name="dep", + display_name="dep", + uri_path="/dep", + ), + ] + ) + + @pytest.mark.asyncio + async def test_gate_503_when_dependency_disabled(self, session) -> None: + """The gate 503s on the dependent's key when a required app is disabled.""" + session.add(AppState(app_key="dep", lifecycle_state=AppLifecycleEnum.DISABLED)) + await session.commit() + with patch( + "app.sep.apps.framework.registry.get_app_registry", + return_value=self._dependent_registry(), + ): + gate = require_app_enabled("dependent") + with pytest.raises(HTTPServiceUnavailableException) as exc_info: + await gate(session) + assert "dependent" in exc_info.value.detail + + @pytest.mark.asyncio + async def test_gate_passes_when_dependency_enabled(self, session) -> None: + """The gate passes when both the app and its dependency are enabled.""" + with patch( + "app.sep.apps.framework.registry.get_app_registry", + return_value=self._dependent_registry(), + ): + gate = require_app_enabled("dependent") + assert await gate(session) is None + + @pytest.mark.asyncio + async def test_gate_fail_open_on_db_error(self, session) -> None: + """A DB read failure degrades to allowing the request (fail-open).""" + with patch.object( + AppStateManager, + "all_lifecycle_states", + side_effect=SQLAlchemyError("db down"), + ): + gate = require_app_enabled("snippets") + assert await gate(session) is None + class TestGetToggleableAppKey: """Test app-key resolver used by the app-state toggle endpoint.""" @@ -1775,6 +1832,79 @@ async def test_missing_row_includes_plugin( keys = {p.key for p in context["plugins"]} assert keys == {"inventory", "snippets", "checksums"} + @staticmethod + def _dependent_registry() -> AppRegistry: + """Build a registry where ``dependent`` requires ``snippets``.""" + return AppRegistry( + [ + BaseApp( + key="inventory", + name="inventory", + display_name="Inventory", + uri_path="/inventory", + ), + BaseApp( + key="snippets", + name="snippets", + display_name="Snippets", + uri_path="/snippets", + ), + BaseApp( + key="dependent", + name="dependent", + display_name="Dependent", + uri_path="/dependent", + requires_apps=("snippets",), + ), + ] + ) + + @pytest.mark.asyncio + @pytest.mark.usefixtures("mock_get_username_mapping") + async def test_app_hidden_when_dependency_disabled( + self, session, dummy_request, regular_user + ) -> None: + """An app is dropped from the sidebar when a required app is disabled.""" + session.add( + AppState(app_key="snippets", lifecycle_state=AppLifecycleEnum.DISABLED) + ) + await session.commit() + with ( + patch( + "app.sep.apps.framework.registry.get_app_registry", + return_value=self._dependent_registry(), + ), + patch("app.sep.deps.settings"), + ): + context = await get_default_context( + dummy_request, regular_user, None, session + ) + + keys = {p.key for p in context["plugins"]} + assert "dependent" not in keys + assert "snippets" not in keys + assert "inventory" in keys + + @pytest.mark.asyncio + @pytest.mark.usefixtures("mock_get_username_mapping") + async def test_app_shown_when_dependency_enabled( + self, session, dummy_request, regular_user + ) -> None: + """An app stays in the sidebar when its dependency is enabled (missing row).""" + with ( + patch( + "app.sep.apps.framework.registry.get_app_registry", + return_value=self._dependent_registry(), + ), + patch("app.sep.deps.settings"), + ): + context = await get_default_context( + dummy_request, regular_user, None, session + ) + + keys = {p.key for p in context["plugins"]} + assert {"inventory", "snippets", "dependent"} <= keys + @pytest.mark.asyncio @pytest.mark.usefixtures("mock_get_username_mapping") async def test_db_failure_degrades_to_showing_all_apps( diff --git a/tests/app/sep/test_main.py b/tests/app/sep/test_main.py index 2edc47b27e..cac8fab5f0 100644 --- a/tests/app/sep/test_main.py +++ b/tests/app/sep/test_main.py @@ -103,8 +103,8 @@ def dummy_context() -> dict[str, str]: def dummy_access_token() -> str: """Override get_access_token_from_cookie and return dummy access token.""" fake_access_token = "access-token" - sep_app.dependency_overrides[get_access_token_from_cookie] = ( - lambda: fake_access_token + sep_app.dependency_overrides[get_access_token_from_cookie] = lambda: ( + fake_access_token ) yield fake_access_token sep_app.dependency_overrides = {} @@ -887,6 +887,34 @@ async def test_inventory_ui_route_never_503s( assert response.status_code != status.HTTP_503_SERVICE_UNAVAILABLE + @pytest.mark.asyncio + async def test_atw_json_route_503s_when_snippets_disabled( + self, guarded_client: TestClient, session + ) -> None: + """Atw's JSON route 503s when the ``snippets`` app it requires is disabled. + + The gate reports atw's own key, so the user never sees the raw + ``App 'snippets' is currently disabled`` leak from the execute path. + """ + session.add( + AppState(app_key="snippets", lifecycle_state=AppLifecycleEnum.DISABLED) + ) + await session.commit() + + response = guarded_client.get("/api/apps/atw/") + + assert response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE + assert "atw" in response.json()["detail"] + + @pytest.mark.asyncio + async def test_atw_json_route_reachable_when_snippets_enabled( + self, guarded_client: TestClient, session + ) -> None: + """Atw's JSON route is reachable when snippets is enabled (no regression).""" + response = guarded_client.get("/api/apps/atw/") + + assert response.status_code != status.HTTP_503_SERVICE_UNAVAILABLE + def test_ui_mount_loop_guards_non_protected_plugins(self) -> None: """Every non-protected UI plugin route carries the app-state guard.""" guarded_prefixes = { From cf250d10f33dd55f615e2f45e753d3e49158ce22 Mon Sep 17 00:00:00 2001 From: Marcus Cruz Date: Mon, 13 Jul 2026 17:30:31 -0300 Subject: [PATCH 2/4] SEP-1432: Fail closed on cycle guard and memoize effective-enabled projection Address PR review: the defensive cycle-guard branch in _effective_enabled() now returns False (fail closed) instead of True, so an unexpected cycle gates the app off rather than reporting it enabled. resolve_effective_enabled() takes an optional per-invocation memo dict; the sidebar filter and GET /api/apps projections share one memo so shared dependency subtrees are walked once instead of re-walked per app. --- app/sep/api/routes/apps.py | 3 +- app/sep/apps/framework/registry.py | 39 +++++++++++++++-- app/sep/deps.py | 5 ++- tests/app/sep/apps/framework/test_registry.py | 43 +++++++++++++++++++ 4 files changed, 84 insertions(+), 6 deletions(-) diff --git a/app/sep/api/routes/apps.py b/app/sep/api/routes/apps.py index 5172d976a6..a1f93fb290 100644 --- a/app/sep/api/routes/apps.py +++ b/app/sep/api/routes/apps.py @@ -99,10 +99,11 @@ async def list_apps_for_navigation(session: SessionDep) -> list[AppKeyResponse]: """ states = await AppStateManager.all_lifecycle_states(session) registry = get_app_registry() + memo: dict[str, bool] = {} return [ AppKeyResponse( app_key=app.key, - enabled=registry.resolve_effective_enabled(app.key, states), + enabled=registry.resolve_effective_enabled(app.key, states, memo), sidebar=app.sidebar, uri_path=app.uri_path, display_name=app.display_name, diff --git a/app/sep/apps/framework/registry.py b/app/sep/apps/framework/registry.py index 20334ed3e8..e9c6652a5c 100644 --- a/app/sep/apps/framework/registry.py +++ b/app/sep/apps/framework/registry.py @@ -119,6 +119,7 @@ def resolve_effective_enabled( self, key: str, states: Mapping[str, AppLifecycleEnum], + memo: dict[str, bool] | None = None, ) -> bool: """Return whether ``key``'s app is effectively enabled. @@ -132,24 +133,53 @@ def resolve_effective_enabled( :param key: The registry key of the app to resolve. :param states: A ``{state_key: lifecycle}`` map (e.g. from :meth:`AppStateManager.all_lifecycle_states`). + :param memo: An optional ``{key: bool}`` cache shared across a + full-registry projection so shared dependency subtrees are walked + once rather than re-walked per app. Only reuse a memo within a + single ``states`` snapshot; a fresh ``states`` needs a fresh memo. :return: ``True`` when the app and every dependency are enabled. """ app = self._by_key.get(key) if app is None: return False - return self._effective_enabled(app, states, frozenset()) + return self._effective_enabled(app, states, frozenset(), memo) def _effective_enabled( self, app: BaseApp, states: Mapping[str, AppLifecycleEnum], stack: frozenset[str], + memo: dict[str, bool] | None = None, ) -> bool: """Resolve ``app``'s effective-enabled state along a DFS path. :param app: The app being resolved. :param states: The ``{state_key: lifecycle}`` map. :param stack: The keys on the current recursion path (cycle guard). + :param memo: An optional ``{key: bool}`` cache; a resolved result is + stored under ``app.key`` and reused on the next visit. + :return: ``True`` when the app and every dependency are enabled. + """ + if memo is not None and app.key in memo: + return memo[app.key] + result = self._resolve_effective(app, states, stack, memo) + if memo is not None: + memo[app.key] = result + return result + + def _resolve_effective( + self, + app: BaseApp, + states: Mapping[str, AppLifecycleEnum], + stack: frozenset[str], + memo: dict[str, bool] | None, + ) -> bool: + """Compute ``app``'s effective-enabled state (the un-memoized body). + + :param app: The app being resolved. + :param states: The ``{state_key: lifecycle}`` map. + :param stack: The keys on the current recursion path (cycle guard). + :param memo: The projection cache threaded into dependency resolution. :return: ``True`` when the app and every dependency are enabled. """ if app.state_key in PROTECTED_APP_KEYS: @@ -159,12 +189,13 @@ def _effective_enabled( if not self._own_enabled(app, states): return False if app.key in stack: - # Defensive: the build rejects cycles, so this is unreachable. - return True + # Defensive: the build rejects cycles, so this is unreachable. Fail + # closed -- an unexpected cycle gates the app off, never on. + return False stack = stack | {app.key} for dep_key in app.requires_apps: dep = self._by_key.get(dep_key) - if dep is None or not self._effective_enabled(dep, states, stack): + if dep is None or not self._effective_enabled(dep, states, stack, memo): return False return True diff --git a/app/sep/deps.py b/app/sep/deps.py index b3e0b33619..bc10e86770 100644 --- a/app/sep/deps.py +++ b/app/sep/deps.py @@ -614,8 +614,11 @@ async def get_default_context( from app.sep.apps.framework.registry import get_app_registry registry = get_app_registry() + memo: dict[str, bool] = {} plugins = [ - app for app in registry if registry.resolve_effective_enabled(app.key, states) + app + for app in registry + if registry.resolve_effective_enabled(app.key, states, memo) ] return { "user": user, diff --git a/tests/app/sep/apps/framework/test_registry.py b/tests/app/sep/apps/framework/test_registry.py index 32fa1dd28a..91fc674540 100644 --- a/tests/app/sep/apps/framework/test_registry.py +++ b/tests/app/sep/apps/framework/test_registry.py @@ -927,6 +927,49 @@ def test_transitive_dependency_gates(self) -> None: ) assert registry.resolve_effective_enabled("a", {}) is True + def test_cycle_at_resolve_time_fails_closed(self) -> None: + """Gate an app off if a dependency cycle is present at resolve time. + + The build rejects cycles, so this guards the defensive branch against + post-build mutation or an unexpected graph shape: a detected cycle must + fail closed (gate the app off), never fall through to enabled. + """ + registry = AppRegistry([_dep_app("a", requires_apps=("b",)), _dep_app("b")]) + # Inject an ``a -> b -> a`` cycle past the build-time validation. + registry._by_key["b"] = registry._by_key["b"].model_copy( + update={"requires_apps": ("a",)} + ) + assert registry.resolve_effective_enabled("a", {}) is False + + def test_memo_caches_shared_subtree_across_calls(self) -> None: + """Reuse a memoized dependency result across a full-registry projection.""" + registry = AppRegistry( + [ + _dep_app("a", requires_apps=("shared",)), + _dep_app("b", requires_apps=("shared",)), + _dep_app("shared"), + ] + ) + memo: dict[str, bool] = {} + assert registry.resolve_effective_enabled("a", {}, memo) is True + assert registry.resolve_effective_enabled("b", {}, memo) is True + # The shared subtree is resolved once and cached for later reuse. + assert memo["shared"] is True + + def test_memo_matches_unmemoized_result_when_dep_disabled(self) -> None: + """Produce identical gating with and without a memo for a disabled dep.""" + registry = AppRegistry( + [ + _dep_app("a", requires_apps=("shared",)), + _dep_app("b", requires_apps=("shared",)), + _dep_app("shared"), + ] + ) + states = {"shared": AppLifecycleEnum.DISABLED} + memo: dict[str, bool] = {} + assert registry.resolve_effective_enabled("a", states, memo) is False + assert registry.resolve_effective_enabled("b", states, memo) is False + class TestChildApps: """Cover ``child_apps`` structural registration in ``build_app_registry``.""" From 576417af85418d21cf2f8aaa290b83a084012bc0 Mon Sep 17 00:00:00 2001 From: Marcus Cruz Date: Wed, 15 Jul 2026 13:52:50 -0300 Subject: [PATCH 3/4] Regenerate SEP API client for updated nav docstring --- frontend/packages/api/src/generated/sep.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frontend/packages/api/src/generated/sep.ts b/frontend/packages/api/src/generated/sep.ts index 59a1584b30..a81ee4c050 100644 --- a/frontend/packages/api/src/generated/sep.ts +++ b/frontend/packages/api/src/generated/sep.ts @@ -136,10 +136,12 @@ export interface paths { * @description Return per-app state for the current user's navigation. * * Protected apps are always reported ``enabled=True``. Every other app reflects - * the DB state of the row governing it (a missing row -> ``enabled=True``: a - * configured plugin is active until explicitly disabled). A child app owns no - * row, so it resolves through its parent via - * :attr:`~app.sep.apps.framework.base.BaseApp.state_key`. + * its *effective* state via :meth:`AppRegistry.resolve_effective_enabled`: its + * own row must be ``ENABLED`` (a missing row -> ``enabled=True``: a configured + * plugin is active until explicitly disabled) **and** every app it declares in + * ``requires_apps`` must be effectively enabled, so the shell hides an app when + * a dependency is off. A child app owns no row, so it resolves through its + * parent via :attr:`~app.sep.apps.framework.base.BaseApp.state_key`. * * :param session: The database session. * :return: The per-app navigation list. From 213a425a65dc66a8c381c627113e625077172f77 Mon Sep 17 00:00:00 2001 From: Marcus Cruz Date: Wed, 15 Jul 2026 14:04:57 -0300 Subject: [PATCH 4/4] Reject child_apps combined with requires_apps at registry build time --- app/sep/apps/framework/registry.py | 11 +++++++- tests/app/sep/apps/framework/test_registry.py | 25 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/app/sep/apps/framework/registry.py b/app/sep/apps/framework/registry.py index d9322ceabb..00908821e7 100644 --- a/app/sep/apps/framework/registry.py +++ b/app/sep/apps/framework/registry.py @@ -80,9 +80,18 @@ def _validate_dependencies(self) -> None: """Reject self-dependencies, dangling deps, and cycles at build time. :raises ValueError: When a ``requires_apps`` entry names the app itself, - names an unregistered key, or participates in a dependency cycle. + names an unregistered key, participates in a dependency cycle, or is + declared alongside ``child_apps`` (unsupported -- children resolve + their own state through the parent's ``state_key`` but do not inherit + the parent's ``requires_apps``, so gating the parent would leave its + children reachable). """ for app in self._apps: + if app.child_apps and app.requires_apps: + raise ValueError( + f"App {app.key!r} combines child_apps with requires_apps, " + "which is not supported.", + ) for dep_key in app.requires_apps: if dep_key == app.key: raise ValueError( diff --git a/tests/app/sep/apps/framework/test_registry.py b/tests/app/sep/apps/framework/test_registry.py index e149e41a5b..8be7ede7d3 100644 --- a/tests/app/sep/apps/framework/test_registry.py +++ b/tests/app/sep/apps/framework/test_registry.py @@ -858,6 +858,31 @@ def test_valid_dependency_graph_builds(self) -> None: registry = AppRegistry([_dep_app("a", requires_apps=("b",)), _dep_app("b")]) assert registry.keys() == ["a", "b"] + def test_child_apps_with_requires_apps_raises(self) -> None: + """Reject combining ``child_apps`` with ``requires_apps``. + + A child resolves its own state through the parent's ``state_key`` but does + not inherit the parent's ``requires_apps``, so the combination would leave + a child reachable while its parent is gated -- reject it until supported. + """ + child = BaseApp( + key="parent_app/sub", + name="sub", + display_name="Sub", + uri_path="/parent_app/sub", + parent_key="parent_app", + ) + parent = BaseApp( + key="parent_app", + name="parent_app", + display_name="Parent App", + uri_path="/parent_app", + child_apps=(child,), + requires_apps=("dep",), + ) + with pytest.raises(ValueError, match="child_apps"): + AppRegistry([parent, _dep_app("dep")]) + class TestEffectiveEnabled: """Cover the centralized effective-enabled resolver."""