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

module security broken for new courses

    Details

    • Testing Instructions:
      Hide
      1. Go to Site administration > Security > Module security
      2. Switch 'Restrict modules for..' to 'all course'
      3. Tick 'Restrict modules by default'
      4. Select a few modules in the list and save
      5. Navigate to Site admin > Courses > Add/edit courses
      6. Click "Add a new course"
      7. Scroll down to the "Restrict activity modules?" (why does this end in an ?) section

      Expected result: The restricted modules list should appear to have the same set of modules set as the global setting.

      Show
      Go to Site administration > Security > Module security Switch 'Restrict modules for..' to 'all course' Tick 'Restrict modules by default' Select a few modules in the list and save Navigate to Site admin > Courses > Add/edit courses Click "Add a new course" Scroll down to the "Restrict activity modules?" (why does this end in an ?) section Expected result: The restricted modules list should appear to have the same set of modules set as the global setting.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE

      Description

      To reproduce

      • Got to Site administration > Security > Module security
      • Switch on 'Restrict modules for..' to 'all course'
      • Tick 'Restrict modules by default'
      • Select a few modules in the list and save
      • Create a new course
      • The restricted modules list appears in the course settings and is enabled but no modules are selected. The requested ones (in the admin page) should be selected.

      I confirmed this in 2.2.3+ (Build: 20120519) and in the latest 2.0 branch. Not tried in 2.1 or master

        Gliffy Diagrams

          Activity

          Hide
          poltawski Dan Poltawski added a comment -

          This feature has been changed in 2.3 to use an entirely permissions based approach, partly I think because its broken like this.

          See:
          http://docs.moodle.org/dev/Module_security_improvements
          http://tracker.moodle.org/browse/MDL-19125
          http://moodle.org/mod/forum/discuss.php?d=196939

          Show
          poltawski Dan Poltawski added a comment - This feature has been changed in 2.3 to use an entirely permissions based approach, partly I think because its broken like this. See: http://docs.moodle.org/dev/Module_security_improvements http://tracker.moodle.org/browse/MDL-19125 http://moodle.org/mod/forum/discuss.php?d=196939
          Hide
          salvetore Michael de Raadt added a comment -

          Hi, Howard.

          I tried this in Moodle 2.2.3+ (Build: 20120519) and it did carry over the selected items.

          The setting was also affective in the course page when I logged in as a teacher (rather than an admin).

          Could there be some step I'm missing?

          Show
          salvetore Michael de Raadt added a comment - Hi, Howard. I tried this in Moodle 2.2.3+ (Build: 20120519) and it did carry over the selected items. The setting was also affective in the course page when I logged in as a teacher (rather than an admin). Could there be some step I'm missing?
          Hide
          howardsmiller Howard Miller added a comment -

          Sorry... I should have been clearer.

          • Create the course as admin, as above
          • Log in as teacher *Resist the temptation to go to the Settings page*
          • Turn editing on and try to add new activities.
          • You CAN add the restricted ones.

          The bug is that the restriction gets added when someone visits the settings page of an existing course. The restriction is not applied to new courses.

          Show
          howardsmiller Howard Miller added a comment - Sorry... I should have been clearer. Create the course as admin, as above Log in as teacher * Resist the temptation to go to the Settings page * Turn editing on and try to add new activities. You CAN add the restricted ones. The bug is that the restriction gets added when someone visits the settings page of an existing course. The restriction is not applied to new courses.
          Hide
          howardsmiller Howard Miller added a comment -

          Suggested fixes for 2.0, 2.1 and 2.2 added. Written by my colleague Derick Turner.

          Show
          howardsmiller Howard Miller added a comment - Suggested fixes for 2.0, 2.1 and 2.2 added. Written by my colleague Derick Turner.
          Hide
          salvetore Michael de Raadt added a comment -

          Hi, Howard.

          I now see what you meant about the list of restrictions not matching the site-wide setting as I was creating the course. That should definitely be fixed and your patch looks to be targeting that.

          However, no matter how I tried, when I logged in as a teacher, even before visiting the settings page, I was still restricted to the global list modules.

          Show
          salvetore Michael de Raadt added a comment - Hi, Howard. I now see what you meant about the list of restrictions not matching the site-wide setting as I was creating the course. That should definitely be fixed and your patch looks to be targeting that. However, no matter how I tried, when I logged in as a teacher, even before visiting the settings page, I was still restricted to the global list modules.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for the spot-on patch, Howard.
          Pushing it for peer-review.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for the spot-on patch, Howard. Pushing it for peer-review.
          Hide
          fred Frédéric Massart added a comment -

          Hi Howard, Raj,

          The patch looks good to me and can go straight to integration. I just noticed the commit message which is not following our standards, up to you!

          Cheers!

          Show
          fred Frédéric Massart added a comment - Hi Howard, Raj, The patch looks good to me and can go straight to integration. I just noticed the commit message which is not following our standards, up to you! Cheers!
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks Fred,

          I have seen integrators ignoring commit msg for provided patches, so hope this will be fine.

          Pushing it for integration review.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Fred, I have seen integrators ignoring commit msg for provided patches, so hope this will be fine. Pushing it for integration review.
          Hide
          poltawski Dan Poltawski added a comment -

          We need a bug number in the commit message.

          We may have missed a bad commit message, but certainly not ignored it.

          Show
          poltawski Dan Poltawski added a comment - We need a bug number in the commit message. We may have missed a bad commit message, but certainly not ignored it.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks Dan,

          I have rephrased commit message.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Dan, I have rephrased commit message.
          Hide
          nebgor Aparup Banerjee added a comment -

          marking as a minor security issue.

          Show
          nebgor Aparup Banerjee added a comment - marking as a minor security issue.
          Hide
          nebgor Aparup Banerjee added a comment -

          Thanks for the patch. its straight forward and fixes the issue for me.

          Integrated into 21 and 22 branches.up for more testing.

          ps: two if's in sequence are actually quiet readable

          Show
          nebgor Aparup Banerjee added a comment - Thanks for the patch. its straight forward and fixes the issue for me. Integrated into 21 and 22 branches.up for more testing. ps: two if's in sequence are actually quiet readable
          Hide
          rwijaya Rossiani Wijaya added a comment -

          This looks good.

          Test passed.

          Show
          rwijaya Rossiani Wijaya added a comment - This looks good. Test passed.
          Hide
          nebgor Aparup Banerjee added a comment -

          yay, it works!

          This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

          Thank you all for taking the time to get us here.

          cheers!

          Show
          nebgor Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!
          Hide
          salvetore Michael de Raadt added a comment -

          I'm removing the security level on this issue. I don't believe there is any real risk or opportunity from the problem resolved here. In relation to other security issues, it not reportable as a security issue.

          Show
          salvetore Michael de Raadt added a comment - I'm removing the security level on this issue. I don't believe there is any real risk or opportunity from the problem resolved here. In relation to other security issues, it not reportable as a security issue.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Sep/12