Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-29762

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            abias 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
            abias 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
            tsala 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
            tsala 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
            abias 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
            abias 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
            rwijaya 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
            rwijaya 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
            abias 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
            abias 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
            rwijaya 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
            rwijaya 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
            rajeshtaneja 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
            rajeshtaneja 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
            rwijaya 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
            rwijaya 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            rajeshtaneja 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
            rajeshtaneja 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
            rwijaya 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
            rwijaya 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
            rajeshtaneja 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
            rajeshtaneja 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
            nebgor 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
            nebgor 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
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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
            dmonllao David Monllaó added a comment -

            Tested in 22, 23 and master. It passes.

            Show
            dmonllao David Monllaó added a comment - Tested in 22, 23 and master. It passes.
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  12/Nov/12