Opencast-Moodle / moodle-filter_opencast

Filter to embed Opencast videos in Moodle content
GNU General Public License v3.0
9 stars 9 forks source link

Iframes can load before authenticated #22

Closed t-schroeder closed 2 years ago

t-schroeder commented 5 years ago

I noticed that sometimes I get login prompts inside some of the Opencast videos on a page when visiting it for the first time after logging in to Moodle.

I guess the reason is that this plugin sends one LTI request max per page which is triggered by JavaScript. And what happens to me is that sometimes the iframes already get loaded before the LTI request has been sent.

One way to solve this would be to get each Opencast video through an LTI request where you additionally send the custom_tool parameter to desired video's URL, e.g. /engage/theodul/ui/core.html?id=55abe714-c7d4-4449-9592-106b39241241.

t-schroeder commented 5 years ago

I looked into this a bit more and found that this problem (iframes loading before the user is authenticated) never happens with the first iframe on a page. Then I saw what the problem was: After rendering the first video filter_opencast::$loginrendered is true and thus for the remaining videos on that page $loggedin doesn't get set to false. That means the iframes get rendered with the URL in the src attribute as opposed to the data-framesrc attribute. Therefore all but the first iframe on the page can load the video right away.

Looks like this error was introduced here: https://github.com/learnweb/moodle-filter_opencastfilter/commit/918dde1e1d1dc5a528522f06e02a54d4c0bb6eb3

NinaHerrmann commented 3 years ago

Hey, sorry for the late reply. We are working on the plugin to make it ready for the moodle plugin directory. Could you provide a description for a setup where a iFrame on a page is loaded although the student should not have access? Because either students have access to a whole series or the iFrame is not visible to them, or am I wrong? Thanks in case you still have time to answer, otherwise, we will consider this issue before releasing the plugin.

t-schroeder commented 3 years ago

Imagine you have a course with multiple labels (mod_label) each with an Opencast video in it and each visible on the main course page. So the filter() function of this plugin will be called multiple times. For the first label $loginrendered will be false initially, then the login form is rendered and afterwards it will be true. So for each of the following labels $loginrendered will be true. Consequently $loggedin will be true for all following calls to the filter() function on the course main page. For the first label (containing the first video) $loggedin was false, though.

The value of $loggedin is being passed to the mustache template and controls whether the video URL is set as the "src" or the "data-frameSrc" attribute of the iframe. It should be set as the "data-frameSrc" attribute for all videos on the page. But as I explained above it will only be set correctly for the first video on the page. So all but the first iframes will often already load the video before the LTI authentication is even performed (by form.js in the amd folder).

Note: This is only a problem if you have multiple text fields containing videos that are displayed on the same page like with the multiple labels on the course main page. If all videos visible on the page are in one single text field, e.g. all in one page (mod_page), this is not a problem.

Note 2: Since the user usually stays authenticated in Opencast for a while you will only encounter this when visiting the respective page after your last Opencast session has expired and you are no longer authenticated. So to test this I usually open the page in a new private browser window.

t-schroeder commented 3 years ago

The way I see it the $loggedin variable has no longer a reason to exist. It's probably a relic of a previous versions of this filter. Previously if the Opencast session cookie was already set, the login form wouldn't be rendered and all the video iframes on the page would directly be rendered with the "src" attribute. But since that functionality is no longer there, I suggest just removing that variable entirely.