JillElaine / jquery-idleTimeout

Idle activity timeout and logout redirect for jQuery for multiple windows & tabs
Other
73 stars 79 forks source link

Repeated calls to redirect url even if the user loggedout #26

Open gogulsekar opened 8 years ago

gogulsekar commented 8 years ago

pre-requisite: the logout procedure involves redirection of the site to a third party authentication url, inorder to clear session on the third party end(federation authentication).

bug description when the user clicks the logout button, if the redirect url(MVC action) takes long time to complete the request, the plugin is posting another request which makes the previous request to cancel.

gogulsekar commented 8 years ago

i have fixed this issue, soon create a pull request to the author

JillElaine commented 8 years ago

Could you please post your fix so that we may review it? Thank you.

gogulsekar commented 8 years ago

jquery-idleTimeout.zip i just start using a store data "userLoggedOut". when the user logout, this value will be set to true. this value is used to stop setting timer if user logout.

JillElaine commented 8 years ago

Thank you for the information. Before changes to code, we need to test how the changes affect the function of the script. Please test your site with https://github.com/JillElaine/jquery-idleTimeout/blob/master/jquery-idleTimeout-for-testing.js (unaltered) and watch the Console Log? Then add your changes and appropriate 'console.log' phrases to the 'testing' script and test again? I use the add-on, Firebug, on Mozilla Firefox to view the console log. It's a good way to understand where the issue occurs and why.

gogulsekar commented 8 years ago

when signout url(mvc action) is being loaded/called when user logout, if any third party redirection happen, still the browser is rendering the same view where user clicked the signout/logout option. here if user moves the cursor on the body of html document, the timer(startIdleTimer ) is being reset without checking the condition whether user is loggedout or not, which is causing a http call(duplicate) for the url defined for signout option(via checkIdleTimeoutLoop).

JillElaine commented 8 years ago

Okay. It is a good change to check if the user has already timed out and is in the process of being logged out, however slowly, before restarting the idletimer. But why add another 'store' variable , 'userLoggedOut', when existing 'store' variable, 'idleTimerLoggedOut', could be used for this purpose?

activityDetector = function () {

      $('body').on(currentConfig.activityEvents, function () {

        if ((!currentConfig.enableDialog || (currentConfig.enableDialog && isDialogOpen() !== true)) && store.get('idleTimerLoggedOut') == false) {
          startIdleTimer();
        }
      });
    };
JillElaine commented 8 years ago

I believe this is the same or similar issue: https://github.com/JillElaine/jquery-idleTimeout/issues/21

gogulsekar commented 8 years ago

Yes, to my knowledge we can use the exisiting store variable "idleTimerLoggedOut", but still in order to distinguish between those two seperate events(user logout & timeout due to idleness) i have introduced another variable "userLoggedOut" for separation of concern. If you think that "idleTimerLoggedOut" is good to go, then lets go with that.

rhetherington commented 8 years ago

Hi JillElaine / gogulsekar, my JavaScript isn't likely a good as yours, however I am good with logic and keen to get this bug fixed as it's generating thousands of errors in my systems at the moment. I am happy to help with browser testing etc, if a patch / fix is available. I'm also relatively ignorant to how github works so If I can help and you would like me to get involved I'd be grateful for a nudge as to what you need me to do.

If a patch hasn't yet been created, I'm happy to look at this also. Please point me in the right direction and I'll do some testing/tweaking also.

Hope this helps.

Richard Hetherington

gogulsekar commented 8 years ago

Hi Richard, Please check the zip file one which i have attached on my Feb 25th comment. That file has the fix, am not sure if the fix is merged to main or not.Please have a try with that one, that should work.

Regards, Gokulnathan

rhetherington commented 8 years ago

Hi gogulsekar, I have reviewed the attached revised file on your Feb 25th comment and subsequently I have tested it against Firefox, Chrome and IE and everything checks out as far as I can see. It fixes both the NaN Dialog countdown bug and Redirection loop bugs.

I guess if everyone is also in agreement we would need to get the iframe and testing version updated and merged to the master branch??

Richard Hetherington

JillElaine commented 8 years ago

Hello gogulsekar & rhetherington. Thank you for verifying gogulsekar's fix. When a user triggers the logoutUser function, there are two functions that need to be 'stopped': the idleTimer loop & the activityDetector.

To stop the idleTimer and use DRY coding principle, add the stopIdleTimer() call to the logoutUser function, rather than calling it repeatedly after every call to logoutUser. See code snippet below.


    //----------- LOGOUT USER FUNCTION --------------//
    logoutUser = function () {
      stopIdleTimer(); 
      store.set('idleTimerLoggedOut', true);

      if (currentConfig.sessionKeepAliveTimer) {
        stopKeepSessionAlive();
      }

      if (currentConfig.customCallback) {
        currentConfig.customCallback();
      }

      if (currentConfig.redirectUrl) {
        window.location.href = currentConfig.redirectUrl;
      }
    };

For the activityDetector, see code snippet below.

    //----------- ACTIVITY DETECTION FUNCTION --------------//
    activityDetector = function () {
      if ((store.get('idleTimerLoggedOut') === false) && (!currentConfig.enableDialog || (currentConfig.enableDialog && isDialogOpen() !== true))) {
        $('body').on(currentConfig.activityEvents, function () {  
          startIdleTimer();  
        });
      }
    };

These two code changes need to be added to jquery-idleTimeout.js, jquery-idleTimeout-iframes.js, and jquery-idleTimeout-for-testing.js. The 'minified' scripts can be created with http://jscompress.com/ once the code changes are tested & verified.

I am not able to modify my plugin right now. Please fork and test the code changes, rhetherington, if you can. If all tests well, I'll reference your fork in the related issues.

rhetherington commented 8 years ago

Hi JillElaine, I have forked your repository, modified all of the files that I believe needed modified, incremented the version to 1.0.11 and added entry to Change Log.

I only implemented the logoutUser() stopIdleTimer(); line as the secondary activityDetector() change stopped the dialog from being interacted with. Not sure why you switched the code around in this one. The plugin works well for me with only your one line change as per above. I ran the test file with one line update and all checked out as far as I could see.

All 5 .js files updated including 2 minified files.

Many thanks for your help with this.

Do I need to issue a PULL request to get this change committed to yours?

Regards,

Richard Hetherington

JillElaine commented 8 years ago

Awesome, rhetherington. No pull request needed at this time. Users, please see rhetherington's fork of this plugin for a fix of this issue: https://github.com/rhetherington/jquery-idleTimeout

kevinpohlmeier commented 6 years ago

The suggested change to activityDetector did not solve my problem, but moving the logged out check inside the event handler did.

Since mousemove is one of the activity events, I would reproduce the issue by clicking the logout button and then moving my mouse around a bunch. It kept firing the mousemove event, which triggered the startIdleTimer, which fired a web request to the logout url. That resulted in a LOT of calls to the logout url.

The following code skips that startIdleTimer if the logout method has already been run.

    //----------- ACTIVITY DETECTION FUNCTION --------------//
    activityDetector = function () {
        $('body').on(currentConfig.activityEvents, function () {
            if ((store.get('idleTimerLoggedOut') === false) && (!currentConfig.enableDialog || (currentConfig.enableDialog && isDialogOpen() !== true))) {
                startIdleTimer();
            }
        });
    };
qxx commented 2 years ago

In addition to the IdleTimer, DialogTimer also needs to be stopped.

//----------- LOGOUT USER FUNCTION --------------//
logoutUser = function () {
  stopIdleTimer(); // the fix added by @kevinpohlmeier 
  stopDiaglogTimer(); // there's one more timer to stop

  store.set('idleTimerLoggedOut', true);

  if (currentConfig.sessionKeepAliveTimer) {
    stopKeepSessionAlive();
  }

  if (currentConfig.customCallback) {
    currentConfig.customCallback();
  }

  if (currentConfig.redirectUrl) {
    window.location.href = currentConfig.redirectUrl;
  }
};
JillElaine commented 2 years ago

Thank you for your code. I am unable to maintain this script anymore. I would be happy if someone wanted to take ownership of the script on github to merge, test, and upgrade this useful script so that works with modern browsers. If you are interested in taking ownership of the script, please contact me via github. I made the code easy to understand and modify by using clear naming conventions and lots of comments!