diff --git a/CHANGELOG.rst b/CHANGELOG.rst index cc5c1324..6d6c355a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -51,6 +51,12 @@ Added ``openedx_authz/engine/schema/paragon_icons.py`` and regenerated with ``make paragon_icons`` (ADR 0029). +Changed +======== + +* Removed ``authz.policy`` and Casbin action grouping. Built-in roles list every permission explicitly in the authorization schema, so the existing ``g2`` rows did not change their access. Migration ``0012_remove_action_grouping`` removes stored ``g2`` rows from existing databases. +* Calls to the deprecated ``load_policies`` command without an explicit policy file are now a no-op while deployment integrations move to ``load_authz_schema``. + Notes ===== diff --git a/README.rst b/README.rst index 71799be0..9388d493 100644 --- a/README.rst +++ b/README.rst @@ -18,7 +18,7 @@ Integration with edx-platform This repository became an edx-platform's dependency starting with the Ulmo release. From that release onwards, system policies are automatically updated. -If you need to update the policies manually, it is recommended to use the ``./manage.py lms load_policies`` command. +To update the static authorization definitions manually, run ``./manage.py lms load_authz_schema``. .. note:: Currently, this package only supports the `content libraries' roles and permissions as documented here`_, and the migration of data from the old system to the new one is performed automatically. diff --git a/docs/decisions/0022-defer-authorization-implications.rst b/docs/decisions/0022-defer-authorization-implications.rst index 8f67ec84..3438d845 100644 --- a/docs/decisions/0022-defer-authorization-implications.rst +++ b/docs/decisions/0022-defer-authorization-implications.rst @@ -45,6 +45,13 @@ Consequences * The current policy loader continues to handle existing Casbin grouping rows. * Introducing implication later requires a schema-version change and migration guidance. +Addendum: Remove Existing Action Grouping (2026-10-16) +====================================================== + +The built-in roles list every permission covered by the existing ``g2`` action-grouping rows, so those rows do not change their effective permissions. The Casbin model now uses exact action matches and a data migration removes stored ``g2`` rows. + +Permission grouping and role inheritance remain future work in `Issue #91`_ and `Issue #523`_. Any later implementation still requires the schema-version change and migration guidance described in this ADR. + Rejected Alternatives ********************* @@ -62,5 +69,9 @@ References ********** * `ADR 0017`_ +* `Issue #91`_ +* `Issue #523`_ .. _ADR 0017: 0017-static-authorization-schema.rst +.. _Issue #91: https://github.com/openedx/openedx-authz/issues/91 +.. _Issue #523: https://github.com/openedx/openedx-authz/issues/523 diff --git a/src/openedx_authz/authz/schema/course_permissions.yaml b/src/openedx_authz/authz/schema/course_permissions.yaml index bf168b94..c34fadb0 100644 --- a/src/openedx_authz/authz/schema/course_permissions.yaml +++ b/src/openedx_authz/authz/schema/course_permissions.yaml @@ -56,7 +56,7 @@ permission_categories: description: Additional permissions not included in other categories, such as viewing checklists. icon: DrawShapes # Backend-only category for legacy compatibility actions that have no frontend - # display metadata. These are referenced by legacy roles in authz.policy. + # display metadata. These are referenced by legacy roles in course_roles.yaml. - id: course_legacy display_name: Legacy compatibility description: Internal compatibility permissions that map legacy course roles to the authorization system. Not shown as individually assignable permissions. diff --git a/src/openedx_authz/engine/config/authz.policy b/src/openedx_authz/engine/config/authz.policy deleted file mode 100644 index 9172df30..00000000 --- a/src/openedx_authz/engine/config/authz.policy +++ /dev/null @@ -1,190 +0,0 @@ -############################################ -# Open edX AuthZ — Casbin Policy Configuration -# -# This file defines policies that work with the model configuration. -# Uses namespaced subjects, actions, and scopes for maximum flexibility. -############################################ - -# Policy definitions - format: p = subject(role), action, scope, effect -# For role definitions use: lib^*, course^*, org^* to specify the scope of the role - -# Library Admin Role Policies -p, role^library_admin, act^content_libraries.view_library, lib^*, allow -p, role^library_admin, act^content_libraries.manage_library_tags, lib^*, allow -p, role^library_admin, act^content_libraries.delete_library, lib^*, allow -p, role^library_admin, act^content_libraries.edit_library_content, lib^*, allow -p, role^library_admin, act^content_libraries.publish_library_content, lib^*, allow -p, role^library_admin, act^content_libraries.reuse_library_content, lib^*, allow -p, role^library_admin, act^content_libraries.view_library_team, lib^*, allow -p, role^library_admin, act^content_libraries.manage_library_team, lib^*, allow -p, role^library_admin, act^content_libraries.create_library_collection, lib^*, allow -p, role^library_admin, act^content_libraries.edit_library_collection, lib^*, allow -p, role^library_admin, act^content_libraries.delete_library_collection, lib^*, allow - -# Library Author Role Policies -p, role^library_author, act^content_libraries.view_library, lib^*, allow -p, role^library_author, act^content_libraries.manage_library_tags, lib^*, allow -p, role^library_author, act^content_libraries.edit_library_content, lib^*, allow -p, role^library_author, act^content_libraries.publish_library_content, lib^*, allow -p, role^library_author, act^content_libraries.reuse_library_content, lib^*, allow -p, role^library_author, act^content_libraries.view_library_team, lib^*, allow -p, role^library_author, act^content_libraries.create_library_collection, lib^*, allow -p, role^library_author, act^content_libraries.edit_library_collection, lib^*, allow -p, role^library_author, act^content_libraries.delete_library_collection, lib^*, allow - -# Library Contributor Role Policies -p, role^library_contributor, act^content_libraries.view_library, lib^*, allow -p, role^library_contributor, act^content_libraries.manage_library_tags, lib^*, allow -p, role^library_contributor, act^content_libraries.edit_library_content, lib^*, allow -p, role^library_contributor, act^content_libraries.reuse_library_content, lib^*, allow -p, role^library_contributor, act^content_libraries.view_library_team, lib^*, allow -p, role^library_contributor, act^content_libraries.create_library_collection, lib^*, allow -p, role^library_contributor, act^content_libraries.edit_library_collection, lib^*, allow -p, role^library_contributor, act^content_libraries.delete_library_collection, lib^*, allow - -# Library User Role Policies -p, role^library_user, act^content_libraries.view_library, lib^*, allow -p, role^library_user, act^content_libraries.reuse_library_content, lib^*, allow -p, role^library_user, act^content_libraries.view_library_team, lib^*, allow - -# Action Inheritance (g2) - format: g2 = granted_action, implied_action -# Higher-level permissions automatically grant lower-level permissions -# If a user has the granted_action, they also have the implied_action -# Example: g2, act^content_libraries.delete_library, act^content_libraries.view_library means delete permission includes view permission -# Library -g2, act^content_libraries.manage_library_tags, act^content_libraries.edit_library_content -g2, act^content_libraries.delete_library, act^content_libraries.edit_library_content - -# Content -g2, act^content_libraries.publish_library_content, act^content_libraries.edit_library_content -g2, act^content_libraries.edit_library_content, act^content_libraries.view_library -g2, act^content_libraries.reuse_library_content, act^content_libraries.view_library -g2, act^content_libraries.publish_library_content, act^content_libraries.view_library - -# Team -g2, act^content_libraries.manage_library_team, act^content_libraries.view_library_team - -# Collections -g2, act^content_libraries.delete_library_collection, act^content_libraries.edit_library_collection -g2, act^content_libraries.create_library_collection, act^content_libraries.edit_library_collection -g2, act^content_libraries.edit_library_collection, act^content_libraries.view_library - - -## Course Policies - -# Course Auditor Role Policies -p, role^course_auditor, act^courses.view_course, course-v1^*, allow -p, role^course_auditor, act^courses.view_course_updates, course-v1^*, allow -p, role^course_auditor, act^courses.view_pages_and_resources, course-v1^*, allow -p, role^course_auditor, act^courses.view_files, course-v1^*, allow -p, role^course_auditor, act^courses.view_grading_settings, course-v1^*, allow -p, role^course_auditor, act^courses.view_checklists, course-v1^*, allow -p, role^course_auditor, act^courses.view_course_team, course-v1^*, allow -p, role^course_auditor, act^courses.view_schedule_and_details, course-v1^*, allow -p, role^course_auditor, act^courses.view_advanced_settings, course-v1^*, allow -p, role^course_auditor, act^courses.view_certificates, course-v1^*, allow -p, role^course_auditor, act^courses.view_group_configurations, course-v1^*, allow -p, role^course_auditor, act^courses.view_library_updates, course-v1^*, allow - -# Course Editor Role Policies -p, role^course_editor, act^courses.view_course, course-v1^*, allow -p, role^course_editor, act^courses.view_course_updates, course-v1^*, allow -p, role^course_editor, act^courses.view_pages_and_resources, course-v1^*, allow -p, role^course_editor, act^courses.view_files, course-v1^*, allow -p, role^course_editor, act^courses.view_grading_settings, course-v1^*, allow -p, role^course_editor, act^courses.view_checklists, course-v1^*, allow -p, role^course_editor, act^courses.view_course_team, course-v1^*, allow -p, role^course_editor, act^courses.view_schedule_and_details, course-v1^*, allow -p, role^course_editor, act^courses.view_advanced_settings, course-v1^*, allow -p, role^course_editor, act^courses.view_certificates, course-v1^*, allow -p, role^course_editor, act^courses.view_group_configurations, course-v1^*, allow -p, role^course_editor, act^courses.view_library_updates, course-v1^*, allow -p, role^course_editor, act^courses.edit_course_content, course-v1^*, allow -p, role^course_editor, act^courses.manage_library_updates, course-v1^*, allow -p, role^course_editor, act^courses.manage_course_updates, course-v1^*, allow -p, role^course_editor, act^courses.manage_pages_and_resources, course-v1^*, allow -p, role^course_editor, act^courses.create_files, course-v1^*, allow -p, role^course_editor, act^courses.edit_files, course-v1^*, allow -p, role^course_editor, act^courses.edit_grading_settings, course-v1^*, allow -p, role^course_editor, act^courses.manage_group_configurations, course-v1^*, allow -p, role^course_editor, act^courses.edit_details, course-v1^*, allow -p, role^course_editor, act^courses.manage_tags, course-v1^*, allow - -# Course Staff Role Policies -p, role^course_staff, act^courses.legacy_staff_role_permissions, course-v1^*, allow -p, role^course_staff, act^courses.view_course, course-v1^*, allow -p, role^course_staff, act^courses.view_course_updates, course-v1^*, allow -p, role^course_staff, act^courses.view_pages_and_resources, course-v1^*, allow -p, role^course_staff, act^courses.view_files, course-v1^*, allow -p, role^course_staff, act^courses.view_grading_settings, course-v1^*, allow -p, role^course_staff, act^courses.view_checklists, course-v1^*, allow -p, role^course_staff, act^courses.view_course_team, course-v1^*, allow -p, role^course_staff, act^courses.view_schedule_and_details, course-v1^*, allow -p, role^course_staff, act^courses.view_advanced_settings, course-v1^*, allow -p, role^course_staff, act^courses.view_certificates, course-v1^*, allow -p, role^course_staff, act^courses.view_group_configurations, course-v1^*, allow -p, role^course_staff, act^courses.view_library_updates, course-v1^*, allow -p, role^course_staff, act^courses.edit_course_content, course-v1^*, allow -p, role^course_staff, act^courses.manage_library_updates, course-v1^*, allow -p, role^course_staff, act^courses.manage_course_updates, course-v1^*, allow -p, role^course_staff, act^courses.manage_pages_and_resources, course-v1^*, allow -p, role^course_staff, act^courses.create_files, course-v1^*, allow -p, role^course_staff, act^courses.edit_files, course-v1^*, allow -p, role^course_staff, act^courses.edit_grading_settings, course-v1^*, allow -p, role^course_staff, act^courses.manage_group_configurations, course-v1^*, allow -p, role^course_staff, act^courses.edit_details, course-v1^*, allow -p, role^course_staff, act^courses.manage_tags, course-v1^*, allow -p, role^course_staff, act^courses.publish_course_content, course-v1^*, allow -p, role^course_staff, act^courses.delete_files, course-v1^*, allow -p, role^course_staff, act^courses.edit_schedule, course-v1^*, allow -p, role^course_staff, act^courses.manage_advanced_settings, course-v1^*, allow -p, role^course_staff, act^courses.manage_certificates, course-v1^*, allow -p, role^course_staff, act^courses.import_course, course-v1^*, allow -p, role^course_staff, act^courses.export_course, course-v1^*, allow -p, role^course_staff, act^courses.export_tags, course-v1^*, allow - - -# Course Admin Role Policies -p, role^course_admin, act^courses.legacy_instructor_role_permissions, course-v1^*, allow -p, role^course_admin, act^courses.view_course, course-v1^*, allow -p, role^course_admin, act^courses.view_course_updates, course-v1^*, allow -p, role^course_admin, act^courses.view_pages_and_resources, course-v1^*, allow -p, role^course_admin, act^courses.view_files, course-v1^*, allow -p, role^course_admin, act^courses.view_grading_settings, course-v1^*, allow -p, role^course_admin, act^courses.view_checklists, course-v1^*, allow -p, role^course_admin, act^courses.view_course_team, course-v1^*, allow -p, role^course_admin, act^courses.view_schedule_and_details, course-v1^*, allow -p, role^course_admin, act^courses.view_advanced_settings, course-v1^*, allow -p, role^course_admin, act^courses.view_certificates, course-v1^*, allow -p, role^course_admin, act^courses.view_group_configurations, course-v1^*, allow -p, role^course_admin, act^courses.view_library_updates, course-v1^*, allow -p, role^course_admin, act^courses.edit_course_content, course-v1^*, allow -p, role^course_admin, act^courses.manage_library_updates, course-v1^*, allow -p, role^course_admin, act^courses.manage_course_updates, course-v1^*, allow -p, role^course_admin, act^courses.manage_pages_and_resources, course-v1^*, allow -p, role^course_admin, act^courses.create_files, course-v1^*, allow -p, role^course_admin, act^courses.edit_files, course-v1^*, allow -p, role^course_admin, act^courses.edit_grading_settings, course-v1^*, allow -p, role^course_admin, act^courses.manage_group_configurations, course-v1^*, allow -p, role^course_admin, act^courses.edit_details, course-v1^*, allow -p, role^course_admin, act^courses.manage_tags, course-v1^*, allow -p, role^course_admin, act^courses.publish_course_content, course-v1^*, allow -p, role^course_admin, act^courses.delete_files, course-v1^*, allow -p, role^course_admin, act^courses.edit_schedule, course-v1^*, allow -p, role^course_admin, act^courses.manage_advanced_settings, course-v1^*, allow -p, role^course_admin, act^courses.manage_certificates, course-v1^*, allow -p, role^course_admin, act^courses.import_course, course-v1^*, allow -p, role^course_admin, act^courses.export_course, course-v1^*, allow -p, role^course_admin, act^courses.export_tags, course-v1^*, allow -p, role^course_admin, act^courses.manage_course_team, course-v1^*, allow -p, role^course_admin, act^courses.manage_taxonomies, course-v1^*, allow - - -# Course Limited Staff Role Policies (legacy role) -p, role^course_limited_staff, act^courses.legacy_limited_staff_role_permissions, course-v1^*, allow - -# Course Data Researcher Role Policies (legacy role) -p, role^course_data_researcher, act^courses.legacy_data_researcher_permissions, course-v1^*, allow - -# Course Beta Tester Role Policies (legacy role) -p, role^course_beta_tester, act^courses.legacy_beta_tester_permissions, course-v1^*, allow diff --git a/src/openedx_authz/engine/config/model.conf b/src/openedx_authz/engine/config/model.conf index b4707590..1ccf9fa3 100644 --- a/src/openedx_authz/engine/config/model.conf +++ b/src/openedx_authz/engine/config/model.conf @@ -3,7 +3,6 @@ # # This model supports: # - Scoped role assignments (user roles tied to specific contexts) -# - Action grouping (manage → read/write/edit/delete to reduce duplication) # - System-wide roles (global scope "*" applies everywhere) # - Negative rules (deny overrides allow for exceptions) # - Namespace support (user^, role^, act^, lib^, org^, course^, etc.) @@ -52,16 +51,6 @@ p = sub, act, scope, eft # g, role^org_admin, role^org_editor, org^OpenedX # org_admin inherits org_editor permissions g = _, _, _ -# g2: Action grouping and implications -# Maps high-level actions to specific actions to reduce policy duplication -# -# Examples: -# g2, act^manage, act^edit # manage implies edit -# g2, act^manage, act^delete # manage implies delete -# g2, act^edit_courses, act^read # edit_courses implies read (for resource access) -# g2, act^edit_courses, act^write # edit_courses implies write (for resource modification) -g2 = _, _ - [policy_effect] # Deny-override policy: allow if any rule allows AND no rule denies # This enables negative rules/exceptions (e.g., "manage all courses except course Z") @@ -84,12 +73,8 @@ e = some(where (p.eft == allow)) && !some(where (p.eft == deny)) # SCOPE MATCHING: # - keyMatch(r.scope, p.scope): scope matches pattern # -# ACTION MATCHING: -# - r.act == p.act: exact action match -# - g2(p.act, r.act): policy action implies requested action via grouping -# # All conditions must be true for a policy to match: # 1. Subject must have role in scope OR global role # 2. Scope must match pattern -# 3. Action must match OR inherit via action grouping -m = is_staff_or_superuser(r.sub, r.act, r.scope) || (g(r.sub, p.sub, r.scope) || g(r.sub, p.sub, "*")) && keyMatch(r.scope, p.scope) && (r.act == p.act || g2(p.act, r.act)) +# 3. Action must match exactly +m = is_staff_or_superuser(r.sub, r.act, r.scope) || (g(r.sub, p.sub, r.scope) || g(r.sub, p.sub, "*")) && keyMatch(r.scope, p.scope) && r.act == p.act diff --git a/src/openedx_authz/engine/filter.py b/src/openedx_authz/engine/filter.py index fd05953a..536892ae 100644 --- a/src/openedx_authz/engine/filter.py +++ b/src/openedx_authz/engine/filter.py @@ -37,7 +37,6 @@ class Filter: - ``p`` → Policy rule (permissions). - ``g`` → Grouping rule (user ↔ role). - - ``g2`` → Action grouping (parent action ↔ child action). """ v0: Optional[list[str]] = attr.field(factory=list) @@ -45,7 +44,6 @@ class Filter: - For ``p`` → Subject (e.g., ``role^org_admin``, ``user^alice``). - For ``g`` → User (e.g., ``user^alice``). - - For ``g2`` → Parent action (e.g., ``act^manage``). """ v1: Optional[list[str]] = attr.field(factory=list) @@ -53,7 +51,6 @@ class Filter: - For ``p`` → Action (e.g., ``act^manage``, ``act^edit``). - For ``g`` → Role (e.g., ``role^org_admin``). - - For ``g2`` → Child action (e.g., ``act^edit``). """ v2: Optional[list[str]] = attr.field(factory=list) @@ -61,7 +58,6 @@ class Filter: - For ``p`` → Object or resource (e.g., ``lib^*``, ``org^MIT``). - For ``g`` → Scope or resource (e.g., ``org^MIT``). - - For ``g2`` → Not used. """ v3: Optional[list[str]] = attr.field(factory=list) diff --git a/src/openedx_authz/engine/renderer.py b/src/openedx_authz/engine/renderer.py index c580914f..4d391187 100644 --- a/src/openedx_authz/engine/renderer.py +++ b/src/openedx_authz/engine/renderer.py @@ -6,8 +6,7 @@ * ``render`` builds the Casbin ``p`` rows for a :class:`CompiledSchema` in memory, without touching the database. * ``apply`` persists the rows in a single transaction, while preserving data - owned by other services (ADR 0018 §3): dynamic roles, user assignments, and - the legacy ``g2`` action-inheritance rows that still live in ``authz.policy``. + owned by other services (ADR 0018 §3), including dynamic roles and user assignments. Key semantics: * Idempotent (ADR 0018 §2): re-applying identical definitions changes @@ -33,7 +32,7 @@ that makes precise pruning safe, and it is consulted rather than assumed: removal candidates are intersected with the stored role-permission grants (see :meth:`SchemaApplier._managed_rows`), so unmanaged ``p`` rows, dynamic roles, -user assignments, and legacy ``g2`` action inheritance are all preserved. +and user assignments are all preserved. ``render`` is pure in behavior — it touches no database and holds no Casbin state. ``plan``/``apply`` resolve the enforcer only when called (not at import), @@ -145,8 +144,8 @@ class PolicyRenderer: def render(self, schema: CompiledSchema) -> RenderedPolicy: """Produce one ``p`` row per (role, permission, supported scope). - Emits definition (``p``) rows only — never ``g`` (assignments) or ``g2`` - (action inheritance). Applies the internal Casbin namespacing here. + Emits definition (``p``) rows only, never ``g`` assignments. Applies the + internal Casbin namespacing here. Performs no database access. Output order is deterministic. """ rows: list[PolicyRow] = [] @@ -224,7 +223,7 @@ def apply( Adds rendered rows not already present, removes stale schema-owned rows no longer rendered, prunes definition/source records the compiled schema no longer contains, and invalidates the policy cache so the enforcer - reloads. Preserves dynamic roles, user assignments, and ``g2`` rows. + reloads. Preserves dynamic roles and user assignments. A role slated for removal that still has user assignments is blocking: without ``force`` the apply aborts before any write; with ``force`` the diff --git a/src/openedx_authz/engine/utils.py b/src/openedx_authz/engine/utils.py index 344ab4e8..3b6e361c 100644 --- a/src/openedx_authz/engine/utils.py +++ b/src/openedx_authz/engine/utils.py @@ -31,7 +31,7 @@ logger = logging.getLogger(__name__) -GROUPING_POLICY_PTYPES = ["g", "g2", "g3", "g4", "g5", "g6"] +GROUPING_POLICY_PTYPES = ["g", "g3", "g4", "g5", "g6"] # Map new roles back to legacy roles for rollback purposes diff --git a/src/openedx_authz/management/commands/enforcement.py b/src/openedx_authz/management/commands/enforcement.py index b3507a2d..02363989 100644 --- a/src/openedx_authz/management/commands/enforcement.py +++ b/src/openedx_authz/management/commands/enforcement.py @@ -13,7 +13,7 @@ The command supports: - Interactive testing with format: subject action scope - Real-time enforcement results with visual feedback (✓ ALLOWED / ✗ DENIED) -- Display of loaded policies, role assignments, and action grouping rules +- Display of loaded policies and role assignments Example usage: # Use policies from database with default model @@ -83,7 +83,7 @@ def add_arguments(self, parser: argparse.ArgumentParser) -> None: help=( "Path to the Casbin policy CSV file. When provided, switches to file mode using a " "custom enforcer instead of the database. Supports CSV format with policies, roles, " - "and action grouping." + "and role assignments." ), ) parser.add_argument( @@ -178,18 +178,15 @@ def _handle_file_mode(self, policy_file_path: str, model_file_path: str) -> None raise CommandError(f"Error creating Casbin enforcer: {str(e)}") from e def _display_loaded_policies(self, enforcer: Enforcer) -> None: - """Display statistics about loaded policies, roles, and action grouping. + """Display statistics about loaded policies and roles. Args: enforcer (Enforcer): The Casbin enforcer instance with loaded policies. """ policies = enforcer.get_policy() roles = enforcer.get_grouping_policy() - action_grouping = enforcer.get_named_grouping_policy("g2") - self.stdout.write(f"✓ Loaded {len(policies)} policies") self.stdout.write(f"✓ Loaded {len(roles)} role assignments") - self.stdout.write(f"✓ Loaded {len(action_grouping)} action grouping rules") self.stdout.write("") def _run_interactive_mode(self) -> None: diff --git a/src/openedx_authz/management/commands/load_policies.py b/src/openedx_authz/management/commands/load_policies.py index 9b2d2232..08a21b36 100644 --- a/src/openedx_authz/management/commands/load_policies.py +++ b/src/openedx_authz/management/commands/load_policies.py @@ -1,7 +1,9 @@ -"""Django management command to load policies into the authz Django model. +"""Django management command to load Casbin policy files into the authz Django model. The command supports: -- Specifying the path to the Casbin policy file. Default is 'openedx_authz/engine/config/authz.policy'. + +- Specifying the path to a Casbin policy file. Calls without a file are a no-op + while deployments move to ``load_authz_schema``. - Specifying the Casbin model configuration file. Default is 'openedx_authz/engine/config/model.conf'. - Optionally clearing existing policies in the database before loading new ones. """ @@ -24,10 +26,10 @@ class Command(BaseCommand): the Django database model used by the Casbin adapter. This allows for easy management and persistence of authorization policies within the Django application. - Example Usage: - python manage.py load_policies --policy-file-path /path/to/authz.policy - python manage.py load_policies --policy-file-path /path/to/authz.policy --model-file-path /path/to/model.conf - python manage.py load_policies + Example usage:: + + python manage.py load_policies --policy-file-path /path/to/custom.policy + python manage.py load_policies --policy-file-path /path/to/custom.policy --model-file-path /path/to/model.conf """ help = "Load policies from a Casbin policy file into the Django database model." @@ -42,7 +44,7 @@ def add_arguments(self, parser) -> None: "--policy-file-path", type=str, default=None, - help="Path to the Casbin policy file (supports CSV format with policies, roles, and action grouping)", + help="Path to a Casbin policy file", ) parser.add_argument( "--model-file-path", @@ -74,7 +76,14 @@ def handle(self, *args, **options): options["model_file_path"], ) if policy_file_path is None: - policy_file_path = os.path.join(ROOT_DIRECTORY, "engine", "config", "authz.policy") + self.stdout.write( + self.style.WARNING( + "No policy file supplied; nothing was loaded. " + "Use load_authz_schema for static role-permission policies." + ) + ) + return + if model_file_path is None: model_file_path = os.path.join(ROOT_DIRECTORY, "engine", "config", "model.conf") @@ -93,23 +102,13 @@ def handle(self, *args, **options): ): self._delete_existing_roles(target_enforcer) - if click.confirm( - click.style( - "Do you want to delete existing permissions inheritance?", - fg="yellow", - bold=True, - ), - default=False, - ): - self._delete_permissions_inheritance(target_enforcer) - source_enforcer = casbin.Enforcer(model_file_path, policy_file_path) self.migrate_policies(source_enforcer, target_enforcer) def migrate_policies(self, source_enforcer, target_enforcer): """Migrate policies from the source enforcer to the target enforcer. - This method copies all policies, role assignments, and action groupings + This method copies all policies and role assignments from the source enforcer (file-based) to the target enforcer (database-backed). Optionally clears existing policies in the target before migration. @@ -130,15 +129,3 @@ def _delete_existing_roles(self, target_enforcer): result = target_enforcer.delete_role(role) if result: click.echo(f"Deleted role: {role}") - - def _delete_permissions_inheritance(self, target_enforcer): - """Delete existing permissions inheritance from the target enforcer. - - Args: - target_enforcer: The Casbin enforcer instance to delete permissions inheritance from. - """ - list_of_permissions = target_enforcer.get_named_grouping_policy("g2") - for permission in list(list_of_permissions): - result = target_enforcer.remove_named_grouping_policy("g2", *permission) - if result: - click.echo(f"Deleted permission inheritance: {permission}") diff --git a/src/openedx_authz/migrations/0012_remove_action_grouping.py b/src/openedx_authz/migrations/0012_remove_action_grouping.py new file mode 100644 index 00000000..9ede0a8d --- /dev/null +++ b/src/openedx_authz/migrations/0012_remove_action_grouping.py @@ -0,0 +1,40 @@ +"""Remove obsolete Casbin action-grouping rows.""" + +import logging + +from django.db import migrations +from django.db.utils import DatabaseError + +logger = logging.getLogger(__name__) + + +def remove_action_grouping(apps, schema_editor): + """Delete g2 rows when their historical model and table are available.""" + try: + casbin_rule = apps.get_model("casbin_adapter", "CasbinRule") + except LookupError: + logger.warning("Skipping action-grouping cleanup because CasbinRule is unavailable.") + return + + database = schema_editor.connection.alias + try: + casbin_rule.objects.using(database).filter(ptype="g2").delete() + except DatabaseError: + # This cleanup must not block a deployment. The current Casbin model does not + # define g2, so any rows left behind are ignored and can be removed later. + logger.warning( + "Could not remove action-grouping rows. They are ignored by the current Casbin model.", + exc_info=True, + ) + + +class Migration(migrations.Migration): + """Remove stored action inheritance that is no longer supported.""" + + dependencies = [ + ("openedx_authz", "0011_authz_schema_definitions"), + ] + + operations = [ + migrations.RunPython(remove_action_grouping, migrations.RunPython.noop, atomic=False), + ] diff --git a/src/openedx_authz/tests/api/test_roles.py b/src/openedx_authz/tests/api/test_roles.py index 04372acf..62a2d44b 100644 --- a/src/openedx_authz/tests/api/test_roles.py +++ b/src/openedx_authz/tests/api/test_roles.py @@ -5,10 +5,8 @@ roles and permissions within specific scopes. """ -from importlib.resources import files from unittest.mock import patch -import casbin from ddt import data as ddt_data from ddt import ddt, unpack from django.test import TestCase @@ -50,8 +48,8 @@ LIBRARY_USER_PERMISSIONS, ) from openedx_authz.engine.enforcer import AuthzEnforcer -from openedx_authz.engine.utils import migrate_policy_between_enforcers from openedx_authz.models import ExtendedCasbinRule, Scope, Subject +from openedx_authz.tests.test_utils import seed_static_authorization_policies def _mock_get_or_create_scope(scope_data): @@ -83,13 +81,7 @@ def _seed_database_with_policies(cls): """ global_enforcer = AuthzEnforcer.get_enforcer() global_enforcer.load_policy() - model_path = str(files("openedx_authz.engine").joinpath("config/model.conf")) - policy_path = str(files("openedx_authz.engine").joinpath("config/authz.policy")) - - migrate_policy_between_enforcers( - source_enforcer=casbin.Enforcer(model_path, policy_path), - target_enforcer=global_enforcer, - ) + seed_static_authorization_policies(global_enforcer) global_enforcer.clear_policy() # Clear to simulate fresh start for each test @classmethod @@ -181,7 +173,7 @@ def setUpClass(cls): super().setUpClass() # Define specific assignments for this test class assignments = [ - # Basic library roles from authz.policy + # Basic library roles from the authorization schema { "subject_name": "alice", "role_name": roles.LIBRARY_ADMIN.external_key, @@ -202,7 +194,7 @@ def setUpClass(cls): "role_name": roles.LIBRARY_USER.external_key, "scope_name": "lib:Org1:english_101", }, - # Basic course roles from authz.policy + # Basic course roles from the authorization schema { "subject_name": "daniel", "role_name": roles.COURSE_STAFF.external_key, @@ -430,17 +422,17 @@ def test_assign_role_fails_when_extended_rule_not_created(self): self.assertFalse(ExtendedCasbinRule.objects.filter(subject=subj_obj, scope=scope_obj).exists()) @ddt_data( - # Library Admin role with actual permissions from authz.policy + # Library Admin role with permissions from the authorization schema ( roles.LIBRARY_ADMIN.external_key, LIBRARY_ADMIN_PERMISSIONS, ), - # Library Author role with actual permissions from authz.policy + # Library Author role with permissions from the authorization schema ( roles.LIBRARY_AUTHOR.external_key, LIBRARY_AUTHOR_PERMISSIONS, ), - # Library Contributor role with actual permissions from authz.policy + # Library Contributor role with permissions from the authorization schema ( roles.LIBRARY_CONTRIBUTOR.external_key, LIBRARY_CONTRIBUTOR_PERMISSIONS, @@ -466,7 +458,7 @@ def test_get_permissions_for_roles(self, role_name, expected_permissions): """ assigned_permissions = get_permissions_for_single_role(RoleData(external_key=role_name)) - self.assertEqual(assigned_permissions, expected_permissions) + self.assertEqual(sorted(assigned_permissions, key=str), sorted(expected_permissions, key=str)) @ddt_data( # Role assigned to multiple users in different scopes @@ -502,8 +494,8 @@ def test_get_permissions_for_active_role_in_specific_scope(self, role_name, scop self.assertIn(role_name, assigned_permissions) self.assertEqual( - assigned_permissions[role_name]["permissions"], - expected_permissions, + sorted(assigned_permissions[role_name]["permissions"], key=str), + sorted(expected_permissions, key=str), ) @ddt_data( @@ -544,14 +536,17 @@ def test_get_role_definitions_in_scope( """Test retrieving role definitions from policy for a namespace wildcard scope. Expected result: - - Role definitions defined in authz.policy for the scope are returned. + - Role definitions compiled from the schema for the scope are returned. - Each role includes the permissions declared in its policy rules. """ roles_in_scope = get_role_definitions_in_scope(ScopeData(namespaced_key=namespaced_scope)) roles_by_key = {role.external_key: role for role in roles_in_scope} self.assertEqual(set(roles_by_key.keys()), expected_roles) - self.assertEqual(roles_by_key[sample_role].permissions, expected_permissions) + self.assertEqual( + sorted(roles_by_key[sample_role].permissions, key=str), + sorted(expected_permissions, key=str), + ) def test_get_role_definitions_in_scope_returns_empty_for_concrete_scope(self): """Concrete scopes do not match namespace wildcard policy definitions.""" diff --git a/src/openedx_authz/tests/integration/test_views.py b/src/openedx_authz/tests/integration/test_views.py index bd136007..8dd334de 100644 --- a/src/openedx_authz/tests/integration/test_views.py +++ b/src/openedx_authz/tests/integration/test_views.py @@ -1,10 +1,8 @@ """Integration tests for openedx_authz views.""" -import os import uuid from urllib.parse import urlencode -import casbin import pytest from django.contrib.auth import get_user_model from django.test import TestCase @@ -12,12 +10,11 @@ from rest_framework import status from rest_framework.test import APIClient -from openedx_authz import ROOT_DIRECTORY from openedx_authz.api.users import assign_role_to_user_in_scope from openedx_authz.engine.enforcer import AuthzEnforcer -from openedx_authz.engine.utils import migrate_policy_between_enforcers from openedx_authz.models.core import ExtendedCasbinRule from openedx_authz.tests.integration.test_models import create_test_library +from openedx_authz.tests.test_utils import seed_static_authorization_policies User = get_user_model() @@ -30,19 +27,10 @@ class TestRoleAssignmentView(TestCase): def setUpClass(cls): """Set up test class - seed database with policies.""" super().setUpClass() - # Seed the database with policies from the policy file - # This loads the policy definitions (p, g rules) that define what permissions each role has + # Seed schema-defined role-permission policies. global_enforcer = AuthzEnforcer.get_enforcer() global_enforcer.load_policy() - - # Use absolute paths based on the package ROOT_DIRECTORY - model_conf = os.path.join(ROOT_DIRECTORY, "engine", "config", "model.conf") - authz_policy = os.path.join(ROOT_DIRECTORY, "engine", "config", "authz.policy") - - migrate_policy_between_enforcers( - source_enforcer=casbin.Enforcer(model_conf, authz_policy), - target_enforcer=global_enforcer, - ) + seed_static_authorization_policies(global_enforcer) def setUp(self): """Set up the test client and any required data.""" diff --git a/src/openedx_authz/tests/rest_api/test_views.py b/src/openedx_authz/tests/rest_api/test_views.py index 4342f828..7e3c05f5 100644 --- a/src/openedx_authz/tests/rest_api/test_views.py +++ b/src/openedx_authz/tests/rest_api/test_views.py @@ -1332,7 +1332,7 @@ class TestArbitraryRoleSupport(ViewTestMixin): def setUpClass(cls): super().setUpClass() # Defined with the same wildcard library scope as the built-in library roles - # (see authz.policy), so it's visible for any specific library scope. + # (see library_roles.yaml), so it's visible for any specific library scope. AuthzEnforcer.get_enforcer().add_policy( make_role_key(cls.CUSTOM_ROLE), make_action_key(cls.CUSTOM_ACTION), diff --git a/src/openedx_authz/tests/test_commands.py b/src/openedx_authz/tests/test_commands.py index 8d270571..6f7a5d67 100644 --- a/src/openedx_authz/tests/test_commands.py +++ b/src/openedx_authz/tests/test_commands.py @@ -11,7 +11,6 @@ from django.core.management import call_command from django.core.management.base import CommandError -from openedx_authz import ROOT_DIRECTORY from openedx_authz import api as authz_api from openedx_authz.api.data import ContentLibraryData from openedx_authz.constants.permissions import ( @@ -87,17 +86,9 @@ def setUp(self): make_wildcard_key(ContentLibraryData.NAMESPACE), ] ] - self.action_grouping = [ - [ - make_action_key(DELETE_LIBRARY.identifier), - make_action_key(VIEW_LIBRARY.identifier), - ] - ] - self.enforcer = Mock() self.enforcer.get_policy.return_value = self.policies self.enforcer.get_grouping_policy.return_value = self.roles - self.enforcer.get_named_grouping_policy.return_value = self.action_grouping @patch.object(AuthzEnforcer, "get_enforcer") @patch("openedx_authz.management.commands.enforcement.disabled_logging") @@ -170,7 +161,6 @@ def test_display_loaded_policies(self, mock_get_enforcer: Mock): output = self.buffer.getvalue() self.assertIn(f"✓ Loaded {len(self.policies)} policies", output) self.assertIn(f"✓ Loaded {len(self.roles)} role assignments", output) - self.assertIn(f"✓ Loaded {len(self.action_grouping)} action grouping rules", output) @patch.object(AuthzEnforcer, "get_enforcer") @patch.object(authz_api, "is_user_allowed") @@ -328,9 +318,7 @@ class LoadPoliciesCommandTests(TestCase): """ Tests for the `load_policies` Django management command. - This test class verifies the behavior of the load_policies command, including: - - Default file path handling - - Clearing existing policies + This test class verifies custom policy files and optional role clearing. """ def setUp(self): @@ -338,72 +326,38 @@ def setUp(self): self.buffer = io.StringIO() @patch("openedx_authz.engine.enforcer.AuthzEnforcer.get_enforcer") - @patch("casbin.Enforcer") - @patch("os.path.join") - @patch("click.confirm") - def test_handle_with_default_paths(self, mock_confirm, mock_join, mock_casbin_enforcer, mock_get_enforcer): - """Test handle method with default policy and model paths.""" - # Setup mocks - mock_target_enforcer = Mock() - mock_get_enforcer.return_value = mock_target_enforcer - - mock_source_enforcer = Mock() - mock_casbin_enforcer.return_value = mock_source_enforcer + def test_handle_without_policy_file_is_a_deprecated_noop(self, mock_get_enforcer): + """Existing deployment calls remain safe until they move to the schema command.""" + command = LoadPoliciesCommand(stdout=self.buffer) - policy_path = f"{ROOT_DIRECTORY}/engine/config/authz.policy" - model_path = f"{ROOT_DIRECTORY}/engine/config/model.conf" - - # Define paths that will be joined - mock_join.side_effect = ( - policy_path, - model_path, - ) - - # Create command instance - command = LoadPoliciesCommand() - command.migrate_policies = Mock() - - # Call handle method command.handle(policy_file_path=None, model_file_path=None, clear_existing=False) - # Assertions - mock_casbin_enforcer.assert_called_once_with( - model_path, - policy_path, - ) - mock_join.assert_any_call(ROOT_DIRECTORY, "engine", "config", "authz.policy") - mock_join.assert_any_call(ROOT_DIRECTORY, "engine", "config", "model.conf") - mock_confirm.assert_not_called() - command.migrate_policies.assert_called_once_with(mock_source_enforcer, mock_target_enforcer) + self.assertIn("No policy file supplied", self.buffer.getvalue()) + mock_get_enforcer.assert_not_called() @patch("openedx_authz.engine.enforcer.AuthzEnforcer.get_enforcer") @patch("casbin.Enforcer") @patch("click.confirm") def test_handle_with_custom_paths(self, mock_confirm, mock_casbin_enforcer, mock_get_enforcer): """Test handle method with custom policy and model paths.""" - # Setup mocks mock_target_enforcer = Mock() mock_get_enforcer.return_value = mock_target_enforcer mock_source_enforcer = Mock() mock_casbin_enforcer.return_value = mock_source_enforcer - # Create command instance command = LoadPoliciesCommand() command.migrate_policies = Mock() - # Custom paths policy_path = "/custom/path/to/policy.csv" model_path = "/custom/path/to/model.conf" - # Call handle method command.handle( policy_file_path=policy_path, model_file_path=model_path, clear_existing=False, ) - # Assertions mock_casbin_enforcer.assert_called_once_with(model_path, policy_path) mock_confirm.assert_not_called() command.migrate_policies.assert_called_once_with(mock_source_enforcer, mock_target_enforcer) @@ -416,96 +370,46 @@ def test_handle_clear_existing_roles_confirmed( self, mock_style, mock_confirm, mock_casbin_enforcer, mock_get_enforcer ): """Test handle method with clear_existing and confirmed delete roles.""" - # Setup mocks mock_target_enforcer = Mock() mock_get_enforcer.return_value = mock_target_enforcer mock_source_enforcer = Mock() mock_casbin_enforcer.return_value = mock_source_enforcer - # Setup click mocks mock_style.return_value = "styled message" - mock_confirm.side_effect = [True, False] # Confirm roles, deny permissions + mock_confirm.return_value = True - # Create command instance command = LoadPoliciesCommand() command.migrate_policies = Mock() command._delete_existing_roles = Mock() - command._delete_permissions_inheritance = Mock() - # Call handle method - command.handle(policy_file_path=None, model_file_path=None, clear_existing=True) + command.handle(policy_file_path="/custom/policy.csv", model_file_path=None, clear_existing=True) - # Assertions mock_target_enforcer.load_policy.assert_called_once() mock_confirm.assert_called_with(mock_style.return_value, default=False) command._delete_existing_roles.assert_called_once_with(mock_target_enforcer) - command._delete_permissions_inheritance.assert_not_called() - command.migrate_policies.assert_called_once_with(mock_source_enforcer, mock_target_enforcer) - - @patch("openedx_authz.engine.enforcer.AuthzEnforcer.get_enforcer") - @patch("casbin.Enforcer") - @patch("click.confirm") - @patch("click.style") - def test_handle_clear_existing_permissions_confirmed( - self, mock_style, mock_confirm, mock_casbin_enforcer, mock_get_enforcer - ): - """Test handle method with clear_existing and confirmed delete permissions.""" - # Setup mocks - mock_target_enforcer = Mock() - mock_get_enforcer.return_value = mock_target_enforcer - - mock_source_enforcer = Mock() - mock_casbin_enforcer.return_value = mock_source_enforcer - - # Setup click mocks - mock_style.return_value = "styled message" - mock_confirm.side_effect = [False, True] # Deny roles, confirm permissions - - # Create command instance - command = LoadPoliciesCommand() - command.migrate_policies = Mock() - command._delete_existing_roles = Mock() - command._delete_permissions_inheritance = Mock() - - # Call handle method - command.handle(policy_file_path=None, model_file_path=None, clear_existing=True) - - # Assertions - mock_target_enforcer.load_policy.assert_called_once() - mock_confirm.assert_called_with(mock_style.return_value, default=False) - command._delete_existing_roles.assert_not_called() - command._delete_permissions_inheritance.assert_called_once_with(mock_target_enforcer) command.migrate_policies.assert_called_once_with(mock_source_enforcer, mock_target_enforcer) @patch("openedx_authz.engine.enforcer.AuthzEnforcer.get_enforcer") @patch("casbin.Enforcer") @patch("click.confirm") - def test_handle_clear_existing_both_denied(self, mock_confirm, mock_casbin_enforcer, mock_get_enforcer): - """Test handle method with clear_existing but denied deletions.""" - expected_mock_confirm_calls = 2 - # Setup mocks + def test_handle_clear_existing_roles_denied(self, mock_confirm, mock_casbin_enforcer, mock_get_enforcer): + """Declining role deletion still imports the supplied file.""" mock_target_enforcer = Mock() mock_get_enforcer.return_value = mock_target_enforcer mock_source_enforcer = Mock() mock_casbin_enforcer.return_value = mock_source_enforcer - # Setup click mocks - mock_confirm.side_effect = [False, False] # Deny both roles and permissions + mock_confirm.return_value = False - # Create command instance command = LoadPoliciesCommand() command.migrate_policies = Mock() command._delete_existing_roles = Mock() - command._delete_permissions_inheritance = Mock() - # Call handle method - command.handle(policy_file_path=None, model_file_path=None, clear_existing=True) + command.handle(policy_file_path="/custom/policy.csv", model_file_path=None, clear_existing=True) - # Assertions mock_target_enforcer.load_policy.assert_called_once() - assert mock_confirm.call_count == expected_mock_confirm_calls + mock_confirm.assert_called_once() command._delete_existing_roles.assert_not_called() - command._delete_permissions_inheritance.assert_not_called() command.migrate_policies.assert_called_once_with(mock_source_enforcer, mock_target_enforcer) diff --git a/src/openedx_authz/tests/test_enforcement.py b/src/openedx_authz/tests/test_enforcement.py index 93dd82b0..a707f907 100644 --- a/src/openedx_authz/tests/test_enforcement.py +++ b/src/openedx_authz/tests/test_enforcement.py @@ -58,16 +58,6 @@ class AuthRequest(TypedDict): expected_result: bool -COMMON_ACTION_GROUPING = [ - # manage implies edit and delete - ["g2", make_action_key("manage"), make_action_key("edit")], - ["g2", make_action_key("manage"), make_action_key("delete")], - # edit implies read and write - ["g2", make_action_key("edit"), make_action_key("read")], - ["g2", make_action_key("edit"), make_action_key("write")], -] - - @pytest.mark.django_db @ddt class CasbinEnforcementTestCase(TestCase): @@ -98,16 +88,15 @@ def _load_policy(self, policy: list[str]) -> None: Load policy rules into the Casbin enforcer. This method clears any existing policies and loads the provided policy rules - into the appropriate policy stores (p for policies, g for role assignments, - g2 for action groupings). + into the appropriate policy stores (p for policies and g for role assignments). Args: policy (list[str]): List of policy rules where each rule is a - list starting with the rule type ('p', 'g', or 'g2') followed by + list starting with the rule type ('p' or 'g') followed by the rule parameters. Raises: - ValueError: If a policy rule has an invalid type (not 'p', 'g', or 'g2'). + ValueError: If a policy rule has an invalid type. """ self.enforcer.clear_policy() for rule in policy: @@ -115,8 +104,6 @@ def _load_policy(self, policy: list[str]) -> None: self.enforcer.add_named_policy("p", rule[1:]) elif rule[0] == "g": self.enforcer.add_named_grouping_policy("g", rule[1:]) - elif rule[0] == "g2": - self.enforcer.add_named_grouping_policy("g2", rule[1:]) else: raise ValueError(f"Invalid policy rule: {rule}") @@ -149,7 +136,7 @@ class SystemWideRoleTests(CasbinEnforcementTestCase): POLICY = [ ["p", make_role_key("platform_admin"), make_action_key("manage"), GLOBAL_SCOPE_WILDCARD, "allow"], ["g", make_user_key("user-1"), make_role_key("platform_admin"), GLOBAL_SCOPE_WILDCARD], - ] + COMMON_ACTION_GROUPING + ] GENERAL_CASES = [ { @@ -173,64 +160,17 @@ def test_platform_admin_general_access(self, request: AuthRequest): @ddt -class ActionGroupingTests(CasbinEnforcementTestCase): - """ - Tests for action grouping and permission inheritance. - - This test class verifies that action grouping works correctly, where high-level - actions (like 'manage') automatically grant access to lower-level actions - (like 'edit', 'read', 'write', 'delete') through the g2 grouping mechanism. - """ - - ORG_COURSE_SCOPE = make_course_key("course-v1:any-org+*") - - POLICY = [ - [ - "p", - make_role_key("role-1"), - make_action_key("manage"), - make_scope_key("course-v1", GLOBAL_SCOPE_WILDCARD), - "allow", - ], - [ - "g", - make_user_key("user-1"), - make_role_key("role-1"), - ORG_COURSE_SCOPE, - ], - ] + COMMON_ACTION_GROUPING - - CASES = [ - { - "subject": make_user_key("user-1"), - "action": make_action_key("edit"), - "scope": ORG_COURSE_SCOPE, - "expected_result": True, - }, - { - "subject": make_user_key("user-1"), - "action": make_action_key("read"), - "scope": ORG_COURSE_SCOPE, - "expected_result": True, - }, - { - "subject": make_user_key("user-1"), - "action": make_action_key("write"), - "scope": ORG_COURSE_SCOPE, - "expected_result": True, - }, - { - "subject": make_user_key("user-1"), - "action": make_action_key("delete"), - "scope": ORG_COURSE_SCOPE, - "expected_result": True, - }, - ] +class ExplicitActionTests(CasbinEnforcementTestCase): + """Tests that every authorized action requires an explicit policy.""" - @data(*CASES) - def test_action_grouping_access(self, request: AuthRequest): - """Test that users have access through action grouping.""" - self._test_enforcement(self.POLICY, request) + def test_action_grouping_is_not_supported(self): + """The authorization model accepts only explicit action policies.""" + with self.assertRaises(KeyError): + self.enforcer.add_named_grouping_policy( + "g2", + make_action_key("manage"), + make_action_key("edit"), + ) @ddt @@ -354,7 +294,7 @@ class RoleAssignmentTests(CasbinEnforcementTestCase): make_role_key(roles.LIBRARY_AUTHOR.external_key), make_library_key("lib:DemoX:CSPROB"), ], - ] + COMMON_ACTION_GROUPING + ] CASES = [ { @@ -440,7 +380,7 @@ class DeniedAccessTests(CasbinEnforcementTestCase): "deny", ], ["g", make_user_key("user-1"), make_role_key("platform_admin"), GLOBAL_SCOPE_WILDCARD], - ] + COMMON_ACTION_GROUPING + ] CASES = [ { @@ -516,7 +456,7 @@ class WildcardScopeTests(CasbinEnforcementTestCase): # Role assignments ["g", make_user_key("user-1"), make_role_key(roles.COURSE_ADMIN.external_key), GLOBAL_SCOPE_WILDCARD], ["g", make_user_key("user-2"), make_role_key(roles.LIBRARY_ADMIN.external_key), GLOBAL_SCOPE_WILDCARD], - ] + COMMON_ACTION_GROUPING + ] @data( (make_course_key("course-v1:OpenedX+DemoX+CS101"), True), diff --git a/src/openedx_authz/tests/test_enforcer.py b/src/openedx_authz/tests/test_enforcer.py index 7a6466a1..096db6f0 100644 --- a/src/openedx_authz/tests/test_enforcer.py +++ b/src/openedx_authz/tests/test_enforcer.py @@ -5,36 +5,34 @@ that would be used in production environments. """ -import os import time from uuid import uuid4 -import casbin from ddt import data as ddt_data from ddt import ddt from django.conf import settings from django.test import TestCase, TransactionTestCase, override_settings -from openedx_authz import ROOT_DIRECTORY from openedx_authz.engine.enforcer import AuthzEnforcer from openedx_authz.engine.filter import Filter -from openedx_authz.engine.utils import migrate_policy_between_enforcers from openedx_authz.models.engine import PolicyCacheControl -from openedx_authz.tests.test_utils import make_action_key, make_role_key, make_scope_key, make_user_key - -MODEL_CONF_PATH = os.path.join(ROOT_DIRECTORY, "engine", "config", "model.conf") -AUTHZ_POLICY_PATH = os.path.join(ROOT_DIRECTORY, "engine", "config", "authz.policy") +from openedx_authz.tests.test_utils import ( + make_action_key, + make_role_key, + make_scope_key, + make_user_key, + seed_static_authorization_policies, +) class PolicyLoadingTestSetupMixin(TestCase): """Mixin providing policy loading test utilities.""" - @staticmethod - def _count_policies_in_file(scope_pattern: str = None, role: str = None): - """Count policies in the authz.policy file matching the given criteria. + def _count_schema_policies(self, scope_pattern: str = None, role: str = None): + """Count rendered schema policies matching the given criteria. This provides a dynamic way to get expected policy counts without - hardcoding values that might change as the policy file evolves. + hardcoding values that might change as the schema evolves. Args: scope_pattern: Scope pattern to match (e.g., 'lib^*') @@ -43,32 +41,14 @@ def _count_policies_in_file(scope_pattern: str = None, role: str = None): Returns: int: Number of matching policies """ - count = 0 - with open(AUTHZ_POLICY_PATH, "r") as f: - for line in f: - line = line.strip() - if not line or line.startswith("#"): - continue - if not line.startswith("p,"): - continue - - parts = [p.strip() for p in line.split(",")] - if len(parts) < 4: - continue - - # parts[0] = 'p', parts[1] = role, parts[2] = action, parts[3] = scope - matches = True - if role and parts[1] != role: - matches = False - if scope_pattern and parts[3] != scope_pattern: - matches = False - - if matches: - count += 1 - return count + return sum( + 1 + for policy in self.schema_policies + if (not role or policy[0] == role) and (not scope_pattern or policy[2] == scope_pattern) + ) def _seed_database_with_policies(self): - """Seed the database with policies from the policy file. + """Seed static role-permission policies from the authorization schema. This simulates the one-time database seeding that would happen during application deployment, separate from runtime policy loading. @@ -77,13 +57,7 @@ def _seed_database_with_policies(self): global_enforcer = AuthzEnforcer.get_enforcer() global_enforcer.clear_policy() - migrate_policy_between_enforcers( - source_enforcer=casbin.Enforcer( - MODEL_CONF_PATH, - AUTHZ_POLICY_PATH, - ), - target_enforcer=global_enforcer, - ) + self.schema_policies = seed_static_authorization_policies(global_enforcer) # Ensure enforcer memory is clean for test isolation global_enforcer.clear_policy() @@ -169,7 +143,7 @@ class TestPolicyLoadingStrategies(PolicyLoadingTestSetupMixin): These tests demonstrate how policy loading would work in real-world scenarios, including scope-based loading, user-context loading, and role-specific loading. - All based on our basic policy setup in authz.policy file. + Static role-permission policies come from the authorization schema. """ LIBRARY_ROLES = [ @@ -190,7 +164,7 @@ def tearDown(self): super().tearDown() @ddt_data( - "lib^*", # Library policies from authz.policy file + "lib^*", # Library policies from the authorization schema "course^*", # No course policies in basic setup "org^*", # No org policies in basic setup ) @@ -206,7 +180,7 @@ def test_scope_based_policy_loading(self, scope): - Policy count matches expected for scope """ global_enforcer = AuthzEnforcer.get_enforcer() - expected_policy_count = self._count_policies_in_file(scope_pattern=scope) + expected_policy_count = self._count_schema_policies(scope_pattern=scope) initial_policy_count = len(global_enforcer.get_policy()) self._load_policies_for_scope(scope) @@ -382,7 +356,7 @@ def test_filtered_loading_by_role(self, role_name): - All loaded policies contain the specified role """ global_enforcer = AuthzEnforcer.get_enforcer() - expected_count = self._count_policies_in_file(role=role_name) + expected_count = self._count_schema_policies(role=role_name) self._load_policies_for_role_management(role_name) loaded_policies = global_enforcer.get_policy() @@ -404,8 +378,8 @@ def test_multi_scope_filtering(self): course_scope = "course-v1^*" org_scope = "org^*" - expected_lib_count = self._count_policies_in_file(scope_pattern=lib_scope) - expected_course_count = self._count_policies_in_file(scope_pattern=course_scope) + expected_lib_count = self._count_schema_policies(scope_pattern=lib_scope) + expected_course_count = self._count_schema_policies(scope_pattern=course_scope) self._add_test_policies_for_multiple_scopes() # The previous function added 6 custom policies for courses, add to expected expected_course_count += 6 @@ -449,17 +423,11 @@ def setUp(self): AuthzEnforcer._enforcer = None # pylint: disable=protected-access def _seed_database_with_policies(self): - """Seed the database with policies from the policy file.""" + """Seed static role-permission policies from the authorization schema.""" global_enforcer = AuthzEnforcer.get_enforcer() global_enforcer.clear_policy() - migrate_policy_between_enforcers( - source_enforcer=casbin.Enforcer( - MODEL_CONF_PATH, - AUTHZ_POLICY_PATH, - ), - target_enforcer=global_enforcer, - ) + seed_static_authorization_policies(global_enforcer) global_enforcer.clear_policy() def _wait_for_auto_load(self) -> None: diff --git a/src/openedx_authz/tests/test_engine_utils.py b/src/openedx_authz/tests/test_engine_utils.py index 87b790ba..7181d4d1 100644 --- a/src/openedx_authz/tests/test_engine_utils.py +++ b/src/openedx_authz/tests/test_engine_utils.py @@ -1,385 +1,129 @@ -""" -Test suite for engine/utils.py policy migration functionality. - -This module tests the migration of policies from file-based storage (authz.policy) -to database-backed storage, which is the real-world scenario for the load_policies -management command. -""" +"""Tests for migrating explicit Casbin policies between enforcers.""" import os +from tempfile import NamedTemporaryFile import casbin from casbin_adapter.models import CasbinRule -from ddt import data, ddt, unpack from django.db.models import Count from django.test import TestCase from openedx_authz import ROOT_DIRECTORY -from openedx_authz.constants import permissions, roles from openedx_authz.engine.enforcer import AuthzEnforcer from openedx_authz.engine.utils import migrate_policy_between_enforcers -from openedx_authz.tests.test_utils import make_action_key, make_role_key, make_scope_key, make_user_key +from openedx_authz.tests.test_utils import ( + make_action_key, + make_role_key, + make_scope_key, + make_user_key, + seed_static_authorization_policies, +) + +REMOVED_ACTION_RELATIONSHIPS = ( + ("content_libraries.manage_library_tags", "content_libraries.edit_library_content"), + ("content_libraries.delete_library", "content_libraries.edit_library_content"), + ("content_libraries.publish_library_content", "content_libraries.edit_library_content"), + ("content_libraries.edit_library_content", "content_libraries.view_library"), + ("content_libraries.reuse_library_content", "content_libraries.view_library"), + ("content_libraries.publish_library_content", "content_libraries.view_library"), + ("content_libraries.manage_library_team", "content_libraries.view_library_team"), + ("content_libraries.delete_library_collection", "content_libraries.edit_library_collection"), + ("content_libraries.create_library_collection", "content_libraries.edit_library_collection"), + ("content_libraries.edit_library_collection", "content_libraries.view_library"), +) -@ddt class TestMigratePolicyBetweenEnforcers(TestCase): - """ - Test case for migrate_policy_between_enforcers function. - - Tests the migration of policies from the authz.policy file to the database: - - Loading all policies from file to DB - - Idempotent migration (running twice doesn't duplicate) - - Partial migration (some policies already in DB) - - Preserving existing DB policies not in file - """ + """The legacy importer still handles explicit policies and role assignments.""" @classmethod def setUpClass(cls): - """Set up the Casbin model and policy file paths.""" super().setUpClass() - engine_config_dir = os.path.join(ROOT_DIRECTORY, "engine", "config") - cls.model_file = os.path.join(engine_config_dir, "model.conf") - cls.policy_file = os.path.join(engine_config_dir, "authz.policy") - - if not os.path.isfile(cls.model_file): - raise FileNotFoundError(f"Model file not found: {cls.model_file}") - if not os.path.isfile(cls.policy_file): - raise FileNotFoundError(f"Policy file not found: {cls.policy_file}") + cls.model_file = os.path.join(ROOT_DIRECTORY, "engine", "config", "model.conf") def setUp(self): - """Set up fresh enforcers for each test. - - Creates enforcers matching the load_policies command pattern: - - Source enforcer: file-based (loads from authz.policy file, read-only) - - Target enforcer: database-backed (global_enforcer, will be cleared) - """ - # Source enforcer loads policies from the authz.policy file - self.source_enforcer = casbin.Enforcer(self.model_file, self.policy_file) - - # Target enforcer is the database-backed global enforcer + super().setUp() + policy_file = NamedTemporaryFile(mode="w", suffix=".policy", delete=False) + policy_file.write("p, role^custom, act^custom.read, lib^*, allow\n") + policy_file.write("g, user^alice, role^custom, lib^demo\n") + policy_file.close() + self.addCleanup(os.unlink, policy_file.name) + + self.source_enforcer = casbin.Enforcer(self.model_file, policy_file.name) self.target_enforcer = AuthzEnforcer.get_enforcer() - - # Clear the target enforcer's database to start fresh - # This simulates a clean database state before migration - self._clear_target_enforcer() - - def _clear_target_enforcer(self): - """Clear all policies from the target (database) enforcer.""" - # Clear the database directly to ensure a clean state CasbinRule.objects.all().delete() - - # Reload the enforcer to sync with the now-empty database self.target_enforcer.load_policy() - def test_migrate_all_file_policies_to_database(self): - """Test migration of all policies from authz.policy file to database. - - Expected Result: - - All policies from the file are loaded into the database - - The file contains 132 regular policies (p rules) - - Policy content matches expected file content - """ - expected_policy_count = 132 - + def test_migrates_explicit_policy_and_role_assignment(self): + """The importer copies explicit policies and scoped role assignments.""" migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - self.target_enforcer.load_policy() - - target_policies = self.target_enforcer.get_policy() - self.assertEqual( - len(target_policies), - expected_policy_count, - f"Expected {expected_policy_count} policies from file, got {len(target_policies)}", - ) - - self.assertIn( - [ - make_role_key(roles.LIBRARY_ADMIN.external_key), - make_action_key(permissions.DELETE_LIBRARY.identifier), - make_scope_key("lib", "*"), - "allow", - ], - target_policies, - ) - self.assertIn( - [ - make_role_key(roles.LIBRARY_USER.external_key), - make_action_key(permissions.VIEW_LIBRARY.identifier), - make_scope_key("lib", "*"), - "allow", - ], - target_policies, - ) - - def test_migrate_no_grouping_policies_from_file(self): - """Test that no grouping policies (g rules) exist in the authz.policy file. - - Expected Result: - - The authz.policy file contains no g rules (role assignments) - - These are expected to come from the database, not the file - - Migration should result in 0 grouping policies - """ - migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - self.target_enforcer.load_policy() - target_grouping = self.target_enforcer.get_grouping_policy() - # The file contains no g rules - those come from database/runtime assignment self.assertEqual( - len(target_grouping), - 0, - "authz.policy file should not contain user role assignments (g rules)", + self.target_enforcer.get_policy(), + [["role^custom", "act^custom.read", "lib^*", "allow"]], ) - - def test_migrate_action_inheritance_from_file(self): - """Test migration of g2 policies (action inheritance) from authz.policy file. - - Expected Result: - - All g2 rules from the file are migrated to database - - The file contains 10 g2 rules defining action hierarchies - - Action inheritance relationships are preserved - """ - expected_g2_count = 10 - - migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - self.target_enforcer.load_policy() - - target_g2 = self.target_enforcer.get_named_grouping_policy("g2") self.assertEqual( - len(target_g2), - expected_g2_count, - f"Expected {expected_g2_count} g2 rules from file, got {len(target_g2)}", + self.target_enforcer.get_grouping_policy(), + [["user^alice", "role^custom", "lib^demo"]], ) - # Verify a sample of expected g2 rules from the file - self.assertIn( - [ - make_action_key(permissions.DELETE_LIBRARY.identifier), - make_action_key(permissions.EDIT_LIBRARY_CONTENT.identifier), - ], - target_g2, - ) - self.assertIn( - [ - make_action_key(permissions.MANAGE_LIBRARY_TEAM.identifier), - make_action_key(permissions.VIEW_LIBRARY_TEAM.identifier), - ], - target_g2, - ) - - def test_migrate_idempotent(self): - """Test that running migration twice doesn't duplicate policies. - - Expected Result: - - Running migration twice results in same number of policies - - No duplicate policies are created in the database - - Duplicate detection works correctly for file-to-DB migration - """ + def test_migration_is_idempotent(self): + """Importing the same file twice does not create duplicate rows.""" migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - self.target_enforcer.load_policy() - - first_policy_count = len(self.target_enforcer.get_policy()) - first_g2_count = len(self.target_enforcer.get_named_grouping_policy("g2")) - migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - self.target_enforcer.load_policy() - - second_policy_count = len(self.target_enforcer.get_policy()) - second_g2_count = len(self.target_enforcer.get_named_grouping_policy("g2")) - - self.assertEqual( - first_policy_count, - second_policy_count, - "Running migration twice should not duplicate policies", - ) - self.assertEqual( - first_g2_count, - second_g2_count, - "Running migration twice should not duplicate g2 rules", - ) - duplicates = CasbinRule.objects.values("v0", "v1", "v2").annotate(total=Count("*")).filter(total__gt=1) - duplicate_list = list(duplicates) - self.assertEqual( - len(duplicate_list), - 0, - f"Found {len(duplicate_list)} duplicate policies in database: {duplicate_list}", - ) - - def test_migrate_complete_file_contents(self): - """Test that all policy types from the file are migrated correctly. - - Expected Result: - - All regular policies (p) are migrated (132 rules) - - No role assignments (g) - these come from database - - All action inheritance rules (g2) are migrated (10 rules) - """ - migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - - self.assertEqual( - len(self.target_enforcer.get_policy()), - 132, - "Should have 132 regular policies from file", - ) - self.assertEqual( - len(self.target_enforcer.get_grouping_policy()), - 0, - "Should have 0 g rules (not stored in file)", - ) - self.assertEqual( - len(self.target_enforcer.get_named_grouping_policy("g2")), - 10, - "Should have 10 g2 rules from file", - ) - - def test_migrate_partial_duplicates(self): - """Test migration when database already has some policies from the file. - - Expected Result: - - Only new policies from file are added to database - - Existing policies are not duplicated - - Mixed state is handled correctly - """ - self.target_enforcer.add_policy( - make_role_key(roles.LIBRARY_ADMIN.external_key), - make_action_key(permissions.DELETE_LIBRARY.identifier), - make_scope_key("lib", "*"), - "allow", - ) - - migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - - target_policies = self.target_enforcer.get_policy() - self.assertEqual( - len(target_policies), - 132, - "Should have 132 policies total, with no duplicates", - ) - - duplicates = CasbinRule.objects.values("v0", "v1", "v2").annotate(total=Count("*")).filter(total__gt=1) - duplicate_list = list(duplicates) - self.assertEqual( - len(duplicate_list), - 0, - f"Found {len(duplicate_list)} duplicate policies in database: {duplicate_list}", - ) - - @data( - ( - make_role_key(roles.LIBRARY_ADMIN.external_key), - make_action_key(permissions.DELETE_LIBRARY.identifier), - make_scope_key("lib", "*"), - ), - ( - make_role_key(roles.LIBRARY_USER.external_key), - make_action_key(permissions.VIEW_LIBRARY.identifier), - make_scope_key("lib", "*"), - ), - ( - make_role_key(roles.LIBRARY_AUTHOR.external_key), - make_action_key(permissions.EDIT_LIBRARY_CONTENT.identifier), - make_scope_key("lib", "*"), - ), - ) - @unpack - def test_migrate_specific_file_policies(self, role, action, scope): - """Test that specific policies from the file are migrated correctly. - - Expected Result: - - Specific policies from authz.policy file are present in database - - Policy format and content are preserved - """ - migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - - target_policies = self.target_enforcer.get_policy() - self.assertIn( - [role, action, scope, "allow"], - target_policies, - f"Policy {role}, {action}, {scope} should be in database", - ) - - @data( - ( - make_action_key(permissions.DELETE_LIBRARY.identifier), - make_action_key(permissions.EDIT_LIBRARY_CONTENT.identifier), - ), - ( - make_action_key(permissions.EDIT_LIBRARY_CONTENT.identifier), - make_action_key(permissions.VIEW_LIBRARY.identifier), - ), - ( - make_action_key(permissions.MANAGE_LIBRARY_TEAM.identifier), - make_action_key(permissions.VIEW_LIBRARY_TEAM.identifier), - ), - ) - @unpack - def test_migrate_specific_action_inheritance(self, parent_action, child_action): - """Test that specific action inheritance rules from file are migrated correctly. - - Expected Result: - - Specific g2 rules from authz.policy file are present in database - - Action inheritance relationships are preserved - """ - migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - - target_g2 = self.target_enforcer.get_named_grouping_policy("g2") - self.assertIn( - [parent_action, child_action], - target_g2, - f"Action inheritance {parent_action} -> {child_action} should be in database", - ) - - def test_migrate_preserves_existing_db_policies(self): - """Test that migration preserves existing database policies not in the file. - - Expected Result: - - Existing database policies that aren't in the file remain intact - - File policies are added to the database - - No policies are removed - """ - custom_policy = [ - make_role_key("custom_admin"), - make_action_key("custom_action"), - make_scope_key("org", "custom"), + duplicates = CasbinRule.objects.values("ptype", "v0", "v1", "v2", "v3").annotate( + total=Count("*") + ).filter(total__gt=1) + self.assertEqual(list(duplicates), []) + + def test_migration_preserves_existing_policies(self): + """Importing a file preserves policies already stored in the target.""" + existing_policy = [ + make_role_key("existing"), + make_action_key("existing.read"), + make_scope_key("org", "example"), "allow", ] - self.target_enforcer.add_policy(*custom_policy) + self.target_enforcer.add_policy(*existing_policy) migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - target_policies = self.target_enforcer.get_policy() - self.assertEqual(len(target_policies), 133, "Should have 132 file policies + 1 custom policy") - self.assertIn(custom_policy, target_policies, "Custom database policy should be preserved") - - def test_migrate_preserves_user_role_assignments_in_db(self): - """Test that migration preserves user role assignments (g rules) in the database. + self.assertIn(existing_policy, self.target_enforcer.get_policy()) - Expected Result: - - User role assignments in database are preserved - - File policies (p and g2) are added - - No user assignments are removed - """ - self.target_enforcer.add_grouping_policy( - make_user_key("user-1"), - make_role_key(roles.LIBRARY_ADMIN.external_key), - make_scope_key("lib", "demo"), - ) - self.target_enforcer.add_grouping_policy( - make_user_key("user-2"), - make_role_key(roles.LIBRARY_USER.external_key), - make_scope_key("lib", "*"), - ) + def test_migration_preserves_existing_role_assignments(self): + """Importing a file preserves role assignments already in the target.""" + existing_assignment = [ + make_user_key("bob"), + make_role_key("existing"), + make_scope_key("lib", "example"), + ] + self.target_enforcer.add_grouping_policy(*existing_assignment) migrate_policy_between_enforcers(self.source_enforcer, self.target_enforcer) - target_grouping = self.target_enforcer.get_grouping_policy() - self.assertEqual(len(target_grouping), 2, "User role assignments should be preserved") - self.assertIn( - [ - make_user_key("user-1"), - make_role_key(roles.LIBRARY_ADMIN.external_key), - make_scope_key("lib", "demo"), - ], - target_grouping, - ) - - target_policies = self.target_enforcer.get_policy() - self.assertEqual(len(target_policies), 132, "All 132 policies from file should be loaded") + self.assertIn(existing_assignment, self.target_enforcer.get_grouping_policy()) + + def test_builtin_roles_explicitly_include_previously_implied_permissions(self): + """Removing action inheritance does not reduce built-in role permissions.""" + schema_policies = seed_static_authorization_policies(self.target_enforcer) + permissions_by_role = {} + for role, action, _scope, _effect in schema_policies: + permissions_by_role.setdefault(role, set()).add(action) + + for parent, implied in REMOVED_ACTION_RELATIONSHIPS: + parent_action = make_action_key(parent) + implied_action = make_action_key(implied) + roles_with_parent = { + role for role, actions in permissions_by_role.items() if parent_action in actions + } + self.assertTrue(roles_with_parent, f"No built-in role includes {parent_action}") + self.assertEqual( + { + role + for role in roles_with_parent + if implied_action not in permissions_by_role[role] + }, + set(), + f"Built-in roles with {parent_action} must explicitly include {implied_action}", + ) diff --git a/src/openedx_authz/tests/test_filter.py b/src/openedx_authz/tests/test_filter.py index 475aa533..bf6dd4ab 100644 --- a/src/openedx_authz/tests/test_filter.py +++ b/src/openedx_authz/tests/test_filter.py @@ -106,19 +106,12 @@ def test_filter_grouping_rules_only(self): self.assertEqual(f.ptype, ["g"]) self.assertIn("g", f.ptype) - def test_filter_action_grouping_only(self): - """Test filter for action grouping (g2) only.""" - f = Filter(ptype=["g2"]) - self.assertEqual(f.ptype, ["g2"]) - self.assertIn("g2", f.ptype) - def test_filter_multiple_policy_types(self): """Test filter for multiple policy types.""" - f = Filter(ptype=["p", "g", "g2"]) - self.assertEqual(len(f.ptype), 3) + f = Filter(ptype=["p", "g"]) + self.assertEqual(len(f.ptype), 2) self.assertIn("p", f.ptype) self.assertIn("g", f.ptype) - self.assertIn("g2", f.ptype) def test_filter_user_permissions(self): """Test filter for a specific user's permissions.""" @@ -151,12 +144,6 @@ def test_filter_specific_action(self): self.assertEqual(f.ptype, ["p"]) self.assertEqual(f.v1, [make_action_key("edit"), make_action_key("delete")]) - def test_filter_action_hierarchy(self): - """Test filter for action grouping hierarchy.""" - f = Filter(ptype=["g2"], v0=[make_action_key("manage")]) - self.assertEqual(f.ptype, ["g2"]) - self.assertEqual(f.v0, [make_action_key("manage")]) - def test_filter_deny_policies(self): """Test filter for deny effect policies.""" f = Filter(ptype=["p"], v3=["deny"]) diff --git a/src/openedx_authz/tests/test_migrations.py b/src/openedx_authz/tests/test_migrations.py index 53b63a66..5fe5422d 100644 --- a/src/openedx_authz/tests/test_migrations.py +++ b/src/openedx_authz/tests/test_migrations.py @@ -1,11 +1,15 @@ """Unit Tests for openedx_authz migrations.""" +from importlib import import_module from unittest.mock import MagicMock, patch +from casbin_adapter.models import CasbinRule from ddt import data, ddt, unpack +from django.apps import apps from django.contrib.auth import get_user_model from django.contrib.auth.models import Group from django.core.management import CommandError, call_command +from django.db import DatabaseError from django.test import TestCase from opaque_keys.edx.django.models import CourseKeyField @@ -59,6 +63,65 @@ User = get_user_model() + +class TestRemoveActionGroupingMigration(TestCase): + """The model cleanup removes obsolete action-grouping rows only.""" + + def test_remove_action_grouping_rows(self): + """The migration deletes g2 rows and preserves explicit policies.""" + CasbinRule.objects.create(ptype="g2", v0="act^manage", v1="act^view") + explicit_policy = CasbinRule.objects.create( + ptype="p", + v0="role^admin", + v1="act^view", + v2="lib^*", + v3="allow", + ) + + migration = import_module("openedx_authz.migrations.0012_remove_action_grouping") + schema_editor = MagicMock() + schema_editor.connection.alias = "default" + migration.remove_action_grouping(apps, schema_editor) + + self.assertFalse(CasbinRule.objects.filter(ptype="g2").exists()) + self.assertTrue(CasbinRule.objects.filter(pk=explicit_policy.pk).exists()) + + def test_missing_casbin_model_does_not_block_migration(self): + """A missing historical CasbinRule model does not block migration.""" + historical_apps = MagicMock() + historical_apps.get_model.side_effect = LookupError("CasbinRule is unavailable") + migration = import_module("openedx_authz.migrations.0012_remove_action_grouping") + + with self.assertLogs(migration.logger, level="WARNING") as logs: + migration.remove_action_grouping(historical_apps, MagicMock()) + + self.assertIn("Skipping action-grouping cleanup", logs.output[0]) + + def test_database_error_does_not_block_migration(self): + """A database cleanup error does not block migration.""" + historical_apps = MagicMock() + casbin_rule = historical_apps.get_model.return_value + casbin_rule.objects.using.return_value.filter.return_value.delete.side_effect = DatabaseError("unavailable") + schema_editor = MagicMock() + schema_editor.connection.alias = "authz" + migration = import_module("openedx_authz.migrations.0012_remove_action_grouping") + + with self.assertLogs(migration.logger, level="WARNING") as logs: + migration.remove_action_grouping(historical_apps, schema_editor) + + self.assertIn("Could not remove action-grouping rows", logs.output[0]) + + def test_cleanup_uses_migration_database(self): + """The cleanup runs against the database selected by the schema editor.""" + historical_apps = MagicMock() + schema_editor = MagicMock() + schema_editor.connection.alias = "authz" + migration = import_module("openedx_authz.migrations.0012_remove_action_grouping") + + migration.remove_action_grouping(historical_apps, schema_editor) + + historical_apps.get_model.return_value.objects.using.assert_called_once_with("authz") + # Specify a unique prefix to avoid collisions with existing data OBJECT_PREFIX = "tmlp_" diff --git a/src/openedx_authz/tests/test_utils.py b/src/openedx_authz/tests/test_utils.py index 7c68574b..9277b3cf 100644 --- a/src/openedx_authz/tests/test_utils.py +++ b/src/openedx_authz/tests/test_utils.py @@ -1,4 +1,4 @@ -"""Test utilities for creating namespaced keys using class constants.""" +"""Test utilities for authorization schemas and namespaced keys.""" from types import SimpleNamespace from unittest.mock import MagicMock, patch @@ -15,11 +15,30 @@ ScopeData, UserData, ) +from openedx_authz.engine.renderer import PolicyRenderer +from openedx_authz.engine.schema.compilation import SchemaCompiler +from openedx_authz.engine.schema.discovery import SchemaDiscovery +from openedx_authz.engine.schema.loading import SchemaLoader from openedx_authz.utils import get_waffle_flag_states FLAG_NAME = "authz.enable_course_authoring" +def seed_static_authorization_policies(target_enforcer) -> list[list[str]]: + """Seed schema-defined role-permission policies.""" + discovered = SchemaDiscovery( + passed_in_directories=["openedx_authz/authz/schema"] + ).discover() + documents = SchemaLoader().load(discovered) + schema = SchemaCompiler().compile(documents) + policies = [row.as_policy() for row in PolicyRenderer().render(schema).rows] + + for policy in policies: + target_enforcer.add_policy(*policy) + + return policies + + def make_policy(role_key: str, action_key: str, scope_key: str, effect: str = "allow") -> list[str]: """Create a policy.