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

      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.

        Gliffy Diagrams

        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 Skoda added a comment -

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

            Show
            Petr Skoda 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: