fix: RoleCache.has_role should be case-insensitive #35

shadinaif · 2024-08-15T12:21:47Z

Description

RoleCache compares org in a case-sensitive manner while the value of org is assumed as case-insensitive by the database collation, and by the django admin form. This PR is to fix the issue to make RoleCache behave as the rest of the platform

Supporting information

Testing instructions

Preperation to test:

Assume a normal user in the platform, for example test_user
Assuming we have two courses in demo organization: course-v1:Demo+topic1+index1 and course-v1:demo+topic2+index2. We have the first course with organization Demo in course-overview instead of demo. This is acceptable in the platform

Before the fix:

The admin will not be able to grant test_user an instructor on the entire demo organization, the admin will have to grant it course by course. This is because CourseAccessRole will not allow two similar records like [user: test_user, course_id: "", role: instructor, org: demo] and [user: test_user, course_id: "", role: instructor, org: Demo]. The key constraint treats org as case-insensitive

After the fix:

Adding a single record like [user: test_user, course_id: "", role: instructor, org: demo], [user: test_user, course_id: "", role: instructor, org: Demo], or even [user: test_user, course_id: "", role: instructor, org: DEMO] will make test_user an instructor on all courses as expected

Deadline

Other information

This is an upstream issue. We'll contribute the fix to upstream once we have a code review here

shadinaif · 2024-08-15T12:23:14Z

I'll contribute this to Upstream once we have a code review here

OmarIthawi

@shadinaif few minor notes regarding testing. They're important for upstream contribution.

OmarIthawi · 2024-08-16T06:51:30Z

common/djangoapps/student/roles.py

