Moodle
  1. Moodle
  2. MDL-36369

admin/settings/plugins.php undefined variable

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide

      1. To test this you need to install some third-party qtypes. Ideally install https://github.com/moodleou/moodle-qtype_pmatch (which uses an admin externalpage) and https://github.com/maths/moodle-qtype_stack (which uses a normal settings page). (You just need to install the STACK plugins. You don't need to install Maxima.)

      2. Log in as admin.

      3. Verify that you can see / access / use the manange qtypes and manage qbehaviours pages.

      4. Verify that you can see / access / use the settings page for qtype STACK and qtype pmatch.

      5. Log in as a manager, so you have moodle/question:config but but not moodle/site:config

      6. Verify that you CANNOT see / access / use the manange qtypes and manage qbehaviours pages.

      7. Verify that you still can see / access / use the settings page for qtype STACK and qtype pmatch.

      Show
      1. To test this you need to install some third-party qtypes. Ideally install https://github.com/moodleou/moodle-qtype_pmatch (which uses an admin externalpage) and https://github.com/maths/moodle-qtype_stack (which uses a normal settings page). (You just need to install the STACK plugins. You don't need to install Maxima.) 2. Log in as admin. 3. Verify that you can see / access / use the manange qtypes and manage qbehaviours pages. 4. Verify that you can see / access / use the settings page for qtype STACK and qtype pmatch. 5. Log in as a manager, so you have moodle/question:config but but not moodle/site:config 6. Verify that you CANNOT see / access / use the manange qtypes and manage qbehaviours pages. 7. Verify that you still can see / access / use the settings page for qtype STACK and qtype pmatch.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      45178

      Description

      Bug introduced by MDL-35661

      Notice: Undefined variable: allplugins in /html/admin/settings/plugins.php on line 304
      
      Warning: Invalid argument supplied for foreach() in /html/admin/settings/plugins.php on line 304
      

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Thanks Marina, i've integrated this now

          Show
          Dan Poltawski added a comment - Thanks Marina, i've integrated this now
          Hide
          Rajesh Taneja added a comment - - edited

          Sorry Marina,

          I am not sure how a user can get to question types settings menu if she doesn't have site:config capability.

          Steps I followed:

          1. I assigned user as manager who has question:config capability but not site:config.
          2. Checked settings navigation
          3. plugins node is not visible, hence can't get to question types settings node (which is under plugins.)

          Let me know if I am missing something. Failing for now, to get attention.

          Show
          Rajesh Taneja added a comment - - edited Sorry Marina, I am not sure how a user can get to question types settings menu if she doesn't have site:config capability. Steps I followed: I assigned user as manager who has question:config capability but not site:config. Checked settings navigation plugins node is not visible, hence can't get to question types settings node (which is under plugins.) Let me know if I am missing something. Failing for now, to get attention.
          Hide
          Marina Glancy added a comment -

          Tim, added you as a watcher.

          Hi Raj,
          I noticed that error appearing on qa.moodle.net if you log in as manager. I just saw the line in code that produces it and corrected it.

          But what you say is actually a very good point - the question types/behaviours navigation branch is unavailable anyway, because it is added to 'Plugins' branch which does not exist if user does not have site:config capability.

          So this seems to be completely different bug - question types branch should only be loaded if user has site:config capability. So basically the fix should be:

          --- a/admin/settings/plugins.php
          +++ b/admin/settings/plugins.php
          @@ -293,7 +293,7 @@ if ($hassiteconfig) {
           }
           
           // Question type settings
          -if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) {
          +if ($hassiteconfig && has_capability('moodle/question:config', $systemcontext)) {
               // Question behaviour settings.
               $ADMIN->add('modules', new admin_category('qbehavioursettings', new lang_string('questionbehaviours', 'admin')));
               $ADMIN->add('qbehavioursettings', new admin_page_manageqbehaviours());
          

          Which if you think raises the question - why do we need a moodle/question:config capability at all? How is it possible that user has site:config capability but does not have moodle/question:config ? (This is a question to Tim)

          So I pushed a completely different fix for this issue. Up to integrators if they want to re-integrate it now or in the next round. This fix just removes a notice displayed to user.

          Show
          Marina Glancy added a comment - Tim, added you as a watcher. Hi Raj, I noticed that error appearing on qa.moodle.net if you log in as manager. I just saw the line in code that produces it and corrected it. But what you say is actually a very good point - the question types/behaviours navigation branch is unavailable anyway, because it is added to 'Plugins' branch which does not exist if user does not have site:config capability. So this seems to be completely different bug - question types branch should only be loaded if user has site:config capability. So basically the fix should be: --- a/admin/settings/plugins.php +++ b/admin/settings/plugins.php @@ -293,7 +293,7 @@ if ($hassiteconfig) { } // Question type settings - if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) { + if ($hassiteconfig && has_capability('moodle/question:config', $systemcontext)) { // Question behaviour settings. $ADMIN->add('modules', new admin_category('qbehavioursettings', new lang_string('questionbehaviours', 'admin'))); $ADMIN->add('qbehavioursettings', new admin_page_manageqbehaviours()); Which if you think raises the question - why do we need a moodle/question:config capability at all? How is it possible that user has site:config capability but does not have moodle/question:config ? (This is a question to Tim) So I pushed a completely different fix for this issue. Up to integrators if they want to re-integrate it now or in the next round. This fix just removes a notice displayed to user.
          Hide
          Rajesh Taneja added a comment -

          Thanks Marina,

          Can you please let me know how you got to see question type settings as manager. I just logged in on QA as manager and can't see plugin node under site administration.
          Not sure what I am missing here.

          Show
          Rajesh Taneja added a comment - Thanks Marina, Can you please let me know how you got to see question type settings as manager. I just logged in on QA as manager and can't see plugin node under site administration. Not sure what I am missing here.
          Hide
          Tim Hunt added a comment -

          The use-case for moodle/question:config is that you may have staff who need to be able to configure questions, but who should not be trusted with moodle/site:config.

          The OU does have staff like this. Therefore ...

          -1 for the (most recent) proposed fix.

          The plugins category should be added to the admin tree if any of its child nodes are going to be there.

          Show
          Tim Hunt added a comment - The use-case for moodle/question:config is that you may have staff who need to be able to configure questions, but who should not be trusted with moodle/site:config. The OU does have staff like this. Therefore ... -1 for the (most recent) proposed fix. The plugins category should be added to the admin tree if any of its child nodes are going to be there.
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim for explaining the issue.

          As per conversation on Dev chat, user with question:config and not site:config capability should:

          1. See link in "site administration"
          2. Able to access "manage question page" (which is currently not possible).

          Will create another issue, blocking this one as this can't be tested without user having access to "Manage question type" page.

          Show
          Rajesh Taneja added a comment - Thanks Tim for explaining the issue. As per conversation on Dev chat, user with question:config and not site:config capability should: See link in "site administration" Able to access "manage question page" (which is currently not possible). Will create another issue, blocking this one as this can't be tested without user having access to "Manage question type" page.
          Hide
          Dan Poltawski added a comment -

          I'm going to revert this, because its not clear to me that this simple fix is actually caused by the underlying permissions tree issue.

          Show
          Dan Poltawski added a comment - I'm going to revert this, because its not clear to me that this simple fix is actually caused by the underlying permissions tree issue.
          Hide
          Dan Poltawski added a comment -

          OK reverted and reopening.

          Show
          Dan Poltawski added a comment - OK reverted and reopening.
          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
          Tim Hunt added a comment -

          I am looking at the code inside moodle.git/master. In admin/settings/plugins.php

          So, the bug is that the code

              require_once("$CFG->libdir/pluginlib.php");
              $allplugins = plugin_manager::instance()->get_plugins();
          

          is inside

          if ($hassiteconfig) {
          

          (which is good for performance for non-admins) but $allplugins is used inside both

          if ($hassiteconfig) {
          if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) {
          

          Ah! Note that the code relating to local plugins has already been changed to allow for this. See the comment:

          // extend settings for each local plugin. Note that their settings may be in any part of the
          // settings tree and may be visible not only for administrators. We can not use $allplugins here
          

          So, there are two possible fixes:

          1. Move

              require_once("$CFG->libdir/pluginlib.php");
              $allplugins = plugin_manager::instance()->get_plugins();
          

          inside an

          if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) {
          

          check.

          2. Change the qtype code to use get_plugin_list('qtype'), instead of using $allplugins there.

          I think that 1. is better. Do you want to make the fix, or shall I?

          Show
          Tim Hunt added a comment - I am looking at the code inside moodle.git/master. In admin/settings/plugins.php So, the bug is that the code require_once( "$CFG->libdir/pluginlib.php" ); $allplugins = plugin_manager::instance()->get_plugins(); is inside if ($hassiteconfig) { (which is good for performance for non-admins) but $allplugins is used inside both if ($hassiteconfig) { if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) { Ah! Note that the code relating to local plugins has already been changed to allow for this. See the comment: // extend settings for each local plugin. Note that their settings may be in any part of the // settings tree and may be visible not only for administrators. We can not use $allplugins here So, there are two possible fixes: 1. Move require_once( "$CFG->libdir/pluginlib.php" ); $allplugins = plugin_manager::instance()->get_plugins(); inside an if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) { check. 2. Change the qtype code to use get_plugin_list('qtype'), instead of using $allplugins there. I think that 1. is better. Do you want to make the fix, or shall I?
          Hide
          Marina Glancy added a comment - - edited

          Hi Tim,
          this was exactly what I did in my first version of fix. But Raj noticed that Question types and behaviours branches are not available anyway if user does not have site:config capability.
          The whole 'Plugins' branch ('modules') does not appear for user with manager role and I can't find why.

          Also if I try (as manager) to access http://localhost/admin/qtypes.php directly, I get:

          Access denied
          
          More information about this error
          Debug info:
          Error code: accessdenied
          Stack trace:
          
              line 467 of /lib/setuplib.php: moodle_exception thrown
              line 5995 of /lib/adminlib.php: call to print_error()
              line 40 of /admin/qtypes.php: call to admin_externalpage_setup()
          
          

          I don't know how it works for you in OU Tim.

          Show
          Marina Glancy added a comment - - edited Hi Tim, this was exactly what I did in my first version of fix. But Raj noticed that Question types and behaviours branches are not available anyway if user does not have site:config capability. The whole 'Plugins' branch ('modules') does not appear for user with manager role and I can't find why. Also if I try (as manager) to access http://localhost/admin/qtypes.php directly, I get: Access denied More information about this error Debug info: Error code: accessdenied Stack trace: line 467 of /lib/setuplib.php: moodle_exception thrown line 5995 of /lib/adminlib.php: call to print_error() line 40 of /admin/qtypes.php: call to admin_externalpage_setup() I don't know how it works for you in OU Tim.
          Hide
          Marina Glancy added a comment -

          anyway, I'm actually on holidays. Not sure if I get online before Wednesday 14.11
          Feel free to take this issue from me. My previous version of fix:

          diff --git a/admin/settings/plugins.php b/admin/settings/plugins.php
          index 29d0665..3a56564 100644
          --- a/admin/settings/plugins.php
          +++ b/admin/settings/plugins.php
          @@ -294,6 +294,10 @@ if ($hassiteconfig) {
           
           // Question type settings
           if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) {
          +    if (!$hassiteconfig) {
          +        require_once("$CFG->libdir/pluginlib.php");
          +        $allplugins = plugin_manager::instance()->get_plugins();
          +    }
               // Question behaviour settings.
               $ADMIN->add('modules', new admin_category('qbehavioursettings', new lang_string('questionbehaviours', 'admin')));
               $ADMIN->add('qbehavioursettings', new admin_page_manageqbehaviours());
          
          Show
          Marina Glancy added a comment - anyway, I'm actually on holidays. Not sure if I get online before Wednesday 14.11 Feel free to take this issue from me. My previous version of fix: diff --git a/admin/settings/plugins.php b/admin/settings/plugins.php index 29d0665..3a56564 100644 --- a/admin/settings/plugins.php +++ b/admin/settings/plugins.php @@ -294,6 +294,10 @@ if ($hassiteconfig) { // Question type settings if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) { + if (!$hassiteconfig) { + require_once( "$CFG->libdir/pluginlib.php" ); + $allplugins = plugin_manager::instance()->get_plugins(); + } // Question behaviour settings. $ADMIN->add('modules', new admin_category('qbehavioursettings', new lang_string('questionbehaviours', 'admin'))); $ADMIN->add('qbehavioursettings', new admin_page_manageqbehaviours());
          Hide
          Tim Hunt added a comment -

          Ah, I think it is correct that admin_page_manageqtypes requires moodle/site:config. To reproduce this problem, you need a question type that has configuration options (none of the core ones do, yet). So, for example, install qtype_pmatch.

          Then, manager should be able to see Plugins -> question types -> Pmatch, but they cannot.

          The problem is with plugininfo_qtype::load_settings. That is incorrectly doing if ($hassiteconfig). That was introduced by MDL-35661.

          Show
          Tim Hunt added a comment - Ah, I think it is correct that admin_page_manageqtypes requires moodle/site:config. To reproduce this problem, you need a question type that has configuration options (none of the core ones do, yet). So, for example, install qtype_pmatch. Then, manager should be able to see Plugins -> question types -> Pmatch, but they cannot. The problem is with plugininfo_qtype::load_settings. That is incorrectly doing if ($hassiteconfig). That was introduced by MDL-35661 .
          Hide
          Marina Glancy added a comment -

          Submitting for integration.

          It's easier to fix it than argue with Tim.
          See github diff

          Show
          Marina Glancy added a comment - Submitting for integration. It's easier to fix it than argue with Tim. See github diff
          Hide
          Marina Glancy added a comment -

          Sorry Tim, did not see your comment before I submitted. Please feel free to take this issue from me or create a new one if you think that capability should be changed.

          Show
          Marina Glancy added a comment - Sorry Tim, did not see your comment before I submitted. Please feel free to take this issue from me or create a new one if you think that capability should be changed.
          Hide
          Tim Hunt added a comment -

          Yes. I'll take this and finish it off. Dear integrator, please could you reopen this? Thanks.

          Show
          Tim Hunt added a comment - Yes. I'll take this and finish it off. Dear integrator, please could you reopen this? Thanks.
          Hide
          Dan Poltawski added a comment -

          Done

          Show
          Dan Poltawski added a comment - Done
          Hide
          Tim Hunt added a comment -

          I think this is it. It works for me, for both admin and manager.

          Show
          Tim Hunt added a comment - I think this is it. It works for me, for both admin and manager.
          Hide
          Dan Poltawski added a comment -

          Thanks Tim & Marina.

          I've integrated this now.

          Show
          Dan Poltawski added a comment - Thanks Tim & Marina. I've integrated this now.
          Hide
          David Monllaó added a comment -

          It passes, I've replicated the problem without the patch and it works as expected with the patch. Tested with default manager role capabilities

          Show
          David Monllaó added a comment - It passes, I've replicated the problem without the patch and it works as expected with the patch. Tested with default manager role capabilities
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many, many thanks for your effort!

          Millions of people will enjoy the results of your work, yay!

          Closing as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many, many thanks for your effort! Millions of people will enjoy the results of your work, yay! Closing as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: