Moodle

Not 100% sure if these are related but very similar background.

Comment found in the restore code:

/**

• if user has manageactivities in any course and we are not restoring from within SITEID
• existingcoursedeleting
• existingcourseadding
• else we show
• currentcoursedeleting
• currentcourse
• if user has course:create in any category, we show
• newcourse
  */

This seems insane. This restriction does seem deliberate. Why?

Found another comment.... have to say I'm really annoyed by this.....

There must have been a million better solutions that half crippling restore and not telling anybody. Grrrrrr.....

// NOTE: SITEID is prevented as a source for restoring because it's public area and we must encourage admins
// to avoid using it as a "repository" for courses storage.

I have to agree with Howard that restricting administrators in this way is at best baffling i.e. moodle/site:doanything.

My vote to make restore permissions consistent in all files areas.

+100 to fix the behaviour to the original behaviour and allow backups to be restored normally from site files.

The same argument for backups applies to ANY sensitive information. eg a file of grades or an assignment solution or your credit card records.

We can encourage users to avoid using it as a repository for course storage by:

• improving the warning at the top of the page to make it explicit about backups and the dangers of leaving them there
• adding a check to the security report for .zip files in the site files area, aksing admins to make sure they are not leaving course backups there

Thanks Martin!

I really needed your opinion to finally unbalance this to one of the alternatives! I will do as you've suggested. So I'm closing MDL-19163 (the original problem addressed there is fixed) and will continue here with the plan.

Ciao

How about the following for an updated message:

Note: This area is not secure

Files placed here can be accessed by anyone who knows (or can guess) the path to the file. You should not use this area to store files containing sensitive or valuable data (including course backup files).

I've added a note about our recommendation that backup files are deleted immediately on the following documentation pages:

http://docs.moodle.org/en/Site_files
http://docs.moodle.org/en/Administration_FAQ
http://docs.moodle.org/en/Restore

Ray, thanks for suggesting an updated message.

My suggestion, just rewording yours a little, is as follows:

Note: Files placed here can be accessed by anyone who knows (or can guess) the URL. For security reasons, it is recommended that any backup files are deleted immediately after restoring them.

Yup, looks good.

A simple and elegant solution

+1 too for that string. Also Docs are perfect.

I'll be working on this later today. Thanks!

Hi, just to confirm before start implementing it:

1) if the user can restore to current course, grant the "current" options (deleting and adding)
2) if user can create courses anywhere, grant the "new course" option
3) if the user can restore to 2 or more courses (or to just 1 but different from current), grant the "existing" options (deleting and adding)

No distinction at all between SITE and normal courses at all. As requested. Correct?

It's going to be a bit harder, because the "current" and the "existing" options were being considered mutually exclusive until now by restore internals but I think I can split them successfully.

Ciao

Adding patch that:

1. introduces new (5) constants to define each "restoreto" (destination) option, avoiding old mutual exclusion between "current" and "existing", so both can be showed at the same time.
2. apply them to the whole restore code
3. disable any restriction when calculating "restoreto" based on SITEID
4. fulfil the rules in previous comment:
   • if the user can restore to current course, grant the "current" options
   • if user can create any course, grant the "new course" option
   • if the user can restore to 2 or more courses (or to just 1 but different from current), grant the "existing" options

It doesn't includes the "category bug" fix (MDL-15355) at all, to avoid mixing things.

I've tested here as admin, as teacher, in site, in normal courses, without courses, with 1 course and with > 1 course and everything seems to be in place.

Just leaving it here 1 day, in case anyone wants to test it before commit. Then I'll commit it.

Ciao

I've done some more tests and haven't found any problematic combination at all. Patch committed to 19_STABLE, so is available in CVS / next weekly.

Leaving this open until I merge changes to HEAD, gtg NOW!

Please, test and feedback wil be welcome if anything isn't working as expected. Ciao

Perfect! Thanks a lot Eloy

I'm going to test things on Tuesday as part of our http://docs.moodle.org/en/Development:Weekly_Code_Review

Just noting my suggested lang string change:

Replace publicsitefileswarning with publicsitefileswarning2:

Note: Files placed here can be accessed by anyone who knows (or can guess) the URL. For security reasons, it is recommended that any backup files are deleted immediately after restoring them.

Resolving as fixed. String is now in 1.9 and HEAD, only used by 1.9.

Thanks everybody and ciao

PS: if you detect any inconsistency, feel free to reopen/feedback here.

Created improvement issue for security overview report as suggested by Martin D.

Thanks to Howard for your report, thanks for everyone's comments, and thanks to Eloy for fixing it.

Closing now, as everything I can think of testing is working perfectly

And thanks to you Helen!