feat: Course roles service #33734
feat: Course roles service #33734
Conversation
* feat: add CourseRolesService model * feat: add CourseRolesRole model * feat: add CourseRolesPermission model * feat: add CourseRolesRolePermissions model * feat: add CourseRolesUserRole model * feat: remove wrong unique_together in CourseRolesUserRole * feat: add coures_roles app to cms and lms common envs * feat: add migrations * feat: delete wrong migration * feat: change many to many relationships to foreign keys * feat: add many to many relationships between role-user and role-permission * feat: add initial migration * docs: add readme file * docs: update readme file * chore: add course_roles in quality matrix CI * feat: remove fields related to custom roles * docs: update docstrings * feat: add minimal string representation change * feat: remove descriptions from role and permission tables * feat: swich the role to service relationship into a many to many relationship * docs: update role docstring * feat: update initial migration * feat: change on_delete for org and coures on UserRole model to cascade * feat: update initial migration * docs: update CourseRolesPermission model docstring
* feat: add permission_check function def * test: add permission_check base test case * test: add permission check tests * feat: add permission check logic * test: remove allowed field from role permission * test: update tests * feat: split permission_check function * feat: update CourseRolesUserRole model * feat: update initial migration * test: rewrite the permission check tests and add new cases * chore: update unit test shards * docs: update docstrings
* feat: add migration to load permissions in the database * feat: add Permission enum * feat: change Permission enum name to CourseRolesPermission * feat: replace permission constants with the CourseRolesPermission enum * feat: add unique decorator to permissions enum * feat: add course_roles_permissions dict with names and descriptions (with i18n) * docs: add CourseRolesPermission docstring * style: fix pylint errors * style: fix pylint errors * docs: add permissions module docstring
* feat: Roles 15 - permission checks back end changes part 1 (#33347) * test: add test cases for permission list check functions * test: update tests * feat: add helper functions to check lists of permissions * style: improve code style * feat: add course roles checks in the contentstore app * feat: add course roles checks in the student app * feat: add course roles checks in the lms discussion app * feat: add course roles checks in the lms instructor app * feat: add course roles checks in the Learning Sequences package * style: fix code style * fix: course_permission_check calls * feat: add validation for AnonymousUser in course permission check helper functions * fix: disable some pylint warnings * test: update number of querys asserted in has_course_author_access * feat: add helper functions to check course or organization permissions * test: update course_roles tests * feat: replace course or organization helper functions in auth * docs: update course_roles docstrings * feat: ROLES-23 Create permissions in db table (#33394) * feat: add migration to load permissions in the database * feat: add Permission enum * feat: change Permission enum name to CourseRolesPermission * feat: replace permission constants with the CourseRolesPermission enum * feat: add unique decorator to permissions enum * feat: add course_roles_permissions dict with names and descriptions (with i18n) * docs: add CourseRolesPermission docstring * style: fix pylint errors * style: fix pylint errors * docs: add permissions module docstring * feat: add helper function to get user permissions for a course * test: add test for get_all_user_permissions_for_a_course * feat: add views to coures roles api * test: add test for course roles views * feat: add urls for course roles api * feat: add course roles api urls to lms and cms * docs: add comment to indicate which urls are from course roles api * feat: add translation to ValueError exception message * feat: add translation to ValueError exception message * feat: raise exeption if course does not exist * feat: add instance permissions in get_all_user_permissions_for_a_course helper * feat: improve validations in get_all_user_permissions_for_a_course * feat: improve validations in UserPermissionsView * test: update get user permissions tests * docs: update UserPermissionsView docstring * feat: change message errors * docs: update docstrings in test_views * fix: add missing super method call in a class * fix: add password to test user * fix: chain re-raising exceptions
hsinkoff
force-pushed
the
CourseRoles_service
branch
2 times, most recently
from
1c52fc0 to
7433fa7
Compare
hsinkoff
force-pushed
the
CourseRoles_service
branch
from
7433fa7 to
7b4a7dc
Compare
| except InvalidKeyError as exc: | ||
| raise ParseError('Invalid course_id parameter') from exc | ||
| try: | ||
| user = User.objects.get(pk=user_id) |
There was a problem hiding this comment.
Is any user allowed to see any other user's set of permissions for a given course? This might leak a lot of information. FYI to @feanil on the security aspect of this.
There was a problem hiding this comment.
I agree, we should not let an actor look up the permissions information for any arbitrary user, besids privacy concerns, this would also lead to a user enumeration vulnerability, and ease potential phishing attacks by making it easier for an attacker build a better profile of the user.
I think the endpoint should be limited to the user making the request unless the user is a staff or superuser of the system.
There was a problem hiding this comment.
We will be limiting the endpoint to return the permissions of the user making the request. If at a later date a use case for additional access (is_staff or superuser or masquerading user) is found, the endpoint can be updated at that time.
| UserRole.objects.create( | ||
| user=self.user_1, role=self.role_2, course_id=self.course_1.id, org=self.organization_1 | ||
| ) | ||
| self.clear_caches() |
There was a problem hiding this comment.
If we're going to sometimes modify user roles in the same request where we're going to make queries on it, maybe we should also have an API call that will encapsulate UserRole.objects.create(), and let that new API call also remove the request cache entry for that user (i.e. do its own cache invalidation)?
| def __init__(self, *roles): | ||
| self.role = roles | ||
|
|
||
| def check(self, user, instance=None): |
There was a problem hiding this comment.
What happens in this method if instance is None?
There was a problem hiding this comment.
Considering that this rule will be executed to validate the permissions with the method user.has_perm(permission, course), I will modify the HasPermissionRule so that if a course is not passed when calling .has_perm, the permission will be validated for all courses, which would be the behavior expected by django. And I will modify HasForumsRolesRule and HasRolesRule to return False if the permission is queried for all courses. I need to do this so that these types of queries (which are currently not used) don't break.
|
|
||
|
|
||
| perms[f'course_roles.{CourseRolesPermission.MANAGE_CONTENT.value.name}'] = ( | ||
| HasRolesRule('staff', 'instructor') |
There was a problem hiding this comment.
Isn't the role to permissions mapping already encoded into the data tables with this PR? Why do we still have to check against these roles here?
There was a problem hiding this comment.
The roles checks are being included to ensure that a user who already has a student_courseaccessrole continues to have the proper access. The current plan is to add permissions checks to ensure the definitions of the permissions match the access granted by the code. This code ensures that a user with a student_courseaccessrole will retain the access their role should have even if their role has not yet been migrated to course_roles.
Existing data is planned to be migrated one role at a time. Once all existing student_courseaccessrole role data has been migrated to the course_roles schema, it will no longer be necessary to include the roles checks in this permissions file. The role checks for a role will be removed from this file as each role is migrated.
| in the course_roles_permission table in database. | ||
|
|
||
| The readable_name and description are used in the UI. | ||
| """ |
There was a problem hiding this comment.
Long term note: One architectural failure mode I'm worried about here is if people find it difficult to add new permissions and instead start using other permissions that imply a role.
For instance, say someone wants to add a permission for the Staff Debug information under Components in the LMS rendering. But adding a permission and doing the data migration sounds hard. Then they browse through this class and think to themselves, "Well, if they can VIEW_ALL_CONTENT AND MANAGE_STUDENTS, then they're course staff and they should be able to see the Staff Debug info..." and we get into the mode where people are re-deriving roles implicitly from permissions and extrapolating those onto other implied permissions.
I'm not saying we're doomed to this. Just that it's a risk and that we'll need docs and outreach, presentations, etc. in order to make sure that developers working on this will have the right mindset. This gets more likely if there's any real friction to adding permissions–long approval process, unclear docs, any perception of operational risk in the event of a rollback, etc.
Come to think on it, this whole thing would make for a great Open edX Conference talk or a Meetup presentation.
There was a problem hiding this comment.
We agree this presents a risk and have been thinking through strategies to mitigate for this risk. We do not think this presents an immediate risk, but plan to add documentation around how and when to add a permission in OEP-66 and within the djangoapps/course_roles/docs folder.
| try: | ||
| user = User.objects.get(pk=user_id) | ||
| except User.DoesNotExist as exc: | ||
| raise NotFound(f'user_id: {user_id} not found') from exc |
There was a problem hiding this comment.
FWIW, this seems like it shouldn't be a reachable state any longer, given that we're requiring a logged in user checking against themselves. You don't have to get rid of it, but I don't see how you'd test it either?
There was a problem hiding this comment.
I agree that it should not be possible to get to this state, but considering that we plan to allow querying other users' information in the near future, I prefer to leave this as is, and add a test that validates specifically this part by patching the User.get method
Description
This pull request adds the new CourseRoles model that will be used for creating roles with associated permissions that can be assigned to users on a course, as an org-wide course role (the user has access as if they have the role on all courses in the org), or instance-wide course role (the user has access as if they have the role on all courses within the instance).
This initial PR does not create any roles, but does setup the ability to do so in the future.
Supporting information
This PR is the initial work for the Tech Spec related to Course Roles.
Testing instructions
In order to run the code fully, a waffle flag (course_roles.use_permission_checks) should be created. If the waffle flag is not present or is false, the permissions checks will return false. Only if the waffle flag is true will the permissions checks check the db for access.
There is no intended change in functionality for a user. Testing should reflect no noticeable user experience changes as there are currently no course_roles_roles created and no users who are assigned the new course_roles_roles.
Other information
openedx/core/djangoapps/course_roles/docs/decisions/0001-course_level_roles.rst provides context on these changes and may be a good starting point for reviewing the PR
This was originally included as a part of a larger draft PR that is being split into multiple PRs.