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
    • Rank:
      42599

      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() 
      

        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: