Make impersonate_user local to a course #670
Conversation
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/<id>) 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
Outdated
| @@ -234,6 +258,7 @@ def impersonate(request): | |||
| impersonee = form.cleaned_data["user"] | |||
|
|
|||
| request.session['impersonate_id'] = impersonee.id | |||
| request.session['impersonate_course'] = course_identifier | |||
There was a problem hiding this comment.
It might be useful to store the primary key and not the identifier. That's cheaper to use (no database roundtrip).
There was a problem hiding this comment.
How do you filter by the primary key?
Participation.objects.filter(
user=impersonator,
status=participation_status.active,
course_id=course_id)
?
There was a problem hiding this comment.
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)
course/auth.py
Outdated
|
|
||
| my_participations = Participation.objects.filter( | ||
| user=impersonator, | ||
| status=participation_status.active) | ||
| status=participation_status.active, | ||
| course=course) |
There was a problem hiding this comment.
This should only consider the participation in the course to which the impersonation is scoped.
course/auth.py
Outdated
| 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'] |
There was a problem hiding this comment.
Could you change this variable to be end in _id (assuming you take the other suggestion on what goes in the session)?
course/auth.py
Outdated
| course_identifier=cur_course) | ||
| else: | ||
| qset = get_impersonable_user_qset(cast(User, request.user), | ||
| course_identifier=imp_course) |
There was a problem hiding this comment.
If the impersonation is course-scoped, no impersonation should happen if cur_course is None.
There was a problem hiding this comment.
I'll have to selectively allow /stop_impersonating (or change stop_impersonating url to also be course based) and signout page, right?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll have to stop the error message though, otherwise clicking that button would give me,
There was a problem hiding this comment.
Sure. But if they're already impersonating someone, it's fine to assume that they were (at some point) permitted to do that.
| @@ -209,12 +228,17 @@ def __init__(self, *args, **kwargs): | |||
| self.helper.add_input(Submit("submit", _("Impersonate"))) | |||
|
|
|||
|
|
|||
| def impersonate(request): | |||
| def impersonate(request, course_identifier): | |||
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Would they be superusers? Or users with a relate permission level?
|
Thanks for your work on this! |
|
Could you merge in master to get the latest CI? Also, could you remind me what the status is here? |
Codecov Report
@@ Coverage Diff @@
## master #670 +/- ##
===========================================
- Coverage 96.59% 63.83% -32.77%
===========================================
Files 45 45
Lines 11105 11126 +21
Branches 2065 2072 +7
===========================================
- Hits 10727 7102 -3625
- Misses 302 3414 +3112
- Partials 76 610 +534
Continue to review full report at Codecov.
|
Need to write tests. |
If a user is a super-user, they'll be able to impersonate anyone.
If not, a user (ta) can 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.