Merge pull request #20250 from cpennington/bridgekeeper-adr

Write up Bridgekeeper proposal for permissions

lms/djangoapps/courseware/docs/decisions/0002-permissions-via-django-rules.rst

@@ -1,5 +1,5 @@
-1. Use `django-rules`_ for Permissions and Tracks
-*************************************************
+0002. Use `django-rules`_ for Permissions and Tracks
+****************************************************
 
 ~~~~~~~
 Context
@@ -157,7 +157,7 @@ so that we can track the remaining work with INCR tickets.
 Status
 ~~~~~~
 
-Proposed
+Accepted
 
 ~~~~~~~~~~~~
 Consequences

lms/djangoapps/courseware/docs/decisions/0003-permissions-via-bridgekeeper.rst

@@ -0,0 +1,148 @@
+0003. Use `bridgekeeper`_ for Permissions and Tracks
+****************************************************
+
+~~~~~~~
+Context
+~~~~~~~
+
+In `0002. Use django-rules for Permissions and Tracks`_, the case was
+laid out for using `django-rules`_ to manage permissions. In the intervening
+time, no active work has been done to complete the conversion. During that
+period, authorization checking was observed to be a significant performance
+in several list views of courses (the CourseListView from course_api and
+the student dashboard view). In both cases, access checking had to be performed
+after the CourseOverview had been loaded from the database, and resulted
+in significant numbers of additional queries.
+
+`bridgekeeper`_ is a library with an interface that is very similar to
+`django-rules`_, except that it also supports specifying the permissions
+as Django query filters. By inspection, it seems to support non-boolean
+return values for the Python-based permissions checks.
+
+.. _bridgekeeper: https://bridgekeeper.readthedocs.io/en/latest/index.html
+.. _django-rules: https://github.com/dfunckt/django-rules
+
+~~~~~~~~
+Decision
+~~~~~~~~
+
+Rather than use `django-rules`_ in edx-platform, we will convert permissions
+checks to use `bridgekeeper`_. The conversion will follow a similar outline
+to the plan in `0002`_.
+
+We will update all uses of `has_access`_, and all `CourseMode`_ membership
+checks, to use `bridgekeeper`_ and named permissions.
+
+.. _`0002. Use django-rules for Permissions and Tracks`: https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/docs/decisions/0002-permissions-via-django-rules.rst
+.. _`0002`: https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/docs/decisions/0002-permissions-via-django-rules.rst
+
+Plan of Action
+==============
+
+#. 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. These `bridgekeeper`_ rules would implement
+      ``query`` to raise an exception when called, and implement ``check``
+      to pass through to ``has_access``.
+
+#. Refactor contents of ``has_access`` out into their own predicates that can
+   be used to implement specific permissions. Where possible, implement
+   ``query``, either manually by specifying a custom rule, or by using
+   ``Attribute`` or ``Relation`` from `bridgekeeper`_.
+
+#. For any listing views that are currently checking access as a filter
+   after querying the database, improve their performance by using the
+   `bridgekeeper`_ permission to filter the queryset instead. If those
+   views need to display the existence (but not contents) of the object,
+   two separate permissions can be used: the first to limit the query
+   to only those objects to be displayed, and the second to check whether
+   the user has permissions to view the contents of the object.
+
+#. 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
+=======
+
+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 `bridgekeeper`_, those clauses could be converted
+to smaller individual Rules, 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.
+
+Converting all of these checks to use `bridgekeeper`_ will allow list-view
+query performance to be optimized.