Add permission checks to role checks - lib/api/permissions#34008
Add permission checks to role checks - lib/api/permissions#34008hsinkoff merged 2 commits intoCourseRolesfrom
Conversation
hsinkoff
force-pushed
the
ROLES-69-roles_checks_to_permission_checks
branch
2 times, most recently
from
c50c48d to
b630279
Compare
hsinkoff
force-pushed
the
ROLES-69-roles_checks_to_permission_checks
branch
from
b630279 to
82580c5
Compare
dianakhuang
left a comment
dianakhuang
left a comment
There was a problem hiding this comment.
I think this seems reasonable. The only thing that I'd be concerned about is the performance impact of adding new checks. It seems like the fields are already attached to the existing request user, though. One other thing is that I'd be looking for updated/new tests as this roles out since it seems like it would be really bad if this breaks in the future.
We are aware that adding checks might have a performance impact, but it will also mitigate the risks of switching course level authorization to CourseRoles from CourseAccessRoles and CommentClientRoles. We think this is the correct tradeoff, but agree it will be necessary to monitor performance when this is rolled out. It will be controlled by a waffle flag initially so it will be possibly to quickly stop the additional db hits. We have added new tests for checking permissions, but are unsure what other tests to add until a user can be assigned a role built with the permissions. Do you have any suggestions of specific places tests can/should be added? |
dianakhuang
left a comment
dianakhuang
left a comment
There was a problem hiding this comment.
Great! Just as long as there are plans to keep an eye on performance and write tests later.
3 participants
Description
This PR adds permissions checks alongside the existing roles checks for course level permissions in lib/api/permissions. These permissions are designed to be assigned to course level roles that will be assigned to a user.
This PR should have no immediate impact on any users. Later PRs will create new course_roles and migrate existing student_courseaccessrole user roles to the new course_roles user roles. Only after that time will these permissions grant access using lib/api/permissions.
Supporting information
course_roles tech spec
Testing instructions
Testing will be completed on the feature branch once additional services have been updated to add permissions checks. Testing will involve creating a course_roles role and assigning it to a user. This user will then be used to confirm the correct access is granted to the user.
Other information
This is a PR to a feature branch.