From dfde126d00f953612dad3be49b3d03e65b4f0f14 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Mon, 2 Sep 2019 16:45:53 -0500 Subject: [PATCH 1/4] Make impersonate_user local to a course If a user is a super-user, they'll be able to impersonate anyone. If not, a user (ta) be able to impersonate a user who they have access to (student) in that particular course. Note that when impersonating, a ta will only be able to access the courses of that student that he/she has permission for. Access to other courses will be denied. (Any urls starting with /course/) One possible issue with this approach is that a ta will be able to see the home page of the student, which means they'll be able to see the titles of some private courses that the student has access to. --- course/auth.py | 41 ++++++++++++++++++++++++++++++-------- relate/templates/base.html | 2 +- relate/urls.py | 7 ++++--- 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/course/auth.py b/course/auth.py index dd264f00c..254bfd0d3 100644 --- a/course/auth.py +++ b/course/auth.py @@ -59,8 +59,9 @@ user_status, participation_status, participation_permission as pperm, + COURSE_ID_REGEX, ) -from course.models import Participation, ParticipationRole, AuthenticationToken # noqa +from course.models import Participation, ParticipationRole, AuthenticationToken, Course # noqa from accounts.models import User from course.utils import render_course_page, course_view @@ -82,14 +83,15 @@ def get_pre_impersonation_user(request): return None -def get_impersonable_user_qset(impersonator): +def get_impersonable_user_qset(impersonator, course_identifier): # type: (User) -> query.QuerySet - if impersonator.is_superuser: - return User.objects.exclude(pk=impersonator.pk) + + course = Course.objects.get(identifier=course_identifier) my_participations = Participation.objects.filter( user=impersonator, - status=participation_status.active) + status=participation_status.active, + course=course) impersonable_user_qset = User.objects.none() for part in my_participations: @@ -120,6 +122,14 @@ def get_impersonable_user_qset(impersonator): return impersonable_user_qset +def _get_current_course_from_request(request): + course_match = re.match("^/course/"+COURSE_ID_REGEX+"/", request.get_full_path()) + if course_match is None: + return None + else: + return course_match.group("course_identifier") + + class ImpersonateMiddleware(object): def __init__(self, get_response): self.get_response = get_response @@ -127,6 +137,8 @@ def __init__(self, get_response): def __call__(self, request): if 'impersonate_id' in request.session: imp_id = request.session['impersonate_id'] + imp_course = request.session['impersonate_course'] + cur_course = _get_current_course_from_request(request) impersonee = None try: @@ -140,13 +152,20 @@ def __call__(self, request): if request.user.is_superuser: may_impersonate = True else: - qset = get_impersonable_user_qset(cast(User, request.user)) + if cur_course is not None: + qset = get_impersonable_user_qset(cast(User, request.user), + course_identifier=cur_course) + else: + qset = get_impersonable_user_qset(cast(User, request.user), + course_identifier=imp_course) if qset.filter(pk=cast(User, impersonee).pk).count(): may_impersonate = True if may_impersonate: request.relate_impersonate_original_user = request.user request.user = impersonee + elif cur_course is not None and cur_course != imp_course: + raise PermissionDenied() else: messages.add_message(request, messages.ERROR, _("Error while impersonating.")) @@ -209,12 +228,17 @@ def __init__(self, *args, **kwargs): self.helper.add_input(Submit("submit", _("Impersonate"))) -def impersonate(request): +def impersonate(request, course_identifier): # type: (http.HttpRequest) -> http.HttpResponse if not request.user.is_authenticated: raise PermissionDenied() - impersonable_user_qset = get_impersonable_user_qset(cast(User, request.user)) + impersonator = cast(User, request.user) + if impersonator.is_superuser: + impersonable_user_qset = User.objects.exclude(pk=impersonator.pk) + else: + impersonable_user_qset = get_impersonable_user_qset(impersonator, + course_identifier) if not impersonable_user_qset.count(): raise PermissionDenied() @@ -234,6 +258,7 @@ def impersonate(request): impersonee = form.cleaned_data["user"] request.session['impersonate_id'] = impersonee.id + request.session['impersonate_course'] = course_identifier request.session['relate_impersonation_header'] = form.cleaned_data[ "add_impersonation_header"] diff --git a/relate/templates/base.html b/relate/templates/base.html index f58bb5b36..412eca4c9 100644 --- a/relate/templates/base.html +++ b/relate/templates/base.html @@ -108,7 +108,7 @@ {% if currently_impersonating %}
  • {% trans "Stop impersonating" %}
    • {% else %} -
  • {% trans "Impersonate user" %}
    • +
  • {% trans "Impersonate user" %}
    • {% endif %} {% if pperm.set_fake_time or request.relate_impersonate_original_user|may_set_fake_time %}
  • {% trans "Set fake time" %}
    • diff --git a/relate/urls.py b/relate/urls.py index 0e084ced3..f4396b303 100644 --- a/relate/urls.py +++ b/relate/urls.py @@ -99,11 +99,12 @@ name="relate-monitor_task"), # {{{ troubleshooting - - url(r'^user/impersonate/$', + url(r"^course" + "/" + COURSE_ID_REGEX + + "/impersonate/$", course.auth.impersonate, name="relate-impersonate"), - url(r'^user/stop_impersonating/$', + url(r"^user/stop_impersonating/$", course.auth.stop_impersonating, name="relate-stop_impersonating"), From a7f9942957cf9af1fc6d7ac176e657df4f47c17f Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Tue, 3 Sep 2019 23:54:23 -0500 Subject: [PATCH 2/4] Try course_id and don't allow other pages --- course/auth.py | 49 +++++++++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/course/auth.py b/course/auth.py index 254bfd0d3..c28a14603 100644 --- a/course/auth.py +++ b/course/auth.py @@ -83,15 +83,14 @@ def get_pre_impersonation_user(request): return None -def get_impersonable_user_qset(impersonator, course_identifier): +def get_impersonable_user_qset(impersonator, course_id): # type: (User) -> query.QuerySet - course = Course.objects.get(identifier=course_identifier) - + print("course_id", course_id) my_participations = Participation.objects.filter( user=impersonator, status=participation_status.active, - course=course) + course__id=course_id) impersonable_user_qset = User.objects.none() for part in my_participations: @@ -137,8 +136,7 @@ def __init__(self, get_response): def __call__(self, request): if 'impersonate_id' in request.session: imp_id = request.session['impersonate_id'] - imp_course = request.session['impersonate_course'] - cur_course = _get_current_course_from_request(request) + cur_course_identifier = _get_current_course_from_request(request) impersonee = None try: @@ -148,24 +146,35 @@ def __call__(self, request): pass may_impersonate = False + permission_error = False if impersonee is not None: if request.user.is_superuser: may_impersonate = True else: - if cur_course is not None: - qset = get_impersonable_user_qset(cast(User, request.user), - course_identifier=cur_course) + imp_course_id = request.session['impersonate_course_id'] + if cur_course_identifier is not None: + cur_course = Course.objects.get(identifier=cur_course_identifier) + if cur_course.id == imp_course_id: + qset = get_impersonable_user_qset(cast(User, request.user), + course_id=imp_course_id) + if qset.filter(pk=cast(User, impersonee).pk).count(): + may_impersonate = True + else: + permission_error = True else: - qset = get_impersonable_user_qset(cast(User, request.user), - course_identifier=imp_course) - if qset.filter(pk=cast(User, impersonee).pk).count(): - may_impersonate = True + permission_error = True if may_impersonate: request.relate_impersonate_original_user = request.user request.user = impersonee - elif cur_course is not None and cur_course != imp_course: - raise PermissionDenied() + elif request.get_full_path() == "/user/stop_impersonating/": + pass + elif request.get_full_path().startswith("/logout/"): + pass + elif permission_error: + messages.add_message(request, messages.WARNING, + _("Permission denied to impersonate this page. Falling back to " + \ + cast(User, request.user).username)) else: messages.add_message(request, messages.ERROR, _("Error while impersonating.")) @@ -236,9 +245,11 @@ def impersonate(request, course_identifier): impersonator = cast(User, request.user) if impersonator.is_superuser: impersonable_user_qset = User.objects.exclude(pk=impersonator.pk) + coutse = None else: + course = Course.objects.get(identifier=course_identifier) impersonable_user_qset = get_impersonable_user_qset(impersonator, - course_identifier) + course_id=course.id) if not impersonable_user_qset.count(): raise PermissionDenied() @@ -258,7 +269,8 @@ def impersonate(request, course_identifier): impersonee = form.cleaned_data["user"] request.session['impersonate_id'] = impersonee.id - request.session['impersonate_course'] = course_identifier + if course is not None: + request.session['impersonate_course_id'] = course.id request.session['relate_impersonation_header'] = form.cleaned_data[ "add_impersonation_header"] @@ -285,7 +297,7 @@ def stop_impersonating(request): if "stop_impersonating" not in request.POST: raise SuspiciousOperation(_("odd POST parameters")) - if not hasattr(request, "relate_impersonate_original_user"): + if not 'impersonate_id' in request.session: # prevent user without pperm to stop_impersonating my_participations = Participation.objects.filter( user=request.user, @@ -1234,6 +1246,7 @@ def wrapper(request, course_identifier, *args, **kwargs): token = find_matching_token(**auth_data) if token is None: raise PermissionDenied("invalid authentication token") + print(course_identifier) from django.contrib.auth import authenticate, login user = authenticate(**auth_data) From f6d4c830e2ab1e1c2a81c76835b6e2538cced1a0 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Tue, 3 Sep 2019 23:58:09 -0500 Subject: [PATCH 3/4] Use course_identifier and save database call --- course/auth.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/course/auth.py b/course/auth.py index c28a14603..163b676cf 100644 --- a/course/auth.py +++ b/course/auth.py @@ -83,14 +83,13 @@ def get_pre_impersonation_user(request): return None -def get_impersonable_user_qset(impersonator, course_id): +def get_impersonable_user_qset(impersonator, course_identifier): # type: (User) -> query.QuerySet - print("course_id", course_id) my_participations = Participation.objects.filter( user=impersonator, status=participation_status.active, - course__id=course_id) + course__identifier=course_identifier) impersonable_user_qset = User.objects.none() for part in my_participations: @@ -151,12 +150,11 @@ def __call__(self, request): if request.user.is_superuser: may_impersonate = True else: - imp_course_id = request.session['impersonate_course_id'] + imp_course_identifier = request.session['impersonate_course_identifier'] if cur_course_identifier is not None: - cur_course = Course.objects.get(identifier=cur_course_identifier) - if cur_course.id == imp_course_id: + if cur_course_identifier == imp_course_identifier: qset = get_impersonable_user_qset(cast(User, request.user), - course_id=imp_course_id) + course_identifier=imp_course_identifier) if qset.filter(pk=cast(User, impersonee).pk).count(): may_impersonate = True else: @@ -245,11 +243,9 @@ def impersonate(request, course_identifier): impersonator = cast(User, request.user) if impersonator.is_superuser: impersonable_user_qset = User.objects.exclude(pk=impersonator.pk) - coutse = None else: - course = Course.objects.get(identifier=course_identifier) impersonable_user_qset = get_impersonable_user_qset(impersonator, - course_id=course.id) + course_identifier=course_identifier) if not impersonable_user_qset.count(): raise PermissionDenied() @@ -269,8 +265,8 @@ def impersonate(request, course_identifier): impersonee = form.cleaned_data["user"] request.session['impersonate_id'] = impersonee.id - if course is not None: - request.session['impersonate_course_id'] = course.id + if course_identifier is not None: + request.session['impersonate_course_identifier'] = course_identifier request.session['relate_impersonation_header'] = form.cleaned_data[ "add_impersonation_header"] @@ -1246,7 +1242,6 @@ def wrapper(request, course_identifier, *args, **kwargs): token = find_matching_token(**auth_data) if token is None: raise PermissionDenied("invalid authentication token") - print(course_identifier) from django.contrib.auth import authenticate, login user = authenticate(**auth_data) From 04c38aea98088be34e6c6aba4119c7f88d304492 Mon Sep 17 00:00:00 2001 From: Isuru Fernando Date: Wed, 4 Sep 2019 00:01:09 -0500 Subject: [PATCH 4/4] don't send to home --- course/auth.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/course/auth.py b/course/auth.py index 163b676cf..5ebfeaff6 100644 --- a/course/auth.py +++ b/course/auth.py @@ -265,10 +265,11 @@ def impersonate(request, course_identifier): impersonee = form.cleaned_data["user"] request.session['impersonate_id'] = impersonee.id - if course_identifier is not None: - request.session['impersonate_course_identifier'] = course_identifier request.session['relate_impersonation_header'] = form.cleaned_data[ "add_impersonation_header"] + if course_identifier is not None: + request.session['impersonate_course_identifier'] = course_identifier + return redirect("/course/{}".format(course_identifier)) # Because we'll likely no longer have access to this page. return redirect("relate-home")