4. Go into a course in this site, go to the course settings, make sure module security is turned on there, and that the list of allowed modules is right, then Save the form.

5. Backup this course, and restore it into your Moodle 2.3 dev install. Verify that role overrides are created to prevent Teachers from adding the disallowed modules.

6. As for steps 4. and 5. but set things up on, and backup and restore from, a Moodle 1.9 site.

7. Upgrade the Moodle 2.2 site that you used in steps 4. and 5. to the latest master. Verify that the correct role overrides are created; and that the old course_allowed_modules table, and course.retrictmodules column and restrictmodulesfor, restrictbydefault and defaultallowedmodules config settings are dropped.)

We are mostly testing the new functionality in 2.3, but part of that is testing that data is upgraded and restored properly from 2.2.
1. With default settings, go to any course and ensure you can add any type of activity.
2. Change the role definition for the Teacher role, to remove mod/chat:addinstance, and make sure you cannot add a Chat in any course when logged in as a Teacher.
3. Using a Moodle 2.2 site, turn on the Module security feature, and set it to restrict modules for all courses, with a default list. (Using the old http://docs.moodle.org/22/en/Managing_activities#Module_security feature)
4. Go into a course in this site, go to the course settings, make sure module security is turned on there, and that the list of allowed modules is right, then Save the form.
5. Backup this course, and restore it into your Moodle 2.3 dev install. Verify that role overrides are created to prevent Teachers from adding the disallowed modules.
6. As for steps 4. and 5. but set things up on, and backup and restore from, a Moodle 1.9 site.
7. Upgrade the Moodle 2.2 site that you used in steps 4. and 5. to the latest master. Verify that the correct role overrides are created; and that the old course_allowed_modules table, and course.retrictmodules column and restrictmodulesfor, restrictbydefault and defaultallowedmodules config settings are dropped.)
8. Test a fresh install just to be sure

Description

This is kind of useless, as an empty whitelist is ambiguous. It could either mean "use the site default", or it could mean "i don't want to allow anything at all".

One way to get around this is a new setting (maybe course.restrictedmodules) which indicates that the course has overridden the site default.

Another way is to add a capability to each module 'canaddtocourse' or similar, and override it for a role at a course context. I think the latter is probably more correct in a 'moodle sense' but has the following drawbacks:

1. Third party module authors must implement this capability, for their module to implement this setting (stupid)
2. Eh, roles are complicated enough as it is, and Howard will kill me if we must override things further.

The former is probably simpler (and easier to implement) but means adding yet another field to course. Do courses have extra settings? Maybe we could have a course_preferences table or something.

'restrictmodulesfor' has also the following issue:
If you change it to 'none', the restricted flag on courses is not removed. This results in the modules in this course still being restricted and not even an administrator can change this since the corresponding setting is no longer available in the course settings.

Adrian Schlegel
added a comment - 08/May/09 10:45 PM 'restrictmodulesfor' has also the following issue:
If you change it to 'none', the restricted flag on courses is not removed. This results in the modules in this course still being restricted and not even an administrator can change this since the corresponding setting is no longer available in the course settings.

1. Make the 'Add an activity' code in Moodle do has_capability('mod/whatever:addinstance', $coursecontext) - if that capability exists.
2. Add that capability to all core modules.
3. Add debugging output to warn about modules that do not define this capability.
4. Remove the old code for the 'Module security' feature.

This work is planned to be done for Moodle 2.3.

Regarding 3. Where should this notice appear? Options are:
a. only during install/upgrade
b. on any course page when editing is on
c. something else.

Do we need to add an extra task:
3.5. Write upgrade code to convert the old module security settings into permission overrides?
I really hope not. I think it is sufficient to warn about these changes in the Moodle 2.3 release notes.

Tim Hunt
added a comment - 07/Mar/12 5:41 AM So, the current plan is to implement "proposal two" from http://docs.moodle.org/dev/Module_security_improvements#Proposal_two
The necessary work is:
1. Make the 'Add an activity' code in Moodle do has_capability('mod/whatever:addinstance', $coursecontext) - if that capability exists.
2. Add that capability to all core modules.
3. Add debugging output to warn about modules that do not define this capability.
4. Remove the old code for the 'Module security' feature.
This work is planned to be done for Moodle 2.3.
Regarding 3. Where should this notice appear? Options are:
a. only during install/upgrade
b. on any course page when editing is on
c. something else.
Do we need to add an extra task:
3.5. Write upgrade code to convert the old module security settings into permission overrides?
I really hope not. I think it is sufficient to warn about these changes in the Moodle 2.3 release notes.

Tim - it might be worth putting the code for 1. at the top of the 'course_allowed_module' in course/lib.php as that should cover all situations where there needs to be a check if a module can be added (at least that is what I've done when I was asked to implement a similar feature on a 1.9 and a 2.2 site yesterday).

For 3. I'd, personally, want to add a developer only debug message to the course page, to nag them into fixing it, but default to allowing the module to be added if the capability does not exist ( if (!get_capability_info($capabilityname)) ).

And as an aside, the suggestion in the forums to automatically add the capability to modules where it doesn't exist turns out to be a bad idea in terms of lang files - visiting the 'define roles' page produces a long list of developer warnings as the 'MODNAME:addinstance' strings don't exist in the modules concerned.

Davo Smith
added a comment - 07/Mar/12 4:21 PM Tim - it might be worth putting the code for 1. at the top of the 'course_allowed_module' in course/lib.php as that should cover all situations where there needs to be a check if a module can be added (at least that is what I've done when I was asked to implement a similar feature on a 1.9 and a 2.2 site yesterday).
For 3. I'd, personally, want to add a developer only debug message to the course page, to nag them into fixing it, but default to allowing the module to be added if the capability does not exist ( if (!get_capability_info($capabilityname)) ).
And as an aside, the suggestion in the forums to automatically add the capability to modules where it doesn't exist turns out to be a bad idea in terms of lang files - visiting the 'define roles' page produces a long list of developer warnings as the 'MODNAME:addinstance' strings don't exist in the modules concerned.

Tim Hunt
added a comment - 07/Mar/12 5:21 PM Thanks Davo for the tip. That saved me some time.
Is that really enough to cover all code paths I wonder? (e.g. backup/restore, web services etc. I guess it should be.)
I agree about maximum nagging. The most natural place to add the debug code is in course_allowed_module - I just need to check where that makes the debug message appear.
And I agree about not doing it automatically.

I think this is a working implementation. Can anyone peer review it for me please?

Note that course_allowed_module is used on all relevant code paths. (There are currently no web services that add an activity of any type, and Eloy confirmed that we should not be calling it from restore code.)

Tim Hunt
added a comment - 08/Mar/12 12:29 AM I think this is a working implementation. Can anyone peer review it for me please?
Note that course_allowed_module is used on all relevant code paths. (There are currently no web services that add an activity of any type, and Eloy confirmed that we should not be calling it from restore code.)
And, in the end, I did implement upgrade.

Tim Hunt
added a comment - 08/Mar/12 7:06 AM Docs required:
Add to release notes
In the 2.3 equivalent of http://docs.moodle.org/22/en/Managing_activities#Module_security , change it to say use the capabilities instead.
Document the new capabilities in the user docs.
In the developer docs for writing a new mod, mention the new capability.
QA tests required:
Remove any existing test cases for 'Module security'
Add a new test case, checking that turning off this capability for any module stops a teacher adding the module.

You are not checking a capabilities existence when calling assign_capability() to prevent access on upgrade/restore. As we are pretty certain there are going to be third-party modules without the addinstance capability have you checked if that has any impact? Does assign_capabilty just ignore it?

I think $string['imscp:addinstance'] should be 'Add a new IMS content package'

It might be nice to phpdoc $legacyrestrictmodules and $legacyallowedmodules in restore_course_structure_step

Dan Poltawski
added a comment - 08/Mar/12 12:34 PM Hi Tim,
Looks good! I can't really find fault as usual..
Some comments:
You are not checking a capabilities existence when calling assign_capability() to prevent access on upgrade/restore. As we are pretty certain there are going to be third-party modules without the addinstance capability have you checked if that has any impact? Does assign_capabilty just ignore it?
I think $string ['imscp:addinstance'] should be 'Add a new IMS content package'
It might be nice to phpdoc $legacyrestrictmodules and $legacyallowedmodules in restore_course_structure_step

Thanks for the quick review Dan. (Can I just say that when reviewing, it is better to use numbered lists, not bullet lists. That makes it easier to respond to individual points.)

2. and 3. are good changes. I will do them today.

1. is the key one. I know I am not checking that capabilities exist before calling assign_capability. There are two reasons for this:

a. During upgrade, system is done before mods, so actually at this point in the upgrade script, the capabilities don't exist in the DB yet!

b. Even if the upgrade order was different, consider this scenario: site with complex module security restrictions and custom mods is upgraded to 2.3; only then do they see the warnings about needing a new capability and add them to their mods. With the current code, the overrides that correspond to the old module security settings will already be there, and will start working. If we did not do it the way I have done, there would be dataloss.

It seems that sometimes there are advantages to not having referential integrity enforced

Tim Hunt
added a comment - 08/Mar/12 2:27 PM Thanks for the quick review Dan. (Can I just say that when reviewing, it is better to use numbered lists, not bullet lists. That makes it easier to respond to individual points.)
2. and 3. are good changes. I will do them today.
1. is the key one. I know I am not checking that capabilities exist before calling assign_capability. There are two reasons for this:
a. During upgrade, system is done before mods, so actually at this point in the upgrade script, the capabilities don't exist in the DB yet!
b. Even if the upgrade order was different, consider this scenario: site with complex module security restrictions and custom mods is upgraded to 2.3; only then do they see the warnings about needing a new capability and add them to their mods. With the current code, the overrides that correspond to the old module security settings will already be there, and will start working. If we did not do it the way I have done, there would be dataloss.
It seems that sometimes there are advantages to not having referential integrity enforced

Tim Hunt
added a comment - 10/Mar/12 4:05 AM I have rebased this.
Submitting for integration now.
To INEGRATORS: please be warned that this was a horrible rebase to have to do, because of the UNSIGNED chagnes last week. I hope everything has been sorted out OK. If not, please accept my apologies.

the main moodle.git repository has 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.

Eloy Lafuente (stronk7)
added a comment - 10/Mar/12 5:30 AM Some hours ago...
the main moodle.git repository has 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

The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

Eloy Lafuente (stronk7)
added a comment - 14/Mar/12 9:07 AM The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

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.

Eloy Lafuente (stronk7)
added a comment - 15/Mar/12 11:21 PM 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

Awesome effort, this has just been integrated now.
During my review I noted a handful of things and after integration added a commit to fix up two inline commets (typo, and relevance).
I also noted the following, while not blocking integration I would like to get your thoughts on them.

Lang string modulesecurity,admin appears to be no longer used. Can it be removed now.

Argument change to course_allowed_module should be mentioned somewhere

I am wondering what the effect of calling assign_capability is for a capability that doesn't exist. For instance when upgrading a site with restricted modules that have not yet defined the addinstance capability. Looking though code it appears the record will be created and will be an orphan. I suppose this is the desired effect in this case as eventually all modules should be updated (and thanks to the debugging notice will be)

Sam Hemelryk
added a comment - 19/Mar/12 5:20 AM Hi Tim,
Awesome effort, this has just been integrated now.
During my review I noted a handful of things and after integration added a commit to fix up two inline commets (typo, and relevance).
I also noted the following, while not blocking integration I would like to get your thoughts on them.
Lang string modulesecurity,admin appears to be no longer used. Can it be removed now.
Argument change to course_allowed_module should be mentioned somewhere
I am wondering what the effect of calling assign_capability is for a capability that doesn't exist. For instance when upgrading a site with restricted modules that have not yet defined the addinstance capability. Looking though code it appears the record will be created and will be an orphan. I suppose this is the desired effect in this case as eventually all modules should be updated (and thanks to the debugging notice will be)
Cheers
Sam

Sam Hemelryk
added a comment - 19/Mar/12 6:03 AM Made a very quick commit just now, there was a typo in upgrade.php (retrictmodules) ... oops!
Now fixed, anyone who has upgraded in the last hour will still have the coure.restrictmodules field.

1. Yes, those lang strings are now dead, and should be been removed. Do you want to create a new MDL issue?

2. I really don't think so. This API is only used in a few places in core code, and only one of them was using numerical arguments. I don't believe anyone outside core would be calling this API. Well, they might, but it is really unlikely, so I think it is over-the-top to add it to the release notes, etc.

3. You saw the above discussion.

4. Oops. That is the problem with our if (column_exists) logic in upgrade scripts. It hides errors like this. This one is odd. I am sure I tested the upgrade, including looking at the updated DB table definitions.

Tim Hunt
added a comment - 19/Mar/12 3:07 PM 1. Yes, those lang strings are now dead, and should be been removed. Do you want to create a new MDL issue?
2. I really don't think so. This API is only used in a few places in core code, and only one of them was using numerical arguments. I don't believe anyone outside core would be calling this API. Well, they might, but it is really unlikely, so I think it is over-the-top to add it to the release notes, etc.
3. You saw the above discussion.
4. Oops. That is the problem with our if (column_exists) logic in upgrade scripts. It hides errors like this. This one is odd. I am sure I tested the upgrade, including looking at the updated DB table definitions.
Thanks for sorting out the mess.

Next time it might be better to not use assign_capability(), get_roles_with_cap_in_context() and contexts in general in core upgrade (it is ok in plugins because roles must be fully upgraded there). The recommended way is to use low level SQL. The reason is if we change roles internals all this could break, hopefully we will not change rewrite accesslib anytime soon...

Petr Skoda
added a comment - 20/Mar/12 5:31 AM Next time it might be better to not use assign_capability(), get_roles_with_cap_in_context() and contexts in general in core upgrade (it is ok in plugins because roles must be fully upgraded there). The recommended way is to use low level SQL. The reason is if we change roles internals all this could break, hopefully we will not change rewrite accesslib anytime soon...

Tim Hunt
added a comment - 20/Mar/12 7:08 AM Bum! how many different people reviewed the code (especially me) and did not see that.
Thank you Petr and Sam.
And, good point about being careful with API functions.

Question with step 2 from testing instruction. Should it be done from 2.2 or 2.3?

If i'm not mistaken, mod/chat:addinstance only occurs in 2.3. The next question is adding database. could you be more specific on this? I'm pretty sure it is not the database activity correct? If it is database activity, then the test is failed because with mod/chat:addinstance disabled I still can add db activity.

Rossiani Wijaya
added a comment - 21/Mar/12 5:14 PM Hi Tim,
Question with step 2 from testing instruction. Should it be done from 2.2 or 2.3?
If i'm not mistaken, mod/chat:addinstance only occurs in 2.3. The next question is adding database. could you be more specific on this? I'm pretty sure it is not the database activity correct? If it is database activity, then the test is failed because with mod/chat:addinstance disabled I still can add db activity.

Tim Hunt
added a comment - 22/Mar/12 4:09 PM Does backup and restore from a 2.0 or 2.1 site work?
The problem going from 1.9 is most likely to be with the change of backup format between 1.9 and 2.0, not with my changes. If so, that is a separate bug.

Looking at moodle1_course_header_handler in backup/converter/moodle1/handlerlib.php it seems to hard-code 'restrictmodules' => 0.

So, that looks like a restore-from-1,9 bug to me. Not a bug with my new code. I suggest you file a new but for that "Module security settings lost when restoring from 1.9 -> 2.0". Steps 4. and 5. of the testing instructions show that restore works from 2.0/1/2 -> 2.3.

Tim Hunt
added a comment - 22/Mar/12 4:16 PM Looking at moodle1_course_header_handler in backup/converter/moodle1/handlerlib.php it seems to hard-code 'restrictmodules' => 0.
So, that looks like a restore-from-1,9 bug to me. Not a bug with my new code. I suggest you file a new but for that "Module security settings lost when restoring from 1.9 -> 2.0". Steps 4. and 5. of the testing instructions show that restore works from 2.0/1/2 -> 2.3.

Rossiani Wijaya
added a comment - 22/Mar/12 5:34 PM Hi Tim,
Thank for the feedback. I spoke with Eloy and suggested to fix the restore issue on new issue.
The new issue to address the restore issue is available at: MDL-32158 .
I will pass this issue once it back to testing.

Sam Hemelryk
added a comment - 23/Mar/12 9:27 AM Congratulations are in order, you've made it, or at least your code has!
It's now part of Moodle and both the git and cvs repositories have been updated.
This issue is being marked as fixed and closed.
Thank you.

Helen Foster
added a comment - 17/Apr/12 5:14 PM Just noting that on http://qa.moodle.net the roles of manager and teacher needed resetting to defaults in order for the add a resource and add an activity drop down menus to appear.

Tim Hunt
added a comment - 17/Apr/12 5:34 PM I am worried about this. The definitions of the new capabilities looks like
'mod/assignment:addinstance' => array(
'riskbitmask' => RISK_XSS,
'captype' => 'write',
'contextlevel' => CONTEXT_COURSE,
'archetypes' => array(
'editingteacher' => CAP_ALLOW,
'manager' => CAP_ALLOW
),
'clonepermissionsfrom' => 'moodle/course:manageactivities'
),
I thought that meant that on upgrade, the mod/assignment:addinstance capability would be set to the same thing as the moodle/course:manageactivities capability, for all roles.
However, we observed the same problem when upgrading the OU's site. No roles got the :addinstance capabilities, and we had to edit the roles by hand.
I am afraid that I don't have time to investigate this now, but it would be really good if someone could.