Moodle
  1. Moodle
  2. MDL-34250

Error shown when logging in as a user into a course in a hidden category

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.1, 2.4
    • Fix Version/s: 2.3.2
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      Be sure that 'navshowmycoursecategories' is set to 'Yes'

      As Administrator:
      1. Set Miscellaneous to NOT show (grayed out)
      2. Within Miscellaneous create a cours i.e. Test123 (visible, not-grayed out)
      3. Course full name test123 and Course short name test123
      4. Enrol the administrator in this new course.

      Verify that the navigation block is rendered correctly

      Also try enrolling a student into the test123 coure, and verify that when logging in as a student the navigation block is built correctly.

      Show
      Be sure that 'navshowmycoursecategories' is set to 'Yes' As Administrator: 1. Set Miscellaneous to NOT show (grayed out) 2. Within Miscellaneous create a cours i.e. Test123 (visible, not-grayed out) 3. Course full name test123 and Course short name test123 4. Enrol the administrator in this new course. Verify that the navigation block is rendered correctly Also try enrolling a student into the test123 coure, and verify that when logging in as a student the navigation block is built correctly.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-34250_M24

      Description

      Upgrade to 2.3.1 - (similar issue with 2.3 upgrade) - 'navshowmycoursecategories' set to 'Yes'

      Fatal error generated when logging on as a user enrolled on a course located within a 'hidden' top level category (ie category where parent = 0) - error also generated if user is enrolled on a course within a 'viewable' sub-catagory within a 'hidden' top-level category. No error generated if user enrolled on a course within a 'hidden' sub-category where the relevant top level category is 'viewable'.

      Debug error message:

      Coding error detected, it must be fixed by a programmer: PHP catchable fatal error 
      Debug info: Argument 2 passed to has_capability() must be an instance of context, boolean given, called in \lib\navigationlib.php on line 1134 and defined
       
      Error code: codingerror
       
      Stack trace: 
      line 397 of \lib\setuplib.php: coding_exception thrown
      line 355 of \lib\accesslib.php: call to default_error_handler()
      line 1134 of \lib\navigationlib.php: call to has_capability()
      line 3036 of \lib\navigationlib.php: call to global_navigation->initialise()
      line 778 of \lib\pagelib.php: call to navbar->has_items()
      line 4 of \theme\afterburner\layout\default.php: call to moodle_page->has_navbar()
      line 765 of \lib\outputrenderers.php: call to include()
      line 712 of \lib\outputrenderers.php: call to core_renderer->render_page_layout()
      line ? of unknownfile: call to core_renderer->header()
      line 1416 of \lib\setuplib.php: call to call_user_func_array()
      line 98 of \index.php: call to bootstrap_renderer->__call()
      line 98 of \index.php: call to bootstrap_renderer->header() 

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for reporting this.

            I've put that on the backlog.

            In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

            Show
            Michael de Raadt added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
            Hide
            Michael de Raadt added a comment -

            This has been duplicated a couple of times now, so I am bumping this issue.

            There is some valuable information in MDL-34324.

            Show
            Michael de Raadt added a comment - This has been duplicated a couple of times now, so I am bumping this issue. There is some valuable information in MDL-34324 .
            Hide
            Stephen Bourget added a comment -

            I think this may be easily fixed by simply changing the call to has_capability to check the $category->id in place of $category->parent. This will prevent the fatal error when the since the current category can never have a value of zero.

            I've attached a patch

            Show
            Stephen Bourget added a comment - I think this may be easily fixed by simply changing the call to has_capability to check the $category->id in place of $category->parent. This will prevent the fatal error when the since the current category can never have a value of zero. I've attached a patch
            Hide
            David Monllaó added a comment -

            Hi Stephen, your patch looks ok, it was checking the parent instead of the category it is adding, thanks

            Show
            David Monllaó added a comment - Hi Stephen, your patch looks ok, it was checking the parent instead of the category it is adding, thanks
            Hide
            Stephen Bourget added a comment -

            Can you send it for integration, since I don't seem to have access to do that.

            Show
            Stephen Bourget added a comment - Can you send it for integration, since I don't seem to have access to do that.
            Hide
            David Monllaó added a comment -

            Done!

            Show
            David Monllaó added a comment - Done!
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) added a comment -

            Uhm, I'm not sure if that is the correct solution for the problem.

            That piece of code is within one loop where it's decided if one hidden category is shown of no. And that check must be performed in parent category context for sure.

            Say you have:

            SYSTEM -> Cat A -> Cat B

            and Cat B is hidden. To decide if it is visible, you must check for the capability in Cat A, not it Cat B. And that's exactly what the code does.

            Obviously if fails for "root" cats (those not having parent), but the solution is not to check the permission in the cat itself, but at parent context (system in this case).

            So I'd propose something like:

            if (!$category->visible) {
                if ($category->parent == '0') {
                    $contexttocheck = context_system::instance();
                } else {
                    $contexttocheck = context_coursecat::instance($category->parent);
                }
                if (!has_capability('moodle/category:viewhiddencategories', $contexttocheck)) {
                    ...
                    ...
            

            Anyway, I'm not 100% sure, so I'm keeping this open for now and asking SamH for confirmation... ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Uhm, I'm not sure if that is the correct solution for the problem. That piece of code is within one loop where it's decided if one hidden category is shown of no. And that check must be performed in parent category context for sure. Say you have: SYSTEM -> Cat A -> Cat B and Cat B is hidden. To decide if it is visible, you must check for the capability in Cat A, not it Cat B. And that's exactly what the code does. Obviously if fails for "root" cats (those not having parent), but the solution is not to check the permission in the cat itself, but at parent context (system in this case). So I'd propose something like: if (!$category->visible) { if ($category->parent == '0') { $contexttocheck = context_system::instance(); } else { $contexttocheck = context_coursecat::instance($category->parent); } if (!has_capability('moodle/category:viewhiddencategories', $contexttocheck)) { ... ... Anyway, I'm not 100% sure, so I'm keeping this open for now and asking SamH for confirmation... ciao
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            After sharing my proposal with SamH, it seems that it is the correct one, performing cap checks at the correct context (always parent one), so I'm going to integrate that solution both for 23_STABLE and master.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited After sharing my proposal with SamH, it seems that it is the correct one, performing cap checks at the correct context (always parent one), so I'm going to integrate that solution both for 23_STABLE and master. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated (23 & master), thanks!

            Note it would be great if you can confirm this is fixing your problems. Links to the commits:

            Thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated (23 & master), thanks! Note it would be great if you can confirm this is fixing your problems. Links to the commits: 23_STABLE: https://github.com/stronk7/moodle/compare/MOODLE_23_STABLE...MDL-34250_23 master: https://github.com/stronk7/moodle/compare/master...MDL-34250 Thanks!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Ah Stephen, just realized I forgot to add credit to you in the commits I have integrated. Apologies!

            Show
            Eloy Lafuente (stronk7) added a comment - Ah Stephen, just realized I forgot to add credit to you in the commits I have integrated. Apologies!
            Hide
            Andrew Davis added a comment -

            Seems to be working ok. I'm going to record what I'm seeing. I have a hidden course category containing a visible course.

            As admin under my courses I can see a greyed out category containing the course I'm enrolled in.

            As student I have nothing under my courses. The visible course is within a hidden category and is thus hidden itself. I'm not 100% sure that this is correct however its not clearly wrong and I'm certainly not getting any error.

            Passing.

            Show
            Andrew Davis added a comment - Seems to be working ok. I'm going to record what I'm seeing. I have a hidden course category containing a visible course. As admin under my courses I can see a greyed out category containing the course I'm enrolled in. As student I have nothing under my courses. The visible course is within a hidden category and is thus hidden itself. I'm not 100% sure that this is correct however its not clearly wrong and I'm certainly not getting any error. Passing.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I'm so proud...of you, many thanks!

            http://youtu.be/n64CdfDRnZY

            Closing as fixed, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao

              People

              • Votes:
                4 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: