Skip to content

Fix dont save timeout path to session#8142

Merged
leio10 merged 3 commits intodecidim:developfrom
mainio:fix/dont_save_timeout_path_to_session
Jun 23, 2021
Merged

Fix dont save timeout path to session#8142
leio10 merged 3 commits intodecidim:developfrom
mainio:fix/dont_save_timeout_path_to_session

Conversation

@lahdeero
Copy link
Copy Markdown
Contributor

@lahdeero lahdeero commented Jun 16, 2021

🎩 What? Why?

When "Force users to authenticate before access organization" is set from system panel, automatic session timeout isn't working as expected: it's not always singing out in all tabs and stores /timeouts/seconds_until_timeout location (as redirect path) to user's session. This is because ensure_authenticated! method calls store_location_for which stores /timeouts/seconds_until_timeout location to session.

Testing

  1. OPTIONAL: config.timeout_in = 2.minutes @ devise.rb
  2. Go to system panel and set "Force users to authenticate before access organization"
  3. Sign in
  4. Open two tabs (e.g. /processes and /assemblies)
  5. Wait for timeout modal to popup
  6. Click "Continue session" in one tab
  7. Wait for automatic sign out (don't press anything when timeout modal appears)
  8. All tabs SHOULD have location /users/sign_in (without timeout modal)
  9. When signing in again SHOULD NOT see error (screenshot below)

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

image

♥️ Thank you!

leio10
leio10 previously approved these changes Jun 21, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@leio10 leio10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 👍

decidim-core/app/controllers/decidim/timeouts_controller.rb Outdated
class TimeoutsController < Decidim::ApplicationController
prepend_before_action :skip_timeout, only: :seconds_until_timeout
# Skip these methods because they can call Devise's store_location_for, which can save timeouts path to session.
skip_before_action :store_current_location, :ensure_authenticated!
Copy link
Copy Markdown
Contributor

@leio10 leio10 Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Mmm, should we add :ensure_authenticated! to all lines calling skip_before_action :store_current_location?
We could even create a ShouldNotStorePath concern to be more DRY. What do you think? (it's not needed to merge this, of course)

Copy link
Copy Markdown
Contributor Author

@lahdeero lahdeero Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

speech_balloon Mmm, should we add :ensure_authenticated! to all lines calling skip_before_action :store_current_location?
We could even create a ShouldNotStorePath concern to be more DRY. What do you think? (it's not needed to merge this, of course)

Hmmm.. Could we just remove store_location_for , from ensure_authenticated! method? It seems to me that we are saving location twice if "Force users to authenticate before access organization" is set.

Copy link
Copy Markdown
Contributor

@leio10 leio10 Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right. It seems that that call was added in #6300 because the redirect was done before storing the current location, but I'd say that after #8036 it should work properly without the store_location_for call. Can you remove it and the skip_before_action for ensure_authenticated! and check if it works? Thanks!

@leio10 leio10 merged commit be772ff into decidim:develop Jun 23, 2021
@ahukkanen ahukkanen deleted the fix/dont_save_timeout_path_to_session branch June 28, 2021 18:18
lahdeero added a commit to mainio/decidim that referenced this pull request Jun 29, 2021
@lahdeero
Fix dont save timeout path to session (decidim#8142)
88187ea
@lahdeero lahdeero mentioned this pull request Jun 29, 2021
Merged
entantoencuanto added a commit that referenced this pull request Jun 29, 2021
@entantoencuanto
Merge branch 'develop' into active_storage_migration
13dd1f4 
* develop: (47 commits)
  New Crowdin updates (#8150)
  Move the webpacker config override to @decidim/webpacker (#8158)
  Fix admin stylesheet dynamic imports (#8154)
  Fix session timeout conflicting with remember me (#7467)
  Allow to create online meetings without an URL (#8152)
  Fix verification route issues (#8146)
  Fix dont save timeout path to session (#8142)
  Fix access to import CSV results in accountability (#8132)
  Fix user report notification reported user name (#8130)
  Allow users to comment and delete their own comments (#8072)
  New Crowdin updates (#8124)
  Fix webpacker issues (#8136)
  Add comments in participatory space presentation page stats block (#8034)
  Split NPM dependencies to more granular packages (#8121)
  Metric is not shown when value is zero for blocked and reported users (#8117)
  Add missing templates translations (#8133)
  Add missing translation for authorization_modals (#8129)
  Polls in meetings (#8065)
  Fix flaky test on initiatives (#8128)
  Filter participants admin (#8104)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@leio10 leio10 leio10 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

in-review module: core target: user-experience type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lahdeero @leio10