@@ -90,7 +90,7 @@ def has_role(self, role, course_id, org):
        return any(
            access_role.role in self.get_roles(role) and
            access_role.course_id == course_id and
-            access_role.org == org
+            access_role.org.lower() == org.lower()


OmarIthawi · 2024-08-16T06:54:08Z

common/djangoapps/student/tests/test_roles.py

+            expected_has_role = (
+                target[0] == other_target[0] and
+                (target[1] or '') == (other_target[1] or '') and
+                target[2].lower() == other_target[2].lower()
+            )


Such code is a bit too complicated for testing. Testing cases should have as few branches as possible.

Suggested change

expected_has_role = (

target[0] == other_target[0] and

(target[1] or '') == (other_target[1] or '') and

target[2].lower() == other_target[2].lower()

)

OmarIthawi · 2024-08-16T06:57:19Z

common/djangoapps/student/tests/test_roles.py

        (CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')),
+        (CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'EDX')),


Add expected_has_role to this list to avoid introducing complicated logic in testing

Suggested change

(CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')),

(CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'EDX')),

(CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX'), True), # Should have role

(CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'EDX'), False), # Should have role despite case difference

sounds reasonable, thank you!

it'll not work exactly like this because the test iterates on all records, but I'll find a way 👍🏼

Done 👍🏼

OmarIthawi

@shadinaif please note that I think the fix in the code is pretty fine, but tests are very hard to understand.

I'm just reviewing with very high quality standards i.e. upstream quality, let me know if you think we should stop and focus on making the fix locally and then worry about upstream contribution.

OmarIthawi · 2024-08-16T13:39:32Z

common/djangoapps/student/tests/test_roles.py

+        for _, other_target, other_test_id in self.ROLES:
+            assert cache.has_role(*other_target) == (test_id == other_test_id)


@shadinaif I presume I'm very familiar with the issue being presented here, but the test still isn't understanble.

I think the real cause is that the original test_only_in_role function was cryptic.

In such case I would support creating a new test case e.g. test_only_in_role_case_insensitive so it doesn't have for-loop or at least it doesn't have to be so generic and becomes hard to understand.

shadinaif · 2024-08-18T13:16:26Z

I've update the tests @OmarIthawi I believe it's clear now. Thank you!

OmarIthawi

super!

andrey-canon · 2024-09-09T21:40:58Z

hi @OmarIthawi and @shadinaif, just asking, is this a backport? Or is there an equivalent upstream PR?

shadinaif · 2024-09-10T08:44:31Z

unfortunately, it's not equivalent to upstream. When I tried to create a PR, I found that the code there had changed and a cherry-pick was not possible. I'm planning to report/fix upstream by the end of this month. I'll keep updated @andrey-canon , thank you for checking 👍🏼

OmarIthawi · 2024-09-10T11:43:59Z

Thanks @shadinaif!!

andrey-canon · 2024-09-10T17:41:31Z

Hi @shadinaif thanks for you answer and I was asking because we have a migration file where we track all the custom commits. This file helps us to decide which commits we must include in the next release, so if the commit is not there, there is a chance that we will miss it, so could you please add the commit data into the file?
please check the section colors and change the row color

shadinaif · 2024-09-11T04:49:12Z

Thank you @andrey-canon . I've added it as gray and explained the reason in the sheet. The fix is needed, but this code should be discarded in the upgrade process (this is why I set it as gray). I'll follow up with you after having the fix in upstream

andrey-canon · 2024-09-11T16:05:55Z

d be discarded in the upgrade process (this is why I set it as gray). I'll follow up with you after having the fix in upstream

thanks @shadinaif

shadinaif requested a review from OmarIthawi August 15, 2024 12:21

shadinaif force-pushed the shadinaif/fix-roles-cache branch 2 times, most recently from 9707186 to a738c4f Compare August 15, 2024 13:20

OmarIthawi approved these changes Aug 16, 2024

View reviewed changes

OmarIthawi requested changes Aug 16, 2024

View reviewed changes

shadinaif force-pushed the shadinaif/fix-roles-cache branch from a738c4f to db728cf Compare August 16, 2024 11:09

shadinaif requested a review from OmarIthawi August 16, 2024 11:10

OmarIthawi requested changes Aug 16, 2024

View reviewed changes

fix: RoleCache.has_role should be case-insensitive

bdb520d

shadinaif force-pushed the shadinaif/fix-roles-cache branch from db728cf to bdb520d Compare August 18, 2024 12:48

shadinaif requested a review from OmarIthawi August 18, 2024 13:15

OmarIthawi approved these changes Aug 18, 2024

View reviewed changes

shadinaif merged commit 9c0ce39 into open-release/palm.nelp Aug 19, 2024
40 checks passed

shadinaif deleted the shadinaif/fix-roles-cache branch August 19, 2024 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: RoleCache.has_role should be case-insensitive #35

fix: RoleCache.has_role should be case-insensitive #35

shadinaif commented Aug 15, 2024 •

edited

Loading

shadinaif commented Aug 15, 2024

OmarIthawi left a comment

OmarIthawi Aug 16, 2024

OmarIthawi Aug 16, 2024

OmarIthawi Aug 16, 2024

shadinaif Aug 16, 2024

shadinaif Aug 16, 2024

shadinaif Aug 16, 2024

OmarIthawi left a comment •

edited

Loading

OmarIthawi Aug 16, 2024

shadinaif commented Aug 18, 2024 •

edited

Loading

OmarIthawi left a comment

andrey-canon commented Sep 9, 2024

shadinaif commented Sep 10, 2024

OmarIthawi commented Sep 10, 2024

andrey-canon commented Sep 10, 2024 •

edited

Loading

shadinaif commented Sep 11, 2024

andrey-canon commented Sep 11, 2024

		(CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')),
		(CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'EDX')),

		for _, other_target, other_test_id in self.ROLES:
		assert cache.has_role(*other_target) == (test_id == other_test_id)

fix: RoleCache.has_role should be case-insensitive #35

fix: RoleCache.has_role should be case-insensitive #35

Conversation

shadinaif commented Aug 15, 2024 • edited Loading

Description

Supporting information

Testing instructions

Deadline

Other information

shadinaif commented Aug 15, 2024

OmarIthawi left a comment

Choose a reason for hiding this comment

OmarIthawi Aug 16, 2024

Choose a reason for hiding this comment

OmarIthawi Aug 16, 2024

Choose a reason for hiding this comment

OmarIthawi Aug 16, 2024

Choose a reason for hiding this comment

shadinaif Aug 16, 2024

Choose a reason for hiding this comment

shadinaif Aug 16, 2024

Choose a reason for hiding this comment

shadinaif Aug 16, 2024

Choose a reason for hiding this comment

OmarIthawi left a comment • edited Loading

Choose a reason for hiding this comment

OmarIthawi Aug 16, 2024

Choose a reason for hiding this comment

shadinaif commented Aug 18, 2024 • edited Loading

OmarIthawi left a comment

Choose a reason for hiding this comment

andrey-canon commented Sep 9, 2024

shadinaif commented Sep 10, 2024

OmarIthawi commented Sep 10, 2024

andrey-canon commented Sep 10, 2024 • edited Loading

shadinaif commented Sep 11, 2024

andrey-canon commented Sep 11, 2024

shadinaif commented Aug 15, 2024 •

edited

Loading

OmarIthawi left a comment •

edited

Loading

shadinaif commented Aug 18, 2024 •

edited

Loading

andrey-canon commented Sep 10, 2024 •

edited

Loading