Fix User leakage in Batch enrollment by melvinsoft · Pull Request #435 · appsembler/edx-platform

melvinsoft · 2019-08-07T16:51:14Z

We're experiencing problems with the Batch Enrollment section, that is causing small leaks between sites. But before, is important to understand how it works.

The Batch enrollment section is basically a TextArea in where you can enter a comma / line separated list of usernames or emails (yeah, all mixed in the same list), the batch enrollment will iterate over all of those users and do the following:

If the user exists by username or email: Will be enrolled in course.
If the user doesn't exists by username, it won't do nothing and notify in the UI that doesn't exists.
If the user doesn't exists by email, it will be pre-enrolled and an invitation will be sent. Pre-enrolled means that EnrollmentAllowed record is created, so at the time the user registers, it'll be automatically enrolled in the course.

In Tahoe we're having several issues, because we're emulating a multi tenant system:
1 - If a user is registered in any Tahoe site with a certain email, it can be automatically enrolled in other Tahoe site by email, this causes data leakage in reports, and also the user receives an invitation for a course in a site that cannot access.
2 - If you paste random username like carlos and by any chance a user with that username exists in another Tahoe site, it will be enrolled in the course, causing the same problem described above.

The tricky part of this issue, is that we cannot remove the functionality of invite non registered users by email, because is the charm of the feature, invite user to register and enroll in a course.

The PR solves most of those issues, but simply check if the user exists inside the organization, if is searched by username and not exists in the site, even if exists in another site, it simply will return a not exists. If the user is searched by email, despite if exists in another site or not, but don't exists in the current one, it'll be invited by email.

The edge case is, if the user exists in another site, it won't be able to sign up, but that's a different problem that will be tackled here: https://github.com/appsembler/tech-design-proposals/pull/8

add missing comment

OmarIthawi

Thanks @melvinsoft.

Before I go in full details, I'd like to ask about two things:

Make this change gated by a feature flag e.g. FEATURES['TAHOE_MULTITENANT_BULK_ENROLLMENT']
Add few tests to ensure this is working as expected

I'll give it another detailed round of review soon.

johnbaldwin · 2019-08-09T03:56:41Z

        # handled at a higher level (i.e. in enroll_email).  Besides, this
        # class creates readonly objects.
-        exists_user = User.objects.filter(email=email).exists()
+        organization = get_current_site().organizations.first()


This can raise an exception if the RequestCache class does not cache the current request object. If we are not in a view class that has a request object, we should instead pass the site instance to classes and methods that need it. This both makes it explicit and helps makes tests more robust. For the specific case of EmailEnrollmentState's constructor should take a site object

Another improvement is to then move the 'user exists' check into a sites module so that site handling (setters and getters for site specific filtering) is handled in the single module.

For an example, see the sites module in the appsembler api: https://github.com/appsembler/edx-platform/blob/appsembler/tahoe/master/openedx/core/djangoapps/appsembler/api/sites.py

Making a central point for setting/getting site specific data lets us both reuse code and do focused unit testing for site isolation.

So we could have a sites.user_exists_in_site(user=user, site=site) call or just try to get the user as in

user = sites.get_user_for_site(user_email=email, site=site)

I like the refactoring idea you're suggesting @johnbaldwin, .organizations.first() is being used a lot and it should be placed in a function instead of copying the pattern all over tahoe.

johnbaldwin · 2019-08-09T04:59:32Z

        # class creates readonly objects.
-        exists_user = User.objects.filter(email=email).exists()
+        organization = get_current_site().organizations.first()
+        exists_user = organization.userorganizationmapping_set.filter(user__email=email).exists()


so this call could be exists_user = sites.site_has_user(site=site, user_email=email)

But what would be simpler is to just try to get the user for the site with a function that returns None if the user is not in the site. So we'd define a function in a common sites module and we could make the call work like this:

user = sites.get_user_for_site(site=site, user_email=email) if user: mode, is_active = CourseEnrollment... ... else: ... exists_allowed = ... state_auto_enroll = ... self.user = bool(user) ...

And it looks like EmailEnrollmentState is only used in this module (instructor.enrollments), so when we add site as a constructor param, we only have to change it in this module and the test_enrollment module

