180 lines
8.0 KiB
ReStructuredText
180 lines
8.0 KiB
ReStructuredText
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.
|