- Issue created by @joseph.olstad
- Merge request !65Issue #3469258 by joseph.olstad: Multiple dialogs open / have to click multiple times β (Open) created by joseph.olstad
- Issue was unassigned.
- Status changed to Needs review
4 months ago 5:58pm 20 August 2024 - πΊπΈUnited States scott_earnest
Upon testing I am finding that the dialog only appears once, and clicking "yes" will make it go away as fixed by the patch.
Unfortunately I am seeing a deprecation. I find that if you don't click anything, nothing happens. The expected behavior would be to auto-logout as defined by the "Timeout padding" setting.
Moving back to "needs work". I will try to help with a solve thank you for the patch.
- Status changed to Needs work
4 months ago 10:55pm 20 August 2024 - π¨π¦Canada joseph.olstad
@scott_earnest
I just re-tested patch #4 and the MR
by clicking nothing, I was logged out.
- Status changed to Needs review
4 months ago 5:13pm 21 August 2024 - π¨π¦Canada joseph.olstad
There's no code additional change to make, the logout behavior is optional, please set the use alt logout method option to change the behavior as desired.
It's in the autologout settings.
- Assigned to joseph.olstad
- Status changed to Active
4 months ago 2:26pm 22 August 2024 - π¨π¦Canada joseph.olstad
I'm actually still reviewing this and may retool the patch using something similar to what I mentioned in comment #8
- π¨π¦Canada joseph.olstad
Appears that clicking "No" does the same thing as clicking "Ok".
However closing the dialog with the (X) works as expected.
also, the dialog should only last 60 seconds
- Issue was unassigned.
- π¨π¦Canada joseph.olstad
MR is now functional however the automated tests need fixing and don't jive with the functionality actually working this way.
- π¨π¦Canada joseph.olstad
Ok, so patch 14 mostly works, however the dialog does not automatically vanish after the grace period has elapsed.
- Assigned to joseph.olstad
- Issue was unassigned.
- Status changed to Needs review
4 months ago 3:50pm 27 August 2024 - πΊπΈUnited States dan.d
@joseph.olstad, about a week ago I started looking into this issue reported @ https://www.drupal.org/project/autologout/issues/3468020 π Multiple clicks to extend session needed Closed: duplicate . Since then, you've been leading this conversation here. I want to contribute, if you don't mind, with setting a bit of context based on my observations.
Clean Installation
First of all, I've decided to troubleshoot this issue in an "untainted" environment, so I started off with a clean installation of Drupal 10.3.x. Then I installed and enabled Autologout module. I set the timeout to the lowest-allowed number (60 seconds) and tested the feature on a single page (no page reloads to re-fresh the session). This is a good case when you have a micro frontend (or a collection of such) running on a single page and you expect your visitors/users to spend some time there.
The Hidden Element
During the initialization the script adds a hidden element in the DOM. Which needs to be added once (the reason for its creation is irrelevant at this point). However, I've noticed that there are multiple instances of the element being added.
Nesting of the Hidden Element
There seems to be an issue with the AJAX handler, as well as a redundant creation of simultaneously running background timers. It's possible this is where the actual issue originates. If you confirm the continuation of a session via the auto logout prompt (modal), you will notice that the number of nested hidden elements increases with each interaction (with the modal).
The Growing Number of Modal Overlays
At the same time, you will notice that each consecutive aoutologout prompt will trigger a greater number of modals (roughly equal to the number of nested hidden elements). At some point the number went over 200 modals stacked on top of each other. Which, as you can imagine, will lead to issues with rendering and, as a result, session expiration.
Further Actions:
I believe we will have to refactor and modernize the JavaScript implementation for this module, concentrating on the issues related to AJAX handlers and the background timers racing against each other. I will create a fork for this project and will keep on updating this thread with my further discoveries.
- π¨π¦Canada joseph.olstad
Hi @dan.d , sounds like you tested without patching which is consistent with what we also saw without patching. Wondering, have you tried patch #17 or the latest merge request patch?
With this patch we no longer get multiple dialogs and our language prefix issue is resolved. This patch should work regardless of if you're using a language prefix or even multisite should also work using this patch. I explained it in comment #17.
- Status changed to Needs work
4 months ago 3:35pm 29 August 2024 - πΊπΈUnited States dan.d
Hi @joseph.olstad! I've applied the patch #17 π Multiple dialogs open / have to click multiple times Active and I'm observing the following behavior, similar to what was described in comment #14 π Multiple dialogs open / have to click multiple times Active .
Context
Installed D10.3.x. Installed and enabled autologout module. Applied patch #17. Set Timeout value @60 seconds and Timeout padding @20 seconds.
Observed inconsistent behavior
Upon the expiration of the allowed inactivity period (~1 min), the prompt modal displays with the options to keep the session going (Yes) or to immediately log out (No).
1. The expected behavior is to auto logout the user if there's no interaction with the modal. Unfortunately, the modal stays open and the user session can be extended by clicking on the confirmation button ("Yes" default label) at any time beyond the timeout padding time.
2. There was a case when the user got logged out automatically without an interaction with the modal within the proper timeframes. But I couldn't replicate this behavior in my consecutive attempts. This will require further investigation.
- π¨π¦Canada joseph.olstad
@dan.d , when I was testing this firefox would not let go of the cached autologout.js for some reason. Can you please re-try in a different web browser completely (other than the one you normally use?) ?.
It could be cache related. This functionality is working for us.
With that said, I have other patches.
"drupal/autologout": { "3308456 - Secure attribute for cookie.": "https://www.drupal.org/files/issues/2024-08-14/3308456-30.patch", "3456716 - TypeError LoggerChannelInterface.": "https://git.drupalcode.org/project/autologout/-/merge_requests/62.patch", "3469258 - Multiple dialogs open.": "https://www.drupal.org/files/issues/2024-08-27/3469258-17.patch", "3372010 - Undefined array key in AutologoutSubscriber->onRequest().": "https://www.drupal.org/files/issues/2024-06-18/autologout_3372010-29.patch" },
You might want to re-try using all of these patches.
- Status changed to Needs review
4 months ago 9:55pm 29 August 2024 - π¨π¦Canada joseph.olstad
If it's configuration related, you could compare with our configuration:
enabled: true timeout: 60 max_timeout: 172800 padding: 30 logout_regardless_of_activity: false no_individual_logout_threshold: true role_logout: false role_logout_max: false redirect_url: / include_destination: true no_dialog: false message: 'We are about to log you out for inactivity. If we do, you will lose any unsaved work. Do you need more time?' inactivity_message: 'You have been logged out due to inactivity.' inactivity_message_type: status modal_width: 450 enforce_admin: true jstimer_format: '%hours%:%mins%:%secs%' jstimer_js_load_option: false use_alt_logout_method: false use_watchdog: true dialog_title: 'Autologout Alert' disable_buttons: false yes_button: '' no_button: '' whitelisted_ip_addresses: '' cookie_secure: true cookie_httponly: false cookie_samesite: Lax cookie_lifetime: 31536000
- π¨π¦Canada joseph.olstad
I've tested the latest patch #23 with the improvements from 3395581 merged in and:
Use case:
Open two browser windows, same session
- Keep activity going on the right side window (manually) on my right screen, do not touch left screen.
- As long as the activity is performed before the 30 minute timeout, the time dialog does not show up on either tab, once it's openned however, it doesn't close other dialogs that may be opened if the session timeout has elapsed there could be two dialogs openned at the same time but each doesn't know about the other.
- The good part is, that dialog won't show up as long as there is activity in "one" of the open tabs/windows before 30 minutes (session limit).
Summary:
I think this is good enough for now, as a followup issue we can make a new ticket to investigate closing other possibly open dialogs by pressing OK on one of them.
- π΅πΉPortugal HLopes
I reworked the js code to use a shared worker, so we can keep the dialog actions sync'ed across tabs.
Missing a lot of functionality still, just experimenting.
- π¨π¦Canada joseph.olstad
Thanks for that and it does look promising however as I explained verbosely above multiple times
$front_url = Url::fromRoute('<front>')->toString();
Which is passed into js
drupalSettings.autologout.front_url
is more reliable than
drupalSettings.path.baseUrl
What this does is make this solution compatible with the language_selection_page module. Normally admin/* routes are commonly configured in the language_selection_page "whitelist" however this module uses non-admin routes. My changes ensure that this module works for everyone.
If you have a look at my MR you'll see this.
It would be great also rather than blast the autologout.js and replace it with autologout2.js in a patch file if you made a new MR based off of my MR and continue the work forward, keep two branches going so that we can test your new approach vs the current approach and work through to make it stable.
I've tested the latest version of this branch in a vanilla d10 with no other autologout patches, everything appears to be working correctly. Multiple dialog issue is fixed.
- πΊπΈUnited States dan.d
I will have to break my comment up into 3 parts since I'm responding to multiple posts by multiple users.
@dmrupp,
1. Which branch did you test with? The opening description of the issues states: This affects 2.x AND also 8.x-1.x.
2. Were you able to repeat the session refresh without a page reload (using AJAX) multiple times?@hlopes,
I like your approach! But it seems like it goes well beyond the scope initially described by Joseph. This module is due for some good refactoring, perhaps we can start a completely new branch for it. I'd be interested in contributing to it.@joseph.olstad,
to keep things to the point and to provide the most generic and non-intrusive solution to the issue, I'm suggesting some JS refactoring with a bit of a code clean-up. If you want to take a look and see if this is something we can work with, I'm attaching the patch.In this patch, I'm addressing the following issues:
1. Creation of poorly-managed background timers (which led to an increased number of background processes)
2. Changing the handling of AJAX responses on success (which led to the stacking responses and a growing number of pop-ups with each consecutive AJAX trigger)
3. Check attachment of the behavior to make sure that we initialize it once (which led to the creation of multiple DOM elements for the timer container)---
I will also update my branch with this and will create an MR, once it passes the validation locally. In the meantime, I will try testing it in my projects. - π¨π¦Canada joseph.olstad
@dan.d, I've adapted your version into my branch.
@dan.d: I only tested the 2.x version. I believe I was able to extend the session multiple times without any page refreshes, but I can test again to verify.
Considering there are other issues currently being worked on to replace jQuery with Drupal Dialogs ( https://www.drupal.org/project/autologout/issues/3339695 β¨ Use Drupal.dialog call instead of jQuery dialog RTBC ) I don't know that it makes sense to do a larger refactor of the JS in this branch.
- πΊπΈUnited States dan.d
@dmrupp, thanks for the update! I'll take another look, but the same behavior was observed on both branches for me.
...and I do agree with you on that the current fix should be very minimal. However, after familiarizing myself with the current implementation, I believe, that a switch from the jQuery modal to the D dialog should be a relatively straightforward process.
@joseph.olstad, that's great! I'm glad it turned out to be of use to us ;)
- π΅πΉPortugal HLopes
Indeed it's out of scope, altought touches the 'sync dialogs between tabs' point.
Feel free to create a new branch, just thought that it's a good approach and started experimenting.
Decided to just add a new js file because was just a POC and not sure about SharedWorker support accross browsers and as such one can keep both approaches and just let the user select which one to use (I've worked in a place where older browsers being supported was important)
- πΊπΈUnited States dan.d
My team has been testing this implementation, and we've discovered a small issue with auto-refresh for the session. I'm ready to make an adjustment, but I wanted to run the assumed logic behind the feature.
Current state:
Right now we've got an activity tracker that check whether a user has been active within the last 30 seconds. If so and the main timer is running out (the session is coming to an end), it will automatically re-new the session, without showing the confirmation modal.
So, the issue arrises when a user has been active for an hour and then takes a short break for a minute, looks away and his session expires just because he/she wasn't active within the last 30 seconds of the session.
Proposed solution:
Start a background timer that checks the activity status every 30 seconds. If there was an action within the last 30 seconds, extend the session in the background. Otherwise, let the session expire after a set interval. This solution will send a request to the server every 30 seconds to extend the session while the user is active on the page.
+ We can also adjust that period based on the actual length of the session, let's say 1/10 of the session period. So, for a 20 minute session we'd "check in" with the server every 20 minutes.
Let me know what you think, and I'll update the branch.