Moodle
  1. Moodle
  2. MDL-16658

Teachers are able to create user accounts during restore even they don't have "moodle/user:create"

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9.2
    • Fix Version/s: 1.9.8
    • Component/s: Backup
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      31106

      Description

      Please read http://moodle.org/mod/forum/discuss.php?d=106252 and http://moodle.org/mod/forum/discuss.php?d=104358

      FULL STEPS TO REPRODUCE
      =========================
      1. go to server A
      2. backup a course together with user accounts
      3. go to server B and log in as a Teacher or Course creator
      4. restore the backup

      WHAT WAS EXPECTED
      ===================
      As neither Teacher nor Course creator have granted capability "moodle/user:create", no users are restored

      WHAT ACTUALLY HAPPENS
      =======================
      New users are registered. The Admin does not know anything.

      1. MDL-16658.diff
        1 kB
        Anthony Borrow
      2. MDL-16658-full.patch.txt
        19 kB
        Eloy Lafuente (stronk7)
      3. pre_check_users.patch.txt
        13 kB
        Eloy Lafuente (stronk7)

        Issue Links

          Activity

          Hide
          Brian Warling added a comment -

          I'm hoping this can be addressed soon. Because of this, we've disabled course restore for non-administrators. Thanks... Brian

          Show
          Brian Warling added a comment - I'm hoping this can be addressed soon. Because of this, we've disabled course restore for non-administrators. Thanks... Brian
          Hide
          Anthony Borrow added a comment -

          I agree that the users should not be added; however, we also need to add feedback so that the user attempting to restore understands that they did not have the privileges and that the user was not added. My +1 for working this out. Perhaps the capability for teachers to create users and ability to create users on restore should be distinguished. Peace - Anthony

          Show
          Anthony Borrow added a comment - I agree that the users should not be added; however, we also need to add feedback so that the user attempting to restore understands that they did not have the privileges and that the user was not added. My +1 for working this out. Perhaps the capability for teachers to create users and ability to create users on restore should be distinguished. Peace - Anthony
          Hide
          Jonathan Moore added a comment -

          +1 from us on this too. Seems like a serious circumvention of security model. This could be exploited maliciously, through specially crafted course restore files, all be itt only to create non-privileged users.

          Show
          Jonathan Moore added a comment - +1 from us on this too. Seems like a serious circumvention of security model. This could be exploited maliciously, through specially crafted course restore files, all be itt only to create non-privileged users.
          Hide
          Anthony Borrow added a comment -

          David - This is a simple way to prevent the users from being created. I've not tested it but I just wanted to provide a quick way to prevent the users from actually being created. I do not know what impact this might have on other aspects of the restore in terms of the assignments for users that are not created in the restore. I think ultimately the error message should be specific about the user data that is not restored and the reason. I didn't want to create new language strings for this simple patch but thought it might be a step in the right direction for those who feel like they cannot allow teachers to do restores. Peace - Anthony

          p.s. - Eloy - Do you think that we should label this as a possible security issue? I could see the argument being made but I don't see too much harm in it.

          Show
          Anthony Borrow added a comment - David - This is a simple way to prevent the users from being created. I've not tested it but I just wanted to provide a quick way to prevent the users from actually being created. I do not know what impact this might have on other aspects of the restore in terms of the assignments for users that are not created in the restore. I think ultimately the error message should be specific about the user data that is not restored and the reason. I didn't want to create new language strings for this simple patch but thought it might be a step in the right direction for those who feel like they cannot allow teachers to do restores. Peace - Anthony p.s. - Eloy - Do you think that we should label this as a possible security issue? I could see the argument being made but I don't see too much harm in it.
          Hide
          Thomas Robb added a comment -

          While I fully agree that the creation of users by non-authorized instructors/course creators needs to be restricted, we need to be sure that this capability can be easily allowed in cases where it is desirable.

          I, for example, have a Moodle set up where I am giving course space to teachers at other institutions. They want to use my "MoodleReader module" but for various reasons (mainly the resistance of local staff admins) can't set it up on their own institution's server. I would like them to be able to back up the users in their courses and then restore them to their course on my Moodle so that I don't have to intervene each time. Without the current "bug" they would have to have the students enroll via the mail/confirmation process. This simply would not work efficiently with a large number of students – too many misentered email addresses or other ways that the confirmation message might go astray.

          Show
          Thomas Robb added a comment - While I fully agree that the creation of users by non-authorized instructors/course creators needs to be restricted, we need to be sure that this capability can be easily allowed in cases where it is desirable. I, for example, have a Moodle set up where I am giving course space to teachers at other institutions. They want to use my "MoodleReader module" but for various reasons (mainly the resistance of local staff admins) can't set it up on their own institution's server. I would like them to be able to back up the users in their courses and then restore them to their course on my Moodle so that I don't have to intervene each time. Without the current "bug" they would have to have the students enroll via the mail/confirmation process. This simply would not work efficiently with a large number of students – too many misentered email addresses or other ways that the confirmation message might go astray.
          Hide
          Anthony Borrow added a comment -

          Thomas - Any reason why you could not assign the user create capability to those teachers? It would seem that would allow you to do what you want if I understand it correctly. Peace - Anthony

          Show
          Anthony Borrow added a comment - Thomas - Any reason why you could not assign the user create capability to those teachers? It would seem that would allow you to do what you want if I understand it correctly. Peace - Anthony
          Hide
          Daniel Neis added a comment -

          Hello, here at Federal University Of Santa Catarina, Brazil, we have a system that sincronizes the central database of the university with the moodle database, with that all users are automatically created and the data automatically updated.

          We let the teachers import courses from any moodle installation they want and sometimes it is not possible to do the backup of original course without user information. If we can set a permission to not allow they create users during backup would be great.

          Show
          Daniel Neis added a comment - Hello, here at Federal University Of Santa Catarina, Brazil, we have a system that sincronizes the central database of the university with the moodle database, with that all users are automatically created and the data automatically updated. We let the teachers import courses from any moodle installation they want and sometimes it is not possible to do the backup of original course without user information. If we can set a permission to not allow they create users during backup would be great.
          Hide
          Petr Škoda added a comment -

          why not simply add new capability "moodle/backup:createusers", this would be more backwards compatible imho

          Show
          Petr Škoda added a comment - why not simply add new capability "moodle/backup:createusers", this would be more backwards compatible imho
          Hide
          Daniel Neis added a comment -

          I like Peter's approache. A new capability would be great

          Show
          Daniel Neis added a comment - I like Peter's approache. A new capability would be great
          Hide
          Martin Dougiamas added a comment -

          Eloy, can we add a new capability for 1.9.6 ? This bug has a lot of votes.

          Show
          Martin Dougiamas added a comment - Eloy, can we add a new capability for 1.9.6 ? This bug has a lot of votes.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry,

          really I don't think that using "moodle/user:create" neither "moodle/backup:createusers" is the proper solution. For me there are more "variables" is this problem. Let me explain the thing with one simple use-case:

          Imagine server "A", with course "C" and student "S" enrolled (and with forum posts and other activity on it).

          Teacher makes backup of course "C". Obviously it will include information of user "S" plus all his forum posts.

          Then, for any reason, student is "deleted" from server A (so his login is changed along with other personal information), although forum posts and related info continues there, associated with the "deleted" user.

          Then, teacher tries to restore backup into a new course, or into the same course (A) but deleting data first.

          At this point, student "S" won't be found anymore in the server (check is performed by login - another problem - that has been trashed by the deletion), so restore MUST re-create it or, later in the process, when restoring forum posts, there won't be any user to link the info to). If we rely on any capability to decide if users will be created or no, we can easily end with non-restorable information. So, simple check isn't enough at all (nor for restoring in a different server nor for restoring in the same server, it's important to note that the problem can happen in any server, as the use case above demoes).

          So, IMO, we can follow only 2 "safe" approaches:

          • - The radical one: Preventing to restore ANY user related-info if user lacks the "moodle/user:create" (or "moodle/backup:createusers") capability. Note I'm not talking only about to prevent user creation BUT about to prevent restore of ANY user-related info (forum posts...) what can lead to a BIG loss of functionality for, say, 95% of sites. Of course, it's 100% safe, because now new users will be created nor information will be "orphaned". This is the approach we recently followed when restoring backup files containing mnet users (MDL-17009) that now are only allowed to "moodle/user:create" users. Of course, we followed that approach for mnet users because it only affects "0.0001%" of sites (those using MNET). The question is if the same approach is extensible to all course backups.
          • - The correct one: It involves to pre-check in the early steps of restore (before performing the restore of activity itself), if all the users to be added already exist in destination server. If that pre-check passes, then restore can continue because we have assured that all the activity will be properly linked to existing users, hence nothing will became orphaned. Obviously this pre-check will be performed when the user misses the "moodle/user:create" (or "moodle/backup:createusers") capability only. Users able to create users don't need the check at all. But we have two big problems with this pre-check: First of all, it doesn't fit well in current backup/restore architecture (it will be really hackery to introduce such a check). And second, we need to improve our algorithm of detecting existing users (see MDL-15598 for example), because current match by "login" is really inaccurate).

          So... summarising, we have two alternatives:

          • Completely prevent restoring backup files with user info to users missing the "moodle/user:create" (or "moodle/backup:createusers") capability. Easily doable, although perhaps the default for teachers, for better BC should be "moodle/backup:createusers" => allowed.
          • Implement a complete user pre-check with an stronger match than the current one. More complex to implement but also tons better solution.

          Sorry but the long comment, just trying to explain that it isn't only a matter of adding one capability (unless we want to be radical). Please comment, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, really I don't think that using "moodle/user:create" neither "moodle/backup:createusers" is the proper solution. For me there are more "variables" is this problem. Let me explain the thing with one simple use-case: Imagine server "A", with course "C" and student "S" enrolled (and with forum posts and other activity on it). Teacher makes backup of course "C". Obviously it will include information of user "S" plus all his forum posts. Then, for any reason, student is "deleted" from server A (so his login is changed along with other personal information), although forum posts and related info continues there, associated with the "deleted" user. Then, teacher tries to restore backup into a new course, or into the same course (A) but deleting data first. At this point, student "S" won't be found anymore in the server (check is performed by login - another problem - that has been trashed by the deletion), so restore MUST re-create it or, later in the process, when restoring forum posts, there won't be any user to link the info to). If we rely on any capability to decide if users will be created or no, we can easily end with non-restorable information. So, simple check isn't enough at all (nor for restoring in a different server nor for restoring in the same server, it's important to note that the problem can happen in any server, as the use case above demoes). So, IMO, we can follow only 2 "safe" approaches: - The radical one: Preventing to restore ANY user related-info if user lacks the "moodle/user:create" (or "moodle/backup:createusers") capability. Note I'm not talking only about to prevent user creation BUT about to prevent restore of ANY user-related info (forum posts...) what can lead to a BIG loss of functionality for, say, 95% of sites. Of course, it's 100% safe, because now new users will be created nor information will be "orphaned". This is the approach we recently followed when restoring backup files containing mnet users ( MDL-17009 ) that now are only allowed to "moodle/user:create" users. Of course, we followed that approach for mnet users because it only affects "0.0001%" of sites (those using MNET). The question is if the same approach is extensible to all course backups. - The correct one: It involves to pre-check in the early steps of restore (before performing the restore of activity itself), if all the users to be added already exist in destination server. If that pre-check passes, then restore can continue because we have assured that all the activity will be properly linked to existing users, hence nothing will became orphaned. Obviously this pre-check will be performed when the user misses the "moodle/user:create" (or "moodle/backup:createusers") capability only. Users able to create users don't need the check at all. But we have two big problems with this pre-check: First of all, it doesn't fit well in current backup/restore architecture (it will be really hackery to introduce such a check). And second, we need to improve our algorithm of detecting existing users (see MDL-15598 for example), because current match by "login" is really inaccurate). So... summarising, we have two alternatives: Completely prevent restoring backup files with user info to users missing the "moodle/user:create" (or "moodle/backup:createusers") capability. Easily doable, although perhaps the default for teachers, for better BC should be "moodle/backup:createusers" => allowed. Implement a complete user pre-check with an stronger match than the current one. More complex to implement but also tons better solution. Sorry but the long comment, just trying to explain that it isn't only a matter of adding one capability (unless we want to be radical). Please comment, ciao
          Hide
          Thomas Robb added a comment -

          Eloy says:
          >Implement a complete user pre-check with an stronger match than the current one. More complex to implement but also tons better solution.

          This is fine, but the site administrator needs to be able to elect whether this pre-check is enforced or not. As I stated in my comment of 22/Apr/09, I have sites where I need to permit others to do such restores. Can there be a capability created which, if assigned, would permit specific users to restore without the strict check?

          Show
          Thomas Robb added a comment - Eloy says: >Implement a complete user pre-check with an stronger match than the current one. More complex to implement but also tons better solution. This is fine, but the site administrator needs to be able to elect whether this pre-check is enforced or not. As I stated in my comment of 22/Apr/09, I have sites where I need to permit others to do such restores. Can there be a capability created which, if assigned, would permit specific users to restore without the strict check?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Thomas,

          yes, I think we can use that new proposed capability "moodle/backup:createusers" to allow users not having "moodle/user:create" (admins) to bypass any check/restriction. No problem with that.

          They keys here are which approach we must follow (the "radical" or the "correct" one above) and which defaults (for each legacy role) should we apply to that new capability "moodle/backup:createusers".

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Thomas, yes, I think we can use that new proposed capability "moodle/backup:createusers" to allow users not having "moodle/user:create" (admins) to bypass any check/restriction. No problem with that. They keys here are which approach we must follow (the "radical" or the "correct" one above) and which defaults (for each legacy role) should we apply to that new capability "moodle/backup:createusers". Ciao
          Hide
          Mark van Hoek added a comment -

          My vote goes to the "correct" solution, but it'd be good to give the option to proceed with the restore without the user info.

          IMHO, only admins should get "moodle/backup:createusers" by default.

          Show
          Mark van Hoek added a comment - My vote goes to the "correct" solution, but it'd be good to give the option to proceed with the restore without the user info. IMHO, only admins should get "moodle/backup:createusers" by default.
          Hide
          Enrique Castro added a comment -

          Hi,
          I think both approaches proposed by Eluy have merit. The "correct" one is fine, and the sophisticated way. I vote for this for people with "moodle/backup:createusers" capability.

          But the "radical" has its own merit.
          As a case use, we at ULPGC, as in many Universities, synchronize users from external DB. We want adminds and teh liek to be able to create users on restore but nobody else. We instruct teachers to backup/restore courses from one year to other "Without user activity" and "without users", but there are always errors.

          A configurable switch to disallow teachers to restore users &userdata would be very useful (now, we hack restorelib for this purpose): i.e. vote also for capability check to fall into no users/no user info restore when "moodle/user:create" is not allowed.

          Show
          Enrique Castro added a comment - Hi, I think both approaches proposed by Eluy have merit. The "correct" one is fine, and the sophisticated way. I vote for this for people with "moodle/backup:createusers" capability. But the "radical" has its own merit. As a case use, we at ULPGC, as in many Universities, synchronize users from external DB. We want adminds and teh liek to be able to create users on restore but nobody else. We instruct teachers to backup/restore courses from one year to other "Without user activity" and "without users", but there are always errors. A configurable switch to disallow teachers to restore users &userdata would be very useful (now, we hack restorelib for this purpose): i.e. vote also for capability check to fall into no users/no user info restore when "moodle/user:create" is not allowed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Moving this to 1.9.7.

          I'm aimed to do this in the "correct" way: new capability + pre-check all users, but it isn't trivial to implement now. I hope to be able to do so as we'll need that also for 2.0 (where I'm working now).

          Show
          Eloy Lafuente (stronk7) added a comment - Moving this to 1.9.7. I'm aimed to do this in the "correct" way: new capability + pre-check all users, but it isn't trivial to implement now. I hope to be able to do so as we'll need that also for 2.0 (where I'm working now).
          Hide
          Martin Dougiamas added a comment -

          We talked about a moodle/backup:userinfo setting for 1.9.7 that disallows all user restoring (including creating) and user data, similar to what Enrique mentioned above.

          Show
          Martin Dougiamas added a comment - We talked about a moodle/backup:userinfo setting for 1.9.7 that disallows all user restoring (including creating) and user data, similar to what Enrique mentioned above.
          Hide
          Daniel Neis added a comment -

          Hello,

          i have seen a commit at MOODLE_19_STABLE (http://git.catalyst.net.nz/gw?p=moodle-r2.git;a=commit;h=f484cb3e30e485958bafe54a391064e5d298d6c2) that adds the moodle/backup:userinfo capability, disabled by default for teachers.
          Is it related to this issue? It checks for the capability in the backup process, but not in the restore. Maybe it should be two distinct caps...

          Show
          Daniel Neis added a comment - Hello, i have seen a commit at MOODLE_19_STABLE ( http://git.catalyst.net.nz/gw?p=moodle-r2.git;a=commit;h=f484cb3e30e485958bafe54a391064e5d298d6c2 ) that adds the moodle/backup:userinfo capability, disabled by default for teachers. Is it related to this issue? It checks for the capability in the backup process, but not in the restore. Maybe it should be two distinct caps...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Daniels,

          both MDL-20834 (moodle/backup:userinfo) and MDL-20849 (moodle/restore:userinfo) are there to prevent completely any backup/restore operation related with user data. Not only the creation of users but anything related with users (like forum posts, assignment submissions...).

          Both are being implemented right now and both will debut in Moodle 1.8.11 and Modle 1.9.7.

          About how they are related with this issue, I'd say that only "tangentially"; they are more restrictive than the proposed "moodle/restore:createuser" as far as they prevent ANY operation with user info, not only account creation but ANY user info.

          But, no matter of them, and how every site/institution decides to use them (note that, by default, they aren't granted to teachers!), the "moodle/restore:createuser" must land sooner than later, with proper pre-check and so on, as commented above. And will work together with the moodle/restore:userinfo one, so both will need to be allowed to successfully restore courses needing to create users.

          I hope I'll be able to finish the bugs above (and some more I've pending for 1.8.11, 1.9.7 releases) and then, invest some time with this (new capability + proper pre-check).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Daniels, both MDL-20834 (moodle/backup:userinfo) and MDL-20849 (moodle/restore:userinfo) are there to prevent completely any backup/restore operation related with user data. Not only the creation of users but anything related with users (like forum posts, assignment submissions...). Both are being implemented right now and both will debut in Moodle 1.8.11 and Modle 1.9.7. About how they are related with this issue, I'd say that only "tangentially"; they are more restrictive than the proposed "moodle/restore:createuser" as far as they prevent ANY operation with user info, not only account creation but ANY user info. But, no matter of them, and how every site/institution decides to use them (note that, by default, they aren't granted to teachers!), the "moodle/restore:createuser" must land sooner than later, with proper pre-check and so on, as commented above. And will work together with the moodle/restore:userinfo one, so both will need to be allowed to successfully restore courses needing to create users. I hope I'll be able to finish the bugs above (and some more I've pending for 1.8.11, 1.9.7 releases) and then, invest some time with this (new capability + proper pre-check). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi,

          adding here one partial patch (for public comment) that tries to solve various problems related with restoring of users from backup files.

          VERY IMPORTANT: It's one incomplete patch, so don't apply it at all in your sites!. It only contains the sensible parts to be discussed / agreed.

          In my next comment, I'll explain all the rationale behind that code.

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, adding here one partial patch (for public comment) that tries to solve various problems related with restoring of users from backup files. VERY IMPORTANT: It's one incomplete patch, so don't apply it at all in your sites! . It only contains the sensible parts to be discussed / agreed. In my next comment, I'll explain all the rationale behind that code.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          This is the FLOW behind the patch attached in my previous comment:

          1. It will exist one new capability 'moodle/restore:createuser' (only allowed to admins by default).
          2. There is already (since some days ago) one new capability 'moodle/restore:userinfo' (only allowed to admins by default too).
          3. There is one new function restore_check_user($restore, $user) that will:
            1. For the given user apply the LOGIC (see below for explanation) to decide if:
              1. The user already exists, mapping will happen (return DB user)
              2. The user doesn't exist, needs to be created (return true)
              3. The user can lead to some conflict (return false)
          4. There is one new function restore_precheck_users($xml_file, $restore, &$problems) that will:
            1. Load all the information from backup file related to users
            2. For each user:
              1. Call to the restore_check_user($restore, $user) function (see above)
              2. If the user need to be created, check that all these are true:
                1. 'moodle/restore:createuser' is allowed
                2. 'moodle/restore:userinfo' is allowed
                3. $CFG->restore_create_users_forbidden (new global, config.php setting, for clumsy admins :-P) is not set
              3. If the user cannot be created or the user can lead to conflict, annotate problem, mark final result as false and continue processing.
          5. In the very-early stages of restore_execute, before anything changing DB/filesystem happen, we'll have a pre-check users section that will:
            1. Call to the restore_precheck_users($xml_file, $restore, &$problems) function (see above)
            2. If returns false => Something has gone wrong. Stop restore and inform about $problems found.
            3. If returns true => Those users can be properly restored without problems, continue with restore process

          Notes: This approach forces us to process the users section in the xml file once more time. Not big deal but will be slower (assumable IMO)

          The LOGIC within the restore_check_user($restore, $user) function is:

          • If restoring users from same site backup:
            • 1A - If match by id and username and mnethost => ok, return target user
            • 1B - If match by id and mnethost and user is deleted in DB and match by email LIKE 'backup_email%' => ok, return target user
            • 1C - If match by id and mnethost and user is deleted in backup file and match by email = email_without_time(backup_email) => ok, return target user
            • 1D - If match by username and mnethost and doesn't match by id => conflict, return false
            • 1E - else => user needs to be created, return true
          • if restoring from another site backup:
            • 2A - If match by username and mnethost and email or non-zero firstaccess) => ok, return target user
            • 2B - Note: we cannot handle "deleted" situations here as far as username gets modified and id cannot be used here
            • 2C - If match by username and mnethost and not by (email or non-zero firstaccess) => conflict, return false
            • 2D - else => user needs to be created, return true

          Notes: I know it looks complex, but the problem is complex really because there are a bunch of variables affecting the final decision of what is one "matching user" (site where the backup was created, username, deleted users, mnet status...). This is the final logic I've ended after spending a lot of time looking for different situations and discussing a lot with Petr (thanks!).

          And that's all I can say about the proposal to pre-check users before restoring anything. Note it will help to solve this bug and some others related with restore causing wrong-mappings to happen, that is one point as least as dangerous as this (see MDL-12001, MDL-15585).

          Any feedback will be really welcome. For any discussion, and due to the complexity of both the FLOW and the LOGIC, please, use the list numbers used for clear reference. I think it would be great if we can have this working for 1.9.7 (not sure about backporting it to 1.8.x).

          Review and comment, TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited This is the FLOW behind the patch attached in my previous comment: It will exist one new capability 'moodle/restore:createuser' (only allowed to admins by default). There is already (since some days ago) one new capability 'moodle/restore:userinfo' (only allowed to admins by default too). There is one new function restore_check_user($restore, $user) that will: For the given user apply the LOGIC (see below for explanation) to decide if: The user already exists, mapping will happen (return DB user) The user doesn't exist, needs to be created (return true) The user can lead to some conflict (return false) There is one new function restore_precheck_users($xml_file, $restore, &$problems) that will: Load all the information from backup file related to users For each user: Call to the restore_check_user($restore, $user) function (see above) If the user need to be created, check that all these are true: 'moodle/restore:createuser' is allowed 'moodle/restore:userinfo' is allowed $CFG->restore_create_users_forbidden (new global, config.php setting, for clumsy admins :-P) is not set If the user cannot be created or the user can lead to conflict, annotate problem, mark final result as false and continue processing. In the very-early stages of restore_execute, before anything changing DB/filesystem happen, we'll have a pre-check users section that will: Call to the restore_precheck_users($xml_file, $restore, &$problems) function (see above) If returns false => Something has gone wrong. Stop restore and inform about $problems found. If returns true => Those users can be properly restored without problems, continue with restore process Notes: This approach forces us to process the users section in the xml file once more time. Not big deal but will be slower (assumable IMO) The LOGIC within the restore_check_user($restore, $user) function is: If restoring users from same site backup: 1A - If match by id and username and mnethost => ok, return target user 1B - If match by id and mnethost and user is deleted in DB and match by email LIKE 'backup_email%' => ok, return target user 1C - If match by id and mnethost and user is deleted in backup file and match by email = email_without_time(backup_email) => ok, return target user 1D - If match by username and mnethost and doesn't match by id => conflict, return false 1E - else => user needs to be created, return true if restoring from another site backup: 2A - If match by username and mnethost and email or non-zero firstaccess) => ok, return target user 2B - Note: we cannot handle "deleted" situations here as far as username gets modified and id cannot be used here 2C - If match by username and mnethost and not by (email or non-zero firstaccess) => conflict, return false 2D - else => user needs to be created, return true Notes: I know it looks complex, but the problem is complex really because there are a bunch of variables affecting the final decision of what is one "matching user" (site where the backup was created, username, deleted users, mnet status...). This is the final logic I've ended after spending a lot of time looking for different situations and discussing a lot with Petr (thanks!). And that's all I can say about the proposal to pre-check users before restoring anything. Note it will help to solve this bug and some others related with restore causing wrong-mappings to happen, that is one point as least as dangerous as this (see MDL-12001, MDL-15585). Any feedback will be really welcome. For any discussion, and due to the complexity of both the FLOW and the LOGIC, please, use the list numbers used for clear reference. I think it would be great if we can have this working for 1.9.7 (not sure about backporting it to 1.8.x). Review and comment, TIA and ciao
          Hide
          Daniel Neis added a comment -

          Hello,

          just one point, in FLOW, item 5, subitem 2:

          2 If returns false => Something has gone wrong. Stop restore and inform about $problems found.

          It would be good if, after see info about $problems found, the use could choose to continue the restore process ignoring the problematic users.
          All the rest seems very good. =o)

          ps.: i got a "PERMISSION VIOLATION" error if i try to see MDL-12001 and MDL-15585

          Show
          Daniel Neis added a comment - Hello, just one point, in FLOW, item 5, subitem 2: 2 If returns false => Something has gone wrong. Stop restore and inform about $problems found. It would be good if, after see info about $problems found, the use could choose to continue the restore process ignoring the problematic users. All the rest seems very good. =o) ps.: i got a "PERMISSION VIOLATION" error if i try to see MDL-12001 and MDL-15585
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Daniels,

          thanks for feedback. About your suggestion:

          It would be good if, after see info about $problems found, the use could choose to continue the restore process ignoring the problematic users.

          That's 100% impossible to implement. Users in backup files only can be handled as a whole, so if any user can lead to problems (both conflicts and lack of creation permissions), the whole process must stop. We cannot restore users "partially" or a lot of user-dependent information (posts, submissions... everything!) will end orphaned (without user).

          So we only can allow to continue if all the users have passed the check. If not, stop is the only consistent action IMO.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Daniels, thanks for feedback. About your suggestion: It would be good if, after see info about $problems found, the use could choose to continue the restore process ignoring the problematic users. That's 100% impossible to implement. Users in backup files only can be handled as a whole, so if any user can lead to problems (both conflicts and lack of creation permissions), the whole process must stop. We cannot restore users "partially" or a lot of user-dependent information (posts, submissions... everything!) will end orphaned (without user). So we only can allow to continue if all the users have passed the check. If not, stop is the only consistent action IMO. Ciao
          Hide
          David Mudrak added a comment -

          Two things in 1C:

          $trimemail = preg_replace('/(.*?)[0-9]+/', '\\1', $user->email);

          the regexp can't be used in this way as it removes all numbers from the email address - including those like "girl16@hotmail.com". IMO what we need is:

          preg_replace('/(.*?)\.[0-9]+$/', '\\1', $user->email);

          Also, the email with the timestamp of deletion (like "girl16@hotmail.com.1258987459") is stored in "username" after the user was deleted, not in "email" (which is empty). Has to be fixed in SQL query.

          Show
          David Mudrak added a comment - Two things in 1C: $trimemail = preg_replace('/(.*?)[0-9]+/', '\\1', $user->email); the regexp can't be used in this way as it removes all numbers from the email address - including those like "girl16@hotmail.com". IMO what we need is: preg_replace('/(.*?)\.[0-9]+$/', '\\1', $user->email); Also, the email with the timestamp of deletion (like "girl16@hotmail.com.1258987459") is stored in "username" after the user was deleted, not in "email" (which is empty). Has to be fixed in SQL query.
          Hide
          David Mudrak added a comment -

          Actually, I am confused here. IMO the trimmed email (without the datestamp) should be checked in 1B (the user is deleted in DB but live in backup). 1C seems to be very weird situation where the user is deleted in backup but lives in DB. Can this ever happen?

          Show
          David Mudrak added a comment - Actually, I am confused here. IMO the trimmed email (without the datestamp) should be checked in 1B (the user is deleted in DB but live in backup). 1C seems to be very weird situation where the user is deleted in backup but lives in DB. Can this ever happen?
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Good points David, thanks!

          I've fixed the regular expression

          About 1B and 1C... let me explain something:

          1A will handle situations where the user is deleted in both sides (DB and backup) and where the user isn't deleted.

          1B will handle situations where the user is deleted in DB but not in backup file (pretty normal).

          1C will handle situations where the user in backup file is deleted but the user in BD isn't (less normal but still possible, recovered backup, "revived" manually (not supported!) user...

          So, as we are able to handle all the combinations when restoring in same site... I decided you put all them there, no matter if 1C is highly improbable or not.

          Finally, I've fixed both 1B and 1C, as far as deleted users store their email in the username field. Now both queries do that. Uploading new versions in 5 seconds...

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Good points David, thanks! I've fixed the regular expression About 1B and 1C... let me explain something: 1A will handle situations where the user is deleted in both sides (DB and backup) and where the user isn't deleted. 1B will handle situations where the user is deleted in DB but not in backup file (pretty normal). 1C will handle situations where the user in backup file is deleted but the user in BD isn't (less normal but still possible, recovered backup, "revived" manually (not supported!) user... So, as we are able to handle all the combinations when restoring in same site... I decided you put all them there, no matter if 1C is highly improbable or not. Finally, I've fixed both 1B and 1C, as far as deleted users store their email in the username field. Now both queries do that. Uploading new versions in 5 seconds...
          Hide
          David Mudrak added a comment -

          $CFG->restore_create_users_forbidden does not fit our current coding style for naming variables. Alternatives can be $CFG->restorecreateusersforbidden, $CFG->donotcreateusersonrestore, $CFG->norestorableusers or so (yes, the readability sucks. This coding style rule sucks, too).

          Show
          David Mudrak added a comment - $CFG->restore_create_users_forbidden does not fit our current coding style for naming variables. Alternatives can be $CFG->restorecreateusersforbidden, $CFG->donotcreateusersonrestore, $CFG->norestorableusers or so (yes, the readability sucks. This coding style rule sucks, too).
          Hide
          David Mudrak added a comment -

          Just a detail

          if (has_capability('moodle/restore:createuser', $context) and has_capability('moodle/restore:userinfo', $context))

          should be imo replaced by has_all_capabilities(). In the future, when caps evaluation engine is rewritten, it should be more effective. Currently it does not matter, though.

          Show
          David Mudrak added a comment - Just a detail if (has_capability('moodle/restore:createuser', $context) and has_capability('moodle/restore:userinfo', $context)) should be imo replaced by has_all_capabilities(). In the future, when caps evaluation engine is rewritten, it should be more effective. Currently it does not matter, though.
          Hide
          David Mudrak added a comment -
          $mnethosts = get_records('mnet_host', '', '', 'wwwroot', 'wwwroot, id');

          I would not rely on wwwroot being uniq in that table. mnet host mapping must be implemented using iteration over returned array.

          Show
          David Mudrak added a comment - $mnethosts = get_records('mnet_host', '', '', 'wwwroot', 'wwwroot, id'); I would not rely on wwwroot being uniq in that table. mnet host mapping must be implemented using iteration over returned array.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi David,

          thanks again.

          Agree about the $CFG name, after looking to current config-dist.php existing names I'm going to be using:

          $CFG->disableusercreationonrestore
          

          About possible duplicates in mnet_host table, well, we cannot do anything about it. At some moment we need to decide about one of them and it doesn't matter if we do that in SQL or in PHP. So, here it's done in SQL. Nothing else.

          And finally, about capabilities, agree, although it's executed only once (out from the loop) so I really think it won't hurt too much, specially knowing the trillions of operations already performed by restore. So I think leaving it as is is good enough.

          Also, after some discussion in HQ chat, I think we have sorted out two ways to implement the point 2B above (detect deleted users when restoring from another site) and I'm going to implement them. Then will publish again here one updated "LOGIC" document.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi David, thanks again. Agree about the $CFG name, after looking to current config-dist.php existing names I'm going to be using: $CFG->disableusercreationonrestore About possible duplicates in mnet_host table, well, we cannot do anything about it. At some moment we need to decide about one of them and it doesn't matter if we do that in SQL or in PHP. So, here it's done in SQL. Nothing else. And finally, about capabilities, agree, although it's executed only once (out from the loop) so I really think it won't hurt too much, specially knowing the trillions of operations already performed by restore. So I think leaving it as is is good enough. Also, after some discussion in HQ chat, I think we have sorted out two ways to implement the point 2B above (detect deleted users when restoring from another site) and I'm going to implement them. Then will publish again here one updated "LOGIC" document. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Moving this to 1.9.8 as far as we are going to release 1.9.7 within hours. I hope we'll be able to put this upstream asap after that release.

          Show
          Eloy Lafuente (stronk7) added a comment - Moving this to 1.9.8 as far as we are going to release 1.9.7 within hours. I hope we'll be able to put this upstream asap after that release.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, LOGIC updated to:

          • If restoring users from same site backup:
            • 1A - Normal check: If match by id and username and mnethost => ok, return target user
            • 1B - Handle users deleted in DB and "alive" in backup file:
              • If match by id and mnethost and user is deleted in DB and (match by username LIKE 'backup_email.%' or by non empty email = md5(username)) => ok, return target user
            • 1C - Handle users deleted in backup file and "alive" in DB:
              • If match by id and mnethost and user is deleted in backup file nd match by email = email_without_time(backup_email) => ok, return target user
            • 1D - Conflict: If match by username and mnethost and doesn't match by id => conflict, return false
            • 1E - None of the above, return true => User needs to be created
          • if restoring from another site backup (cannot match by id here, replace it by email/firstaccess combination):
            • 2A - Normal check: If match by username and mnethost and (email or non-zero firstaccess) => ok, return target user
            • 2B - Handle users deleted in DB and "alive" in backup file:
              • 2B1 - If match by mnethost and user is deleted in DB and not empty email = md5(username) and (username LIKE 'backup_email.%' or non-zero firstaccess) => ok, return target user
              • 2B2 - If match by mnethost and user is deleted in DB and username LIKE 'backup_email.%' and non-zero firstaccess) => ok, return target user (to cover situations were md5(username) wasn't implemented on delete we requiere both)
            • 2C - Handle users deleted in backup file and "alive" in DB:
              • If match mnethost and user is deleted in backup file and by email = email_without_time(backup_email) and non-zero firstaccess=> ok, return target user
            • 2D - Conflict: If match by username and mnethost and not by (email or non-zero firstaccess) => conflict, return false
            • 2E - None of the above, return true => User needs to be created
          • Note1: for DB deleted users email is stored in username field, hence we are looking there for emails. See delete_user()
          • Note2: for DB deleted users md5(username) is stored sometimes in the email field, hence we are looking there for usernames if not empty. See delete_user()

          It's more complete than previous one, as we have introduced some checks to detect deleted users in different site and also we are relying optionalyy, for DB deleted users in the md5(username) that is, since some days ago, being stored in the email column.

          Please, review.

          TODO:

          • Add new cap 'moodle/restore:createuser'
          • Make the pre-check results to be stored in backup_ids table so later, when really restoring users, they are already there.
          • Some lang strings
          • Test, test, test.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, LOGIC updated to: If restoring users from same site backup: 1A - Normal check: If match by id and username and mnethost => ok, return target user 1B - Handle users deleted in DB and "alive" in backup file: If match by id and mnethost and user is deleted in DB and (match by username LIKE 'backup_email.%' or by non empty email = md5(username)) => ok, return target user 1C - Handle users deleted in backup file and "alive" in DB: If match by id and mnethost and user is deleted in backup file nd match by email = email_without_time(backup_email) => ok, return target user 1D - Conflict: If match by username and mnethost and doesn't match by id => conflict, return false 1E - None of the above, return true => User needs to be created if restoring from another site backup (cannot match by id here, replace it by email/firstaccess combination): 2A - Normal check: If match by username and mnethost and (email or non-zero firstaccess) => ok, return target user 2B - Handle users deleted in DB and "alive" in backup file: 2B1 - If match by mnethost and user is deleted in DB and not empty email = md5(username) and (username LIKE 'backup_email.%' or non-zero firstaccess) => ok, return target user 2B2 - If match by mnethost and user is deleted in DB and username LIKE 'backup_email.%' and non-zero firstaccess) => ok, return target user (to cover situations were md5(username) wasn't implemented on delete we requiere both) 2C - Handle users deleted in backup file and "alive" in DB: If match mnethost and user is deleted in backup file and by email = email_without_time(backup_email) and non-zero firstaccess=> ok, return target user 2D - Conflict: If match by username and mnethost and not by (email or non-zero firstaccess) => conflict, return false 2E - None of the above, return true => User needs to be created Note1: for DB deleted users email is stored in username field, hence we are looking there for emails. See delete_user() Note2: for DB deleted users md5(username) is stored sometimes in the email field, hence we are looking there for usernames if not empty. See delete_user() It's more complete than previous one, as we have introduced some checks to detect deleted users in different site and also we are relying optionalyy, for DB deleted users in the md5(username) that is, since some days ago, being stored in the email column. Please, review. TODO: Add new cap 'moodle/restore:createuser' Make the pre-check results to be stored in backup_ids table so later, when really restoring users, they are already there. Some lang strings Test, test, test. Ciao
          Hide
          Daniel Neis added a comment -

          Hello, Eloy

          once you are working on backups, could you take a look at MDL-19233, that is another problem with permissions and restore process?

          Thanks,
          Daniel

          Show
          Daniel Neis added a comment - Hello, Eloy once you are working on backups, could you take a look at MDL-19233, that is another problem with permissions and restore process? Thanks, Daniel
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Daniel, sure.

          I've two goals for 1.9.8, the "users" thing (this) that will fix a bunch of problems (users created when should be forbidden, users incorrectly matched...), and "roles" thing that also will fix various problems (roles created when should be forbidden, role assignments granted by user without perms to do so...). So exactly MDL-19233, (and some more) are in my TODO for next release.

          Just fighting with this first. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Daniel, sure. I've two goals for 1.9.8, the "users" thing (this) that will fix a bunch of problems (users created when should be forbidden, users incorrectly matched...), and "roles" thing that also will fix various problems (roles created when should be forbidden, role assignments granted by user without perms to do so...). So exactly MDL-19233, (and some more) are in my TODO for next release. Just fighting with this first. Ciao
          Hide
          David Mudrak added a comment -

          Eloy, the most recent LOGIC looks ok, at least if I understand the pseudo-code correctly. I can review the patch again once you have it prepared.

          Show
          David Mudrak added a comment - Eloy, the most recent LOGIC looks ok, at least if I understand the pseudo-code correctly. I can review the patch again once you have it prepared.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi David,

          it's in CVS (never executed), just look to backup/restorelib.php and the restore_check_user() function contains all the implementation of all the logic above.

          It's called by restore_precheck_users() within a loop and this last is the function to be used within restore_execute()

          Ciao

          PS: I've planned to add some tests about each situation but surely they will be in HEAD only (as far as the code to test is highly DB-based so I'll be using some unit test classes only available there (of course any fail will be fixed and backported to 1.9).

          Show
          Eloy Lafuente (stronk7) added a comment - Hi David, it's in CVS (never executed), just look to backup/restorelib.php and the restore_check_user() function contains all the implementation of all the logic above. It's called by restore_precheck_users() within a loop and this last is the function to be used within restore_execute() Ciao PS: I've planned to add some tests about each situation but surely they will be in HEAD only (as far as the code to test is highly DB-based so I'll be using some unit test classes only available there (of course any fail will be fixed and backported to 1.9).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Attaching "full.patch.txt" that:

          • Adds the 'moodle/restore:createuser' capability (only granted to admins by default).
          • Enables the users pre-check on restore.
          • Apply the logic above to stop restore if conflict/lack of permissions are found.
          • Simplifies restore_create_users() as far as relies on restore_precheck_users() results to know what to do.
          • Two new settings are available via config.php (see config-dist.php for more info):
            • $CFG->disableusercreationonrestore: To completely forbid users creation on restore (no matter of perms).
            • $CFG->forcedifferentsitecheckingusersonrestore: To force more "relaxed" rules to be applied when checking users.
          • some missing addslashes() in previous version.
          • lang strings and other needed details.

          So, after applying the patch, any file being restored should be checked to detect any users inconsistency, stopping if something potentially wrong is found.

          Please, review and test. I've done here a bunch of tests... but any feedback from people experimenting with this in non-production sites and different backups will be really welcome before commit.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Attaching "full.patch.txt" that: Adds the 'moodle/restore:createuser' capability (only granted to admins by default). Enables the users pre-check on restore. Apply the logic above to stop restore if conflict/lack of permissions are found. Simplifies restore_create_users() as far as relies on restore_precheck_users() results to know what to do. Two new settings are available via config.php (see config-dist.php for more info): $CFG->disableusercreationonrestore: To completely forbid users creation on restore (no matter of perms). $CFG->forcedifferentsitecheckingusersonrestore: To force more "relaxed" rules to be applied when checking users. some missing addslashes() in previous version. lang strings and other needed details. So, after applying the patch, any file being restored should be checked to detect any users inconsistency, stopping if something potentially wrong is found. Please, review and test. I've done here a bunch of tests... but any feedback from people experimenting with this in non-production sites and different backups will be really welcome before commit. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Done. Committed both to 19_STABLE and HEAD. Now restore pre-check requires both permissions and absence of conflicts before creating users.

          Ciao

          PS: I guess, ppl will start claiming about those "conflicts" soon. But we must enforce them IMO. Previous situation was really crazy, causing preoblems like MDL-12001 or MDL-15585.

          TODO:

          1) Document changes in 1.9.8 release pages (doing now).
          2) Review the English strings introduced and the comments in config-dist.php... Helen could you take a look to them, plz? TIA!

          Will close this once finished... re-ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Done. Committed both to 19_STABLE and HEAD. Now restore pre-check requires both permissions and absence of conflicts before creating users. Ciao PS: I guess, ppl will start claiming about those "conflicts" soon. But we must enforce them IMO. Previous situation was really crazy, causing preoblems like MDL-12001 or MDL-15585. TODO: 1) Document changes in 1.9.8 release pages (doing now). 2) Review the English strings introduced and the comments in config-dist.php... Helen could you take a look to them, plz? TIA! Will close this once finished... re-ciao
          Hide
          Helen Foster added a comment -
          Show
          Helen Foster added a comment - Thanks Eloy, documentation on the new capability is now available: http://docs.moodle.org/en/Capabilities/moodle/restore:createuser http://docs.moodle.org/en/Moodle_1.9.8_release_notes
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks for all the docs and polishing of the lang strings Helen!
          (and to everybody else for comments / ideas / feedback)

          Resolving this as fixed, finally, yay!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks for all the docs and polishing of the lang strings Helen! (and to everybody else for comments / ideas / feedback) Resolving this as fixed, finally, yay! Ciao
          Hide
          Larry Lippke added a comment -

          Regarding the following logic:

          if restoring from another site backup:

          • 2A - If match by username and mnethost and email or non-zero firstaccess) => ok, return target user
          • 2B - Note: we cannot handle "deleted" situations here as far as username gets modified and id cannot be used here
          • 2C - If match by username and mnethost and not by (email or non-zero firstaccess) => conflict, return false
          • 2D - else => user needs to be created, return true

          What happens if there is a match by email but not by username?

          Show
          Larry Lippke added a comment - Regarding the following logic: if restoring from another site backup: 2A - If match by username and mnethost and email or non-zero firstaccess) => ok, return target user 2B - Note: we cannot handle "deleted" situations here as far as username gets modified and id cannot be used here 2C - If match by username and mnethost and not by (email or non-zero firstaccess) => conflict, return false 2D - else => user needs to be created, return true What happens if there is a match by email but not by username?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          match by email but not by username = no match & no conflict = 2D = user needs to be created.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - match by email but not by username = no match & no conflict = 2D = user needs to be created. Ciao

            People

            • Votes:
              20 Vote for this issue
              Watchers:
              20 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: