diff --git a/docs/decisions/0017-static-authorization-schema.rst b/docs/decisions/0017-static-authorization-schema.rst index b3375d31..1c45dcd9 100644 --- a/docs/decisions/0017-static-authorization-schema.rst +++ b/docs/decisions/0017-static-authorization-schema.rst @@ -19,7 +19,7 @@ Decision 1. Schema format and boundary ============================= -The authz schema is a versioned YAML format for static permissions, permission categories, roles, and changes to existing roles. Every file declares ``schema_version`` and ``priority``, which apply to all definitions in that file. Priority is used only when role extensions conflict; it does not resolve conflicting base definitions (see `ADR 0031`_). Open edX publishes a :download:`machine-readable JSON Schema <../../src/openedx_authz/schema/authz-schema-v1.json>` for this format so that editors, CI, and the compiler all apply the same field and validation rules. +The authz schema is a versioned YAML format for static permissions, permission categories, roles, and changes to existing roles. Every file declares ``schema_version`` and ``priority``, which apply to all definitions in that file. Priority is used only when role extensions conflict; it does not resolve conflicting base definitions (see `ADR 0031`_). Open edX publishes a :download:`machine-readable JSON Schema <../../src/openedx_authz/engine/schema/authz-schema-v1.json>` for this format so that editors, CI, and the compiler all apply the same field and validation rules. The existing static role and permission definitions in Python modules and ``authz.policy`` will move into the schema. Once this migration is complete, the schema becomes the source for static definitions, so developers add a new role or permission there without duplicating it in Python constants or policy files. @@ -155,6 +155,12 @@ Embedded categories on every permission Repeating category labels makes localization and consistent presentation harder. A normalized category gives permissions one stable grouping reference. +Addendum: 2026-10-20 +******************** + +The machine-readable JSON Schema now lives with its validation engine at +:download:`authz-schema-v1.json <../../src/openedx_authz/engine/schema/authz-schema-v1.json>`. + References ********** diff --git a/docs/decisions/0018-authorization-schema-lifecycle.rst b/docs/decisions/0018-authorization-schema-lifecycle.rst index ff88dc03..0cdf31f7 100644 --- a/docs/decisions/0018-authorization-schema-lifecycle.rst +++ b/docs/decisions/0018-authorization-schema-lifecycle.rst @@ -107,6 +107,25 @@ Two limitations are accepted: * **Single-database assumption.** Serialization holds only when the generated policy and the lock live in the same database. The engine already assumes one database for its policy storage, so this is consistent with the rest of the system. * **Blocking wait without observability.** A second apply waits for the first with no timeout and no distinct signal, so a blocked apply is operationally indistinguishable from a slow one. This is acceptable because apply runs at deployment time, not on the request path. +Addendum: Separate structural and semantic validation +====================================================== + +**2026-10-20** + +Schema processing separates parsing, structural validation, typed loading, and semantic validation into ordered phases. + +The lifecycle begins with these phases: + +1. **Discover** identifies the contributed schema resources. +2. **Parse** reads each resource as YAML and preserves the resulting value with its source information. +3. **Validate structure** checks every parsed value against the configured JSON Schema. The pipeline reports all structural errors and stops when it finds any. +4. **Load** constructs typed schema definitions from structurally valid mappings. +5. **Validate semantics** checks relationships across definitions, conflicts between contributions, supported scopes, and values that depend on application data such as the vendored Paragon icon names. + +Compile, render, apply, and consume keep the responsibilities defined above. Typed loading and semantic validation do not run when structural validation fails. This ordering lets JSON Schema report malformed collection shapes and value types before typed construction assumes those structures are valid. + +The separation adds an intermediate parsed-document representation and another explicit pipeline boundary. Callers that previously treated loading as both YAML parsing and typed construction must use the corresponding phase for the representation they need. + Consequences ************ diff --git a/docs/references/authorization-schema-json.rst b/docs/references/authorization-schema-json.rst index 0c0b5d78..0fdaf32a 100644 --- a/docs/references/authorization-schema-json.rst +++ b/docs/references/authorization-schema-json.rst @@ -3,116 +3,116 @@ Generated Authorization Schema ############################## -This page renders the exact validation rules from the :download:`authz JSON Schema <../../src/openedx_authz/schema/authz-schema-v1.json>`. See the :doc:`Authorization Schema Reference ` for guidance, examples, and validation performed by the compiler across schema contributions. +This page renders the exact validation rules from the :download:`authz JSON Schema <../../src/openedx_authz/engine/schema/authz-schema-v1.json>`. See the :doc:`Authorization Schema Reference ` for guidance, examples, and validation performed by the compiler across schema contributions. Top-level fields **************** -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/properties/schema_version +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/properties/schema_version :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/properties/priority +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/properties/priority :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/properties/permission_categories +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/properties/permission_categories :lift_description: :hide_key: /items -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/properties/permissions +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/properties/permissions :lift_description: :hide_key: /items -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/properties/roles +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/properties/roles :lift_description: :hide_key: /items -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/properties/role_extensions +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/properties/role_extensions :lift_description: :hide_key: /items Permission category fields ************************** -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission_category/properties/id +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission_category/properties/id :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission_category/properties/display_name +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission_category/properties/display_name :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission_category/properties/description +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission_category/properties/description :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission_category/properties/icon +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission_category/properties/icon :lift_description: Permission fields ***************** -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission/properties/namespace +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission/properties/namespace :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission/properties/name +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission/properties/name :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission/properties/display_name +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission/properties/display_name :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission/properties/description +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission/properties/description :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission/properties/category_id +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission/properties/category_id :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission/properties/scopes +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission/properties/scopes :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/permission/properties/icon +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/permission/properties/icon :lift_description: Role fields *********** -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role/properties/id +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role/properties/id :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role/properties/display_name +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role/properties/display_name :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role/properties/description +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role/properties/description :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role/properties/scopes +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role/properties/scopes :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role/properties/permissions +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role/properties/permissions :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role/properties/icon +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role/properties/icon :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role/properties/hidden +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role/properties/hidden :lift_description: Role extension fields ********************* -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role_extension/properties/role_id +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role_extension/properties/role_id :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role_extension/properties/add_permissions +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role_extension/properties/add_permissions :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role_extension/properties/remove_permissions +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role_extension/properties/remove_permissions :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role_extension/properties/display_name +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role_extension/properties/display_name :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role_extension/properties/description +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role_extension/properties/description :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role_extension/properties/icon +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role_extension/properties/icon :lift_description: -.. jsonschema:: ../../src/openedx_authz/schema/authz-schema-v1.json#/$defs/role_extension/properties/hidden +.. jsonschema:: ../../src/openedx_authz/engine/schema/authz-schema-v1.json#/$defs/role_extension/properties/hidden :lift_description: diff --git a/docs/references/authorization-schema.rst b/docs/references/authorization-schema.rst index 136d59b6..2e482ef2 100644 --- a/docs/references/authorization-schema.rst +++ b/docs/references/authorization-schema.rst @@ -19,7 +19,7 @@ Use this reference when creating or reviewing an authz schema file. The examples Machine-readable schema *********************** -The :download:`downloadable authz JSON Schema <../../src/openedx_authz/schema/authz-schema-v1.json>` describes the fields, types, required values, and identifier formats for schema version ``1.0``. You can also :doc:`browse the generated schema ` or `view the schema source `_. Editors and validation tools can use it to check YAML files because JSON Schema applies to the data represented by both JSON and YAML. +The :download:`downloadable authz JSON Schema <../../src/openedx_authz/engine/schema/authz-schema-v1.json>` describes the fields, types, required values, and identifier formats for schema version ``1.0``. You can also :doc:`browse the generated schema ` or `view the schema source `_. Editors and validation tools can use it to check YAML files because JSON Schema applies to the data represented by both JSON and YAML. The JSON Schema checks the structure of one file. The compiler also checks references across the combined schema, registered scope namespaces, available Paragon icons, and conflicts between contributions. diff --git a/pyproject.toml b/pyproject.toml index 1b71fab9..2def5524 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -36,6 +36,7 @@ dependencies = [ "edx-drf-extensions", "edx-opaque-keys", "edx-organizations", + "jsonschema>=4.0", "openedx-atlas", "openedx-events", "openedx-filters>=3.15.0", diff --git a/src/openedx_authz/constants/schema.py b/src/openedx_authz/constants/schema.py index 494d0f27..adaadf03 100644 --- a/src/openedx_authz/constants/schema.py +++ b/src/openedx_authz/constants/schema.py @@ -12,6 +12,9 @@ from enum import Enum +AUTHZ_JSON_SCHEMA_SETTING = "OPENEDX_AUTHZ_JSON_SCHEMA" +DEFAULT_AUTHZ_JSON_SCHEMA = "openedx_authz.engine.schema/authz-schema-v1.json" + class SchemaOriginKind(str, Enum): """Whether a schema contribution is a base definition or an extension. diff --git a/src/openedx_authz/schema/authz-schema-v1.json b/src/openedx_authz/engine/schema/authz-schema-v1.json similarity index 98% rename from src/openedx_authz/schema/authz-schema-v1.json rename to src/openedx_authz/engine/schema/authz-schema-v1.json index 111854bb..60427cd3 100644 --- a/src/openedx_authz/schema/authz-schema-v1.json +++ b/src/openedx_authz/engine/schema/authz-schema-v1.json @@ -1,6 +1,6 @@ { "$schema": "https://json-schema.org/draft/2020-12/schema", - "$id": "https://raw.githubusercontent.com/openedx/openedx-authz/main/src/openedx_authz/schema/authz-schema-v1.json", + "$id": "https://raw.githubusercontent.com/openedx/openedx-authz/main/src/openedx_authz/engine/schema/authz-schema-v1.json", "title": "Open edX Authorization schema", "description": "A version 1.0 Open edX authorization schema contribution. Applications and deployment configuration use this format to define static permission categories, permissions, roles, and changes to existing roles.", "type": "object", @@ -121,7 +121,7 @@ "scope_namespace": { "description": "A scope namespace registered by a ScopeData type.", "type": "string", - "minLength": 1, + "pattern": "^[a-z][a-z0-9_-]*$", "examples": ["course-v1"] }, "display_name": { @@ -139,7 +139,7 @@ "icon": { "description": "A case-sensitive icon name exported by @openedx/paragon/icons.", "type": "string", - "minLength": 1, + "pattern": "^[A-Z][A-Za-z0-9]*$", "examples": ["Visibility"] }, "permission_list": { diff --git a/src/openedx_authz/engine/schema/issues.py b/src/openedx_authz/engine/schema/issues.py new file mode 100644 index 00000000..933575a5 --- /dev/null +++ b/src/openedx_authz/engine/schema/issues.py @@ -0,0 +1,37 @@ +"""Validation finding types shared by structural and semantic checks.""" + +from dataclasses import dataclass +from enum import StrEnum + + +class IssueLevel(StrEnum): + """Severity of a :class:`ValidationIssue`. + + Members: + ERROR: Blocks deployment. + WARNING: Reported only, does not block deployment. + """ + + ERROR = "error" + WARNING = "warning" + + +@dataclass(frozen=True) +class ValidationIssue: + """A single validation finding. + + Attributes: + level: :attr:`IssueLevel.ERROR` (blocks deployment) or + :attr:`IssueLevel.WARNING` (reported only). + message: Human-readable description. + source_id: The contributing source, when the issue is file-specific. + """ + + level: IssueLevel + message: str + source_id: str | None = None + + @property + def is_error(self) -> bool: + """Return whether this finding blocks the pipeline.""" + return self.level == IssueLevel.ERROR diff --git a/src/openedx_authz/engine/schema/loading.py b/src/openedx_authz/engine/schema/loading.py index c5c70f68..bf7e85de 100644 --- a/src/openedx_authz/engine/schema/loading.py +++ b/src/openedx_authz/engine/schema/loading.py @@ -1,25 +1,17 @@ -"""Read discovered resources into schema documents (the ``load`` step, ADR 0018). +"""Convert parsed mappings into schema documents (the ``load`` step, ADR 0018). -Parses each ``.yaml`` schema resource into a :class:`SchemaDocument`, attaching -its :class:`SourceRecord` (including a content digest computed from the exact -bytes read). This step performs only parsing and structural shaping; semantic -checks belong to :mod:`.validation` and cross-file resolution to -:mod:`.compilation`. +Each parsed document already contains its YAML mapping and source record. This +step performs only typed structural shaping. Semantic checks belong to +:mod:`.validation` and cross-file resolution belongs to :mod:`.compilation`. No Casbin or Django imports, so it stays unit-testable in isolation. """ from __future__ import annotations -import hashlib -import logging -from importlib import metadata - -import yaml - -from openedx_authz.engine.schema.discovery import DiscoveredResource from openedx_authz.engine.schema.exceptions import SchemaLoadError from openedx_authz.engine.schema.types import ( + ParsedSchemaDocument, PermissionCategory, PermissionDefinition, RoleDefinition, @@ -28,177 +20,36 @@ SourceRecord, ) -logger = logging.getLogger(__name__) - class SchemaLoader: - """Turns discovered resources into typed schema documents. + """Turn structurally valid parsed documents into typed schema documents. - Reading a resource's bytes is the ``load`` step's responsibility (ADR 0018): - each :class:`DiscoveredResource` knows how to read itself, so the loader - controls *when* files are read and computes the content digest from the - exact bytes it parses. + Resource reading, YAML parsing, content digests, and distribution metadata + belong to :class:`~openedx_authz.engine.schema.parsing.SchemaParser`. This + class owns only the conversion into typed definitions. """ - _UNKNOWN_DISTRIBUTION = "unknown" - _distribution_ambiguity_warned: set[tuple[str, str, str]] = set() - """Tracks warned (package, resource_path, selected) combinations to deduplicate logs.""" - - def load(self, resources: list[DiscoveredResource]) -> list[SchemaDocument]: - """Load every discovered resource into a :class:`SchemaDocument`. + def load(self, documents: list[ParsedSchemaDocument]) -> list[SchemaDocument]: + """Load every parsed mapping into a :class:`SchemaDocument`. Raises: - SchemaLoadError: On invalid YAML or an unusable document structure. - """ - documents: list[SchemaDocument] = [] - for resource in resources: - contents = resource.read_bytes() - raw = self._parse_yaml(contents, resource) - schema_version = str(raw.get("schema_version", "")) - source = self._build_source_record(resource, contents, schema_version) - documents.append(self._build_document(raw, source)) - return documents - - def _parse_yaml(self, contents: bytes, resource: DiscoveredResource) -> dict: - """Parse YAML bytes into a mapping, raising on malformed input.""" - try: - data = yaml.safe_load(contents) - except yaml.YAMLError as exc: - raise SchemaLoadError(f"Invalid YAML in {resource.package}:{resource.resource_path}: {exc}") from exc - - if data is None: - data = {} - if not isinstance(data, dict): - raise SchemaLoadError( - f"Schema file {resource.package}:{resource.resource_path} must be a mapping " - f"at the top level, got {type(data).__name__}." - ) - return data - - def _build_source_record(self, resource: DiscoveredResource, contents: bytes, schema_version: str) -> SourceRecord: - """Assemble packaging metadata + content digest into a SourceRecord. - - Resolves the installed distribution name/version that owns the resource - package via ``importlib.metadata`` and hashes ``contents`` for the - digest. Falls back to ``"unknown"`` when the package is not tied to an - installed distribution (e.g. operator-supplied settings resources). - """ - distribution, version = self._resolve_distribution(resource) - content_digest = hashlib.sha256(contents).hexdigest() - return SourceRecord( - distribution=distribution, - distribution_version=version, - module=resource.module, - resource_path=resource.resource_path, - schema_version=schema_version, - content_digest=content_digest, - ) - - @classmethod - def _resolve_distribution(cls, resource: DiscoveredResource) -> tuple[str, str]: - """Map a discovered resource to its providing distribution name and version. - - A top-level import package can be provided by more than one installed - distribution (namespace packages, overlapping/legacy installs), so - ``packages_distributions()`` returns a *list* of candidate names. The - same source must resolve to the same distribution under every deployment - layout (ADR 0019 §2), so an arbitrary ``candidates[0]`` is not good - enough: when several distributions claim the top-level package we pick - the one that actually ships this resource's file, and only fall back to - a deterministic (sorted) choice when no single owner can be identified. - """ - top_level = resource.package.split(".", 1)[0] - try: - mapping = metadata.packages_distributions() - except Exception: # pylint: disable=broad-exception-caught # noqa: BLE001 - defensive; metadata quirks across envs - mapping = {} - # ``packages_distributions()`` can list the same distribution more than - # once for one top-level package (observed with editable installs and - # overlapping metadata). Those are not competing owners, so collapse to - # the distinct names before resolving — otherwise a lone real owner that - # happens to be listed twice looks "ambiguous" and the fallback path - # warns on every resource for a non-problem. - candidates = sorted(set(mapping.get(top_level) or [])) - if not candidates: - return top_level, cls._UNKNOWN_DISTRIBUTION - - distribution = cls._select_owning_distribution(candidates, resource) - try: - return distribution, metadata.version(distribution) - except metadata.PackageNotFoundError: - return distribution, cls._UNKNOWN_DISTRIBUTION - - @classmethod - def _select_owning_distribution(cls, candidates: list[str], resource: DiscoveredResource) -> str: - """Choose the distribution that ships ``resource`` from the candidate list. - - With a single candidate there is nothing to disambiguate. With several, - we match each distribution's recorded file list against the resource's - installed path (``package/resource_path``) and return the one that owns - it. If exactly zero or more than one distribution claims the file (or the - file lists are unavailable), we cannot know the true owner, so we return - the first candidate in sorted order — a stable choice across environments - — and log the ambiguity once per distinct (package, resource_path, selected) - combination. + SchemaLoadError: If called with a root value that is not a mapping. """ - if len(candidates) == 1: - return candidates[0] - - installed_path = f"{resource.package}/{resource.resource_path}" - owners = [name for name in candidates if cls._distribution_ships(name, installed_path)] - if len(owners) == 1: - return owners[0] - - fallback = sorted(candidates)[0] - # Deduplicate warnings by tracking (package, resource_path, selected) combinations - warn_key = (resource.package, resource.resource_path, fallback) - if warn_key not in cls._distribution_ambiguity_warned: - cls._distribution_ambiguity_warned.add(warn_key) - logger.info( - "Schema resource for package '%s' is claimed by multiple distributions %s; " - "deterministically selected '%s'.", - resource.package, - sorted(candidates), - fallback, - extra={ - "package": resource.package, - "resource_path": resource.resource_path, - "candidates": sorted(candidates), - "matched_owners": sorted(owners), - "selected": fallback, - }, - ) - return fallback - - @staticmethod - def _distribution_ships(distribution: str, installed_path: str) -> bool: - """Return whether ``distribution`` records a file matching ``installed_path``. - - ``Distribution.files`` lists the files the distribution installed, as - anchor-relative ``PackagePath`` values (e.g. - ``openedx_authz/authz/schema/course_roles.yaml``). A resource belongs to - the distribution when one of those paths ends with the resource's - ``package/resource_path``. Metadata gaps (``files`` is ``None`` or the - distribution is missing) mean "cannot confirm ownership", not an error. - """ - try: - files = metadata.distribution(distribution).files or [] - except metadata.PackageNotFoundError: - return False - return any(str(path).endswith(installed_path) for path in files) + loaded = [] + for document in documents: + if not isinstance(document.data, dict): + raise SchemaLoadError(f"{document.source.source_id}: schema document must be a mapping.") + loaded.append(self._build_document(document.data, document.source)) + return loaded def _build_document(self, raw: dict, source: SourceRecord) -> SchemaDocument: """Map the parsed mapping's blocks into a typed SchemaDocument. ``priority`` is a *required* field per ``authz-schema-v1.json``; a higher value wins when contributions conflict (see - ``docs/references/authorization-schema.rst``). Enforcing that it is - present is the ``validate`` step's job, not this one — the loader only - parses and shapes (ADR 0018), and it defers required-field enforcement - to validation exactly as it does for ``schema_version``. So a missing - ``priority`` is read as ``0`` (the lowest precedence) here and rejected - later by validation, whereas a *malformed* (non-integer) ``priority`` is - raised now because it cannot be parsed at all. + ``docs/references/authorization-schema.rst``). JSON Schema validates + its presence and type before this method runs. The fallback to ``0`` + and conversion error remain defensive behavior for direct loader use. """ try: priority = int(raw.get("priority", 0)) diff --git a/src/openedx_authz/engine/schema/parsing.py b/src/openedx_authz/engine/schema/parsing.py new file mode 100644 index 00000000..e8703b8c --- /dev/null +++ b/src/openedx_authz/engine/schema/parsing.py @@ -0,0 +1,169 @@ +"""Read discovered resources into parsed documents (the ``parse`` step, ADR 0018). + +Parses each YAML schema resource without imposing authorization-schema +structure, attaching its :class:`SourceRecord` with a content digest computed +from the exact bytes read. JSON Schema validation owns structural requirements, +typed shaping belongs to :mod:`.loading`, and semantic checks belong to +:mod:`.validation`. + +No Casbin or Django imports, so parsing stays unit-testable in isolation. +""" + +from __future__ import annotations + +import hashlib +import logging +from importlib import metadata + +import yaml + +from openedx_authz.engine.schema.discovery import DiscoveredResource +from openedx_authz.engine.schema.exceptions import SchemaLoadError +from openedx_authz.engine.schema.types import ParsedSchemaDocument, SourceRecord + +logger = logging.getLogger(__name__) + +UNKNOWN_DISTRIBUTION = "unknown" + + +class SchemaParser: + """Read discovered resources into untyped YAML values with provenance. + + Each :class:`DiscoveredResource` knows how to read itself, so the parser + controls when files are read and computes the content digest from the exact + bytes passed to the YAML parser. + """ + + distribution_ambiguity_warned: set[tuple[str, str, str]] = set() + """Tracks warned (package, resource_path, selected) combinations to deduplicate logs.""" + + def parse(self, resources: list[DiscoveredResource]) -> list[ParsedSchemaDocument]: + """Parse every resource without imposing authorization-schema structure.""" + documents = [] + for resource in resources: + contents = resource.read_bytes() + data = self.parse_yaml(contents, resource) + schema_version = str(data.get("schema_version", "")) if isinstance(data, dict) else "" + source = self.build_source_record(resource, contents, schema_version) + documents.append(ParsedSchemaDocument(source=source, data=data)) + return documents + + @staticmethod + def parse_yaml(contents: bytes, resource: DiscoveredResource) -> object: + """Parse YAML bytes and preserve the resulting root value.""" + try: + return yaml.safe_load(contents) + except yaml.YAMLError as exc: + raise SchemaLoadError(f"Invalid YAML in {resource.package}:{resource.resource_path}: {exc}") from exc + + def build_source_record(self, resource: DiscoveredResource, contents: bytes, schema_version: str) -> SourceRecord: + """Assemble packaging metadata and a digest into a SourceRecord. + + Resolves the installed distribution name/version that owns the resource + package via ``importlib.metadata`` and hashes ``contents`` for the + digest. Falls back to ``"unknown"`` when the package is not tied to an + installed distribution (e.g. operator-supplied settings resources). + """ + distribution, version = self.resolve_distribution(resource) + content_digest = hashlib.sha256(contents).hexdigest() + return SourceRecord( + distribution=distribution, + distribution_version=version, + module=resource.module, + resource_path=resource.resource_path, + schema_version=schema_version, + content_digest=content_digest, + ) + + @classmethod + def resolve_distribution(cls, resource: DiscoveredResource) -> tuple[str, str]: + """Map a discovered resource to its providing distribution name and version. + + A top-level import package can be provided by more than one installed + distribution (namespace packages, overlapping/legacy installs), so + ``packages_distributions()`` returns a *list* of candidate names. The + same source must resolve to the same distribution under every deployment + layout (ADR 0019 §2), so an arbitrary ``candidates[0]`` is not good + enough: when several distributions claim the top-level package we pick + the one that actually ships this resource's file, and only fall back to + a deterministic (sorted) choice when no single owner can be identified. + """ + top_level = resource.package.split(".", 1)[0] + try: + mapping = metadata.packages_distributions() + except Exception: # pylint: disable=broad-exception-caught # noqa: BLE001 - defensive; metadata quirks across envs + mapping = {} + # ``packages_distributions()`` can list the same distribution more than + # once for one top-level package (observed with editable installs and + # overlapping metadata). Those are not competing owners, so collapse to + # the distinct names before resolving; otherwise a lone real owner that + # happens to be listed twice looks "ambiguous" and the fallback path + # warns on every resource for a non-problem. + candidates = sorted(set(mapping.get(top_level) or [])) + if not candidates: + return top_level, UNKNOWN_DISTRIBUTION + + distribution = cls.select_owning_distribution(candidates, resource) + try: + return distribution, metadata.version(distribution) + except metadata.PackageNotFoundError: + return distribution, UNKNOWN_DISTRIBUTION + + @classmethod + def select_owning_distribution(cls, candidates: list[str], resource: DiscoveredResource) -> str: + """Choose the distribution that ships ``resource`` from the candidate list. + + With a single candidate there is nothing to disambiguate. With several, + we match each distribution's recorded file list against the resource's + installed path (``package/resource_path``) and return the one that owns + it. If exactly zero or more than one distribution claims the file (or the + file lists are unavailable), we cannot know the true owner, so we return + the first candidate in sorted order, a stable choice across environments, + and log the ambiguity once per distinct (package, resource_path, selected) + combination. + """ + if len(candidates) == 1: + return candidates[0] + + installed_path = f"{resource.package}/{resource.resource_path}" + owners = [name for name in candidates if cls.distribution_ships(name, installed_path)] + if len(owners) == 1: + return owners[0] + + fallback = sorted(candidates)[0] + # Deduplicate warnings by tracking (package, resource_path, selected) combinations. + warn_key = (resource.package, resource.resource_path, fallback) + if warn_key not in cls.distribution_ambiguity_warned: + cls.distribution_ambiguity_warned.add(warn_key) + logger.info( + "Schema resource for package '%s' is claimed by multiple distributions %s; " + "deterministically selected '%s'.", + resource.package, + sorted(candidates), + fallback, + extra={ + "package": resource.package, + "resource_path": resource.resource_path, + "candidates": sorted(candidates), + "matched_owners": sorted(owners), + "selected": fallback, + }, + ) + return fallback + + @staticmethod + def distribution_ships(distribution: str, installed_path: str) -> bool: + """Return whether ``distribution`` records a file matching ``installed_path``. + + ``Distribution.files`` lists the files the distribution installed, as + anchor-relative ``PackagePath`` values (e.g. + ``openedx_authz/authz/schema/course_roles.yaml``). A resource belongs to + the distribution when one of those paths ends with the resource's + ``package/resource_path``. Metadata gaps (``files`` is ``None`` or the + distribution is missing) mean "cannot confirm ownership", not an error. + """ + try: + files = metadata.distribution(distribution).files or [] + except metadata.PackageNotFoundError: + return False + return any(str(path).endswith(installed_path) for path in files) diff --git a/src/openedx_authz/engine/schema/pipeline.py b/src/openedx_authz/engine/schema/pipeline.py index 5d437d8e..8d76ab03 100644 --- a/src/openedx_authz/engine/schema/pipeline.py +++ b/src/openedx_authz/engine/schema/pipeline.py @@ -2,7 +2,7 @@ :class:`SchemaPipeline` wires the steps together: - discover -> load -> validate -> compile -> render -> (plan) -> apply + discover -> parse -> validate structure -> load -> validate semantics -> compile -> render -> (plan) -> apply The Casbin-free steps (discover..compile) live in :mod:`openedx_authz.engine.schema`; render/apply live in :mod:`openedx_authz.engine.renderer`. This orchestrator is @@ -26,9 +26,12 @@ from openedx_authz.engine.schema.compilation import SchemaCompiler from openedx_authz.engine.schema.discovery import SchemaDiscovery from openedx_authz.engine.schema.exceptions import SchemaValidationError +from openedx_authz.engine.schema.issues import ValidationIssue from openedx_authz.engine.schema.loading import SchemaLoader +from openedx_authz.engine.schema.parsing import SchemaParser +from openedx_authz.engine.schema.syntax import SchemaSyntaxValidator from openedx_authz.engine.schema.types import CompiledSchema -from openedx_authz.engine.schema.validation import SchemaValidator, ValidationIssue +from openedx_authz.engine.schema.validation import SchemaValidator logger = logging.getLogger(__name__) @@ -44,6 +47,8 @@ def __init__( self, *, discovery: SchemaDiscovery | None = None, + parser: SchemaParser | None = None, + syntax_validator: SchemaSyntaxValidator | None = None, loader: SchemaLoader | None = None, validator: SchemaValidator | None = None, compiler: SchemaCompiler | None = None, @@ -51,6 +56,8 @@ def __init__( applier: SchemaApplier | None = None, ): self._discovery = discovery or SchemaDiscovery() + self._parser = parser or SchemaParser() + self._syntax_validator = syntax_validator or SchemaSyntaxValidator() self._loader = loader or SchemaLoader() self._validator = validator or SchemaValidator() self._compiler = compiler or SchemaCompiler() @@ -58,7 +65,7 @@ def __init__( self._applier = applier or SchemaApplier() def compile(self) -> CompiledSchema: - """Run discover -> load -> validate -> compile and return the result. + """Run discover -> parse -> validate syntax -> load -> validate semantics -> compile and return the result. Validation gates twice: once on the loaded documents, then again on the compiled schema, because extensions and priority resolution can only be @@ -70,7 +77,9 @@ def compile(self) -> CompiledSchema: SchemaCompileError: On an unresolvable conflict. """ resources = self._discovery.discover() - documents = self._loader.load(resources) + parsed_documents = self._parser.parse(resources) + self._gate(self._syntax_validator.validate(parsed_documents)) + documents = self._loader.load(parsed_documents) self._gate(self._validator.validate(documents)) schema = self._compiler.compile(documents) @@ -87,7 +96,7 @@ def _gate(self, issues: list[ValidationIssue]) -> None: for issue in issues: log = logger.error if issue.is_error else logger.warning log("authz schema %s: %s [%s]", issue.level, issue.message, issue.source_id or "-") - if self._validator.has_errors(issues): + if any(issue.is_error for issue in issues): raise SchemaValidationError([i for i in issues if i.is_error]) def plan(self) -> ChangePlan: diff --git a/src/openedx_authz/engine/schema/syntax.py b/src/openedx_authz/engine/schema/syntax.py new file mode 100644 index 00000000..0c55e37b --- /dev/null +++ b/src/openedx_authz/engine/schema/syntax.py @@ -0,0 +1,58 @@ +"""Validate parsed schema values against the configured JSON Schema. + +The schema owns supported ``schema_version`` values, required fields, +identifier and permission-reference formats, scope formats, collection shapes, +value types, and Paragon icon-name format. Semantic relationship and icon +membership checks remain in :mod:`.validation`. +""" + +from __future__ import annotations + +import json +from importlib import resources + +from django.conf import settings +from django.core.exceptions import ImproperlyConfigured +from jsonschema import Draft202012Validator + +from openedx_authz.constants.schema import AUTHZ_JSON_SCHEMA_SETTING, DEFAULT_AUTHZ_JSON_SCHEMA +from openedx_authz.engine.schema.issues import IssueLevel, ValidationIssue +from openedx_authz.engine.schema.types import ParsedSchemaDocument + + +def document_validator(schema_resource: str) -> Draft202012Validator: + """Load an authorization validator from a package resource.""" + schema_package, separator, schema_filename = schema_resource.partition("/") + if not separator: + raise ValueError(f"{AUTHZ_JSON_SCHEMA_SETTING} must be a package-anchored resource path.") + schema_path = resources.files(schema_package).joinpath(schema_filename) + schema = json.loads(schema_path.read_text(encoding="utf-8")) + Draft202012Validator.check_schema(schema) + return Draft202012Validator(schema) + + +def configured_schema_resource() -> str: + """Return the configured JSON Schema resource, including outside Django.""" + try: + return getattr(settings, AUTHZ_JSON_SCHEMA_SETTING, DEFAULT_AUTHZ_JSON_SCHEMA) + except ImproperlyConfigured: + return DEFAULT_AUTHZ_JSON_SCHEMA + + +class SchemaSyntaxValidator: + """Collect JSON Schema findings for parsed documents.""" + + def validate(self, documents: list[ParsedSchemaDocument]) -> list[ValidationIssue]: + """Return every structural error from every parsed document.""" + validator = document_validator(configured_schema_resource()) + issues = [] + for document in documents: + issues.extend( + ValidationIssue( + IssueLevel.ERROR, + f"{error.json_path}: {error.message}", + document.source.source_id, + ) + for error in validator.iter_errors(document.data) + ) + return issues diff --git a/src/openedx_authz/engine/schema/types/__init__.py b/src/openedx_authz/engine/schema/types/__init__.py index 22c4456e..6990ec49 100644 --- a/src/openedx_authz/engine/schema/types/__init__.py +++ b/src/openedx_authz/engine/schema/types/__init__.py @@ -35,6 +35,7 @@ RoleExtension, ) from openedx_authz.engine.schema.types.loading import ( + ParsedSchemaDocument, SchemaDocument, SourceRecord, ) @@ -47,6 +48,7 @@ "RoleExtension", # loading "SourceRecord", + "ParsedSchemaDocument", "SchemaDocument", # compilation "RelationshipSource", diff --git a/src/openedx_authz/engine/schema/types/loading.py b/src/openedx_authz/engine/schema/types/loading.py index baa7bc24..92c495fc 100644 --- a/src/openedx_authz/engine/schema/types/loading.py +++ b/src/openedx_authz/engine/schema/types/loading.py @@ -60,6 +60,14 @@ def source_id(self) -> str: return f"{self.distribution}:{module_path}/{filename}" +@dataclass(frozen=True) +class ParsedSchemaDocument: + """One parsed YAML value and the provenance of the resource that supplied it.""" + + source: SourceRecord + data: object + + @dataclass class SchemaDocument: """One loaded ``.yaml`` schema file plus its provenance and priority. diff --git a/src/openedx_authz/engine/schema/validation.py b/src/openedx_authz/engine/schema/validation.py index 0ccf69c4..d2c82f5a 100644 --- a/src/openedx_authz/engine/schema/validation.py +++ b/src/openedx_authz/engine/schema/validation.py @@ -3,13 +3,6 @@ Rules come from ADR 0017 §4 and the field reference: Per-document checks: - * ``schema_version`` is a supported, quoted ``major.minor`` value. - * ``namespace``, ``name``, category ``id``, role ``id`` match - :data:`IDENTIFIER_RE` (lowercase snake_case, begins with a letter). - * Casbin forms (``act^...``, ``role^...``) are rejected as identifiers. - * Required fields are present. - * ``scopes`` are non-empty and look like scope namespaces (hyphens allowed, - e.g. ``course-v1``); they are exempt from the identifier regex. * ``icon`` values, where present, are valid ``@openedx/paragon/icons`` names (ADR 0017 §4). The allowed set is vendored in :mod:`paragon_icons` (regenerate with ``make paragon_icons``; see ADR 0029). @@ -27,79 +20,28 @@ * The resolved permissions and categories still exist. Validation collects issues rather than raising on the first problem, so the -deployment report can list every error and warning at once. No Casbin/Django -imports. +deployment report can list every error and warning at once. No Casbin imports. """ from __future__ import annotations -import re from collections.abc import Container -from dataclasses import dataclass -from enum import StrEnum from openedx_authz.constants import SchemaOriginKind +from openedx_authz.engine.schema.issues import IssueLevel, ValidationIssue from openedx_authz.engine.schema.paragon_icons import PARAGON_ICON_NAMES from openedx_authz.engine.schema.types import ( CompiledSchema, SchemaDocument, ) -_IDENTIFIER = r"[a-z][a-z0-9_]*" -IDENTIFIER_RE = re.compile(rf"^{_IDENTIFIER}$") -# A complete permission ID joins ``namespace`` and ``name`` with a single -# period, each part being a bare identifier. Mirrors the ``permission_id`` -# ``$def`` in ``schema/authz-schema-v1.json``. -PERMISSION_ID_RE = re.compile(rf"^{_IDENTIFIER}\.{_IDENTIFIER}$") -# Scope namespaces follow their registered spelling and may contain hyphens. -SCOPE_RE = re.compile(r"^[a-z][a-z0-9_-]*$") -# Paragon icon names are PascalCase component exports, e.g. ``RemoveRedEye``. -ICON_RE = re.compile(r"^[A-Z][A-Za-z0-9]*$") - -# Casbin-internal prefixes that must never appear in a schema identifier. -CASBIN_INTERNAL_PREFIXES = ("act^", "role^", "sub^", "scope^", "g^", "p^") - - -class IssueLevel(StrEnum): - """Severity of a :class:`ValidationIssue`. - - Members: - ERROR: Blocks deployment. - WARNING: Reported only, does not block deployment. - """ - - ERROR = "error" - WARNING = "warning" - - -@dataclass(frozen=True) -class ValidationIssue: - """A single validation finding. - - Attributes: - level: :attr:`IssueLevel.ERROR` (blocks deployment) or - :attr:`IssueLevel.WARNING` (reported only). - message: Human-readable description. - source_id: The contributing source, when the issue is file-specific. - """ - - level: IssueLevel - message: str - source_id: str | None = None - - @property - def is_error(self) -> bool: - return self.level == IssueLevel.ERROR - class SchemaValidator: """Runs per-document and whole-set validation.""" - SUPPORTED_SCHEMA_VERSIONS = frozenset({"1.0"}) - def validate(self, documents: list[SchemaDocument]) -> list[ValidationIssue]: """Run per-document then whole-set validation, returning all issues.""" - issues: list[ValidationIssue] = [] + issues = [] for document in documents: issues.extend(self.validate_document(document)) issues.extend(self.validate_set(documents)) @@ -149,50 +91,22 @@ def validate_compiled(self, schema: CompiledSchema) -> list[ValidationIssue]: return issues - @staticmethod - def has_errors(issues: list[ValidationIssue]) -> bool: - """True if any issue is error-level.""" - return any(issue.is_error for issue in issues) - def validate_document(self, document: SchemaDocument) -> list[ValidationIssue]: """Per-file checks that need no cross-file context.""" issues: list[ValidationIssue] = [] sid = document.source.source_id - if document.source.schema_version not in self.SUPPORTED_SCHEMA_VERSIONS: - issues.append( - ValidationIssue( - IssueLevel.ERROR, - f"Unsupported schema_version {document.source.schema_version!r}; " - f"supported: {sorted(self.SUPPORTED_SCHEMA_VERSIONS)}.", - sid, - ) - ) - for category in document.categories: - issues.extend(self._check_identifier(category.id, "category id", sid)) - issues.extend(self._require(category.id, "category id", sid)) issues.extend(self._check_icon(category.icon, f"category {category.id}", sid)) for permission in document.permissions: - issues.extend(self._check_identifier(permission.namespace, "permission namespace", sid)) - issues.extend(self._check_identifier(permission.name, "permission name", sid)) - issues.extend(self._require(permission.category_id, f"category for {permission.identifier}", sid)) - issues.extend(self._check_scopes(permission.scopes, f"permission {permission.identifier}", sid)) issues.extend(self._check_icon(permission.icon, f"permission {permission.identifier}", sid)) for role in document.roles: - issues.extend(self._check_identifier(role.id, "role id", sid)) - issues.extend(self._check_scopes(role.scopes, f"role {role.id}", sid)) issues.extend(self._check_icon(role.icon, f"role {role.id}", sid)) - for perm_id in role.permissions: - issues.extend(self._check_permission_id(perm_id, f"role {role.id}", sid)) for extension in document.role_extensions: - issues.extend(self._check_identifier(extension.role_id, "role_extension target", sid)) issues.extend(self._check_icon(extension.icon, f"role_extension {extension.role_id}", sid)) - for perm_id in (*extension.add_permissions, *extension.remove_permissions): - issues.extend(self._check_permission_id(perm_id, f"role_extension {extension.role_id}", sid)) return issues @@ -289,49 +203,6 @@ def _register(index: dict, key: str, value, kind: str, sid: str) -> list[Validat return [ValidationIssue(IssueLevel.WARNING, f"Duplicate identical {kind} {key!r}.", sid)] return [ValidationIssue(IssueLevel.ERROR, f"Conflicting {kind} definition for {key!r}.", sid)] - def _check_identifier(self, value: str, label: str, sid: str) -> list[ValidationIssue]: - """Validate a single identifier is lowercase snake_case and not a Casbin form.""" - if not value: - return [] # emptiness handled by _require where relevant - if any(value.startswith(prefix) for prefix in CASBIN_INTERNAL_PREFIXES): - return [ValidationIssue(IssueLevel.ERROR, f"{label} {value!r} uses an internal Casbin form.", sid)] - if not IDENTIFIER_RE.match(value): - return [ - ValidationIssue( - IssueLevel.ERROR, - f"{label} {value!r} must match {IDENTIFIER_RE.pattern} (lowercase snake_case).", - sid, - ) - ] - return [] - - def _check_permission_id(self, value: str, context: str, sid: str) -> list[ValidationIssue]: - """A complete permission id is ``namespace.name`` with both parts valid. - - The shape is checked against :data:`PERMISSION_ID_RE`, which mirrors the - ``permission_id`` definition in ``schema/authz-schema-v1.json``: a single - period joining two lowercase snake_case identifiers, each beginning with - a letter. Internal Casbin forms are rejected with a clearer message. - """ - if any(value.startswith(prefix) for prefix in CASBIN_INTERNAL_PREFIXES): - return [ - ValidationIssue( - IssueLevel.ERROR, - f"{context}: permission id {value!r} uses an internal Casbin form.", - sid, - ) - ] - if not PERMISSION_ID_RE.match(value): - return [ - ValidationIssue( - IssueLevel.ERROR, - f"{context}: permission id {value!r} must match {PERMISSION_ID_RE.pattern} " - "(two lowercase snake_case identifiers joined by a period, e.g. 'courses.view_course').", - sid, - ) - ] - return [] - @staticmethod def _check_role_permission_scope( role_id: str, @@ -388,35 +259,16 @@ def _check_permission_category( ] return [] - def _check_scopes(self, scopes: tuple[str, ...], context: str, sid: str) -> list[ValidationIssue]: - """Validate that at least one scope is declared and each scope namespace is well-formed.""" - if not scopes: - return [ValidationIssue(IssueLevel.ERROR, f"{context} must declare at least one scope.", sid)] - issues: list[ValidationIssue] = [] - for scope in scopes: - if not SCOPE_RE.match(scope): - issues.append(ValidationIssue(IssueLevel.ERROR, f"{context}: invalid scope namespace {scope!r}.", sid)) - return issues - @staticmethod def _check_icon(value: str | None, context: str, sid: str) -> list[ValidationIssue]: """Validate an optional Paragon icon name (ADR 0017 §4). - Icons are optional everywhere, so an empty/``None`` value passes. A - present value must be PascalCase and one of the vendored - ``@openedx/paragon/icons`` names (see :mod:`paragon_icons`, - ``make paragon_icons``). Both failures are errors. + JSON Schema owns the value's type and format. This check verifies that + the formatted name is one of the vendored ``@openedx/paragon/icons`` + exports (see :mod:`paragon_icons`, ``make paragon_icons``). """ - if not value: + if not isinstance(value, str) or not value: return [] - if not ICON_RE.match(value): - return [ - ValidationIssue( - IssueLevel.ERROR, - f"{context}: icon {value!r} must be a PascalCase Paragon icon name.", - sid, - ) - ] if value not in PARAGON_ICON_NAMES: return [ ValidationIssue( @@ -427,12 +279,6 @@ def _check_icon(value: str | None, context: str, sid: str) -> list[ValidationIss ] return [] - @staticmethod - def _require(value: str, label: str, sid: str) -> list[ValidationIssue]: - if not value: - return [ValidationIssue(IssueLevel.ERROR, f"Missing required field: {label}.", sid)] - return [] - @staticmethod def _role_permission_source_id(schema: CompiledSchema, role_id: str, perm_id: str) -> str | None: """Name the source(s) responsible for one role-permission grant. diff --git a/src/openedx_authz/schema/__init__.py b/src/openedx_authz/schema/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/src/openedx_authz/settings/common.py b/src/openedx_authz/settings/common.py index c6cabc24..9f03cd51 100644 --- a/src/openedx_authz/settings/common.py +++ b/src/openedx_authz/settings/common.py @@ -5,6 +5,7 @@ import os from openedx_authz import ROOT_DIRECTORY +from openedx_authz.constants.schema import DEFAULT_AUTHZ_JSON_SCHEMA def plugin_settings(settings): @@ -59,3 +60,6 @@ def plugin_settings(settings): # This setting defines whether to enable automatic course migration. See ADR-0013 for more details. if not hasattr(settings, "ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION"): settings.ENABLE_AUTOMATIC_AUTHZ_COURSE_AUTHORING_MIGRATION = False + + if not hasattr(settings, "OPENEDX_AUTHZ_JSON_SCHEMA"): + settings.OPENEDX_AUTHZ_JSON_SCHEMA = DEFAULT_AUTHZ_JSON_SCHEMA diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/categories.yaml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/categories.yaml new file mode 100644 index 00000000..e2b70518 --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/categories.yaml @@ -0,0 +1,6 @@ +schema_version: "1.0" +priority: 100 +permission_categories: + - id: pipeline_content + display_name: Pipeline content + description: Permissions used by the schema pipeline integration test. diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/extensions.yaml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/extensions.yaml new file mode 100644 index 00000000..edbf0295 --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/extensions.yaml @@ -0,0 +1,5 @@ +schema_version: "1.0" +priority: 200 +role_extensions: + - role_id: pipeline_editor + add_permissions: [pipeline.edit_content] diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/permissions.yaml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/permissions.yaml new file mode 100644 index 00000000..2cb42674 --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/permissions.yaml @@ -0,0 +1,15 @@ +schema_version: "1.0" +priority: 100 +permissions: + - namespace: pipeline + name: view_content + display_name: View content + description: View content in the pipeline integration test. + category_id: pipeline_content + scopes: [course-v1] + - namespace: pipeline + name: edit_content + display_name: Edit content + description: Edit content outside the role scope. + category_id: pipeline_content + scopes: [org-v1] diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/roles.yaml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/roles.yaml new file mode 100644 index 00000000..1460c393 --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_compiled/roles.yaml @@ -0,0 +1,8 @@ +schema_version: "1.0" +priority: 100 +roles: + - id: pipeline_editor + display_name: Pipeline editor + description: Edits content in the pipeline integration test. + scopes: [course-v1] + permissions: [pipeline.view_content] diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_parse/malformed.yaml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_parse/malformed.yaml new file mode 100644 index 00000000..45f7c604 --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_parse/malformed.yaml @@ -0,0 +1,2 @@ +schema_version: "1.0" +priority: [unclosed diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_semantics/unknown_category.yaml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_semantics/unknown_category.yaml new file mode 100644 index 00000000..b79d5001 --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_semantics/unknown_category.yaml @@ -0,0 +1,9 @@ +schema_version: "1.0" +priority: 100 +permissions: + - namespace: pipeline + name: orphaned + display_name: Orphaned permission + description: References a category that does not exist. + category_id: missing_category + scopes: [course-v1] diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_structure/items.yml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_structure/items.yml new file mode 100644 index 00000000..febeb13f --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_structure/items.yml @@ -0,0 +1,4 @@ +schema_version: "1.0" +priority: 100 +permissions: + - invalid item diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_structure/priority.yaml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_structure/priority.yaml new file mode 100644 index 00000000..e6abc503 --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/invalid_structure/priority.yaml @@ -0,0 +1,4 @@ +schema_version: "1.0" +priority: high +roles: + - id: invalid_priority diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/categories.yaml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/categories.yaml new file mode 100644 index 00000000..e2b70518 --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/categories.yaml @@ -0,0 +1,6 @@ +schema_version: "1.0" +priority: 100 +permission_categories: + - id: pipeline_content + display_name: Pipeline content + description: Permissions used by the schema pipeline integration test. diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/extensions.yaml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/extensions.yaml new file mode 100644 index 00000000..edbf0295 --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/extensions.yaml @@ -0,0 +1,5 @@ +schema_version: "1.0" +priority: 200 +role_extensions: + - role_id: pipeline_editor + add_permissions: [pipeline.edit_content] diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/permissions.yml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/permissions.yml new file mode 100644 index 00000000..16af7a0c --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/permissions.yml @@ -0,0 +1,15 @@ +schema_version: "1.0" +priority: 100 +permissions: + - namespace: pipeline + name: view_content + display_name: View content + description: View content in the pipeline integration test. + category_id: pipeline_content + scopes: [course-v1] + - namespace: pipeline + name: edit_content + display_name: Edit content + description: Edit content in the pipeline integration test. + category_id: pipeline_content + scopes: [course-v1] diff --git a/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/roles.yaml b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/roles.yaml new file mode 100644 index 00000000..1460c393 --- /dev/null +++ b/src/openedx_authz/tests/integration/fixtures/schema_pipeline/valid/roles.yaml @@ -0,0 +1,8 @@ +schema_version: "1.0" +priority: 100 +roles: + - id: pipeline_editor + display_name: Pipeline editor + description: Edits content in the pipeline integration test. + scopes: [course-v1] + permissions: [pipeline.view_content] diff --git a/src/openedx_authz/tests/integration/test_schema_pipeline.py b/src/openedx_authz/tests/integration/test_schema_pipeline.py new file mode 100644 index 00000000..6fce2174 --- /dev/null +++ b/src/openedx_authz/tests/integration/test_schema_pipeline.py @@ -0,0 +1,117 @@ +"""Exercise the complete schema pipeline with YAML fixtures and the real database.""" + +from unittest import mock + +import pytest +from django.test import TestCase, override_settings + +from openedx_authz.engine.enforcer import AuthzEnforcer +from openedx_authz.engine.schema.discovery import SchemaDiscovery +from openedx_authz.engine.schema.exceptions import SchemaLoadError, SchemaValidationError +from openedx_authz.engine.schema.pipeline import SchemaPipeline +from openedx_authz.models.schema import ( + AuthzPermissionCategory, + AuthzPermissionDefinition, + AuthzRoleDefinition, + AuthzRolePermission, + AuthzRolePermissionSource, + AuthzSchemaSource, +) + +FIXTURE_ROOT = "openedx_authz/tests/integration/fixtures/schema_pipeline" + + +@pytest.fixture(scope="session") +def django_db_setup(request, django_test_environment, django_db_blocker): # pylint: disable=unused-argument + """Build integration tables from models while platform migrations stay disabled.""" + from django.test.utils import setup_databases, teardown_databases # pylint: disable=import-outside-toplevel + from pytest_django.fixtures import _disable_migrations # pylint: disable=import-outside-toplevel + + _disable_migrations() + with django_db_blocker.unblock(): + database_config = setup_databases(verbosity=request.config.option.verbose, interactive=False) + yield + with django_db_blocker.unblock(): + teardown_databases(database_config, verbosity=request.config.option.verbose) + + +@override_settings(OPENEDX_AUTHZ_SCHEMA_DIRECTORIES=[]) +class TestSchemaPipelineIntegration(TestCase): + """Run discovery through application using packaged YAML fixtures.""" + + def setUp(self): + """Start every integration case without stored policy rows.""" + self.enforcer = AuthzEnforcer.get_enforcer() + self.enforcer.clear_policy() + + def tearDown(self): + """Leave no policy rows for another integration test.""" + self.enforcer.clear_policy() + + def pipeline(self, fixture_set: str) -> SchemaPipeline: + """Build a pipeline that discovers only one packaged fixture set.""" + discovery = SchemaDiscovery(passed_in_directories=[f"{FIXTURE_ROOT}/{fixture_set}"]) + return SchemaPipeline(discovery=discovery) + + @mock.patch("openedx_authz.engine.schema.discovery.metadata.entry_points", return_value=[]) + def test_valid_fixtures_apply_and_reapply_idempotently(self, _entry_points): + """Mixed YAML suffixes traverse every stage and persist stable provenance.""" + pipeline = self.pipeline("valid") + + first = pipeline.apply() + second = pipeline.apply() + + self.assertEqual(first.added, 2) + self.assertEqual(second.added, 0) + self.assertEqual(second.removed, 0) + self.assertTrue(second.unchanged) + self.assertEqual(AuthzPermissionCategory.objects.filter(category_id="pipeline_content").count(), 1) + self.assertEqual(AuthzPermissionDefinition.objects.filter(namespace="pipeline").count(), 2) + self.assertEqual(AuthzRoleDefinition.objects.filter(role_id="pipeline_editor").count(), 1) + self.assertEqual(AuthzRolePermission.objects.filter(role__role_id="pipeline_editor").count(), 2) + self.assertEqual(AuthzSchemaSource.objects.filter(module__contains="schema_pipeline.valid").count(), 1) + edit_link = AuthzRolePermissionSource.objects.get( + role_permission__role__role_id="pipeline_editor", + role_permission__permission__name="edit_content", + ) + self.assertEqual(edit_link.origin_kind, "extension") + + @mock.patch("openedx_authz.engine.schema.discovery.metadata.entry_points", return_value=[]) + def test_structural_errors_from_every_file_stop_before_database_writes(self, _entry_points): + """Structural findings aggregate across files and prevent all persistence.""" + with self.assertRaises(SchemaValidationError) as error: + self.pipeline("invalid_structure").apply() + + source_ids = {issue.source_id for issue in error.exception.issues} + self.assertEqual(len(source_ids), 2) + self.assertFalse(AuthzSchemaSource.objects.exists()) + self.assertFalse(AuthzRoleDefinition.objects.exists()) + + @mock.patch("openedx_authz.engine.schema.discovery.metadata.entry_points", return_value=[]) + def test_semantic_error_stops_before_database_writes(self, _entry_points): + """Structurally valid unknown references fail semantic validation without writes.""" + with self.assertRaises(SchemaValidationError) as error: + self.pipeline("invalid_semantics").apply() + + self.assertTrue(any("unknown category" in issue.message for issue in error.exception.issues)) + self.assertFalse(AuthzSchemaSource.objects.exists()) + self.assertFalse(AuthzPermissionDefinition.objects.exists()) + + @mock.patch("openedx_authz.engine.schema.discovery.metadata.entry_points", return_value=[]) + def test_malformed_yaml_stops_before_database_writes(self, _entry_points): + """Malformed YAML fails during parsing before any persistence begins.""" + with self.assertRaises(SchemaLoadError): + self.pipeline("invalid_parse").apply() + + self.assertFalse(AuthzSchemaSource.objects.exists()) + self.assertFalse(AuthzPermissionDefinition.objects.exists()) + + @mock.patch("openedx_authz.engine.schema.discovery.metadata.entry_points", return_value=[]) + def test_compiled_semantic_error_stops_before_database_writes(self, _entry_points): + """An extension scope conflict fails after compilation without writes.""" + with self.assertRaises(SchemaValidationError) as error: + self.pipeline("invalid_compiled").apply() + + self.assertTrue(any("does not support" in issue.message for issue in error.exception.issues)) + self.assertFalse(AuthzSchemaSource.objects.exists()) + self.assertFalse(AuthzRolePermission.objects.exists()) diff --git a/src/openedx_authz/tests/schema/factories.py b/src/openedx_authz/tests/schema/factories.py index b9a6d0b6..eac9bd0a 100644 --- a/src/openedx_authz/tests/schema/factories.py +++ b/src/openedx_authz/tests/schema/factories.py @@ -4,6 +4,7 @@ from openedx_authz.engine.schema.discovery import DiscoveredResource, Origin from openedx_authz.engine.schema.types import ( + ParsedSchemaDocument, PermissionCategory, PermissionDefinition, RoleDefinition, @@ -46,6 +47,11 @@ def make_document( ) +def make_parsed_document(data: object, name: str = "doc") -> ParsedSchemaDocument: + """Build a parsed document for structural validation tests.""" + return ParsedSchemaDocument(source=make_source(name), data=data) + + def category(cid: str = "cat", **kwargs) -> PermissionCategory: return PermissionCategory( id=cid, diff --git a/src/openedx_authz/tests/schema/test_loading.py b/src/openedx_authz/tests/schema/test_loading.py index 8dc1bf7d..f31b4417 100644 --- a/src/openedx_authz/tests/schema/test_loading.py +++ b/src/openedx_authz/tests/schema/test_loading.py @@ -8,11 +8,10 @@ from openedx_authz.engine.schema.discovery import DiscoveredResource, Origin, SchemaDiscovery from openedx_authz.engine.schema.exceptions import SchemaLoadError from openedx_authz.engine.schema.loading import SchemaLoader +from openedx_authz.engine.schema.parsing import UNKNOWN_DISTRIBUTION, SchemaParser from .factories import InMemoryResource -UNKNOWN_DISTRIBUTION = SchemaLoader._UNKNOWN_DISTRIBUTION # pylint: disable=protected-access - VALID_YAML = b""" schema_version: "1.0" priority: 150 @@ -48,7 +47,7 @@ def _load(contents: bytes): resource = InMemoryResource(contents, package="pkg", module="pkg.mod") - return SchemaLoader().load([resource]) + return SchemaLoader().load(SchemaParser().parse([resource])) class TestDocumentLoading: @@ -107,13 +106,10 @@ def test_empty_document_yields_empty_blocks(self): assert docs[0].categories == [] assert docs[0].roles == [] - def test_completely_empty_file_is_treated_as_an_empty_mapping(self): - """An empty file parses to ``None``; validation rejects it, loading must not.""" - docs = _load(b"") - - assert docs[0].priority == 0 - assert docs[0].source.schema_version == "" - assert docs[0].roles == [] + def test_completely_empty_file_cannot_be_loaded_as_a_typed_document(self): + """An empty root must pass structural validation before typed loading.""" + with pytest.raises(SchemaLoadError, match="must be a mapping"): + _load(b"") def test_invalid_yaml_raises_load_error(self): """Malformed YAML surfaces as ``SchemaLoadError``.""" @@ -164,7 +160,7 @@ def _distribution(name): def test_installed_package_resolves_to_its_distribution(self): """A module shipped by this package resolves to the real distribution.""" discovery = SchemaDiscovery(passed_in_directories=["openedx_authz/authz/schema"]) - docs = SchemaLoader().load(discovery.discover()) + docs = SchemaParser().parse(discovery.discover()) assert {doc.source.distribution for doc in docs} == {"openedx-authz"} assert all(doc.source.distribution_version != UNKNOWN_DISTRIBUTION for doc in docs) @@ -172,7 +168,7 @@ def test_installed_package_resolves_to_its_distribution(self): def test_module_is_recorded_as_the_dotted_path(self): """The source module is recorded as the resource's dotted import path.""" discovery = SchemaDiscovery(passed_in_directories=["openedx_authz/authz/schema"]) - docs = SchemaLoader().load(discovery.discover()) + docs = SchemaParser().parse(discovery.discover()) assert {doc.source.module for doc in docs} == {"openedx_authz.authz.schema"} @@ -376,9 +372,9 @@ def _clear_warned_distributions(self) -> None: Ensures test isolation and order-independence by resetting the dedupe set that prevents duplicate logging of the same ambiguity. """ - SchemaLoader._distribution_ambiguity_warned.clear() # pylint: disable=protected-access + SchemaParser.distribution_ambiguity_warned.clear() yield - SchemaLoader._distribution_ambiguity_warned.clear() # pylint: disable=protected-access + SchemaParser.distribution_ambiguity_warned.clear() def test_single_candidate_no_log( self, @@ -393,7 +389,7 @@ def test_single_candidate_no_log( origin=Origin.ENTRY_POINT, ) - result = SchemaLoader._select_owning_distribution(["dist1"], resource) # pylint: disable=protected-access + result = SchemaParser.select_owning_distribution(["dist1"], resource) assert result == "dist1" assert len(caplog.records) == 0 @@ -416,9 +412,9 @@ def test_unique_owner_no_log( def mock_ships(distribution: str, _installed_path: str) -> bool: return distribution == "dist1" - monkeypatch.setattr(SchemaLoader, "_distribution_ships", staticmethod(mock_ships)) + monkeypatch.setattr(SchemaParser, "distribution_ships", staticmethod(mock_ships)) - result = SchemaLoader._select_owning_distribution(["dist1", "dist2"], resource) # pylint: disable=protected-access + result = SchemaParser.select_owning_distribution(["dist1", "dist2"], resource) assert result == "dist1" assert len(caplog.records) == 0 @@ -438,9 +434,9 @@ def test_ambiguous_fallback_logs_once( ) # Mock _distribution_ships so no distribution claims the resource - monkeypatch.setattr(SchemaLoader, "_distribution_ships", staticmethod(lambda *args: False)) + monkeypatch.setattr(SchemaParser, "distribution_ships", staticmethod(lambda *args: False)) - SchemaLoader._select_owning_distribution(["dist2", "dist1"], resource) # pylint: disable=protected-access + SchemaParser.select_owning_distribution(["dist2", "dist1"], resource) # Should emit exactly one log record assert len(caplog.records) == 1 @@ -460,9 +456,9 @@ def test_log_message_includes_distribution( module="test_package.schema", origin=Origin.ENTRY_POINT, ) - monkeypatch.setattr(SchemaLoader, "_distribution_ships", staticmethod(lambda *args: False)) + monkeypatch.setattr(SchemaParser, "distribution_ships", staticmethod(lambda *args: False)) - result = SchemaLoader._select_owning_distribution(["dist2", "dist1"], resource) # pylint: disable=protected-access + result = SchemaParser.select_owning_distribution(["dist2", "dist1"], resource) assert result == "dist1" log_msg = caplog.records[0].message @@ -482,9 +478,9 @@ def test_log_extra_dict_preserved( module="test_package.schema", origin=Origin.ENTRY_POINT, ) - monkeypatch.setattr(SchemaLoader, "_distribution_ships", staticmethod(lambda *args: False)) + monkeypatch.setattr(SchemaParser, "distribution_ships", staticmethod(lambda *args: False)) - SchemaLoader._select_owning_distribution(["dist2", "dist1"], resource) # pylint: disable=protected-access + SchemaParser.select_owning_distribution(["dist2", "dist1"], resource) record = caplog.records[0] assert hasattr(record, "package") @@ -509,7 +505,7 @@ def test_same_ambiguity_logged_only_once( the same process. """ caplog.set_level(logging.INFO) - monkeypatch.setattr(SchemaLoader, "_distribution_ships", staticmethod(lambda *args: False)) + monkeypatch.setattr(SchemaParser, "distribution_ships", staticmethod(lambda *args: False)) resource = DiscoveredResource( package="test_package", @@ -520,7 +516,7 @@ def test_same_ambiguity_logged_only_once( # Call multiple times with the same resource for _ in range(3): - SchemaLoader._select_owning_distribution(["dist2", "dist1"], resource) # pylint: disable=protected-access + SchemaParser.select_owning_distribution(["dist2", "dist1"], resource) # Should emit exactly one log record despite three calls assert len(caplog.records) == 1 @@ -532,7 +528,7 @@ def test_different_resources_each_logged( ) -> None: """Different resources (even with same candidates) each log once.""" caplog.set_level(logging.INFO) - monkeypatch.setattr(SchemaLoader, "_distribution_ships", staticmethod(lambda *args: False)) + monkeypatch.setattr(SchemaParser, "distribution_ships", staticmethod(lambda *args: False)) resource1 = DiscoveredResource( package="test_package", @@ -547,8 +543,8 @@ def test_different_resources_each_logged( origin=Origin.ENTRY_POINT, ) - SchemaLoader._select_owning_distribution(["dist2", "dist1"], resource1) # pylint: disable=protected-access - SchemaLoader._select_owning_distribution(["dist2", "dist1"], resource2) # pylint: disable=protected-access + SchemaParser.select_owning_distribution(["dist2", "dist1"], resource1) + SchemaParser.select_owning_distribution(["dist2", "dist1"], resource2) # Should emit two log records (one per unique resource_path) assert len(caplog.records) == 2 @@ -565,11 +561,11 @@ def test_selection_logic_unchanged( origin=Origin.ENTRY_POINT, ) # Test with various orderings; should always return sorted[0] - monkeypatch.setattr(SchemaLoader, "_distribution_ships", staticmethod(lambda *args: False)) + monkeypatch.setattr(SchemaParser, "distribution_ships", staticmethod(lambda *args: False)) candidates = ["zebra", "apple", "banana"] - result = SchemaLoader._select_owning_distribution(candidates, resource) # pylint: disable=protected-access + result = SchemaParser.select_owning_distribution(candidates, resource) assert result == "apple" # sorted(candidates)[0] @@ -586,9 +582,9 @@ def test_log_level_is_info_not_warning( module="test_package.schema", origin=Origin.ENTRY_POINT, ) - monkeypatch.setattr(SchemaLoader, "_distribution_ships", staticmethod(lambda *args: False)) + monkeypatch.setattr(SchemaParser, "distribution_ships", staticmethod(lambda *args: False)) - SchemaLoader._select_owning_distribution(["dist2", "dist1"], resource) # pylint: disable=protected-access + SchemaParser.select_owning_distribution(["dist2", "dist1"], resource) assert len(caplog.records) == 1 record = caplog.records[0] @@ -607,7 +603,7 @@ def test_dedupe_key_includes_selected_distribution( identical fallbacks and thus deduplicate. """ caplog.set_level(logging.INFO) - monkeypatch.setattr(SchemaLoader, "_distribution_ships", staticmethod(lambda *args: False)) + monkeypatch.setattr(SchemaParser, "distribution_ships", staticmethod(lambda *args: False)) resource = DiscoveredResource( package="test_package", @@ -617,8 +613,8 @@ def test_dedupe_key_includes_selected_distribution( ) # Both orderings should select the same deterministic fallback - result1 = SchemaLoader._select_owning_distribution(["dist2", "dist1"], resource) # pylint: disable=protected-access - result2 = SchemaLoader._select_owning_distribution(["dist1", "dist2"], resource) # pylint: disable=protected-access + result1 = SchemaParser.select_owning_distribution(["dist2", "dist1"], resource) + result2 = SchemaParser.select_owning_distribution(["dist1", "dist2"], resource) assert result1 == result2 == "dist1" # Only one log record because the warn_key is identical diff --git a/src/openedx_authz/tests/schema/test_parsing.py b/src/openedx_authz/tests/schema/test_parsing.py new file mode 100644 index 00000000..62f44d52 --- /dev/null +++ b/src/openedx_authz/tests/schema/test_parsing.py @@ -0,0 +1,51 @@ +"""Tests for parsing schema resources without imposing document structure.""" + +from __future__ import annotations + +import hashlib + +import pytest + +from openedx_authz.engine.schema.exceptions import SchemaLoadError +from openedx_authz.engine.schema.parsing import SchemaParser +from openedx_authz.tests.schema.factories import InMemoryResource + + +@pytest.mark.parametrize( + ("contents", "expected"), + [ + (b"schema_version: '1.0'\npriority: 10\n", {"schema_version": "1.0", "priority": 10}), + (b"", None), + (b"value", "value"), + (b"- first\n- second\n", ["first", "second"]), + ], +) +def test_parser_preserves_yaml_value(contents, expected): + """The parser preserves each YAML root value for structural validation.""" + document = SchemaParser().parse([InMemoryResource(contents)])[0] + + assert document.data == expected + + +def test_parser_rejects_malformed_yaml(): + """Malformed YAML remains a parsing error compatible with SchemaLoadError.""" + resource = InMemoryResource(b"schema_version: '1.0'\n bad: [unclosed\n") + + with pytest.raises(SchemaLoadError, match="Invalid YAML"): + SchemaParser().parse([resource]) + + +def test_parser_records_provenance_and_exact_content_digest(monkeypatch): + """Parsed documents retain package provenance and hash the bytes read.""" + contents = b"schema_version: '1.0'\npriority: 10\n" + resource = InMemoryResource(contents, package="example", module="example.authz", resource_path="roles.yml") + monkeypatch.setattr("openedx_authz.engine.schema.parsing.metadata.packages_distributions", lambda: {}) + + document = SchemaParser().parse([resource])[0] + + assert document.source.distribution == "example" + assert document.source.distribution_version == "unknown" + assert document.source.module == "example.authz" + assert document.source.resource_path == "roles.yml" + assert document.source.schema_version == "1.0" + assert document.source.content_digest == hashlib.sha256(contents).hexdigest() diff --git a/src/openedx_authz/tests/schema/test_pipeline.py b/src/openedx_authz/tests/schema/test_pipeline.py index 948a7712..89657d3c 100644 --- a/src/openedx_authz/tests/schema/test_pipeline.py +++ b/src/openedx_authz/tests/schema/test_pipeline.py @@ -12,10 +12,10 @@ from openedx_authz.engine.schema.exceptions import SchemaValidationError from openedx_authz.engine.schema.pipeline import SchemaPipeline -from openedx_authz.engine.schema.validation import ValidationIssue +from openedx_authz.engine.schema.validation import IssueLevel, ValidationIssue -def _pipeline(*, issues=None, compiled_issues=None): +def _pipeline(*, syntax_issues=None, issues=None, compiled_issues=None): """Build a SchemaPipeline with every component mocked. ``issues`` seeds the document-level validator result and ``compiled_issues`` @@ -24,13 +24,18 @@ def _pipeline(*, issues=None, compiled_issues=None): discovery = mock.Mock(name="discovery") discovery.discover.return_value = ["resource"] + parser = mock.Mock(name="parser") + parser.parse.return_value = ["parsed-document"] + + syntax_validator = mock.Mock(name="syntax-validator") + syntax_validator.validate.return_value = syntax_issues or [] + loader = mock.Mock(name="loader") loader.load.return_value = ["document"] validator = mock.Mock(name="validator") validator.validate.return_value = issues or [] validator.validate_compiled.return_value = compiled_issues or [] - validator.has_errors.side_effect = lambda found: any(i.is_error for i in found) compiler = mock.Mock(name="compiler") compiler.compile.return_value = "compiled-schema" @@ -42,6 +47,8 @@ def _pipeline(*, issues=None, compiled_issues=None): pipeline = SchemaPipeline( discovery=discovery, + parser=parser, + syntax_validator=syntax_validator, loader=loader, validator=validator, compiler=compiler, @@ -50,6 +57,8 @@ def _pipeline(*, issues=None, compiled_issues=None): ) return pipeline, { "discovery": discovery, + "parser": parser, + "syntax_validator": syntax_validator, "loader": loader, "validator": validator, "compiler": compiler, @@ -62,14 +71,16 @@ class TestCompile: """Cover SchemaPipeline.compile step ordering and validation gating.""" def test_runs_steps_in_order_and_returns_compiled_schema(self): - """Compile runs discover -> load -> validate -> compile -> validate_compiled in order.""" + """Compile runs discover, parse, syntax, load, semantics, compile, and compiled validation in order.""" pipeline, m = _pipeline() result = pipeline.compile() assert result == "compiled-schema" m["discovery"].discover.assert_called_once_with() - m["loader"].load.assert_called_once_with(["resource"]) + m["parser"].parse.assert_called_once_with(["resource"]) + m["syntax_validator"].validate.assert_called_once_with(["parsed-document"]) + m["loader"].load.assert_called_once_with(["parsed-document"]) m["validator"].validate.assert_called_once_with(["document"]) m["compiler"].compile.assert_called_once_with(["document"]) m["validator"].validate_compiled.assert_called_once_with("compiled-schema") @@ -80,7 +91,7 @@ def test_raises_when_compiled_schema_has_errors(self): Extensions and priority resolution can only be checked after they are applied, so validation runs again post-compile. """ - error = ValidationIssue("error", "scope not supported", "src") + error = ValidationIssue(IssueLevel.ERROR, "scope not supported", "src") pipeline, m = _pipeline(compiled_issues=[error]) with pytest.raises(SchemaValidationError) as exc_info: @@ -89,9 +100,34 @@ def test_raises_when_compiled_schema_has_errors(self): assert exc_info.value.issues == [error] m["compiler"].compile.assert_called_once() + def test_structural_errors_stop_before_typed_loading(self): + """All structural findings stop loading and later lifecycle stages.""" + errors = [ + ValidationIssue(IssueLevel.ERROR, "bad first document", "first"), + ValidationIssue(IssueLevel.ERROR, "bad second document", "second"), + ] + pipeline, components = _pipeline(syntax_issues=errors) + + with pytest.raises(SchemaValidationError) as exc_info: + pipeline.compile() + + assert exc_info.value.issues == errors + components["loader"].load.assert_not_called() + components["validator"].validate.assert_not_called() + components["compiler"].compile.assert_not_called() + + def test_structural_warnings_do_not_stop_typed_loading(self): + """Structural warnings allow typed loading and later lifecycle stages.""" + warning = ValidationIssue(IssueLevel.WARNING, "heads up", "src") + pipeline, components = _pipeline(syntax_issues=[warning]) + + assert pipeline.compile() == "compiled-schema" + components["loader"].load.assert_called_once_with(["parsed-document"]) + components["validator"].validate.assert_called_once_with(["document"]) + def test_compiled_errors_stop_before_render_and_apply(self): """A post-compile error aborts apply before rendering or applying.""" - error = ValidationIssue("error", "scope not supported", "src") + error = ValidationIssue(IssueLevel.ERROR, "scope not supported", "src") pipeline, m = _pipeline(compiled_issues=[error]) with pytest.raises(SchemaValidationError): @@ -102,14 +138,14 @@ def test_compiled_errors_stop_before_render_and_apply(self): def test_compiled_warnings_do_not_stop_compilation(self): """A post-compile warning is non-fatal; compilation still returns the schema.""" - warning = ValidationIssue("warning", "heads up", "src") + warning = ValidationIssue(IssueLevel.WARNING, "heads up", "src") pipeline, _ = _pipeline(compiled_issues=[warning]) assert pipeline.compile() == "compiled-schema" def test_document_errors_skip_the_compiled_check(self): """A failed first gate must not reach the second one.""" - error = ValidationIssue("error", "boom", "src") + error = ValidationIssue(IssueLevel.ERROR, "boom", "src") pipeline, m = _pipeline(issues=[error]) with pytest.raises(SchemaValidationError): @@ -119,7 +155,7 @@ def test_document_errors_skip_the_compiled_check(self): def test_raises_when_validation_has_errors(self): """A document-level validation error stops before compilation runs.""" - error = ValidationIssue("error", "boom", "src") + error = ValidationIssue(IssueLevel.ERROR, "boom", "src") pipeline, m = _pipeline(issues=[error]) with pytest.raises(SchemaValidationError) as exc_info: @@ -130,9 +166,20 @@ def test_raises_when_validation_has_errors(self): # Compilation must not run once validation fails. m["compiler"].compile.assert_not_called() + def test_exception_contains_only_errors(self): + """The validation exception excludes warnings from a mixed result.""" + warning = ValidationIssue(IssueLevel.WARNING, "heads up", "src") + error = ValidationIssue(IssueLevel.ERROR, "boom", "src") + pipeline, _ = _pipeline(issues=[warning, error]) + + with pytest.raises(SchemaValidationError) as exc_info: + pipeline.compile() + + assert exc_info.value.issues == [error] + def test_warning_only_issues_do_not_stop_compilation(self): """A document-level warning is non-fatal; compilation still proceeds.""" - warning = ValidationIssue("warning", "heads up", "src") + warning = ValidationIssue(IssueLevel.WARNING, "heads up", "src") pipeline, m = _pipeline(issues=[warning]) result = pipeline.compile() @@ -193,6 +240,8 @@ def test_default_components_are_constructed_when_not_injected(self): # only assert the orchestrator is fully constructed. # pylint: disable=protected-access assert pipeline._discovery is not None + assert pipeline._parser is not None + assert pipeline._syntax_validator is not None assert pipeline._loader is not None assert pipeline._validator is not None assert pipeline._compiler is not None diff --git a/src/openedx_authz/tests/schema/test_syntax.py b/src/openedx_authz/tests/schema/test_syntax.py new file mode 100644 index 00000000..56329671 --- /dev/null +++ b/src/openedx_authz/tests/schema/test_syntax.py @@ -0,0 +1,225 @@ +"""Tests for structural validation against the configured JSON Schema.""" + +import json +from unittest.mock import patch + +import pytest +from django.test import override_settings + +from openedx_authz.engine.schema.discovery import SchemaDiscovery +from openedx_authz.engine.schema.parsing import SchemaParser +from openedx_authz.engine.schema.syntax import SchemaSyntaxValidator, document_validator + +from .factories import make_parsed_document + + +def schema_data(block: str, definitions: list[object]) -> dict: + """Build raw schema data containing one definition block.""" + return {"schema_version": "1.0", "priority": 100, block: definitions} + + +def category_data(category_id: str = "cat", *, icon: object | None = None) -> dict: + """Build parsed data for one permission category.""" + definition = {"id": category_id, "display_name": "Category", "description": "Description."} + if icon is not None: + definition["icon"] = icon + return schema_data("permission_categories", [definition]) + + +def permission_data(*, namespace: str = "courses", category_id: str = "cat", scopes=None) -> dict: + """Build parsed data for one permission.""" + return schema_data( + "permissions", + [ + { + "namespace": namespace, + "name": "view_course", + "display_name": "View course", + "description": "View a course.", + "category_id": category_id, + "scopes": ["course-v1"] if scopes is None else scopes, + } + ], + ) + + +def role_data(*, role_id: str = "course_editor", scopes=None, permissions=None) -> dict: + """Build parsed data for one role.""" + return schema_data( + "roles", + [ + { + "id": role_id, + "display_name": "Course editor", + "description": "Edit a course.", + "scopes": ["course-v1"] if scopes is None else scopes, + "permissions": ["courses.view_course"] if permissions is None else permissions, + } + ], + ) + + +def validation_messages(data: object) -> list[str]: + """Return structural error messages for one parsed value.""" + return [issue.message for issue in SchemaSyntaxValidator().validate([make_parsed_document(data)])] + + +class TestSchemaSyntaxValidator: + """JSON Schema owns validation of parsed document structure.""" + + @override_settings(OPENEDX_AUTHZ_JSON_SCHEMA="custom/schema.json") + def test_uses_configured_schema_resource(self): + """The validator resolves its schema from the central Django setting.""" + with patch("openedx_authz.engine.schema.syntax.document_validator") as get_validator: + get_validator.return_value.iter_errors.return_value = [] + SchemaSyntaxValidator().validate([make_parsed_document({})]) + + get_validator.assert_called_once_with("custom/schema.json") + + def test_schema_resource_is_loaded_for_each_validator_request(self): + """Schema loading has no process-global cache or configuration state.""" + with patch("openedx_authz.engine.schema.syntax.resources.files") as schema_file: + schema_file.return_value.joinpath.return_value.read_text.return_value = json.dumps( + {"$schema": "https://json-schema.org/draft/2020-12/schema", "type": "object"} + ) + + document_validator("uncached_package/schema.json") + document_validator("uncached_package/schema.json") + + assert schema_file.call_count == 2 + + @pytest.mark.parametrize("data", [None, "value", ["value"]]) + def test_rejects_non_mapping_roots(self, data): + """Empty, scalar, and list roots are reported as structural errors.""" + assert SchemaSyntaxValidator().validate([make_parsed_document(data)]) + + def test_collects_errors_from_every_document_with_source_attribution(self): + """One pass returns findings from every invalid parsed document.""" + documents = [ + make_parsed_document({"schema_version": "1.0"}, "first"), + make_parsed_document({"schema_version": "1.0", "unexpected": True}, "second"), + ] + + issues = SchemaSyntaxValidator().validate(documents) + + assert {issue.source_id for issue in issues} == {document.source.source_id for document in documents} + assert all(issue.message.startswith("$") for issue in issues) + + def test_unsupported_schema_version_is_error(self): + """A ``schema_version`` the validator cannot read is rejected.""" + messages = validation_messages({"schema_version": "9.9", "priority": 100, "roles": []}) + + assert any("schema_version" in message and "1.0" in message for message in messages) + + def test_non_snakecase_identifier_is_error(self): + """Identifiers must be lowercase snake_case.""" + assert validation_messages(permission_data(namespace="Courses")) + + def test_casbin_internal_form_rejected(self): + """An identifier using an internal Casbin form (``act^``) is rejected.""" + assert validation_messages(category_data("act^category")) + + def test_missing_scope_is_error(self): + """A permission must declare at least one scope.""" + messages = validation_messages(permission_data(scopes=[])) + + assert any("non-empty" in message for message in messages) + + @pytest.mark.parametrize( + "permission_id", ["courses", "courses.view.course", "courses.", ".view", "courses..view", ""] + ) + def test_permission_id_must_be_namespace_dot_name(self, permission_id): + """A permission id must be exactly two identifiers joined by one period.""" + assert validation_messages(role_data(permissions=[permission_id])) + + def test_permission_id_halves_are_validated(self): + """Both halves of a permission id must be lowercase snake_case.""" + assert validation_messages(role_data(permissions=["Courses.View_Course"])) + + @pytest.mark.parametrize("prefix", ["act^", "role^", "sub^", "scope^", "g^", "p^"]) + def test_every_casbin_prefix_is_rejected(self, prefix): + """Every reserved Casbin prefix (act^, role^, ...) is rejected in an id.""" + assert validation_messages(role_data(role_id=f"{prefix}thing")) + + @pytest.mark.parametrize("scope", ["Course-V1", "1course", "course v1", "course.v1"]) + def test_invalid_scope_namespace_is_error(self, scope): + """A malformed scope namespace (caps, leading digit, spaces, dots) is rejected.""" + assert validation_messages(permission_data(scopes=[scope])) + + def test_hyphenated_scope_is_allowed(self): + """Scope namespaces keep their registered spelling, e.g. ``course-v1``.""" + assert not validation_messages(permission_data(scopes=["course-v1"])) + + def test_role_without_scopes_is_error(self): + """A role must declare at least one scope.""" + assert validation_messages(role_data(scopes=[])) + + def test_empty_category_id_is_error(self): + """A category with an empty id is a missing-required-field error.""" + assert validation_messages(category_data("")) + + def test_permission_without_a_category_is_error(self): + """A permission missing its required category is an error.""" + assert validation_messages(permission_data(category_id="")) + + @pytest.mark.parametrize("icon", ["remove_red_eye", "removeRedEye", "Remove-Red-Eye", "9Mp lowercase"]) + def test_non_pascalcase_icon_is_error(self, icon): + """An icon name that is not PascalCase is rejected before the export check.""" + assert validation_messages(category_data(icon=icon)) + + def test_non_string_icon_is_error(self): + """Icon values must be strings before semantic membership validation.""" + messages = validation_messages(category_data(icon=123)) + + assert any("string" in message for message in messages) + + @pytest.mark.parametrize( + "data", + [ + {"schema_version": 1.0, "priority": 100}, + {"schema_version": "1.0", "priority": "100"}, + schema_data("permission_categories", [5]), + schema_data("permission_categories", [{"id": "act^category"}]), + schema_data("permission_categories", [{"id": ""}]), + schema_data("permissions", [{"namespace": "Courses"}]), + schema_data( + "permissions", + [ + { + "namespace": "pipeline", + "name": "view", + "display_name": "View", + "description": "View content.", + "category_id": "", + "scopes": [], + } + ], + ), + schema_data("roles", [{"id": "role^editor"}]), + schema_data( + "roles", + [ + { + "id": "editor", + "display_name": "Editor", + "description": "Edit content.", + "scopes": ["Course-V1"], + "permissions": ["pipeline"], + } + ], + ), + schema_data("permission_categories", [{"id": "cat", "icon": "remove_eye"}]), + schema_data("permission_categories", [{"id": "cat", "icon": 123}]), + ], + ) + def test_schema_preserves_python_structural_checks(self, data): + """The schema rejects representative types, formats, and required fields.""" + assert SchemaSyntaxValidator().validate([make_parsed_document(data)]) + + def test_shipped_documents_match_the_published_schema(self): + """Every authorization document shipped by this package is structurally valid.""" + resources = SchemaDiscovery(passed_in_directories=["openedx_authz/authz/schema"]).discover() + documents = SchemaParser().parse(resources) + + assert documents + assert not SchemaSyntaxValidator().validate(documents) diff --git a/src/openedx_authz/tests/schema/test_types.py b/src/openedx_authz/tests/schema/test_types.py index 90f9748e..359ba93b 100644 --- a/src/openedx_authz/tests/schema/test_types.py +++ b/src/openedx_authz/tests/schema/test_types.py @@ -8,8 +8,10 @@ from openedx_authz.engine.schema.types import ( CompiledDefinition, CompiledSchema, + PermissionCategory, PermissionDefinition, RoleDefinition, + SchemaDocument, SourceRecord, ) @@ -41,6 +43,16 @@ def test_permission_identifier_joins_namespace_and_name(self): ) assert perm.identifier == "courses.view_course" + def test_schema_document_keeps_typed_definition_fields(self): + """A loaded document contains typed definitions without raw parsed data.""" + source = SourceRecord("dist", "1.0", "pkg", "schema.yaml", "1.0", "digest") + category = PermissionCategory("cat", "Category", "Description") + + document = SchemaDocument(source, 100, [category]) + + assert document.categories == [category] + assert not hasattr(document, "data") + def test_role_permission_pairs_are_sorted_and_flattened(self): """``role_permission_pairs`` returns ``(role, permission)`` pairs sorted deterministically.""" role = RoleDefinition( diff --git a/src/openedx_authz/tests/schema/test_validation.py b/src/openedx_authz/tests/schema/test_validation.py index 790b06a9..5ff2f144 100644 --- a/src/openedx_authz/tests/schema/test_validation.py +++ b/src/openedx_authz/tests/schema/test_validation.py @@ -1,19 +1,14 @@ """Unit tests for the schema validation step. Grouped by concern: per-document validation, duplicate severity, identifier and -scope rules, required fields, extension references, post-compile validation, and -the ``has_errors`` gate the pipeline uses to decide whether to stop. +scope rules, required fields, extension references, and post-compile validation. """ import pytest from openedx_authz.engine.schema.compilation import SchemaCompiler from openedx_authz.engine.schema.paragon_icons import PARAGON_ICON_NAMES -from openedx_authz.engine.schema.validation import ( - IssueLevel, - SchemaValidator, - ValidationIssue, -) +from openedx_authz.engine.schema.validation import SchemaValidator from .factories import category, extension, make_document, make_source, permission, role @@ -43,23 +38,6 @@ def test_valid_document_has_no_errors(self): ) assert not _errors(SchemaValidator().validate([doc])) - def test_unsupported_schema_version_is_error(self): - """A ``schema_version`` the validator cannot read is rejected.""" - doc = make_document(schema_version="9.9", categories=[category()]) - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - assert any("Unsupported schema_version" in m for m in messages) - - def test_non_snakecase_identifier_is_error(self): - """Identifiers must be lowercase snake_case.""" - doc = make_document(permissions=[permission(namespace="Courses")]) - assert _errors(SchemaValidator().validate([doc])) - - def test_casbin_internal_form_rejected(self): - """An identifier using an internal Casbin form (``act^``) is rejected.""" - doc = make_document(categories=[category("act^foo")]) - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - assert any("internal Casbin form" in m for m in messages) - def test_unknown_category_reference_is_error(self): """A permission referencing a category not defined in the schema fails.""" doc = make_document(permissions=[permission(cat="missing")]) @@ -88,15 +66,6 @@ def test_extension_targeting_unknown_role_is_error(self): messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] assert any("unknown role" in m for m in messages) - def test_missing_scope_is_error(self): - """A permission must declare at least one scope.""" - doc = make_document( - categories=[category("cat")], - permissions=[permission(cat="cat", scopes=())], - ) - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - assert any("at least one scope" in m for m in messages) - def test_conflicting_duplicate_definition_is_error(self): """Two definitions with the same id but different content conflict.""" doc = make_document( @@ -151,89 +120,6 @@ def test_warning_names_the_second_source(self): assert [i.source_id for i in issues] == [make_source("second").source_id] -class TestIdentifierAndScopeRules: - """Field-shape rules from ADR 0017 §4.""" - - @pytest.mark.parametrize("value", ["courses", "courses.view.course", "courses.", ".view", "courses..view", ""]) - def test_permission_id_must_be_namespace_dot_name(self, value): - """A permission id must be exactly two identifiers joined by one period.""" - doc = make_document(roles=[role(permissions=(value,))]) - - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - - assert any("permission id" in m and "must match" in m for m in messages) - - def test_permission_id_halves_are_validated(self): - """Both halves of a permission id must be lowercase snake_case.""" - doc = make_document(roles=[role(permissions=("Courses.View_Course",))]) - - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - - assert any("lowercase snake_case" in m for m in messages) - - @pytest.mark.parametrize("prefix", ["act^", "role^", "sub^", "scope^", "g^", "p^"]) - def test_every_casbin_prefix_is_rejected(self, prefix): - """Every reserved Casbin prefix (act^, role^, ...) is rejected in an id.""" - doc = make_document(roles=[role(rid=f"{prefix}thing")]) - - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - - assert any("internal Casbin form" in m for m in messages) - - @pytest.mark.parametrize("scope", ["Course-V1", "1course", "course v1", "course.v1"]) - def test_invalid_scope_namespace_is_error(self, scope): - """A malformed scope namespace (caps, leading digit, spaces, dots) is rejected.""" - doc = make_document( - categories=[category("cat")], - permissions=[permission(cat="cat", scopes=(scope,))], - ) - - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - - assert any("invalid scope namespace" in m for m in messages) - - def test_hyphenated_scope_is_allowed(self): - """Scope namespaces keep their registered spelling, e.g. ``course-v1``.""" - doc = make_document( - categories=[category("cat")], - permissions=[permission(cat="cat", scopes=("course-v1",))], - ) - - assert not _errors(SchemaValidator().validate([doc])) - - def test_role_without_scopes_is_error(self): - """A role must declare at least one scope.""" - doc = make_document(roles=[role(scopes=())]) - - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - - assert any("at least one scope" in m for m in messages) - - -class TestRequiredFields: - """The subset of required fields the validator enforces today. - - Fuller required-field coverage (display fields, unknown keys, sizes) arrives - with JSON Schema validation; these pin the rules already in place. - """ - - def test_empty_category_id_is_error(self): - """A category with an empty id is a missing-required-field error.""" - doc = make_document(categories=[category("")]) - - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - - assert any("Missing required field: category id" in m for m in messages) - - def test_permission_without_a_category_is_error(self): - """A permission missing its required category is an error.""" - doc = make_document(permissions=[permission(cat="")]) - - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - - assert any("Missing required field: category for courses.view_course" in m for m in messages) - - class TestExtensionReferences: """ADR 0023 §3: an extension must target a real role and real permissions.""" @@ -455,15 +341,6 @@ def test_missing_icon_is_allowed(self, icon): assert not _errors(SchemaValidator().validate([doc])) - @pytest.mark.parametrize("icon", ["remove_red_eye", "removeRedEye", "Remove-Red-Eye", "9Mp lowercase"]) - def test_non_pascalcase_icon_is_error(self, icon): - """An icon name that is not PascalCase is rejected before the export check.""" - doc = make_document(categories=[category("cat", icon=icon)]) - - messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - - assert any("must be a PascalCase Paragon icon name" in m for m in messages) - def test_unknown_but_wellformed_icon_is_error(self): """A PascalCase name that Paragon does not export is rejected.""" doc = make_document(categories=[category("cat", icon="NotARealParagonIcon")]) @@ -476,12 +353,15 @@ def test_permission_icon_is_validated(self): """A permission's icon is validated and the error names the permission.""" doc = make_document( categories=[category("cat")], - permissions=[permission(cat="cat", icon="totally_wrong")], + permissions=[permission(cat="cat", icon="NotARealParagonIcon")], ) messages = [i.message for i in _errors(SchemaValidator().validate([doc]))] - assert any("permission courses.view_course" in m and "PascalCase" in m for m in messages) + assert any( + "permission courses.view_course" in message and "not a valid @openedx/paragon/icons" in message + for message in messages + ) def test_role_icon_is_validated(self): """A role's icon is validated and the error names the role.""" @@ -499,26 +379,8 @@ def test_role_extension_icon_is_validated(self): permissions=[permission(cat="cat")], roles=[role()], ) - ext = make_document("ext", role_extensions=[extension("course_editor", icon="not_valid")]) + ext = make_document("ext", role_extensions=[extension("course_editor", icon="NotARealParagonIcon")]) messages = [i.message for i in _errors(SchemaValidator().validate([base, ext]))] assert any("role_extension course_editor" in m for m in messages) - - -class TestHasErrors: - """The gate the pipeline uses to decide whether to stop.""" - - def test_true_when_any_issue_is_an_error(self): - """``has_errors`` is True when any issue is error-level.""" - issues = [ValidationIssue(IssueLevel.WARNING, "heads up"), ValidationIssue(IssueLevel.ERROR, "boom")] - - assert SchemaValidator.has_errors(issues) is True - - def test_false_for_warnings_only(self): - """``has_errors`` is False when every issue is a warning.""" - assert SchemaValidator.has_errors([ValidationIssue(IssueLevel.WARNING, "heads up")]) is False - - def test_false_for_no_issues(self): - """``has_errors`` is False for an empty issue list.""" - assert SchemaValidator.has_errors([]) is False diff --git a/uv.lock b/uv.lock index d83815b3..b6494161 100644 --- a/uv.lock +++ b/uv.lock @@ -1515,6 +1515,7 @@ dependencies = [ { name = "edx-drf-extensions" }, { name = "edx-opaque-keys" }, { name = "edx-organizations" }, + { name = "jsonschema" }, { name = "openedx-atlas" }, { name = "openedx-events" }, { name = "openedx-filters" }, @@ -1607,6 +1608,7 @@ requires-dist = [ { name = "edx-drf-extensions" }, { name = "edx-opaque-keys" }, { name = "edx-organizations" }, + { name = "jsonschema", specifier = ">=4.0" }, { name = "openedx-atlas" }, { name = "openedx-events" }, { name = "openedx-filters", specifier = ">=3.15.0" },