diff --git a/lms/djangoapps/courseware/docs/decisions/permissions.rst b/lms/djangoapps/courseware/docs/decisions/permissions.rst new file mode 100644 index 0000000000..a76d5a5fa7 --- /dev/null +++ b/lms/djangoapps/courseware/docs/decisions/permissions.rst @@ -0,0 +1,179 @@ +1. Use `django-rules`_ for Permissions and Tracks +************************************************* + +~~~~~~~ +Context +~~~~~~~ + +`OEP-9`_ mandates the use of `django-rules`_ to provide a flexible +implementation of permissions in edX applications. `edx-platform`_ +predates that OEP, and uses a home-grown API for checking permissions. +That API, `has_access`_, uses predominately role-based checks and +implicit permissions, rather than explicitly named permissions. That is, +`has_access`_ is called to check if a user has a role in a course, +and then a users ability to perform an action is inferred from +that role. As a result, it is difficult to separate the roles in +the edx-platform system from the permissions that are granted to those +roles (or to grant existing permissions to new roles). + +Similarly, most permissions relating to student course access are +based on the users enrollment in a particular track, and are checked +by examining the `CourseMode`_ of that enrollment. As a result, adding +new ``CourseModes`` with similar but not identical permissions requires +numerous distributed code changes. + +.. _OEP-9: https://open-edx-proposals.readthedocs.io/en/latest/oep-0009-bp-permissions.html +.. _CourseMode: https://github.com/edx/edx-platform/blob/master/common/djangoapps/course_modes/models.py#L37 +.. _edx-platform: https://github.com/edx/edx-platform + +~~~~~~~~ +Decision +~~~~~~~~ + +We will update all uses of `has_access`_, and all `CourseMode`_ membership +checks, to use `django-rules`_ and named permissions. + +Plan of Action +============== + +#. Write a subclass of ``Predicate`` to allow for non-booleans to be returned during + evaluation. +#. Convert built-in predicates to non-boolean predicates in edx-platform. +#. For each caller of ``has_access``: + + #. Convert the caller to use ``user.has_perm`` instead. + #. Implement the new permission created in 1. by referencing to the previous + ``has_access`` call. + +#. Refactor contents of ``has_access`` out into their own predicates that can + be used to implement specific permissions. +#. For each place that checks if a user is enrolled in a specific track: + + #. Convert the that check to use ``user.has_perm`` for a named permission + #. Implement that permission by checking + +Details +======= + +Subclass ``Predicate`` +---------------------- + +The `Predicate`_ class provided by `django-rules`_ takes some +pains to make sure that the results of predicates are explicitly booleans, +rather than just being objects that are truthy. In order to return objects +like `AccessResponse`_, which may encode additional data about the +particular predicate that failed, we need to modify `Predicate`_. + +In particular, we will need to remove two instances of explicit conversion +to boolean: + +* https://github.com/dfunckt/django-rules/blob/master/rules/predicates.py#L154 +* https://github.com/dfunckt/django-rules/blob/master/rules/predicates.py#L214 + +There may be other spots that need adjustment as well, to make sure +that we always return the non-boolean predicate results through, given +the option. We also need a policy for what happens if multiple +non-boolean predicates are being combined with ``&`` and ``|``. Until +proven otherwise, my recommendation is that the first such predicate is +returned. We could in the future add functionality to return a list of all +failing predicates. + +Additionally, https://github.com/dfunckt/django-rules/blob/master/rules/predicates.py#L183 +should convert ``other`` to a non-boolean predicate if it isn't already. +Note, though, that this won't covert a boolean-only predicate to a +non-boolean predicate if the boolean-only predicate is first in the chain. + +Finally, we need to make sure that ``__invert__`` doesn't lose error +messages (https://github.com/dfunckt/django-rules/blob/master/rules/predicates.py#L173) + +.. _django-rules: https://github.com/dfunckt/django-rules +.. _AccessResponse: https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/access_response.py#L10 +.. _Predicate: https://github.com/dfunckt/django-rules/blob/master/rules/predicates.py#L47 + +Convert built-in predicates to non-boolean predicates in edx-platform +--------------------------------------------------------------------- + +`django-rules`_ includes a number of built-in predicates related to standard +django permissions. We should make it easy to convert an existing predicate +into a non-boolean response predicate, and provide convenience versions of +the built-in predicates in edx-platform that have already been converted. +However, we could consider doing this work on-demand as we need the built-ins, +rather than up front. The risk is that it would be easy for future developers +to miss the existence of the edx-platform versions if they aren't commonly +in use already in edx-platform. + +Convert callers of `has_access`_ to use `user.has_perm`_ +-------------------------------------------------------- + +Currently, the LMS uses `has_access`_ to check if a given user has a particular +role on a particular object (usually a course or an xblock). From that, it +assumes various permissions. The primary goal of this project is to convert +those implicit permissions into explicit named permissions that are tied +to roles by the use of various predicates. + +To bootstrap this process, we can wrap `has_access`_ in named permissions by: + +#. Convert each caller of `has_access`_ to use `user.has_perm`_ instead. +#. Implement the new permission created in 1. by referencing to the previous + `has_access`_ call. + +This work can be done incrementally, one call to `has_access`_ at a time, +and can be parallelized. However, at present, there are ~150 calls to +`has_access`_ in edx-platform, so this is not an insignificant amount of +work. + +.. _has_access: https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/access.py#L103 +.. _user.has_perm: https://docs.djangoproject.com/en/2.1/ref/contrib/auth/#django.contrib.auth.models.User.has_perm + +Refactor contents of ``has_access`` +----------------------------------- + +As implemented, `has_access`_ has many subclauses to handle the various +roles and object types. With `django-rules`_, those clauses could be converted +to smaller individual predicates, either divided by roles, object types, +or both. These predicates would then be simpler to test and to use in +determining future permissions. + +Convert track membership tests to permissions +--------------------------------------------- + +Future work in the same vein would be to convert current usage of track +membership into `user.has_perm`_ checks. This would allow disaggregation +of edx-platform features and would make it easier to add new tracks +with variations of those features. + +Offramps +======== + +The primary offramp would be suspending the project after converting all +callers of `has_access`_ to use `user.has_perm`_. If we have more time, +then refactoring `has_access`_ would be a definite positive, but not +required. If we are forced to cut scope, then only partially completing +the conversion of `has_access`_ would be an improvement, perhaps with +the addition of deprecation warnings for direct callers to `has_access`_ +so that we can track the remaining work with INCR tickets. + +~~~~~~ +Status +~~~~~~ + +Proposed + +~~~~~~~~~~~~ +Consequences +~~~~~~~~~~~~ + +When the conversion of `has_access`_ has been completed, it will be easier +to add additional conditions to various permissions checks on specific objects. +It will also allow those conditions (predicates) to be written in +a location that is central to the app they are responsible for, rather +than requiring that they be added to `access.py`_. + +.. _access.py: https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/access.py + +When the conversion of `CourseMode`_ membership checks has been completed, +it will be easier to add new `CourseMode`_ types with similar permissions +schema to the codebase. It will also open the way towards making `CourseMode`_ +permissions be data-driven, rather than being code specific, which would +allow configuration-time specification of `CourseMode`_, rather than requiring +the current combination of code and database entries.