Open markkuriekkinen opened 4 years ago
Furthermore, this disables course A to include JS that would do actions on a course B, e.g., by mistake.
Jaakko's short comment about this issue: https://github.com/apluslms/a-plus/pull/479#issuecomment-607353871
Jaakko suggested that the JWT token could be embedded in an HTML data attribute so that JavaScript may read it from there when the token is needed in AJAX queries. aplus.js code already has a hack for adding the CSRF token to AJAX queries. Something similar could be done with JWT tokens. Jaakko says that the server should read and parse the JWT token in a new Django middleware function so that the course ID is readily available in the Django request object when it is needed. Then, the JWT token would not be parsed in a permission class but it would be more widely available without using certain permission classes.
Jaakko wrote about Branca, which is an alternative to JWT, but we prefer JWT. Alternative to JWT tokens would be Branca with e.g. Protocol Buffers (Google format used with gRPC). https://github.com/tuupola/pybranca https://gist.github.com/tuupola/c4ce6ed3b6fa1cdf62783e976d96cd24 Though, I think JWT has better suite for now, and doesn't include too much extra. Though, Branca+ProtoBuf could be used to replace assestment tool tokens, so they wouldn't be internal implementation. Pros: Branca encrypts the information ProtoBufs pack data to a smaller size, which might become relevant with a lot of content Cons: Branca doesn't include specification for keys like JWT https://www.iana.org/assignments/jwt/jwt.xhtml JWT supports assymetric signing, aka. A+ has private key and assestment service can validate with a public key -> at least assestment api should use JWT or derivate (e.g. LTI 1.3) -> session tokens, api restrictions, api tokens can use Branca+ProtocolBuffers
Removed high priority and labeled for discussion, because this does not seem likely to be among the things we are doing in the coming weeks.
The original need in this issue was to fix the performance of the permission IsTeacherOrAdminOrSelf
. There are potential other uses for the suggested "course JWT token", but they haven't been very relevant for us. The permission IsTeacherOrAdminOrSelf
can be fixed without JWT nowadays. The enrollment model was changed to include the user's role, thus it is faster to query if the user has an enrollment on any course with the role TEACHER and status ACTIVE. There is then no need to loop over all CourseInstances (nor all Enrollments).
For example,
is_teacher_anywhere = Enrollment.objects.filter(user_profile=user.userprofile, role=Enrollment.ENROLLMENT_ROLE.TEACHER, status=Enrollment.ENROLLMENT_STATUS.ACTIVE).exists()
Some API endpoints, such as
/api/v2/users
, are not tied to any course instance. In order to allow teachers to use the API, we need to know the course so that we can check the teachers of the course and if the user is one of the teachers, It is inefficient to check all courses in the database. The course instance can be encoded in a JWT token so that the course instance can be parsed from the request without using URL parameters for that.This is related to https://github.com/apluslms/a-plus/issues/476 and its PR https://github.com/apluslms/a-plus/pull/479. The comment (in #479) mentions the permission
IsTeacherOrAdminOrSelf
https://github.com/apluslms/a-plus/pull/479#issuecomment-607310883The inefficient DB query and loop of the permission
IsTeacherOrAdminOrSelf
: https://github.com/apluslms/a-plus/blob/75a89620f872d0d20234b13b2b11138853b63594/userprofile/permissions.py#L31-L39