Moodle

createdby and modifiedby assigned to wrong people when course restored to another site

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.9.2
  • Fix Version/s: 1.9.4
  • Component/s: Backup, Questions
  • Labels:
    None
  • Database:
    Any
  • Affected Branches:
    MOODLE_19_STABLE
  • Fixed Branches:
    MOODLE_19_STABLE

Description

If I make a backup of a course and then restore that backup into another moodle site, the questions get assigned to a wrong person. Backup file (moodle.xml) includes the following info for each question:
<CREATEDBY>xyz</CREATEDBY>
<MODIFIEDBY>xyz</MODIFIEDBY>
where "xyz" is the id number of the user who created or modified the question. The problem is, if you restore the backup to another moodle site, user xyz is a totally different person

I suggest that if a backup is restored to another site (it can easily be detected during the restore process by comparing <ORIGINAL_WWWROOT> from the moodle xml file to the current www root), both values (CREATEDBY and MODIFIEDBY) should be set to 0 (unknown) or 1 (admin user)

Issue Links

This issue is duplicated by:
MDL-17636 Investigate violations of questions.modifiedby/questions.createdby Minor Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Przemyslaw Stencel added a comment -

Of course I meant "... or 2 (admin user)"

Show
Przemyslaw Stencel added a comment - Of course I meant "... or 2 (admin user)"
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Assigning to Tim Hunt... thanks!

Show
Eloy Lafuente (stronk7) added a comment - Assigning to Tim Hunt... thanks!
Hide
Permalink
Przemyslaw Stencel added a comment -

Tim, I see you've scheduled this issue to be fixed in 1.9.4. How are you planning to implement this? Will CREATEDBY and MODIFIEDBY be set to unknown or to admin? Or are you planning a different fix?

Best,
Przemek

Show
Przemyslaw Stencel added a comment - Tim, I see you've scheduled this issue to be fixed in 1.9.4. How are you planning to implement this? Will CREATEDBY and MODIFIEDBY be set to unknown or to admin? Or are you planning a different fix? Best, Przemek
Hide
Permalink
Tim Hunt added a comment -

I am only planning to fix what happens for future restores.

I don't think there is anything I can do about bad data already in the database. Well, if createdby or modifiedby point to a non-existent user, then I will reset it to NULL, but where they point to an actual user, there is no way for Moodle to know if that is the correct information of the result of a bad restore in the past.

As admin, you could get a list of question_category ids that came from bad backups, and then manually do a database query to fix them. Something like:

UPDATE mdl_question SET createdby = NULL, modifiedby = NULL WHERE category IN (123, 234, 345, ...);

using a tool like phpMyAdmin. However, messing around directly in the DB is not something to be undertaken lightly.

Show
Tim Hunt added a comment - I am only planning to fix what happens for future restores. I don't think there is anything I can do about bad data already in the database. Well, if createdby or modifiedby point to a non-existent user, then I will reset it to NULL, but where they point to an actual user, there is no way for Moodle to know if that is the correct information of the result of a bad restore in the past. As admin, you could get a list of question_category ids that came from bad backups, and then manually do a database query to fix them. Something like: UPDATE mdl_question SET createdby = NULL, modifiedby = NULL WHERE category IN (123, 234, 345, ...); using a tool like phpMyAdmin. However, messing around directly in the DB is not something to be undertaken lightly.
Hide
Permalink
Tim Hunt added a comment -

