From 35a20d0e48da8d29c7e4f754d4b6ac9078c922c6 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 16 Apr 2019 14:10:43 -0400 Subject: [PATCH] Write up Bridgekeeper proposal for permissions --- ... => 0002-permissions-via-django-rules.rst} | 6 +- .../0003-permissions-via-bridgekeeper.rst | 148 ++++++++++++++++++ 2 files changed, 151 insertions(+), 3 deletions(-) rename lms/djangoapps/courseware/docs/decisions/{permissions.rst => 0002-permissions-via-django-rules.rst} (98%) create mode 100644 lms/djangoapps/courseware/docs/decisions/0003-permissions-via-bridgekeeper.rst diff --git a/lms/djangoapps/courseware/docs/decisions/permissions.rst b/lms/djangoapps/courseware/docs/decisions/0002-permissions-via-django-rules.rst similarity index 98% rename from lms/djangoapps/courseware/docs/decisions/permissions.rst rename to lms/djangoapps/courseware/docs/decisions/0002-permissions-via-django-rules.rst index a76d5a5fa7..cd58c5a542 100644 --- a/lms/djangoapps/courseware/docs/decisions/permissions.rst +++ b/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 diff --git a/lms/djangoapps/courseware/docs/decisions/0003-permissions-via-bridgekeeper.rst b/lms/djangoapps/courseware/docs/decisions/0003-permissions-via-bridgekeeper.rst new file mode 100644 index 0000000000..38739775d2 --- /dev/null +++ b/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.