Skip to content
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

Make impersonate_user local to a course #670

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 33 additions & 8 deletions course/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

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

This should only consider the participation in the course to which the impersonation is scoped.


impersonable_user_qset = User.objects.none()
for part in my_participations:
Expand Down Expand Up @@ -120,13 +122,23 @@ 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

def __call__(self, request):
if 'impersonate_id' in request.session:
imp_id = request.session['impersonate_id']
imp_course = request.session['impersonate_course']
Copy link
Owner

Choose a reason for hiding this comment

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

Could you change this variable to be end in _id (assuming you take the other suggestion on what goes in the session)?

cur_course = _get_current_course_from_request(request)
impersonee = None

try:
Expand All @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

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

If the impersonation is course-scoped, no impersonation should happen if cur_course is None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have to selectively allow /stop_impersonating (or change stop_impersonating url to also be course based) and signout page, right?

Copy link
Owner

Choose a reason for hiding this comment

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

No, I don't think that's necessary. /stop_impersonating will look directly at the values in the session and will work OK even if the request.user is not the impersonee.

Copy link
Owner

Choose a reason for hiding this comment

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

Same for sign-out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have to stop the error message though, otherwise clicking that button would give me,

image

Copy link
Owner

Choose a reason for hiding this comment

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

Sure. But if they're already impersonating someone, it's fine to assume that they were (at some point) permitted to do that.

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."))
Expand Down Expand Up @@ -209,12 +228,17 @@ def __init__(self, *args, **kwargs):
self.helper.add_input(Submit("submit", _("Impersonate")))


def impersonate(request):
def impersonate(request, course_identifier):
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should have a non-course entry point (URL) for the (global) impersonation. (Not for me, but I'm thinking of other folks serving as admins of Relate instances.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would they be superusers? Or users with a relate permission level?

Copy link
Owner

Choose a reason for hiding this comment

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

Would they be superusers?

Yep.

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

Expand All @@ -234,6 +258,7 @@ def impersonate(request):
impersonee = form.cleaned_data["user"]

request.session['impersonate_id'] = impersonee.id
request.session['impersonate_course'] = course_identifier
Copy link
Owner

Choose a reason for hiding this comment

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

It might be useful to store the primary key and not the identifier. That's cheaper to use (no database roundtrip).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you filter by the primary key?

Participation.objects.filter(
        user=impersonator,
        status=participation_status.active,
        course_id=course_id)

?

Copy link
Owner

Choose a reason for hiding this comment

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

Yep.

Copy link
Contributor Author

@isuruf isuruf Sep 4, 2019

Choose a reason for hiding this comment

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

We need to check that the impersonate_course_id is equal to the current course which requires a database call. I'll change the filter to following which saves a database call.

Participation.objects.filter(
        user=impersonator,
        status=participation_status.active,
        course__identifier=course_identifier)

request.session['relate_impersonation_header'] = form.cleaned_data[
"add_impersonation_header"]

Expand Down
2 changes: 1 addition & 1 deletion relate/templates/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@
{% if currently_impersonating %}
<li><a onclick=stop_impersonate()>{% trans "Stop impersonating" %}</a></li>
{% else %}
<li><a href="{% url 'relate-impersonate' %}" target="_blank">{% trans "Impersonate user" %}</a></li>
<li><a href="{% url 'relate-impersonate' course.identifier %}" target="_blank">{% trans "Impersonate user" %}</a></li>
{% endif %}
{% if pperm.set_fake_time or request.relate_impersonate_original_user|may_set_fake_time %}
<li><a href="{% url 'relate-set_fake_time' %}" target="_blank">{% trans "Set fake time" %}</a></li>
Expand Down
7 changes: 4 additions & 3 deletions relate/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"),

Expand Down