-
Notifications
You must be signed in to change notification settings - Fork 15
Fix User leakage in Batch enrollment #435
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
Changes from all commits
a3c9489
3ad0a5b
2ea6bb4
6863922
0cc3f69
4d609d0
5e0a8ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,8 +24,14 @@ | |
| from lms.djangoapps.grades.events import STATE_DELETED_EVENT_TYPE | ||
| from lms.djangoapps.grades.signals.handlers import disconnect_submissions_signal_receiver | ||
| from lms.djangoapps.grades.signals.signals import PROBLEM_RAW_SCORE_CHANGED | ||
| from lms.djangoapps.instructor.sites import ( | ||
| user_exists_in_organization, | ||
| get_organization_for_site, | ||
| get_user_in_organization_by_email, | ||
| ) | ||
| from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY | ||
| from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers | ||
| from openedx.core.djangoapps.theming.helpers import get_current_site | ||
| from openedx.core.djangoapps.user_api.models import UserPreference | ||
| from student.models import ( | ||
| CourseEnrollment, | ||
|
|
@@ -52,9 +58,13 @@ def __init__(self, course_id, email): | |
| # N.B. retired users are not a concern here because they should be | ||
| # handled at a higher level (i.e. in enroll_email). Besides, this | ||
| # class creates readonly objects. | ||
| exists_user = User.objects.filter(email=email).exists() | ||
| site = get_current_site() | ||
| organization = get_organization_for_site(site) | ||
| exists_user = user_exists_in_organization(email, organization) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @melvinsoft why do a user check instead of just a single call to try to get the user and use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @johnbaldwin Thanks for asking this, this is edX code, no ours, we trying to fix the issue with the less code changes as possible, since makes future merges way more difficult. I don't like the design, but I choose to stick with the code as much as I can. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, Thanks |
||
| if exists_user: | ||
| user = User.objects.get(email=email) | ||
| # Appsembler Specific: We look for the user inside the organization | ||
| # to avoid leakage if the user belong to another organization. | ||
| user = get_user_in_organization_by_email(email, organization) | ||
| mode, is_active = CourseEnrollment.enrollment_mode_for_user(user, course_id) | ||
| # is_active is `None` if the user is not enrolled in the course | ||
| exists_ce = is_active is not None and is_active | ||
|
|
@@ -144,7 +154,12 @@ def enroll_email(course_id, student_email, auto_enroll=False, email_students=Fal | |
| if previous_state.enrollment: | ||
| course_mode = previous_state.mode | ||
|
|
||
| enrollment_obj = CourseEnrollment.enroll_by_email(student_email, course_id, course_mode) | ||
| # Appsembler Specific: We call our custom method instead the default one | ||
| if settings.FEATURES.get('TAHOE_MULTITENANT_BULK_ENROLLMENT', False): | ||
| enrollment_obj = CourseEnrollment.enroll_by_email_in_organization(student_email, course_id, course_mode) | ||
| else: | ||
| enrollment_obj = CourseEnrollment.enroll_by_email(student_email, course_id, course_mode) | ||
|
|
||
| if email_students: | ||
| email_params['message'] = 'enrolled_enroll' | ||
| email_params['email_address'] = student_email | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| """ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @melvinsoft We may want to move this module out of Another option is to just use the existing |
||
| Heplers for site/org isolations functions. | ||
| """ | ||
| from openedx.core.djangoapps.theming.helpers import get_current_site | ||
|
|
||
| from organizations.models import Organization, UserOrganizationMapping | ||
|
|
||
|
|
||
| def user_exists_in_organization(user_email, organization): | ||
| """ | ||
| Look is a user exists inside an organization based on a given email | ||
|
|
||
| `user_email` is the user email | ||
| `organization` the organization object | ||
|
|
||
| returns True or False | ||
| Representing is the user exists or not inside the org | ||
| """ | ||
| return organization.userorganizationmapping_set.filter(user__email=user_email).exists() | ||
|
|
||
|
|
||
| def get_organization_for_site(site): | ||
| """ | ||
| Returns an organization based in a given site. | ||
|
|
||
| `site` is the Site object | ||
|
|
||
| returns an organization or None | ||
| """ | ||
| return get_current_site().organizations.first() | ||
|
|
||
|
|
||
| def get_user_in_organization_by_email(user_email, organization): | ||
| """ | ||
| Return a user inside an organization based on a given email | ||
|
|
||
| `user_email` is the user email | ||
| `organization` the organization object | ||
|
|
||
| returns the User object or UserOrganizationMapping.DoesNotExist | ||
| """ | ||
| try: | ||
| user = organization.userorganizationmapping_set.get(user__email=user_email).user | ||
| return user | ||
| except UserOrganizationMapping.DoesNotExist: | ||
| return None | ||
Uh oh!
There was an error while loading. Please reload this page.