apluslms / mooc-grader

Automatic assessment framework compatible with A-plus LMS.
15 stars 30 forks source link

Static content URL in feedback templates is not absolute in the production setting #75

Open atilante opened 4 years ago

atilante commented 4 years ago

In short: the variable course.static_url which can be used when rendering a custom feedback template is broken on production server. When I run the course in local development setting, it is correct: course.static_url = "http://localhost:8080/static/default/". When the same template is rendered on production setting, it is: course.static_url = "/static/CS-A1143_2020/", but it should be: "https://grader.cs.hut.fi/static/CS-A1143_2020/".

Longer explanation: I am developing JSAV exercises for Data Structures and Algorithms Y course. There is a need to use a feedback template which is rendered on mooc-grader. The feedback template should refer by URL to CSS and JavaScript files from the static directory of the course, for example, https://grader.cs.hut.fi/static/CS-A1141_2020/OpenDSA/ . Mooc-grader does not know its domain (grader.cs.hut.fi, localhost:8080 in development), because it is running inside a Docker container. The current solution that I know has these hardcoded domains in the Django template or Django view of the exercise. The view or template tries to detect whether it is running in a local development or in a production environment. Reference from A+ manual: https://github.com/apluslms/course-templates/blob/master/exercises/ajax_exercise/template.html From that perspective, it would be nice to have variable similar to course.static_url configured at the container startup so that it is set correctly ("http://localhost:8080/static/" in local, "https://grader.cs.hut.fi:8080/static/" in production).

This feature would assist the work on stop using custom Django views on A+ courses (for example: https://version.aalto.fi/gitlab/course/traky/blob/2020/exercises/jsav/views/iframe.py ), as this is a potential security risk.

raphendyr commented 4 years ago

Apparently static_url is already passed to the exercise renderind context, but that has not been documented: 6de29f34061673af05b6eb2353a8ad6addaaa147

In addition, for local containers a hack was implemented in 0aae89a82f2f4d6ec5b881a38ce83d8e847b05c2 (the reason why static_url seems correct in local content).

MOOC-Grader could configure that value also in production, though MOOC-Grader can't be sure where the static file is really hosted. Thus, I would proceed implementing this in the build pipeline, where we choose where static files are uploaded.

For now, easiest would be adding environment substitutions to the build.sh which is also going to be supported by apluslms/compile-rst container with roman. The end of the build.sh would then look something like this:

url=${STATIC_CONTENT_HOST:-{{ course.static_url }}}
for template in template.html; do
  mkdir -p "_build/templates/${template%/*}"
  sed "s#@@STATIC_CONTENT_URL@@#${host%%#*}#g" \
    "$template" > "_build/templates/$template"
done
make touchrst html

In exercise config yaml, one would then use _build/templates/exercises/ex1_foo.html in place of exercises/ex1_foo.html. Sadly, for this to work on production #57 should be fixed. Alternatively, STATIC_URL_HOST_INJECT should be defined.

That said, I would avoid making absolute value in static_url a part of public API, because it most likely would make future design way harder.

markkuriekkinen commented 2 years ago

57 fixed the course build environment. There is an environment variable for the course build that contains the absolute static URL. However, this issue is about rendering feedback templates at the end of grading submissions.

It would be simple to make the template variable static_url absolute or add a new variable with absolute value. The server setting STATIC_URL_HOST_INJECT seems like it already creates an absolute URL and we could test that again. Nowadays, the course static files are hosted in the Git Manager service, not MOOC-Grader.

MOOC-Grader could configure that value also in production, though MOOC-Grader can't be sure where the static file is really hosted.

I think that the template variable static_url may point to the default location, which used to be in the static files of the MOOC-Grader course and since A+ v1.12, it is in the Git Manager course instance. If someone uses static files hosted elsewhere, they just don't use this variable. We probably won't ever have any complex and more flexible build pipeline with support for multiple static file host locations.

Note: template variables static_url and course.static_url might have different values. I didn't double-check this yet. It looks like static_url is the one that should be used.


57:

The new gitmanager service in A+ v1.12 defines STATIC_CONTENT_HOST.

https://github.com/apluslms/gitmanager/blob/912a7fceff8b232445bbe6cb6d5791f2f4daccf3/builder/builder.py#L112

In RST courses, the build env variable STATIC_CONTENT_HOST should be used in the course's conf.py:

static_host = os.environ.get('STATIC_CONTENT_HOST', 'http://localhost:8080/static/default')