Skip to content

When checking permissions for wiki look at dot format role names too. - #2559

Merged
symbolist merged 5 commits into
masterfrom
usman/lms2136-wiki-access
Mar 10, 2014
Merged

When checking permissions for wiki look at dot format role names too.#2559
symbolist merged 5 commits into
masterfrom
usman/lms2136-wiki-access

Conversation

@symbolist

Copy link
Copy Markdown
Contributor

CourseRole names have a new format (type_org.number.run). Previously
when checking if a user was staff for a course wiki only type_org/number/run
and type_number format role names were checked. Now type.org.number.run
format names are also checked.

LMS-2136

@symbolist

Copy link
Copy Markdown
Contributor Author

@adampalay @sarina A few months back you reviewed a PR which added granular permissions for course wikis. Course role groups have a new format now (type_org.number.run). This PR adds support for recognising users as staff on course wikis if they are members of the new dot format role groups.

@sarina

sarina commented Feb 11, 2014

Copy link
Copy Markdown
Contributor

@symbolist this makes sense to me, but I am unfamiliar with the new format. I'd recommend also tagging whomever is responsible for that change to make sure everything has been implemented correctly.

@symbolist

Copy link
Copy Markdown
Contributor Author

@dmitchell Can you give this a look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the double underscore is confusing... can you do course_310b2 or something?

@dmitchell

Copy link
Copy Markdown
Contributor

👎 Please convert to use CourseStaffRole and CourseInstructorRole from roles.py. I'm sorry I didn't change these usages here when I centralized that.

@symbolist

Copy link
Copy Markdown
Contributor Author

@dmitchell I am not sure where CourseStaff/InstructorRole should be used here?

From a wiki article we can only extract the course number and not the whole course id. Given this constraint user_is_staff_on_course_number (user_groups, course_number) returns True if one of the user instructor or staff groups name contains '/course_number/'. This PR also adds a check for '.course_number.' Of course, this breaks encapsulation and is not a clean solution because the user is granted staff privileges on course wikis of other courses with the same course number, but this is the best that can be done with the way the wikis are currently setup.

@dmitchell

Copy link
Copy Markdown
Contributor

I'm going to pull your branch and add a function for getting the list of courses which contain a given wiki id. You then will remove all of your permission checking code and replace it with course_locs = modulestore().get_courses_for_wiki(wiki_slug) any(student.auth.has_access(user, CourseStaffRole(course_loc)) for course_loc in course_locs)

@dmitchell

Copy link
Copy Markdown
Contributor

Note, I didn't add the version for split mongo yet as it's more complex

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some commas:
To maintain backwards compatibility, the default value of use_unique_wiki_id is False. However, when creating a course in studio, use_unique_wiki_id will be set to True. So, new courses will use a unique wiki_id. XML courses will need to explicitly set use_unique_wiki_id to true in the policy file.

@adampalay

Copy link
Copy Markdown
Contributor

@symbolist I don't understand how this will resolve LMS-2112... unless it's not meant to

@adampalay

Copy link
Copy Markdown
Contributor

I'm also getting an error when I try to run this branch locally: ImportError: Settings cannot be imported, because environment variable DJANGO_SETTINGS_MODULE is undefined.

@symbolist

Copy link
Copy Markdown
Contributor Author

@dmitchell I have refactored this after our last conversation. Can you give it a review? Thanks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, don't put args to Location constructor in a list, just use them directly. (Nitpick meaning you don't need to change it but I'd prefer the change)

@dmitchell

Copy link
Copy Markdown
Contributor

A few minor cleanups but overall 👍 (no need for re-review)

@adampalay

Copy link
Copy Markdown
Contributor

👍

Don Mitchell and others added 5 commits March 10, 2014 16:13
…odulestores.

This method returns a list of courses which use the particular wiki_slug. It is used for
checking permissions in the course_wiki app.

LMS-2136
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
Currently wiki_slug is set to course number. However, since multiple
courses can have the same number this may lead to clashes. So wiki_slug
will be set to org.course.name. To maintain the active wiki_slugs for
xml courses this cannot be changed in the CourseDescriptor.

LMS-2136
symbolist added a commit that referenced this pull request Mar 10, 2014
Refactor of permissions checking for wiki pages.
@symbolist
symbolist merged commit 8d15acc into master Mar 10, 2014
@symbolist
symbolist deleted the usman/lms2136-wiki-access branch March 10, 2014 12:13
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants