Moodle
  1. Moodle
  2. MDL-30810

Referencing a file which requires login leads to a bad redirect when logging in

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 2.2
    • Fix Version/s: None
    • Component/s: Files API
    • Labels:
    • Testing Instructions:
      Hide

      1/ Create a new course
      2/ Upload a new image resource to the course. copy he url of the uploaded image
      3/ Go to the site frontpage,
      4/ Add a topic section to the frontpage, and embed the uploaded image from the previous course to the frontpage
      5/ Start a new browser session (e.g. chrome incognito window) and visit moodle site
      6/ Login to site in incognito window

      Expected result:

      • Logged into moodle as normal

      Actual result:

      • Redirected to image file which was embeded
      Show
      1/ Create a new course 2/ Upload a new image resource to the course. copy he url of the uploaded image 3/ Go to the site frontpage, 4/ Add a topic section to the frontpage, and embed the uploaded image from the previous course to the frontpage 5/ Start a new browser session (e.g. chrome incognito window) and visit moodle site 6/ Login to site in incognito window Expected result: Logged into moodle as normal Actual result: Redirected to image file which was embeded
    • Affected Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
    • Rank:
      33751

      Description

      It seems that the wantsurl redirection is again broken on 2.x similar to MDL-14495, moodle will try and redirect a user to an image if embeded incorrectly before being logged in.

      There is a require_login flag preventing this, although it looks like file_pluginfile calls require_login a crazy amount of times

        Activity

        Hide
        Dan Poltawski added a comment -

        OK, this proof of concept path fixes the issue, but it is bloody ugly.

        Submitting for peer review, hoping that someone can think of a more elegant solution..

        Show
        Dan Poltawski added a comment - OK, this proof of concept path fixes the issue, but it is bloody ugly. Submitting for peer review, hoping that someone can think of a more elegant solution..
        Hide
        Michael de Raadt added a comment -

        Thanks for working on this.

        I'll try to get this peer reviewed for you.

        Show
        Michael de Raadt added a comment - Thanks for working on this. I'll try to get this peer reviewed for you.
        Hide
        Jason Fowler added a comment -

        Code looks good. Is this something that will need to be back ported to previous versions at all?

        Show
        Jason Fowler added a comment - Code looks good. Is this something that will need to be back ported to previous versions at all?
        Hide
        Dan Poltawski added a comment -

        Yes, this needs to go into all the 2.x branches. I will work on this

        Show
        Dan Poltawski added a comment - Yes, this needs to go into all the 2.x branches. I will work on this
        Hide
        Dan Poltawski added a comment -

        Whoops, forgot about this issue. Rebased and created branches and submiting for integration.

        Its a real problem for many people.

        Show
        Dan Poltawski added a comment - Whoops, forgot about this issue. Rebased and created branches and submiting for integration. Its a real problem for many people.
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Sam Hemelryk added a comment -

        Hi Dan,

        Just an idea that strikes me when looking at these changes, I can't help but wonder whether we should be forcing $setwantsurltome to false when serving files in the same way we do with AJAX script.
        It's easier in the case of AJAX as we already have the convenient AJAX_SCRIPT define. Perhaps it is worth looking at introducing a similar define for serving files.

        At this point I am assuming we never want to set the wants url when serving a file, perhaps I am missing something so feel free to correct me there.
        I note that there is still a require_course_login call that has not been converted when serving for modules within file_pluginfile and of course many of the plugin callback implementations will be making similar require_login or require_course_login calls.
        Do these need to be converted as well? If so then I think implementing a file serving define is certainly the way to go.

        I'm leaving this in integration as I suspect I have perhaps missed something here and that these changes are spot on
        Will wait for your thoughts Dan.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Dan, Just an idea that strikes me when looking at these changes, I can't help but wonder whether we should be forcing $setwantsurltome to false when serving files in the same way we do with AJAX script. It's easier in the case of AJAX as we already have the convenient AJAX_SCRIPT define. Perhaps it is worth looking at introducing a similar define for serving files. At this point I am assuming we never want to set the wants url when serving a file, perhaps I am missing something so feel free to correct me there. I note that there is still a require_course_login call that has not been converted when serving for modules within file_pluginfile and of course many of the plugin callback implementations will be making similar require_login or require_course_login calls. Do these need to be converted as well? If so then I think implementing a file serving define is certainly the way to go. I'm leaving this in integration as I suspect I have perhaps missed something here and that these changes are spot on Will wait for your thoughts Dan. Cheers Sam
        Hide
        Dan Poltawski added a comment -

        No, I don't think you have missed anything Sam - this is why I said above " hoping that someone can think of a more elegant solution.."

        In theory maybe we would want wantsurl to work when someone links directly to a file, but that is not a good case compared to when wantsurl is set by a badly embedded resource (an image from a course for e.g.) and then causes the user to be redirected to it with seemingly no way to get back to where they were.

        Show
        Dan Poltawski added a comment - No, I don't think you have missed anything Sam - this is why I said above " hoping that someone can think of a more elegant solution.." In theory maybe we would want wantsurl to work when someone links directly to a file, but that is not a good case compared to when wantsurl is set by a badly embedded resource (an image from a course for e.g.) and then causes the user to be redirected to it with seemingly no way to get back to where they were.
        Hide
        Sam Hemelryk added a comment -

        OK I've just had a chat to Dan about this issue.
        We've agreed that while the define solution (implemented in the same style as AJAX_SCRIPT) is not a perfect solution it is perhaps the lesser evil.
        It will blanket the require_login/require_course_login calls forcing $setwantsurltome to false within those methods for any script that defines itself as a file mediation script.

        Petr I have added you as a watcher to this issue. It'd be great to get your feedback on the way we are planning to proceed. You know this area better than anyone!

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - OK I've just had a chat to Dan about this issue. We've agreed that while the define solution (implemented in the same style as AJAX_SCRIPT) is not a perfect solution it is perhaps the lesser evil. It will blanket the require_login/require_course_login calls forcing $setwantsurltome to false within those methods for any script that defines itself as a file mediation script. Petr I have added you as a watcher to this issue. It'd be great to get your feedback on the way we are planning to proceed. You know this area better than anyone! Cheers Sam
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this issue.

        We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

        If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

        Michael d.

        TW9vZGxlDQo=

        Show
        Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
        Hide
        Michael de Raadt added a comment -

        I'm closing this issue as it has been inactive for over a year has been recorded as affecting versions that are no longer supported.

        This is being done as part of a bulk annual clean-up of issues.

        If you still believe this is an issue in supported versions, please create a new issue.

        Show
        Michael de Raadt added a comment - I'm closing this issue as it has been inactive for over a year has been recorded as affecting versions that are no longer supported. This is being done as part of a bulk annual clean-up of issues. If you still believe this is an issue in supported versions, please create a new issue.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: