Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-19628

create_user_record requires ipaddress, does not work from scripts

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.5
    • Fix Version/s: 1.9.10, 2.0
    • Component/s: Libraries
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE

      Description

      If you call create_user_record from a script or anywhere where $_SERVER['REMOTE_ADDR'] and such are not defined. It will try to insert a user record where lastip is null. This doesn't work because the user table has a non-null constraint on lastip. As a result, things like ldap authentication are broken (see last few comments on MDL-17682).

      Suggestions:

      • Remove non-null constraint. This makes the most sense to me because a user that has just be created by a script shouldn't have a lastip.
      • default to '127.0.0.1' in case getremoteaddr() returns null

        Gliffy Diagrams

        1. mdl-19628-18.diff
          0.5 kB
          Iñaki Arenaza
        2. mdl-19628-19.diff
          0.5 kB
          Iñaki Arenaza
        3. mdl-19628-20.diff
          0.5 kB
          Iñaki Arenaza

          Issue Links

            Activity

            Hide
            trick Alan Trick added a comment -

            This is also an issue with create_course and enrolment modules.

            Show
            trick Alan Trick added a comment - This is also an issue with create_course and enrolment modules.
            Hide
            iarenaza Iñaki Arenaza added a comment -

            I think a better/safer fix is to change create_user_record() to check for a null return value from getremoteaddr and change it to an empty string, which is a valid database value. After all, the only use of getremoteaddr that has trouble with a null return value is create_user_record.

            Something like the attached patch (for 1.8.x, 1.9.x and HEAD) could do it.

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - I think a better/safer fix is to change create_user_record() to check for a null return value from getremoteaddr and change it to an empty string, which is a valid database value. After all, the only use of getremoteaddr that has trouble with a null return value is create_user_record. Something like the attached patch (for 1.8.x, 1.9.x and HEAD) could do it. Saludos. Iñaki.
            Hide
            trick Alan Trick added a comment -

            Does any of the Moodle code expect it to be a well-formed IPv4 address?

            Show
            trick Alan Trick added a comment - Does any of the Moodle code expect it to be a well-formed IPv4 address?
            Hide
            iarenaza Iñaki Arenaza added a comment -

            I don't think so (but I may be wrong).

            Saludos. Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - I don't think so (but I may be wrong). Saludos. Iñaki.
            Hide
            pholden Paul Holden added a comment -

            The problem in this bug is related to the one I've linked in that the function getremoteaddr() doesn't return a value when called from the CLI.

            Show
            pholden Paul Holden added a comment - The problem in this bug is related to the one I've linked in that the function getremoteaddr() doesn't return a value when called from the CLI.
            Hide
            dougiamas Martin Dougiamas added a comment -

            I'm changing getremoteaddr() to return "0.0.0.0" instead of null by default, and checking all the calls (it was a bit of a mess, quite a few things would get upset with the null). 127.0.0.1 is not really safe, because in some checks that use this function the code will assume the client is the local machine. The default can be changed via a new parameter in case you really want a null.

            eg getremoteaddr(null)

            Show
            dougiamas Martin Dougiamas added a comment - I'm changing getremoteaddr() to return "0.0.0.0" instead of null by default, and checking all the calls (it was a bit of a mess, quite a few things would get upset with the null). 127.0.0.1 is not really safe, because in some checks that use this function the code will assume the client is the local machine. The default can be changed via a new parameter in case you really want a null. eg getremoteaddr(null)

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Oct/10