I'm looking for comments on this patch, particularly the advisability of the

} else if ($restore->original_wwwroot == $CFG->wwwroot) {

lines. Thanks.

Show
Tim Hunt added a comment - I'm looking for comments on this patch, particularly the advisability of the } else if ($restore->original_wwwroot == $CFG->wwwroot) { lines. Thanks.
Hide
Permalink
Przemyslaw Stencel added a comment -

> I am only planning to fix what happens for future restores.

Yes, I understand that My question was about future restores.

I just wanted to know which of the following you are planning to implement as a fix for this issue:

1/ if during the restore process the original wwwroot is different than the wwwroot of the target site, then CREATEDBY and MODIFIEDBY will be set to unknown

OR

2/ if during the restore process the original wwwroot is different than the wwwroot of the target site, then CREATEDBY and MODIFIEDBY will be set to admin

Show
Przemyslaw Stencel added a comment - > I am only planning to fix what happens for future restores. Yes, I understand that My question was about future restores. I just wanted to know which of the following you are planning to implement as a fix for this issue: 1/ if during the restore process the original wwwroot is different than the wwwroot of the target site, then CREATEDBY and MODIFIEDBY will be set to unknown OR 2/ if during the restore process the original wwwroot is different than the wwwroot of the target site, then CREATEDBY and MODIFIEDBY will be set to admin
Hide
Permalink
Tim Hunt added a comment -

Neither, read the patch.

If the backup includes users, then we will update the fields to point to the new userid of the user who created/modified the question. If the user is not in the file, we set it to null, unless $restore->original_wwwroot == $CFG->wwwroot.

Show
Tim Hunt added a comment - Neither, read the patch. If the backup includes users, then we will update the fields to point to the new userid of the user who created/modified the question. If the user is not in the file, we set it to null, unless $restore->original_wwwroot == $CFG->wwwroot.
Hide
Permalink
Przemyslaw Stencel added a comment -

Thanks for the explanation.

Show
Przemyslaw Stencel added a comment - Thanks for the explanation.
Hide
Permalink
Tim Hunt added a comment -

Eloy, please could you review my patch. It is only a small one and I would like to get this checked in. Thanks.

Show
Tim Hunt added a comment - Eloy, please could you review my patch. It is only a small one and I would like to get this checked in. Thanks.
Hide
Permalink
Tim Hunt added a comment -

Right, following discussion with Eloy and Nico in developer chat:

The if ($restore->original_wwwroot == $CFG->wwwroot) is dangerous because of this scenario:

1. Admin backs up everything important in their Moodle 1.9.
2. wipes their server.
3. Installs Moodle 1.9.4.
4. Restore just the bits they want, in order to have a nice clean DB.

In this case $restore->original_wwwroot == $CFG->wwwroot but it is a different moodle.

However, there is a $CFG->siteidentifier, which is initialised on install to random_string(32).$_SERVER['HTTP_HOST']. It is used for registration, and a couple of other places. Unfortunately, at the moment, this is not stored in the backup file.

So, the plan is:

1. Change backup, so $CFG->siteidentifier is stored in every backup file.
2. Change restore so that the siteidentifier from the backup file is available during restore, as $restore->originalsiteidentifier.
3. Write a restore_is_same_site($restore) function that returns true or false to let you know if the backup file came from this site. If this is an old backup that does not include siteidentifier, then restore_is_same_site will return false.
4. Use restore_is_same_site($restore) in my code.
5. Search the rest of the restore code (briefly) to see if there are any other hacks that should be replaced by restore_is_same_site.

Hopefully I will be able to do this on Monday.

Show
Tim Hunt added a comment - Right, following discussion with Eloy and Nico in developer chat: The if ($restore->original_wwwroot == $CFG->wwwroot) is dangerous because of this scenario: 1. Admin backs up everything important in their Moodle 1.9. 2. wipes their server. 3. Installs Moodle 1.9.4. 4. Restore just the bits they want, in order to have a nice clean DB. In this case $restore->original_wwwroot == $CFG->wwwroot but it is a different moodle. However, there is a $CFG->siteidentifier, which is initialised on install to random_string(32).$_SERVER['HTTP_HOST']. It is used for registration, and a couple of other places. Unfortunately, at the moment, this is not stored in the backup file. So, the plan is: 1. Change backup, so $CFG->siteidentifier is stored in every backup file. 2. Change restore so that the siteidentifier from the backup file is available during restore, as $restore->originalsiteidentifier. 3. Write a restore_is_same_site($restore) function that returns true or false to let you know if the backup file came from this site. If this is an old backup that does not include siteidentifier, then restore_is_same_site will return false. 4. Use restore_is_same_site($restore) in my code. 5. Search the rest of the restore code (briefly) to see if there are any other hacks that should be replaced by restore_is_same_site. Hopefully I will be able to do this on Monday.
Hide
Permalink
Tim Hunt added a comment -

I'm looking for a code review on my revised patch: backupsamesite.patch.txt

I was suprised how many places were already using a nasty hack to see if this was the same site.

I also refactored the three or four places where $CFG->siteidentifier was initialised, so they all called a function.

Show
Tim Hunt added a comment - I'm looking for a code review on my revised patch: backupsamesite.patch.txt I was suprised how many places were already using a nasty hack to see if this was the same site. I also refactored the three or four places where $CFG->siteidentifier was initialised, so they all called a function.
Hide
Permalink
Tim Hunt added a comment -

Hmm. For security reasons, should I only put md5($CFG->siteidentifier) into the backup file, rather than the plain $CFG->siteidentifier?

Otherwise, people may be able to hijack each-other's site registrations.

Show
Tim Hunt added a comment - Hmm. For security reasons, should I only put md5($CFG->siteidentifier) into the backup file, rather than the plain $CFG->siteidentifier? Otherwise, people may be able to hijack each-other's site registrations.
Hide
Permalink
Martin Dougiamas added a comment -

Can we squeeze this into 1.9.4, Eloy and Petr?

Show
Martin Dougiamas added a comment - Can we squeeze this into 1.9.4, Eloy and Petr?
Hide
Permalink
Tim Hunt added a comment -

I decided that the MD5 hashing was necessary, so attached is a revised patch for review. Would be nice to get it in to this week's weekly build, but I would really like a code review first, please.

Show
Tim Hunt added a comment - I decided that the MD5 hashing was necessary, so attached is a revised patch for review. Would be nice to get it in to this week's weekly build, but I would really like a code review first, please.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

About the backupsamesite.patch.txt, perfect IMO! Hashing the site identifier sounds like a great idea and "same site" detection will be now much better, being 100% compatible with old backups (where detection only can be done with wwwroot).

Also the questions part is ok, with those 2 lines we talked deleted. Perfect!

+1 for stable

Show
Eloy Lafuente (stronk7) added a comment - About the backupsamesite.patch.txt, perfect IMO! Hashing the site identifier sounds like a great idea and "same site" detection will be now much better, being 100% compatible with old backups (where detection only can be done with wwwroot). Also the questions part is ok, with those 2 lines we talked deleted. Perfect! +1 for stable
Hide
Permalink
Tim Hunt added a comment -

Thanks Eloy. Will commit when I get to work in about 20 minutes.

Show
Tim Hunt added a comment - Thanks Eloy. Will commit when I get to work in about 20 minutes.
Hide
Permalink
Tim Hunt added a comment -

I know it is testing day, but this code has been written for a few days, was just waiting for review, so I committed it anyway. Thanks Eloy and others for your help.

Show
Tim Hunt added a comment - I know it is testing day, but this code has been written for a few days, was just waiting for review, so I committed it anyway. Thanks Eloy and others for your help.

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.