Remove unnecessary csrf_exempt from urls #1160
Conversation
In Django's class-based views, function decorators must be used with `method_decorator`. https://docs.djangoproject.com/en/3.2/topics/class-based-views/intro/#decorating-the-class Move the `csrf_exempt` for the class `LtiLaunchView` from urls.py to the view class definition so that it is more consistent with the other LTI Tool views. The same view also requires the `xframe_options_exempt` decorator. Class `LtiSessionMixin` does not need `csrf_exempt`. Most of its child classes are able to normally check CSRF tokens because the HTML forms are first rendered in A+. Class `LtiExerciseView` needs both `csrf_exempt` and `xframe_options_exempt`. * `csrf_exempt` is needed since it is used in class `ExerciseView` and the exercise forms loaded from assessment services do not include any CSRF tokens. * `xframe_options_exempt` is needed since LTI Tools may be embedded in iframes in the LTI Platform. Fixes apluslms#1099
markkuriekkinen
force-pushed
the
csrf-proper-integration
branch
from
fb3382c
to
a4c3dca
Compare
There was a problem hiding this comment.
The pull request was still missing a few important fixes, but I have now finished them myself so that we can merge this pull request.
Class-based views can't use function decorators directly. They need to combine the decorators with method_decorator.
https://docs.djangoproject.com/en/3.2/topics/class-based-views/intro/#decorating-in-urlconf
The old code had done this wrong with csrf_exempt and xframe_options_exempt.
The LTI login and launch views are the ones that need csrf_exempt. They receive HTTP POST requests that are not triggered by a typical form and we can't easily add any CSRF tokens there. Since the request can not include a valid CSRF token, the backend must exempt the CSRF check.
In the LTI content selection views (for LTI Deep Linking), A+ renders first normal HTML forms that include the CSRF token. The backend should then check the CSRF tokens. The old code used to exempt all views.
In local testing (under localhost with insecure HTTP protocol), we need to add CSRF_COOKIE_SAMESITE = 'None' to the A-plus Django settings. Otherwise, the browser refuses to send the CSRF cookie in the iframe used with LTI.
Class ExerciseView used in sending new submissions has always used csrf_exempt. The exercise HTML forms do not include any CSRF tokens since the forms are first fetched from the assessment service such as the MOOC-Grader. I believe that A+ could then manually insert the CSRF token to the HTML form, but this has never been implemented thus far. Therefore, class LtiExerciseView must also use csrf_exempt. Due to class inheritance and different decorators used in each class, the dispatch method is defined again in LtiExerciseView with the specific decorators used in that view.
Fixes #1099
Description
Properly setup exemptions for CSRF, leaving them where they are needed
Why?
CSRF exemptions are needed in some cases, this pull request removes duplicate exemptions and ensures that they are present for all views which need them.
How?
Experiment which exemptions are needed for no errors to occur.
Fixes #1099
Testing
What type of test did you run?
Ensure there are no CSRF errors when opening an exercise as a student and when selecting an exercise as a teacher, on both localhost as well as minus.cs + mycourses-test
Did you test the changes in
Translation
Programming style
Have you updated the README or other relevant documentation?
Is it Done?
Clean up your git commit history before submitting the pull request!