-
Notifications
You must be signed in to change notification settings - Fork 7
[BB-3622] Restrict user create course #319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BB-3622] Restrict user create course #319
Conversation
The condition added to ensure that if the feature is enabled user will not be able to create the course outside of the organisation in which they belong. Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
| has_org_permission = bool(auth.get_user_permissions(request.user, None, org)) | ||
| if not has_org_permission: | ||
| return JsonResponse( | ||
| {'error': _('User is not present in the organisation')}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farhaanbukhsh, we may have to update this message after edX's review. At this point, I'd suggest something like
User does not have the permission to create courses in this organization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that does sound reasonable. Thanks 😄
|
@farhaanbukhsh, I haven't yet finished a review pass or tested these changes. But would like to know the answers for these questions/whether these are handled in your change.
|
Hey, @lgp171188 I have tested these and replied to the query let me know what do you have in mind.
Superuser and global staff are allowed to create courses for any organization. For normal users, they need to be in the organization and have a staff/instructor role in the org to see and create a course.
Yes as the above point, if a user is a superuser/global staff then the user can bypass the flag and create the course because get_user_permission allows that to happen.
Now we use the
Except for the user being the superuser of global staff, I was not able to find a way out. |
Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
7b2e174 to
364a22d
Compare
Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
c606997 to
cc140ee
Compare
Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
| if settings.FEATURES.get('RESTRICT_NON_ORG_COURSE_CREATION'): | ||
| has_org_permission = has_studio_write_access(request.user, None, org) | ||
| if not has_org_permission: | ||
| log.exception("User does not have the permission to create library in this organization") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farhaanbukhsh, nit: create a library or create libraries. Same below. It is also a good idea to log the user id, org and library/course name. What do you think?
cms/envs/common.py
Outdated
| # .. toggle_status: supported | ||
| 'DEPRECATE_OLD_COURSE_KEYS_IN_STUDIO': True, | ||
|
|
||
| # Restricts users from creating courses in organisation which they don't belog to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Restricts users from creating courses in organisation which they don't belog to | |
| # Restricts users from creating courses in organisations which they don't belong to |
| if library is None: | ||
| library = request.json['library'] | ||
| # Allow user to create the library only if they belong to the organization | ||
| if settings.FEATURES.get('RESTRICT_NON_ORG_COURSE_CREATION'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farhaanbukhsh, would it make sense to check and restrict access based on this only when the ENABLE_CREATOR_GROUP feature flag is set enabled?
| ) | ||
|
|
||
| # Allow user to create the course only if they belong to the organisation | ||
| if settings.FEATURES.get('RESTRICT_NON_ORG_COURSE_CREATION'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farhaanbukhsh, this should be added to the default feature flags with its value set to False. Also would it be better to name it RESTRICT_COURSE_CREATION_TO_ORG_ROLES?
When adding this to the FEATURES dict in the settings, remember to document it using the guidelines here.
Also can you update the comment above to indicate that this doesn't affect the global superusers and staff users?
Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
| ) | ||
|
|
||
| @patch.dict('django.conf.settings.FEATURES', {'RESTRICT_COURSE_CREATION_TO_ORG_ROLES': True}) | ||
| def test_library_creation_with_normaL_user_with_non_acess_role(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def test_library_creation_with_normaL_user_with_non_acess_role(self): | |
| def test_library_creation_with_normaL_user_with_non_access_role(self): |
Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
| log.exception("User does not have the permission to create course \ | ||
| in this organization. User: {} Org: {} Course: {}".format(request.user.id, org, course)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farhaanbukhsh, instead of using a trailing slash, it is better to add line breaks after the opening parenthesis and use implicit string concatenation of adjacent strings, no?
| if settings.FEATURES.get('RESTRICT_COURSE_CREATION_TO_ORG_ROLES', False): | ||
| has_org_permission = has_studio_write_access(request.user, None, org) | ||
| if not has_org_permission: | ||
| log.exception("User does not have the permission to create library \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
lgp171188
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farhaanbukhsh, 👍 Please squash the changes to a single commit with an appropriate commit message before merging.
- I tested the changes in this PR and verified that:
- Superusers and global staff users can always create courses in any organization with or without this feature flag enabled.
- When the feature flag is enabled, in addition to having the course creation permission via course creator groups (enabled by default), the user should also have a role (any role would suffice as the user is already a course creator) in the course's organization to be able to create a course under an organization.
- When the feature flag is disabled (the current and default behavior), the existing behavior allowing course creator users to create courses in all organizations is preserved.
- I read through the code
-
I checked for accessibility issuesNA -
Includes documentationNA
* Add course creation condition for organization The condition added to ensure that if the feature is enabled user will not be able to create the course outside of the organization in which they belong. Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
* Add course creation condition for organization The condition added to ensure that if the feature is enabled user will not be able to create the course outside of the organization in which they belong. Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
* Add course creation condition for organization The condition added to ensure that if the feature is enabled user will not be able to create the course outside of the organization in which they belong. Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
* [SE-4101] fix: address VisibleBlocks caching race condition (openedx#27359) (#349) * fix: address VisibleBlocks caching race condition * sets visual block creation in an atomic transaction * refactor: add logging statement to bulk create Co-authored-by: Raul Gallegos <raul@opencraft.com> Co-authored-by: Raul Gallegos <raul@opencraft.com> * BB-3954 Add toggle for enrollment behavior (#351) Adds toggle REDIRECT_UNAUTHENTICATED_USER_TO_LOGIN_ON_ENROLL for enrollment behaviour for unauthenticated user. If true, the user will be redirected to 'signin_user' route. Co-authored-by: Arjun Singh Yadav <arjun@opencraft.com> * [BB-3622] feat:Restrict user create course (#319) (#352) * Add course creation condition for organization The condition added to ensure that if the feature is enabled user will not be able to create the course outside of the organization in which they belong. Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com> * fix: update the xblock-lti-consumer commit * Added new setting CERTIFICATE_DATE_FORMAT for easy customization of (#354) certificate issued date (cherry picked from commit 421e661) * fix: Produce grade report when subsections have future start dates When getting a subsection grade for a user, instead of failing if the user can't access that subsection, fallback to the collected structure. * Revert "Unhide student-generated certificates toggle" This reverts commit 8910ccf. Our clients are no longer using this feature, and edX won't accept this upstream as-is currently. See https://github.com/edx/edx-platform/pull/23735 for more information. Reverting to reduce code drift from upstream. * chore: update Arabic translations * fix:Fix function call to check MFE (#359) Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com> * fix: don't cache ajax request * feat: add site language config Cherry-picked from: https://github.com/edx/edx-platform/pull/27696 * Revert "[FAL-1813] fix: codejail issue when using matplotlib (#343)" (#365) This reverts commit 2143f84. * Update celery routing for celery 4+ (openedx#25567) * Update celery routing - Used routing function instead of class - Move task queues dictionary to Django settings - Removed routing_key parameter - Refactored routing for singleton celery instantiation Co-authored-by: Awais Qureshi <awais.qureshi@arbisoft.com> (cherry picked from commit e3b4d23) * fix: add missing set_code_owner_attribute imports (cherry picked from commit d1060d2) * chore: bump edx-django-utils to 3.12.0 The required `set_code_owner_attribute` decorator was introduced in v3.12.0, therefore we need to bump the dependency. (cherry picked from commit f52b84e) * [SE-4482] Allow delete course content in Studio only for admin users (#360) Co-authored-by: Nizar Mahmoud <nizarmah@hotmail.com> * fix: Password reset page throwing not found error (#364) Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com> * fix: change buttons in wiki modal to anchor tags The action buttons in wiki modal have href attribute but does not have an event listener for click. This PR changes the buttons to anchor tags so that they work as expected when clicked. * fix: use high priority queue for celery heartbeat check Makes HIGH_PRIORITY_QUEUE a derived setting, which allows HEARTBEAT_CELERY_ROUTING_KEY to use the correct config variant default. (cherry picked from commit 2a9067a) * fix: prevent invalidation of allowlisted certificates The allowlisted certificates were getting invalidated upon visiting the Course Progress page by users. This is a rough backport of the Lilac fix (edx#26356). In Lilac, this is gated by the `certificates_revamp.use_allowlist` Waffle flag. In post-Lilac branches, this is working out of the box (the flag has been removed in edx#27576). Jira ticket: BB-4287 * feat: add celery beat configuration Co-authored-by: Raul Gallegos <raul@opencraft.com> Co-authored-by: Farhaan Bukhsh <farhaan@opencraft.com> Co-authored-by: Arjun Singh Yadav <arjun@opencraft.com> Co-authored-by: pkulkark <pooja@opencraft.com> Co-authored-by: Shimul Chowdhury <shimul@opencraft.com> Co-authored-by: João Cabrita <joao.cabrita@opencraft.com> Co-authored-by: Samuel Walladge <samuel@opencraft.com> Co-authored-by: Giovanni Cimolin da Silva <giovannicimolin@gmail.com> Co-authored-by: 0x29a <demid@opencraft.com> Co-authored-by: Dmitry Gamanenko <dmitry.gamanenko@raccoongang.com> Co-authored-by: Jillian Vogel <jill@opencraft.com> Co-authored-by: Muhammad Soban Javed <58461728+iamsobanjaved@users.noreply.github.com> Co-authored-by: Sandeep Choudhary <sandeep@opencraft.com> Co-authored-by: Nizar Mahmoud <nizarmah@hotmail.com> Co-authored-by: Agrendalath <piotr@surowiec.it>
* feat: add canvas integration support (cherry picked from commit fdb818a)
* feat: add canvas integration support (cherry picked from commit fdb818a)
* feat: add canvas integration support (cherry picked from commit fdb818a)
* feat: add canvas integration support (cherry picked from commit fdb818a)
The condition added to ensure that if the feature is enabled
user will not be able to create the course outside of the organisation
in which they belong.
JIRA tickets: BB-3622
Discussions: Link to any public dicussions about this PR or the design/architecture. Otherwise omit this.Dependencies: None
Screenshots:

Sandbox URL: TBD - sandbox is being provisioned.Merge deadline: "None" if there's no rush, "ASAP" if it's critical, or provide a specific date if there is one.Testing instructions:
make dev.shell.studioFEATURESdictionary addRESTRICT_NON_ORG_COURSE_CREATION: truestaffuseradminpage and in localhost:18010/admin/student/courseaccessrole/ create a new entry, add the user, give it instructor privilege, don't add theOrgyetAuthor notes and concerns:
Reviewers