./lms/djangoapps/instructor/tests/test_enrollment.py:26: EmailEnrollmentState, ./lms/djangoapps/instructor/tests/test_enrollment.py:62: ees = EmailEnrollmentState(self.course_key, eobjs.email) ./lms/djangoapps/instructor/tests/test_enrollment.py:94: before = EmailEnrollmentState(self.course_key, eobjs.email) ./lms/djangoapps/instructor/tests/test_enrollment.py:103: after = EmailEnrollmentState(self.course_key, eobjs.email) ./lms/djangoapps/instructor/tests/test_enrollment.py:597:class SettableEnrollmentState(EmailEnrollmentState): ./lms/djangoapps/instructor/enrollment.py:49:class EmailEnrollmentState(object): ./lms/djangoapps/instructor/enrollment.py:126: returns two EmailEnrollmentState's ./lms/djangoapps/instructor/enrollment.py:129: previous_state = EmailEnrollmentState(course_id, student_email) ./lms/djangoapps/instructor/enrollment.py:162: after_state = EmailEnrollmentState(course_id, student_email) ./lms/djangoapps/instructor/enrollment.py:176: returns two EmailEnrollmentState's ./lms/djangoapps/instructor/enrollment.py:179: previous_state = EmailEnrollmentState(course_id, student_email) ./lms/djangoapps/instructor/enrollment.py:196: after_state = EmailEnrollmentState(course_id, student_email)

@johnbaldwin I'd be really careful not to change so many files. It's very easy that we'd forget to migrate future uses when upgrading to a newer release.

Git conflicts can be really unproductive to resolve.

If we really want to be careful, we can still use something like this function:

edx-platform/openedx/core/djangoapps/request_cache/__init__.py

Lines 63 to 72 in 64555c6

def get_request_or_stub():

"""

Return the current request or a stub request.

If called outside the context of a request, construct a fake

request that can be used to build an absolute URI.

This is useful in cases where we need to pass in a request object

but don't have an active request (for example, in tests, celery tasks, and XBlocks).

"""

Which addresses the issues of having the site returned as None.

Reading that function, a dummy site is in the fake request is returned. In this case, the user should be rejected. We should test that this behaves how we want

johnbaldwin · 2019-08-09T05:29:43Z

@melvinsoft I made a bunch of comments suggesting an implementation approach that explicitly passes the current site object instead of relying on the RequestCache to contain the request object that defines the site. Probably more work than we want to do.

After thinking about it after I typed up my comments I decided to leave my comments in place just so you see what I'm thinking based on my efforts to try to make site isolation safer and easier to test

Probably the most expedient things to do instead are

A) Be clear in how the code works for when RequestCache does not have a valid request
B) Add test to handle when this call fails:

edx-platform/lms/djangoapps/instructor/enrollment.py

Line 56 in a3c9489

organization = get_current_site().organizations.first()

C) Add test to run the code for a user that doesn't belong to the site/organization
D) Add test to run the code for a user that DOES belong to the site/organization

For examples, please check out my tests for the Tahoe API: https://github.com/appsembler/edx-platform/tree/appsembler/tahoe/master/openedx/core/djangoapps/appsembler/api/tests

johnbaldwin

We need to add test coverage to make sure that the bug fix does work and that we protect agains regressions

A nice to have is to start centralizing the site isolation handling code. I use this approach in the tahoe api and figures.

melvinsoft · 2019-08-13T14:48:12Z

@johnbaldwin @OmarIthawi Thank you so much for your reviews. I've implemented the first round of changes, basically refactoring some code into functions and adding a feature flag to control which function we call as @OmarIthawi requested.

After chatting with @abeals, since this bug is pressuring us, test may come in a second iteration next sprint.

johnbaldwin · 2019-08-13T15:09:59Z

@@ -0,0 +1,27 @@
+"""


@melvinsoft We may want to move this module out of instructor to a common place. Since our site checking depends on Appsembler's fork of organizations, how about moving it to openedx.core.djangoapps.appsembler.sites.api? The api module seems reasonable to me.

Another option is to just use the existing openedx.core.djangoapps.appsembler.api.sites module, for which I have already created unit tests

johnbaldwin · 2019-08-13T15:10:21Z

+
+def user_exists_in_organization(user_email, organization):
+    """
+    TODO


What are the TODO statements for?

Just to write comments for the functions.

johnbaldwin · 2019-08-13T15:18:10Z

-    return get_user_by_username_or_email(unique_student_identifier)
+    # Appsembler Specific: We call our custom method insted the default one,
+    # to make sure the user is get inside the org.
+    return get_user_by_username_or_email_inside_organization(unique_student_identifier)


@melvinsoft Do we need to add a feature flag here too?

Yes, I missed it, thanks! 👍

johnbaldwin

@melvinsoft Thanks for addressing our comments. I have more, sorry:

One of the get_user... calls doesn't look like it is wrapped by a feature flag:

https://github.com/appsembler/edx-platform/pull/435/files#diff-990c8d66bbe04a524be83d8f73c1b321R72

Also, the new sites module appears to be failing pep8. Please see my inline comments.

For the new functionality in instructor.sites, I think we need to make a team decision as to how we handle site isolation: Via the site context or via the organization context. openedx.core.djangoapps.appsembler.api.sites uses the site object to support isolation. Your new code uses the organization object to support isolation.

The thing I'm concerned with the most is that we fail with site isolation because we've got multiple contexts. Since we've got just one org per site, I'd like to restrict it at the site level just because it seems clearer to me as a story: Customer has a subdomain on Tahoe or they use their own domain. This domain provides the top level isolation.

At some point if we do multiple orgs isolation within sites, we're going to have to rework a lot anyway and that would be a time to do a "step back and think about this architecturally"

Your thoughts?

melvinsoft · 2019-08-14T10:05:17Z

@johnbaldwin I fixed some PEP8 issues, but I think the checks are failing because the files we changed, have PEP8 errors from before, from example the file common/djangoapps/student/models.py is 2962 lines long, and is showing a hundreds of PEP8 errors for lines longer than 79 chars, fixing all of those is out of scope of this PR.

Please give it another round of review and approve it if it's meeting the criteria, according our chat yesterday.

melvinsoft · 2019-08-14T19:36:36Z

@thraxil Thanks for addressing the PEP8 issues!

@johnbaldwin Another ping here for a final review and approval. Thanks!

johnbaldwin · 2019-08-14T19:46:08Z

-        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)


@melvinsoft why do a user check instead of just a single call to try to get the user and use if user instead of if exists_user? Is seems to make an extra call that is not needed.

@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.

johnbaldwin

@melvinsoft Thanks for addressing my comments. Just one small question, otherwise, let's ship it!

melvinsoft changed the title ~~Fix User leakage in Batch enrollment~~ (WIP) Fix User leakage in Batch enrollment Aug 7, 2019

melvinsoft force-pushed the maxi/fix-batch-enrollment-leak branch from 2596240 to e25c8ad Compare August 8, 2019 11:52

Fix instructor dashboard batch enrollment to be site aware

a3c9489

add missing comment

melvinsoft force-pushed the maxi/fix-batch-enrollment-leak branch from e25c8ad to a3c9489 Compare August 8, 2019 11:55

melvinsoft changed the title ~~(WIP) Fix User leakage in Batch enrollment~~ Fix User leakage in Batch enrollment Aug 8, 2019

melvinsoft requested review from OmarIthawi and johnbaldwin August 8, 2019 11:56

OmarIthawi suggested changes Aug 8, 2019

View reviewed changes

johnbaldwin reviewed Aug 9, 2019

View reviewed changes

Comment thread lms/djangoapps/instructor/enrollment.py

johnbaldwin reviewed Aug 9, 2019

View reviewed changes

Comment thread lms/djangoapps/instructor/views/api.py Outdated

johnbaldwin suggested changes Aug 9, 2019

View reviewed changes

melvinsoft added 2 commits August 13, 2019 15:16

factorize organizations calls

3ad0a5b

use feature flag to call regular or tahoe function

2ea6bb4

johnbaldwin reviewed Aug 13, 2019

View reviewed changes

Comment thread lms/djangoapps/instructor/views/api.py

johnbaldwin reviewed Aug 13, 2019

View reviewed changes

johnbaldwin suggested changes Aug 13, 2019

View reviewed changes

melvinsoft added 3 commits August 13, 2019 18:54

add feature flag in one more place

6863922

fix PEP8

0cc3f69

add function comments

4d609d0

pep8 fixes

5e0a8ee

johnbaldwin reviewed Aug 14, 2019

View reviewed changes

johnbaldwin approved these changes Aug 14, 2019

View reviewed changes

melvinsoft merged commit 95bfa4f into appsembler/tahoe/develop Aug 14, 2019

OmarIthawi mentioned this pull request Sep 10, 2019

Refactor and fix the broken API enrollments tests #455

Merged

thraxil deleted the maxi/fix-batch-enrollment-leak branch June 11, 2020 11:28

OmarIthawi mentioned this pull request Jun 29, 2022

Remove UserOrganizationMapping from common; tests for enrollment multi-tenant helpers #1181

Merged

1 task

	def get_request_or_stub():
	"""
	Return the current request or a stub request.

	If called outside the context of a request, construct a fake
	request that can be used to build an absolute URI.

	This is useful in cases where we need to pass in a request object
	but don't have an active request (for example, in tests, celery tasks, and XBlocks).
	"""

Uh oh!

Conversation

melvinsoft commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmarIthawi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnbaldwin Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnbaldwin Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnbaldwin commented Aug 9, 2019

Uh oh!

johnbaldwin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melvinsoft commented Aug 13, 2019

Uh oh!

johnbaldwin Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnbaldwin left a comment

Choose a reason for hiding this comment

Uh oh!

melvinsoft commented Aug 14, 2019

Uh oh!

melvinsoft commented Aug 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnbaldwin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

melvinsoft commented Aug 7, 2019 •

edited

Loading

OmarIthawi left a comment •

edited

Loading

johnbaldwin Aug 9, 2019 •

edited

Loading

johnbaldwin Aug 9, 2019 •

edited

Loading

johnbaldwin left a comment •

edited

Loading

johnbaldwin Aug 13, 2019 •

edited

Loading