Moodle
  1. Moodle
  2. MDL-30370 Meta: Oracle SQL issues
  3. MDL-30051

notify_login_failures in /lib/moodlelib.php crashes when executing from cron.php

    Details

    • Database:
      Oracle
    • Testing Instructions:
      Hide

      This needs testing on all DBs. Note that if you set $CFG->divertallemailsto = 'youremailaddress@foo.com'; can be used to test emails

      Note: need oracle db for testing

      1. Set notifyloginfailures to something other than nobody (in Admin > Security > Notifications)
      2. Attempt to login with incorrect password more than 10 (default) times
      3. run cron (moodle/admin/cron.php)
      4. It should run without any errors
      5. Ensure that login filaures are mailed to the user specified in notifyloginfailures
      Show
      This needs testing on all DBs. Note that if you set $CFG->divertallemailsto = 'youremailaddress@foo.com'; can be used to test emails Note: need oracle db for testing Set notifyloginfailures to something other than nobody (in Admin > Security > Notifications) Attempt to login with incorrect password more than 10 (default) times run cron (moodle/admin/cron.php) It should run without any errors Ensure that login filaures are mailed to the user specified in notifyloginfailures
    • Workaround:
      Hide

      In Oracle there are (at least) two posible fixes of this issue:

      • Change the ORDER BY, and use the column number instead of the column name:
        ...
            $sql = "SELECT l.*, u.firstname, u.lastname
                      FROM {log} l
                      JOIN {cache_flags} cf ON l.ip = cf.name
                 LEFT JOIN {user} u         ON l.userid = u.id
                     WHERE l.module = 'login' AND l.action = 'error'
                           AND l.time > ?
                           AND cf.flagtype = 'login_failure_by_ip'
                UNION ALL
                    SELECT l.*, u.firstname, u.lastname
                      FROM {log} l
                      JOIN {cache_flags} cf ON l.info = cf.name
                 LEFT JOIN {user} u         ON l.userid = u.id
                     WHERE l.module = 'login' AND l.action = 'error'
                           AND l.time > ?
                           AND cf.flagtype = 'login_failure_by_info'
                  ORDER BY 2 DESC";
        
      • Group the query inside a new select:
        ...
            $sql = "SELECT * FROM (SELECT l.*, u.firstname, u.lastname
                                     FROM m_log l
                                     JOIN m_cache_flags cf ON l.ip = cf.name
                                LEFT JOIN m_user u         ON l.userid = u.id
                                    WHERE l.module = 'login' AND l.action = 'error'
                                          AND l.time > ?
                                          AND cf.flagtype = 'login_failure_by_ip'
                               UNION ALL
                                   SELECT l.*, u.firstname, u.lastname
                                     FROM m_log l
                                     JOIN m_cache_flags cf ON l.info = cf.name
                                LEFT JOIN m_user u         ON l.userid = u.id
                                    WHERE l.module = 'login' AND l.action = 'error'
                                          AND l.time > ?
                                          AND cf.flagtype = 'login_failure_by_info') t
                                 ORDER BY t.time DESC";
        
      Show
      In Oracle there are (at least) two posible fixes of this issue: Change the ORDER BY, and use the column number instead of the column name: ... $sql = "SELECT l.*, u.firstname, u.lastname FROM {log} l JOIN {cache_flags} cf ON l.ip = cf.name LEFT JOIN {user} u ON l.userid = u.id WHERE l.module = 'login' AND l.action = 'error' AND l.time > ? AND cf.flagtype = 'login_failure_by_ip' UNION ALL SELECT l.*, u.firstname, u.lastname FROM {log} l JOIN {cache_flags} cf ON l.info = cf.name LEFT JOIN {user} u ON l.userid = u.id WHERE l.module = 'login' AND l.action = 'error' AND l.time > ? AND cf.flagtype = 'login_failure_by_info' ORDER BY 2 DESC"; Group the query inside a new select: ... $sql = "SELECT * FROM (SELECT l.*, u.firstname, u.lastname FROM m_log l JOIN m_cache_flags cf ON l.ip = cf.name LEFT JOIN m_user u ON l.userid = u.id WHERE l.module = 'login' AND l.action = 'error' AND l.time > ? AND cf.flagtype = 'login_failure_by_ip' UNION ALL SELECT l.*, u.firstname, u.lastname FROM m_log l JOIN m_cache_flags cf ON l.info = cf.name LEFT JOIN m_user u ON l.userid = u.id WHERE l.module = 'login' AND l.action = 'error' AND l.time > ? AND cf.flagtype = 'login_failure_by_info') t ORDER BY t.time DESC";
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-30051
    • Rank:
      24681

      Description

      The following sql query used in /lib/moodlelib.php is not valid (in Oracle):

      ...
          $sql = "SELECT l.*, u.firstname, u.lastname
                    FROM {log} l
                    JOIN {cache_flags} cf ON l.ip = cf.name
               LEFT JOIN {user} u         ON l.userid = u.id
                   WHERE l.module = 'login' AND l.action = 'error'
                         AND l.time > ?
                         AND cf.flagtype = 'login_failure_by_ip'
              UNION ALL
                  SELECT l.*, u.firstname, u.lastname
                    FROM {log} l
                    JOIN {cache_flags} cf ON l.info = cf.name
               LEFT JOIN {user} u         ON l.userid = u.id
                   WHERE l.module = 'login' AND l.action = 'error'
                         AND l.time > ?
                         AND cf.flagtype = 'login_failure_by_info'
                ORDER BY time DESC";
      

      It crashes with ORA-00904

        * line 394 of \lib\dml\moodle_database.php: dml_read_exception thrown
        * line 268 of \lib\dml\oci_native_moodle_database.php: call to moodle_database->query_end()
        * line 1059 of \lib\dml\oci_native_moodle_database.php: call to oci_native_moodle_database->query_end()
        * line 8089 of \lib\moodlelib.php: call to oci_native_moodle_database->get_recordset_sql()
        * line 279 of \lib\cronlib.php: call to notify_login_failures()
        * line 61 of \admin\cli\cron.php: call to cron_run()
      

      The reason of this error is that column name use is not allowed in the ORDER BY when using UNION

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that and providing a solution.

          Show
          Michael de Raadt added a comment - Thanks for reporting that and providing a solution.
          Hide
          Rajesh Taneja added a comment - - edited

          increasing the priority and taking this in sprint

          Show
          Rajesh Taneja added a comment - - edited increasing the priority and taking this in sprint
          Hide
          Rajesh Taneja added a comment -

          Thanks Iñigo for reporting this problem and suggesting solution.
          Have created a patch with select wrapper, (as this is cross db compatible), also requested Eloy (db expert) for his second opinion. Will push it for peer-review once I get some update from Eloy.

          Show
          Rajesh Taneja added a comment - Thanks Iñigo for reporting this problem and suggesting solution. Have created a patch with select wrapper, (as this is cross db compatible), also requested Eloy (db expert) for his second opinion. Will push it for peer-review once I get some update from Eloy.
          Hide
          Iñigo Zendegi added a comment -

          FYI: I've tried that patch in Moodle 2.1.5 with Oracle and worked fine.

          Show
          Iñigo Zendegi added a comment - FYI: I've tried that patch in Moodle 2.1.5 with Oracle and worked fine.
          Hide
          Rajesh Taneja added a comment -

          Thanks Iñigo

          Show
          Rajesh Taneja added a comment - Thanks Iñigo
          Hide
          Sam Hemelryk added a comment -

          Changes look fine to me but I am definitely not the DB expert around here. I certainly think it is worth waiting for Eloy's opinion.

          Eloy I've added you as a watcher to this issue, a small patch Raj is working on that we'd like your opinion on.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Changes look fine to me but I am definitely not the DB expert around here. I certainly think it is worth waiting for Eloy's opinion. Eloy I've added you as a watcher to this issue, a small patch Raj is working on that we'd like your opinion on. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,
          Eloy has already looked at this and he checked sql on all db's.
          So, I think it's good to go for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Sam, Eloy has already looked at this and he checked sql on all db's. So, I think it's good to go for integration review.
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated (yesterday) with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated (yesterday) with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          This has been integrated now, thanks.

          Note I have expanded the testing instructions to include testing of whether notify login failures is working (not just that the query doesn't crash).

          NOTE TO TESTER! My instructions probably aren't enough to trigger the notification of login failures (I suspect it has a threshold before sending notifications etc).

          So I ask you to explore this a little when testing. Alternative if Raj you can expand these testing instructions then that would be good.

          Show
          Dan Poltawski added a comment - This has been integrated now, thanks. Note I have expanded the testing instructions to include testing of whether notify login failures is working (not just that the query doesn't crash). NOTE TO TESTER! My instructions probably aren't enough to trigger the notification of login failures (I suspect it has a threshold before sending notifications etc). So I ask you to explore this a little when testing. Alternative if Raj you can expand these testing instructions then that would be good.
          Hide
          Michael de Raadt added a comment -

          Test result Success.

          Tested in Oracle, MySQL, PostgreSQL and SQL Server

          Show
          Michael de Raadt added a comment - Test result Success. Tested in Oracle, MySQL, PostgreSQL and SQL Server
          Hide
          Dan Poltawski added a comment -

          Jolly good show!

          Your changes have made it into the Moodle release - its time to celebrate! I suggest a hot cup of English tea (with milk, no sugar) or a hoppy English ale.

          Tally-ho!

          Show
          Dan Poltawski added a comment - Jolly good show! Your changes have made it into the Moodle release - its time to celebrate! I suggest a hot cup of English tea (with milk, no sugar) or a hoppy English ale. Tally-ho!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: