Moodle

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
  • Pull from Repository:

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

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Aparup Banerjee added a comment -

marking as a minor security issue.

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

This looks good.

Test passed.

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

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: