Moodle
  1. Moodle
  2. MDL-32985

notify_login_failures executed even when no recipients

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      40158

      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

        Activity

        Hide
        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
        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
        Rajesh Taneja added a comment -

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

        Show
        Rajesh Taneja added a comment - Thanks Eric, for spot-on patch Pushing if for integration review.
        Hide
        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
        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
        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
        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
        Eloy Lafuente (stronk7) added a comment -

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

        Show
        Eloy Lafuente (stronk7) added a comment - Nice one, integrated (21, 22 and master).
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Eloy Lafuente (stronk7) added a comment -

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

        Show
        Eloy Lafuente (stronk7) added a comment - New commit integrated, ready for a new testing round. Thanks!
        Hide
        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
        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
        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
        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: