Moodle
  1. Moodle
  2. MDL-29762

HTML Block on category page doesn't show embedded images for not-logged-in users

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2.1
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Blocks
    • Labels:
    • Testing Instructions:
      Hide

      Test 1
      As Admin:

      1. Access the course category page (site admin > courses > add/edit courses > select a categories
      2. Add HTML block, embedded an image for the content and save it
      3. Copy the site url

      On different browser (to make testing easier)

      1. Make sure you are not login to the system
      2. Paste the site url
        Make sure you are able to see the embedded image within the HTML block.

      Test 2
      As Admin:

      1. Access the course category page (site admin > courses > add/edit courses
      2. Add HTML block, embedded an image for the content and save it
        Make sure you are able to see the embedded image within the HTML block.
      Show
      Test 1 As Admin: Access the course category page (site admin > courses > add/edit courses > select a categories Add HTML block, embedded an image for the content and save it Copy the site url On different browser (to make testing easier) Make sure you are not login to the system Paste the site url Make sure you are able to see the embedded image within the HTML block. Test 2 As Admin: Access the course category page (site admin > courses > add/edit courses Add HTML block, embedded an image for the content and save it Make sure you are able to see the embedded image within the HTML block.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      wip-mdl-29762
    • Rank:
      19278

      Description

      Hints for solution:
      /pluginfile.php fetches context information from given context ID - line 55:
      list($context, $course, $cm) = get_context_info_array($contextid);
      Unfortunately, $course is null when viewing a block on a category page.

      Later, /pluginfile.php calls block_html_pluginfile($course, $birecord_or_cm, $context, $filearea, $args, $forcedownload) in /blocks/html/lib.php.
      There, login is required for the given course - line 32:
      require_course_login($course);
      Unfortunately, as $course is null, login requirement always fails for non-logged-in users and image isn't shown.

      A quick and dirty fix would be to replace line 32 in /blocks/html/lib.php

      require_course_login($course);

      with

      if ($course != null)
      require_course_login($course);

      But I'm quite sure that this would provoke some security issues so I would be grateful if you could provide a better fix

        Issue Links

          Activity

          Hide
          Alexander Bias added a comment -

          I just verified that the problem is still present in 2.2.x

          I also refined the patch to ensure better security:

          Please replace in /blocks/html/lib.php

          require_course_login($course);

          with

          if ($course == null)

          { // No course object given - let's check if block is inserted into a category and user isn't logged in global $CFG; $parentcontext = get_context_instance_by_id(get_parent_contextid($context)); if (!($parentcontext->contextlevel == CONTEXT_COURSECAT && !isloggedin() && empty($CFG->forcelogin))) send_file_not_found(); }

          else
          require_course_login($course);

          Show
          Alexander Bias added a comment - I just verified that the problem is still present in 2.2.x I also refined the patch to ensure better security: Please replace in /blocks/html/lib.php require_course_login($course); with if ($course == null) { // No course object given - let's check if block is inserted into a category and user isn't logged in global $CFG; $parentcontext = get_context_instance_by_id(get_parent_contextid($context)); if (!($parentcontext->contextlevel == CONTEXT_COURSECAT && !isloggedin() && empty($CFG->forcelogin))) send_file_not_found(); } else require_course_login($course);
          Hide
          Helen Foster added a comment -

          Alexander, thanks for your report and patch and for confirming that the problem still exists in Moodle 2.2.1. Hopefully someone can review your patch soon.

          Show
          Helen Foster added a comment - Alexander, thanks for your report and patch and for confirming that the problem still exists in Moodle 2.2.1. Hopefully someone can review your patch soon.
          Hide
          Alexander Bias added a comment -

          I don't know if there was a change in require_course_login(), but after updating to Moodle 2.2.2+, logged-in users didn't get images anymore.

          Therefore, I updated the patch once more:

                  if ($course == null) {
                          // No course object given - let's check if block is inserted into a category page
                          global $CFG;
                          $parentcontext = get_context_instance_by_id(get_parent_contextid($context));
                          if ($parentcontext->contextlevel != CONTEXT_COURSECAT || ($CFG->forcelogin && !isloggedin()))
                              send_file_not_found();
                  }
                  else
                          require_course_login($course);
          
          Show
          Alexander Bias added a comment - I don't know if there was a change in require_course_login(), but after updating to Moodle 2.2.2+, logged-in users didn't get images anymore. Therefore, I updated the patch once more: if ($course == null ) { // No course object given - let's check if block is inserted into a category page global $CFG; $parentcontext = get_context_instance_by_id(get_parent_contextid($context)); if ($parentcontext->contextlevel != CONTEXT_COURSECAT || ($CFG->forcelogin && !isloggedin())) send_file_not_found(); } else require_course_login($course);
          Hide
          Rossiani Wijaya added a comment -

          Hi Alex,

          Thank you for reporting and proving the solution.

          In order to access a course page user need to have a specific role to check for their capability to access the page information.

          In this case, not-logged in users won't be able to view file because the system doesn't know their role and capabilities. However, you could set not-loggedin user to to have guest role. This setting is available in site admin > user > permissions > user policies > tick on 'auto-login guests'

          With the above setting, non-logged in user will now see the embedded file.

          Please let me know if it works for you.

          Show
          Rossiani Wijaya added a comment - Hi Alex, Thank you for reporting and proving the solution. In order to access a course page user need to have a specific role to check for their capability to access the page information. In this case, not-logged in users won't be able to view file because the system doesn't know their role and capabilities. However, you could set not-loggedin user to to have guest role. This setting is available in site admin > user > permissions > user policies > tick on 'auto-login guests' With the above setting, non-logged in user will now see the embedded file. Please let me know if it works for you.
          Hide
          Alexander Bias added a comment -

          Rossiani,

          thank you for your response and for thinking about this issue.

          I think that your proposed solution should work in most scenarios, but I think that it's not the best solution. For example, in our moodle installation, we have disabled guest logins with $CFG->guestloginbutton and do not want guest users. And I don't think that this solution is apparent for admins or course managers who simply want to decorate a category page with a nice image - I had to dig very deep to find out why this stupid image isn't shown for not-logged-in users.

          In our moodle installation, the hack which i pasted works for over a year now. But I would be grateful if there would be a official solution for this problem and if you could adopt the patch into moodle core.

          Thanks
          Alex

          Show
          Alexander Bias added a comment - Rossiani, thank you for your response and for thinking about this issue. I think that your proposed solution should work in most scenarios, but I think that it's not the best solution. For example, in our moodle installation, we have disabled guest logins with $CFG->guestloginbutton and do not want guest users. And I don't think that this solution is apparent for admins or course managers who simply want to decorate a category page with a nice image - I had to dig very deep to find out why this stupid image isn't shown for not-logged-in users. In our moodle installation, the hack which i pasted works for over a year now. But I would be grateful if there would be a official solution for this problem and if you could adopt the patch into moodle core. Thanks Alex
          Hide
          Rossiani Wijaya added a comment -

          Hi Alex,

          Thank you for the feedback. The course page is designed to have a specific role to determine their capabilities. However, in this case we might have to bend the rule a little bit in order to support this capability. I will ask other Developer regarding this.

          I created a patch based on your suggestion and made some modification by adding context_system check and changing == null to empty().

          Sending it for peer-review.

          Show
          Rossiani Wijaya added a comment - Hi Alex, Thank you for the feedback. The course page is designed to have a specific role to determine their capabilities. However, in this case we might have to bend the rule a little bit in order to support this capability. I will ask other Developer regarding this. I created a patch based on your suggestion and made some modification by adding context_system check and changing == null to empty(). Sending it for peer-review.
          Hide
          Rajesh Taneja added a comment -

          Hello Rossie,

          I think we are missing some checks on category or system context. If course is null, then context can either be system or category. If yes, then we should send file only if they can view these pages.

          Also, please use $context->get_parent_context() to get parent context, as get_context_instance_by_id is deprecated.

          Added Petr for more feedback, as he introduced require_login check in MDL-22950.

          FYI:
          As discussed, if category is hidden then user can view image if user has image link, whereas trying to access category will give error.

          Show
          Rajesh Taneja added a comment - Hello Rossie, I think we are missing some checks on category or system context. If course is null, then context can either be system or category. If yes, then we should send file only if they can view these pages. Also, please use $context->get_parent_context() to get parent context, as get_context_instance_by_id is deprecated. Added Petr for more feedback, as he introduced require_login check in MDL-22950 . FYI: As discussed, if category is hidden then user can view image if user has image link, whereas trying to access category will give error.
          Hide
          Rossiani Wijaya added a comment -

          Hi Raj,

          Thank you for reviewing. Patch updated.

          Hi Petr,
          Could you give your feedback regarding this issue? Thanks

          Show
          Rossiani Wijaya added a comment - Hi Raj, Thank you for reviewing. Patch updated. Hi Petr, Could you give your feedback regarding this issue? Thanks
          Hide
          Petr Škoda added a comment -

          Hi, I guess it is missing CONTEXT_USER handling. Maybe you could somehow use $context->get_course_context(false) because it tells you if the context is inside a course.

          Show
          Petr Škoda added a comment - Hi, I guess it is missing CONTEXT_USER handling. Maybe you could somehow use $context->get_course_context(false) because it tells you if the context is inside a course.
          Hide
          Rajesh Taneja added a comment -

          There is no easy way to get capability on sticky blocks. After talking with Sam H, it seems we can't do any checking for SYSTEM and USER context.
          Don't see much of a security issue with this, although created MDL-36050, for future reference.

          Show
          Rajesh Taneja added a comment - There is no easy way to get capability on sticky blocks. After talking with Sam H, it seems we can't do any checking for SYSTEM and USER context. Don't see much of a security issue with this, although created MDL-36050 , for future reference.
          Hide
          Rossiani Wijaya added a comment -

          Hi Raj,

          The patch looks great.

          However in the case of category is hidden, as a non-login user I'm still able to access the image file through the url. Shouldn't this access be limited?

          Check the following:
          [y] Syntax
          [y] Output
          [y] Whitespace
          [-] Language
          [-] Databases
          [y] Testing
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Show
          Rossiani Wijaya added a comment - Hi Raj, The patch looks great. However in the case of category is hidden, as a non-login user I'm still able to access the image file through the url. Shouldn't this access be limited? Check the following: [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
          Hide
          Rajesh Taneja added a comment -

          Thanks Rossie,

          If category is hidden then image will not be visible, although I have open another issue for capability check on sticky blocks. There is no nice way to check capability on sticky blocks and it might fail in few cases and show image.

          Show
          Rajesh Taneja added a comment - Thanks Rossie, If category is hidden then image will not be visible, although I have open another issue for capability check on sticky blocks. There is no nice way to check capability on sticky blocks and it might fail in few cases and show image.
          Hide
          Aparup Banerjee 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
          Aparup Banerjee 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
          Dan Poltawski added a comment -

          Thanks Raj, integrated to 22, 23 and master.

          I had a think about this for a while, but really can't come up with a better solution than you've suggested.

          Show
          Dan Poltawski added a comment - Thanks Raj, integrated to 22, 23 and master. I had a think about this for a while, but really can't come up with a better solution than you've suggested.
          Hide
          Dan Poltawski added a comment -

          Please could you add regression testing instructions for other situations, such as normal course page, site pages logged in/logged out too.

          Show
          Dan Poltawski added a comment - Please could you add regression testing instructions for other situations, such as normal course page, site pages logged in/logged out too.
          Hide
          David Monllaó added a comment -

          Tested in 22, 23 and master. It passes.

          Show
          David Monllaó added a comment - Tested in 22, 23 and master. It passes.
          Hide
          Dan Poltawski added a comment -

          Hurray!

          You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

          Show
          Dan Poltawski added a comment - Hurray! You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

            People

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

              Dates

              • Created:
                Updated:
                Resolved: