From 9542230afd6d6f28fc3614b60ae41b3652b5da05 Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Fri, 25 Oct 2013 18:16:22 +0500 Subject: [PATCH] Added granular permissions for managing wiki articles LMS-1355 --- lms/djangoapps/course_wiki/settings.py | 36 +++++ .../course_wiki/tests/test_access.py | 139 ++++++++++++++++++ lms/djangoapps/course_wiki/utils.py | 123 ++++++++++++++++ lms/djangoapps/course_wiki/views.py | 17 +-- lms/envs/common.py | 9 +- 5 files changed, 307 insertions(+), 17 deletions(-) create mode 100644 lms/djangoapps/course_wiki/settings.py create mode 100644 lms/djangoapps/course_wiki/tests/test_access.py create mode 100644 lms/djangoapps/course_wiki/utils.py diff --git a/lms/djangoapps/course_wiki/settings.py b/lms/djangoapps/course_wiki/settings.py new file mode 100644 index 0000000000..5e0f4596f6 --- /dev/null +++ b/lms/djangoapps/course_wiki/settings.py @@ -0,0 +1,36 @@ +""" +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) diff --git a/lms/djangoapps/course_wiki/tests/test_access.py b/lms/djangoapps/course_wiki/tests/test_access.py new file mode 100644 index 0000000000..e09f55caa5 --- /dev/null +++ b/lms/djangoapps/course_wiki/tests/test_access.py @@ -0,0 +1,139 @@ +""" +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)) diff --git a/lms/djangoapps/course_wiki/utils.py b/lms/djangoapps/course_wiki/utils.py new file mode 100644 index 0000000000..5f75dcad94 --- /dev/null +++ b/lms/djangoapps/course_wiki/utils.py @@ -0,0 +1,123 @@ +""" +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 /. 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 '//'. 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 '_'. 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_' and 'staff_' 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_' and 'staff_' + # 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 diff --git a/lms/djangoapps/course_wiki/views.py b/lms/djangoapps/course_wiki/views.py index 322bc9decd..2318fc4c89 100644 --- a/lms/djangoapps/course_wiki/views.py +++ b/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: diff --git a/lms/envs/common.py b/lms/envs/common.py index 8420e53ab1..a6b723c0c3 100644 --- a/lms/envs/common.py +++ b/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