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

notify_login_failures executed even when no recipients

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.6, 2.2.3, 2.3
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide
      • Note: this requires, for sure testing under 21_STABLE. Patch is different.
      1. Login as admin
      2. Set notifyloginfailures to nobody
      3. Run cron and make sure 'Notified login failures' should not appear in cron log
      4. Set notifyloginfailures to "Admin User"
      5. Run cron and 'Notified login failures' should appear in cron log
      6. Set notifyloginfailures to "Admin User"
      7. Run cron and make sure 'Notified login failures' should not appear in cron log
      8. Run cron after 1 hour and 'Notified login failures' should appear in cron log
      Show
      Note: this requires, for sure testing under 21_STABLE. Patch is different. Login as admin Set notifyloginfailures to nobody Run cron and make sure 'Notified login failures' should not appear in cron log Set notifyloginfailures to "Admin User" Run cron and 'Notified login failures' should appear in cron log Set notifyloginfailures to "Admin User" Run cron and make sure 'Notified login failures' should not appear in cron log Run cron after 1 hour and 'Notified login failures' should appear in cron log
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:

      Description

      cron_run() just checks for $CFG->notifyloginfailures empty, but it is set to '$@NONE@$', so notify_login_failures() is triggered every time, even when no notifications are sent.

      If you look in notify_login_failures(), in this case (it is called, but there are no recipients) all the work is done (which on our system is very very non-trivial), but absolutely no updates/changes/anything are done

        Gliffy Diagrams

          Activity

          Hide
          emerrill Eric Merrill added a comment -

          Looking at the code, the same condition happens if when it last happened under an hour ago, it runs, but doesnt update/do anything. Im making a patch that looks at all of these things.

          Show
          emerrill Eric Merrill added a comment - Looking at the code, the same condition happens if when it last happened under an hour ago, it runs, but doesnt update/do anything. Im making a patch that looks at all of these things.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks Eric, for spot-on patch
          Pushing if for integration review.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Eric, for spot-on patch Pushing if for integration review.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated 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
          stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated 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
          poltawski Dan Poltawski added a comment -

          The main moodle.git repository has just been updated 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
          poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated 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
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Nice one, integrated (21, 22 and master).

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Nice one, integrated (21, 22 and master).
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Eric, I've added one extra commit to fix one notice in all branches... also... could you tidy a bit testing instructions... I've added one Note there... but perhaps it would be better to have them written in some more "deterministic" way, to be 100% sure testing goes ok.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Eric, I've added one extra commit to fix one notice in all branches... also... could you tidy a bit testing instructions... I've added one Note there... but perhaps it would be better to have them written in some more "deterministic" way, to be 100% sure testing goes ok.
          Hide
          fred Frédéric Massart added a comment -

          My apologies in advance if this is not supposed to be failed, but what happens is that after step 6, if I refresh on and on the cron, the sentence "Notified login failures" keeps on appearing every time, there is not need to wait for an hour.

          I think that is because the 'lastnotifyfailure' config value is not updated when there is nothing to notify about. I assume that if the function returns true, we should also update the 'lastnotifyfailure'.

          Everything else but this worked fine.

          Show
          fred Frédéric Massart added a comment - My apologies in advance if this is not supposed to be failed, but what happens is that after step 6, if I refresh on and on the cron, the sentence "Notified login failures" keeps on appearing every time, there is not need to wait for an hour. I think that is because the 'lastnotifyfailure' config value is not updated when there is nothing to notify about. I assume that if the function returns true, we should also update the 'lastnotifyfailure'. Everything else but this worked fine.
          Hide
          emerrill Eric Merrill added a comment -

          Hrm, I get what you are saying about the notification with lastnotifyfailure. I've checked the code base, and it is only used in that function, so I think it is safe to slightly change it's meaning to basically be the last time we checked for bad logins, not the last time they occurred.

          I would fix it by moving:
          // Update lastnotifyfailure with current time
          set_config('lastnotifyfailure', time());

          from it's current location to just above "// Finally, delete all the temp records we have created in cache_flags"

          I'm not sure if I should add this to the branches I have setup, because they are already integrated and Eloy had a commit over it, or if it would just be easier for one of you to directly do it. If you want me to, just let me know and I'll update the branches.

          Show
          emerrill Eric Merrill added a comment - Hrm, I get what you are saying about the notification with lastnotifyfailure. I've checked the code base, and it is only used in that function, so I think it is safe to slightly change it's meaning to basically be the last time we checked for bad logins, not the last time they occurred. I would fix it by moving: // Update lastnotifyfailure with current time set_config('lastnotifyfailure', time()); from it's current location to just above "// Finally, delete all the temp records we have created in cache_flags" I'm not sure if I should add this to the branches I have setup, because they are already integrated and Eloy had a commit over it, or if it would just be easier for one of you to directly do it. If you want me to, just let me know and I'll update the branches.
          Hide
          emerrill Eric Merrill added a comment -

          I've updated the instructions so it should be more deterministic - particularly if we integrate the additional fix just described.

          Show
          emerrill Eric Merrill added a comment - I've updated the instructions so it should be more deterministic - particularly if we integrate the additional fix just described.
          Hide
          fred Frédéric Massart added a comment -

          I would move notifylastfailure where you suggested it as well.
          Regarding the patch/branch update, I'll let the integrator/reviewer answer.

          Show
          fred Frédéric Massart added a comment - I would move notifylastfailure where you suggested it as well. Regarding the patch/branch update, I'll let the integrator/reviewer answer.
          Hide
          emerrill Eric Merrill added a comment -

          I've created new commits on top of integration that include this additional fix.

          They are:
          MDL-32985_integration
          https://github.com/merrill-oakland/moodle/commit/54c2756ca21e74ad89a5c922e47287477f3add55
          MDL-32985_integration_22
          https://github.com/merrill-oakland/moodle/commit/4e6fbf663cef70f8d9e7985fc8f79eee1cda9e88
          MDL-32985_integration_21
          https://github.com/merrill-oakland/moodle/commit/bfe1d304442450165cae6baf579f686e48c703e9

          I'm not sure if I should replace the branches listed in the ticket meta-data, since those were still the primary branches, so I'm just putting these right here for now.

          Can someone mark this back for integration?

          Thanks

          Show
          emerrill Eric Merrill added a comment - I've created new commits on top of integration that include this additional fix. They are: MDL-32985 _integration https://github.com/merrill-oakland/moodle/commit/54c2756ca21e74ad89a5c922e47287477f3add55 MDL-32985 _integration_22 https://github.com/merrill-oakland/moodle/commit/4e6fbf663cef70f8d9e7985fc8f79eee1cda9e88 MDL-32985 _integration_21 https://github.com/merrill-oakland/moodle/commit/bfe1d304442450165cae6baf579f686e48c703e9 I'm not sure if I should replace the branches listed in the ticket meta-data, since those were still the primary branches, so I'm just putting these right here for now. Can someone mark this back for integration? Thanks
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          New commit integrated, ready for a new testing round. Thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - New commit integrated, ready for a new testing round. Thanks!
          Hide
          fred Frédéric Massart added a comment -

          Test passed on 2.1, 2.2 and master. But to properly test 2.1 you have to refresh the page more than once as notify_login_failures() only gets executed once out of 5 times (ish).

          Also, I would add to the testing instructions that we should delete the config key 'lastnotifyfailure' in mdl_config table at the beginning of the test.

          Show
          fred Frédéric Massart added a comment - Test passed on 2.1, 2.2 and master. But to properly test 2.1 you have to refresh the page more than once as notify_login_failures() only gets executed once out of 5 times (ish). Also, I would add to the testing instructions that we should delete the config key 'lastnotifyfailure' in mdl_config table at the beginning of the test.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

          Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

          Many thanks for your collaboration!

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                9/Jul/12