Merge pull request #1502 from edx/usman/lms1355-wiki-permissions

Stricter permissions for the wiki

lms/djangoapps/course_wiki/settings.py

+"""
+These callables are used by django-wiki to check various permissions
+a user has on an article.
+"""
+
+from course_wiki.utils import user_is_article_course_staff
+
+
+def CAN_DELETE(article, user):  # pylint: disable=invalid-name
+    """Is user allowed to soft-delete article?"""
+    return _is_staff_for_article(article, user)
+
+
+def CAN_MODERATE(article, user):  # pylint: disable=invalid-name
+    """Is user allowed to restore or purge article?"""
+    return _is_staff_for_article(article, user)
+
+
+def CAN_CHANGE_PERMISSIONS(article, user):  # pylint: disable=invalid-name
+    """Is user allowed to change permissions on article?"""
+    return _is_staff_for_article(article, user)
+
+
+def CAN_ASSIGN(article, user):  # pylint: disable=invalid-name
+    """Is user allowed to change owner or group of article?"""
+    return _is_staff_for_article(article, user)
+
+
+def CAN_ASSIGN_OWNER(article, user):  # pylint: disable=invalid-name
+    """Is user allowed to change group of article to one of its own groups?"""
+    return _is_staff_for_article(article, user)
+
+
+def _is_staff_for_article(article, user):
+    """Is the user staff for article's course wiki?"""
+    return user.is_staff or user.is_superuser or user_is_article_course_staff(user, article)

lms/djangoapps/course_wiki/tests/test_access.py

+"""
+Tests for wiki permissions
+"""
+
+from django.contrib.auth.models import Group
+from student.tests.factories import UserFactory
+from xmodule.modulestore.tests.factories import CourseFactory
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+
+from django.test.utils import override_settings
+from courseware.tests.factories import InstructorFactory, StaffFactory
+from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+
+from wiki.models import URLPath
+from course_wiki.views import get_or_create_root
+from course_wiki.utils import user_is_article_course_staff, course_wiki_slug
+from course_wiki import settings
+
+
+@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
+class TestWikiAccessBase(ModuleStoreTestCase):
+    """Base class for testing wiki access."""
+    def setUp(self):
+
+        self.wiki = get_or_create_root()
+
+        self.course_math101 = CourseFactory.create(org='org', number='math101', display_name='Course')
+        self.course_math101_staff = [InstructorFactory(self.course_math101), StaffFactory(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]
+
+    def create_urlpath(self, parent, slug):
+        """Creates an article at /parent/slug and returns its URLPath"""
+        return URLPath.create_article(parent, slug, title=slug)
+
+
+@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
+class TestWikiAccess(TestWikiAccessBase):
+    """Test wiki access for course staff."""
+    def setUp(self):
+        super(TestWikiAccess, self).setUp()
+
+        self.course_310b = CourseFactory.create(org='org', number='310b', display_name='Course')
+        self.course_310b_staff = [InstructorFactory(self.course_310b), StaffFactory(self.course_310b)]
+        self.course_310b_ = CourseFactory.create(org='org', number='310b_', display_name='Course')
+        self.course_310b__staff = [InstructorFactory(self.course_310b_), StaffFactory(self.course_310b_)]
+
+        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_))
+
+    def test_no_one_is_root_wiki_staff(self):
+        all_course_staff = self.course_math101_staff + self.course_310b_staff + self.course_310b__staff
+        for course_staff in all_course_staff:
+            self.assertFalse(user_is_article_course_staff(course_staff, self.wiki.article))
+
+    def test_course_staff_is_course_wiki_staff(self):
+        for page in self.wiki_math101_pages:
+            for course_staff in self.course_math101_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:
+                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_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))
+
+        for course_staff in self.course_310b__staff:
+            self.assertFalse(user_is_article_course_staff(course_staff, self.wiki_310b.article))
+
+
+@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
+class TestWikiAccessForStudent(TestWikiAccessBase):
+    """Test access for students."""
+    def setUp(self):
+        super(TestWikiAccessForStudent, self).setUp()
+
+        self.student = UserFactory.create()
+
+    def test_student_is_not_root_wiki_staff(self):
+        self.assertFalse(user_is_article_course_staff(self.student, self.wiki.article))
+
+    def test_student_is_not_course_wiki_staff(self):
+        for page in self.wiki_math101_pages:
+            self.assertFalse(user_is_article_course_staff(self.student, page.article))
+
+
+@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
+class TestWikiAccessForNumericalCourseNumber(TestWikiAccessBase):
+    """Test staff has access if course number is numerical and wiki slug has an underscore appended."""
+    def setUp(self):
+        super(TestWikiAccessForNumericalCourseNumber, self).setUp()
+
+        self.course_200 = CourseFactory.create(org='org', number='200', display_name='Course')
+        self.course_200_staff = [InstructorFactory(self.course_200), StaffFactory(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')
+        wiki_200_page_page = self.create_urlpath(wiki_200_page, 'Grandchild')
+        self.wiki_200_pages = [wiki_200, wiki_200_page, wiki_200_page_page]
+
+    def test_course_staff_is_course_wiki_staff_for_numerical_course_number(self):  # pylint: disable=C0103
+        for page in self.wiki_200_pages:
+            for course_staff in self.course_200_staff:
+                self.assertTrue(user_is_article_course_staff(course_staff, page.article))
+
+
+@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
+class TestWikiAccessForOldFormatCourseStaffGroups(TestWikiAccessBase):
+    """Test staff has access if course group has old format."""
+    def setUp(self):
+        super(TestWikiAccessForOldFormatCourseStaffGroups, self).setUp()
+
+        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(self.course_math101c), StaffFactory(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')
+        wiki_math101c_page_page = self.create_urlpath(wiki_math101c_page, 'Grandchild')
+        self.wiki_math101c_pages = [wiki_math101c, wiki_math101c_page, wiki_math101c_page_page]
+
+    def test_course_staff_is_course_wiki_staff(self):
+        for page in self.wiki_math101c_pages:
+            for course_staff in self.course_math101c_staff:
+                self.assertTrue(user_is_article_course_staff(course_staff, page.article))

lms/djangoapps/course_wiki/utils.py

+"""
+Utility functions for course_wiki.
+"""
+
+from django.core.exceptions import ObjectDoesNotExist
+
+
+def user_is_article_course_staff(user, article):
+    """
+    The root of a course wiki is /<course_number>. This means in case there
+    are two courses which have the same course_number they will end up with
+    the same course wiki root e.g. MITX/Phy101/Spring and HarvardX/Phy101/Fall
+    will share /Phy101.
+
+    This looks at the course wiki root of the article and returns True if
+    the user belongs to a group whose name starts with 'instructor_' or
+    'staff_' and contains '/<course_wiki_root_slug>/'. So if the user is
+    staff on course MITX/Phy101/Spring they will be in
+    'instructor_MITX/Phy101/Spring' or 'staff_MITX/Phy101/Spring' groups and
+    so this will return True.
+    """
+
+    course_slug = article_course_wiki_root_slug(article)
+
+    if course_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):
+        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
+
+    return False
+
+
+def slug_is_numerical(slug):
+    """Returns whether the slug can be interpreted as a number."""
+    try:
+        float(slug)
+    except ValueError:
+        return False
+
+    return True
+
+
+def course_wiki_slug(course):
+    """Returns the slug for the course wiki root."""
+    slug = course.wiki_slug
+
+    # Django-wiki expects article slug to be non-numerical. In case the
+    # course number is numerical append an underscore.
+    if slug_is_numerical(slug):
+        slug = slug + "_"
+
+    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:
+    / returns None
+    /Phy101 returns 'Phy101'
+    /Phy101/Mechanics returns 'Phy101'
+    /Chem101/Metals/Iron returns 'Chem101'
+
+    Note that someone can create an article /random-article/sub-article on the
+    wiki. In this case this function will return 'some-random-article' even
+    if no course with course number 'some-random-article' exists.
+    """
+
+    try:
+        urlpath = article.urlpath_set.get()
+    except ObjectDoesNotExist:
+        return None
+
+    # Ancestors of /Phy101/Mechanics/Acceleration/ is a list of URLPaths
+    # ['Root', 'Phy101', 'Mechanics']
+    ancestors = urlpath.cached_ancestors
+
+    course_wiki_root_urlpath = None
+
+    if len(ancestors) == 0:  # It is the wiki root article.
+        course_wiki_root_urlpath = None
+    elif len(ancestors) == 1:  # It is a course wiki root article.
+        course_wiki_root_urlpath = urlpath
+    else:  # It is an article inside a course wiki.
+        course_wiki_root_urlpath = ancestors[1]
+
+    if course_wiki_root_urlpath is not None:
+        return course_wiki_root_urlpath.slug
+
+    return None

lms/djangoapps/course_wiki/views.py

@@ -10,6 +10,7 @@ from wiki.core.exceptions import NoRootURL
 from wiki.models import URLPath, Article
 
 from courseware.courses import get_course_by_id
+from course_wiki.utils import course_wiki_slug
 
 log = logging.getLogger(__name__)
 
@@ -30,21 +31,7 @@ def course_wiki_redirect(request, course_id):
     example, "/6.002x") to keep things simple.
     """
     course = get_course_by_id(course_id)
-
-    course_slug = course.wiki_slug
-
-
-    # cdodge: fix for cases where self.location.course can be interpreted as an number rather than
-    # a string. We're seeing in Studio created courses that people often will enter in a stright number
-    # for 'course' (e.g. 201). This Wiki library expects a string to "do the right thing". We haven't noticed this before
-    # because - to now - 'course' has always had non-numeric characters in them
-    try:
-        float(course_slug)
-        # if the float() doesn't throw an exception, that means it's a number
-        course_slug = course_slug + "_"
-    except:
-        pass
-
+    course_slug = course_wiki_slug(course)
 
     valid_slug = True
     if not course_slug:

lms/envs/common.py

@@ -499,12 +499,17 @@ SIMPLE_WIKI_REQUIRE_LOGIN_EDIT = True
 SIMPLE_WIKI_REQUIRE_LOGIN_VIEW = False
 
 ################################# WIKI ###################################
+from course_wiki import settings as course_wiki_settings
+
 WIKI_ACCOUNT_HANDLING = False
 WIKI_EDITOR = 'course_wiki.editors.CodeMirror'
 WIKI_SHOW_MAX_CHILDREN = 0  # We don't use the little menu that shows children of an article in the breadcrumb
 WIKI_ANONYMOUS = False  # Don't allow anonymous access until the styling is figured out
-WIKI_CAN_CHANGE_PERMISSIONS = lambda article, user: user.is_staff or user.is_superuser
-WIKI_CAN_ASSIGN = lambda article, user: user.is_staff or user.is_superuser
+
+WIKI_CAN_DELETE = course_wiki_settings.CAN_DELETE
+WIKI_CAN_MODERATE = course_wiki_settings.CAN_MODERATE
+WIKI_CAN_CHANGE_PERMISSIONS = course_wiki_settings.CAN_CHANGE_PERMISSIONS
+WIKI_CAN_ASSIGN = course_wiki_settings.CAN_ASSIGN
 
 WIKI_USE_BOOTSTRAP_SELECT_WIDGET = False
 WIKI_LINK_LIVE_LOOKUPS = False