Refactored permissions check for course_wiki pages.

CourseRole names have a new format (type_org.number.run). Previously
when checking if a user was staff for a course wiki type_org/number/run
and type_number format role names were checked by parsing user group names.

This logic has been refactored to first fetch all courses which use the
particular wiki_slug and then use courseware.access.has_access to check if the
user has staff permissions on any of the courses.

LMS-2136

lms/djangoapps/course_wiki/tests/test_access.py

@@ -4,6 +4,7 @@ Tests for wiki permissions
 
 from django.contrib.auth.models import Group
 from student.tests.factories import UserFactory
+from xmodule.modulestore.django import loc_mapper
 from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
@@ -24,21 +25,37 @@ class TestWikiAccessBase(ModuleStoreTestCase):
 
         self.wiki = get_or_create_root()
 
-        self.course_math101 = CourseFactory.create(org='org', number='math101', display_name='Course')
-        self.course_math101_staff = [
-            InstructorFactory(course=self.course_math101.location),
-            StaffFactory(course=self.course_math101.location)
-        ]
+        self.course_math101 = CourseFactory.create(org='org', number='math101', display_name='Course', metadata={'use_unique_wiki_id': 'false'})
+        self.course_math101_staff = self.create_staff_for_course(self.course_math101)
 
         wiki_math101 = self.create_urlpath(self.wiki, course_wiki_slug(self.course_math101))
         wiki_math101_page = self.create_urlpath(wiki_math101, 'Child')
         wiki_math101_page_page = self.create_urlpath(wiki_math101_page, 'Grandchild')
         self.wiki_math101_pages = [wiki_math101, wiki_math101_page, wiki_math101_page_page]
 
+        self.course_math101b = CourseFactory.create(org='org', number='math101b', display_name='Course', metadata={'use_unique_wiki_id': 'true'})
+        self.course_math101b_staff = self.create_staff_for_course(self.course_math101b)
+
+        wiki_math101b = self.create_urlpath(self.wiki, course_wiki_slug(self.course_math101b))
+        wiki_math101b_page = self.create_urlpath(wiki_math101b, 'Child')
+        wiki_math101b_page_page = self.create_urlpath(wiki_math101b_page, 'Grandchild')
+        self.wiki_math101b_pages = [wiki_math101b, wiki_math101b_page, wiki_math101b_page_page]
+
     def create_urlpath(self, parent, slug):
         """Creates an article at /parent/slug and returns its URLPath"""
         return URLPath.create_article(parent, slug, title=slug)
 
+    def create_staff_for_course(self, course):
+        """Creates and returns users with instructor and staff access to course."""
+
+        course_locator = loc_mapper().translate_location(course.id, course.location)
+        return [
+            InstructorFactory(course=course.location),  # Creates instructor_org/number/run role name
+            StaffFactory(course=course.location),  # Creates staff_org/number/run role name
+            InstructorFactory(course=course_locator),  # Creates instructor_org.number.run role name
+            StaffFactory(course=course_locator),  # Creates staff_org.number.run role name
+        ]
+
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class TestWikiAccess(TestWikiAccessBase):
@@ -47,21 +64,15 @@ class TestWikiAccess(TestWikiAccessBase):
         super(TestWikiAccess, self).setUp()
 
         self.course_310b = CourseFactory.create(org='org', number='310b', display_name='Course')
-        self.course_310b_staff = [
-            InstructorFactory(course=self.course_310b.location),
-            StaffFactory(course=self.course_310b.location)
-        ]
-        self.course_310b_ = CourseFactory.create(org='org', number='310b_', display_name='Course')
-        self.course_310b__staff = [
-            InstructorFactory(course=self.course_310b_.location),
-            StaffFactory(course=self.course_310b_.location)
-        ]
+        self.course_310b_staff = self.create_staff_for_course(self.course_310b)
+        self.course_310b2 = CourseFactory.create(org='org', number='310b_', display_name='Course')
+        self.course_310b2_staff = self.create_staff_for_course(self.course_310b2)
 
         self.wiki_310b = self.create_urlpath(self.wiki, course_wiki_slug(self.course_310b))
-        self.wiki_310b_ = self.create_urlpath(self.wiki, course_wiki_slug(self.course_310b_))
+        self.wiki_310b2 = self.create_urlpath(self.wiki, course_wiki_slug(self.course_310b2))
 
     def test_no_one_is_root_wiki_staff(self):
-        all_course_staff = self.course_math101_staff + self.course_310b_staff + self.course_310b__staff
+        all_course_staff = self.course_math101_staff + self.course_310b_staff + self.course_310b2_staff
         for course_staff in all_course_staff:
             self.assertFalse(user_is_article_course_staff(course_staff, self.wiki.article))
 
@@ -70,6 +81,10 @@ class TestWikiAccess(TestWikiAccessBase):
             for course_staff in self.course_math101_staff:
                 self.assertTrue(user_is_article_course_staff(course_staff, page.article))
 
+        for page in self.wiki_math101b_pages:
+            for course_staff in self.course_math101b_staff:
+                self.assertTrue(user_is_article_course_staff(course_staff, page.article))
+
     def test_settings(self):
         for page in self.wiki_math101_pages:
             for course_staff in self.course_math101_staff:
@@ -79,15 +94,27 @@ class TestWikiAccess(TestWikiAccessBase):
                 self.assertTrue(settings.CAN_ASSIGN(page.article, course_staff))
                 self.assertTrue(settings.CAN_ASSIGN_OWNER(page.article, course_staff))
 
+        for page in self.wiki_math101b_pages:
+            for course_staff in self.course_math101b_staff:
+                self.assertTrue(settings.CAN_DELETE(page.article, course_staff))
+                self.assertTrue(settings.CAN_MODERATE(page.article, course_staff))
+                self.assertTrue(settings.CAN_CHANGE_PERMISSIONS(page.article, course_staff))
+                self.assertTrue(settings.CAN_ASSIGN(page.article, course_staff))
+                self.assertTrue(settings.CAN_ASSIGN_OWNER(page.article, course_staff))
+
     def test_other_course_staff_is_not_course_wiki_staff(self):
         for page in self.wiki_math101_pages:
+            for course_staff in self.course_math101b_staff:
+                self.assertFalse(user_is_article_course_staff(course_staff, page.article))
+
+        for page in self.wiki_math101_pages:
             for course_staff in self.course_310b_staff:
                 self.assertFalse(user_is_article_course_staff(course_staff, page.article))
 
         for course_staff in self.course_310b_staff:
-            self.assertFalse(user_is_article_course_staff(course_staff, self.wiki_310b_.article))
+            self.assertFalse(user_is_article_course_staff(course_staff, self.wiki_310b2.article))
 
-        for course_staff in self.course_310b__staff:
+        for course_staff in self.course_310b2_staff:
             self.assertFalse(user_is_article_course_staff(course_staff, self.wiki_310b.article))
 
 
@@ -114,10 +141,7 @@ class TestWikiAccessForNumericalCourseNumber(TestWikiAccessBase):
         super(TestWikiAccessForNumericalCourseNumber, self).setUp()
 
         self.course_200 = CourseFactory.create(org='org', number='200', display_name='Course')
-        self.course_200_staff = [
-            InstructorFactory(course=self.course_200.location),
-            StaffFactory(course=self.course_200.location)
-        ]
+        self.course_200_staff = self.create_staff_for_course(self.course_200)
 
         wiki_200 = self.create_urlpath(self.wiki, course_wiki_slug(self.course_200))
         wiki_200_page = self.create_urlpath(wiki_200, 'Child')
@@ -138,10 +162,7 @@ class TestWikiAccessForOldFormatCourseStaffGroups(TestWikiAccessBase):
 
         self.course_math101c = CourseFactory.create(org='org', number='math101c', display_name='Course')
         Group.objects.get_or_create(name='instructor_math101c')
-        self.course_math101c_staff = [
-            InstructorFactory(course=self.course_math101c.location),
-            StaffFactory(course=self.course_math101c.location)
-        ]
+        self.course_math101c_staff = self.create_staff_for_course(self.course_math101c)
 
         wiki_math101c = self.create_urlpath(self.wiki, course_wiki_slug(self.course_math101c))
         wiki_math101c_page = self.create_urlpath(wiki_math101c, 'Child')

lms/djangoapps/course_wiki/utils.py

@@ -3,7 +3,8 @@ Utility functions for course_wiki.
 """
 
 from django.core.exceptions import ObjectDoesNotExist
-
+from xmodule import modulestore
+import courseware
 
 def user_is_article_course_staff(user, article):
     """
@@ -20,24 +21,24 @@ def user_is_article_course_staff(user, article):
     so this will return True.
     """
 
-    course_slug = article_course_wiki_root_slug(article)
+    wiki_slug = article_course_wiki_root_slug(article)
 
-    if course_slug is None:
+    if wiki_slug is None:
         return False
 
-    user_groups = user.groups.all()
-
     # The wiki expects article slugs to contain at least one non-digit so if
     # the course number is just a number the course wiki root slug is set to
     # be '<course_number>_'. This means slug '202_' can belong to either
     # course numbered '202_' or '202' and so we need to consider both.
 
-    if user_is_staff_on_course_number(user_groups, course_slug):
+    courses = modulestore.django.modulestore().get_courses_for_wiki(wiki_slug)
+    if any(courseware.access.has_access(user, course, 'staff', course.course_id) for course in courses):
         return True
 
-    if (course_slug.endswith('_') and slug_is_numerical(course_slug[:-1]) and
-            user_is_staff_on_course_number(user_groups, course_slug[:-1])):
-        return True
+    if (wiki_slug.endswith('_') and slug_is_numerical(wiki_slug[:-1])):
+        courses = modulestore.django.modulestore().get_courses_for_wiki(wiki_slug[:-1])
+        if any(courseware.access.has_access(user, course, 'staff', course.course_id) for course in courses):
+            return True
 
     return False
 
@@ -64,28 +65,6 @@ def course_wiki_slug(course):
     return slug
 
 
-def user_is_staff_on_course_number(user_groups, course_number):
-    """Returns whether the groups contain a staff group for the course number"""
-
-    # Course groups have format 'instructor_<course_id>' and 'staff_<course_id>' where
-    # course_id = org/course_number/run. So check if user's groups contain a group
-    # whose name starts with 'instructor_' or 'staff_' and contains '/course_number/'.
-    course_number_fragment = '/{0}/'.format(course_number)
-    if [group for group in user_groups if (group.name.startswith(('instructor_', 'staff_')) and
-                                           course_number_fragment in group.name)]:
-        return True
-
-    # Old course groups had format 'instructor_<course_number>' and 'staff_<course_number>'
-    # Check if user's groups contain either of these.
-    old_instructor_group_name = 'instructor_{0}'.format(course_number)
-    old_staff_group_name = 'staff_{0}'.format(course_number)
-    if [group for group in user_groups if (group.name == old_instructor_group_name or
-                                           group.name == old_staff_group_name)]:
-        return True
-
-    return False
-
-
 def article_course_wiki_root_slug(article):
     """
     We assume the second level ancestor is the course wiki root. Examples: