Record a plan for converting has_access to user.has_perm
This commit is contained in:
Calen Pennington
179
lms/djangoapps/courseware/docs/decisions/permissions.rst
Normal file
179
lms/djangoapps/courseware/docs/decisions/permissions.rst
Normal file
@@ -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.
|
||||
Reference in New Issue
Block a user