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

      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

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        Howard Miller added a comment -

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

        Show
        Howard Miller added a comment - Suggested fixes for 2.0, 2.1 and 2.2 added. Written by my colleague Derick Turner.
        Hide
        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
        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
        Rajesh Taneja added a comment -

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

        Show
        Rajesh Taneja added a comment - Thanks for the spot-on patch, Howard. Pushing it for peer-review.
        Hide
        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
        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
        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
        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
        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
        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
        Rajesh Taneja added a comment -

        Thanks Dan,

        I have rephrased commit message.

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

        marking as a minor security issue.

        Show
        Aparup Banerjee added a comment - marking as a minor security issue.
        Hide
        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
        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
        Rossiani Wijaya added a comment -

        This looks good.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This looks good. Test passed.
        Hide
        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
        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
        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
        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: