Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sandbox page redirect #10303

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Fix sandbox page redirect #10303

merged 1 commit into from
Jan 24, 2025

Conversation

CatalinVoineag
Copy link
Contributor

@CatalinVoineag CatalinVoineag commented Jan 24, 2025

Context

On sandbox, when we log out the user. We redirect them to
contents#sandbox.

This controller is not properly setup to deal with one login, especially
when the one login feature is turned off.

Redirects from the page of this controller hit the Authentication. The
concern returns true that the candidate is signed but with db backed
session, not devise. So the user is stuck in a loop because devise
thinks the user is not signed in but our DB backed session concern says
it is.

Clearing the session fixes this issue.

In reality this should not happen because we don't attempt to use the db
session login when one login is off, in any of our other controllers.
But requests from this controller accesses the Authentication concern.

This commit tries to fix this by just clearing the session if one login
feature is not enabled, this will fix this issue. Ideally we would want
to not have this controller send requests to the Authentication concern
if one login is not enabled.

Changes proposed in this pull request

Authentication concern

Guidance to review

Only possible to test on sadbox. Or locally if you're really motivated

Screencast.2025-01-24.12.44.39.mp4

Things to check

  • If the code removes any existing feature flags, a data migration has also been added to delete the entry from the database
  • This code does not rely on migrations in the same Pull Request
  • If this code includes a migration adding or changing columns, it also backfills existing records for consistency
  • If this code adds a column to the DB, decide whether it needs to be in analytics yml file or analytics blocklist, if included inform data insights team of the changes
  • If this code adds a column that may include PII, the sanitise.sql script and 0025-protecting-personal-data-in-production-dump.md ADR have been updated
  • API release notes have been updated if necessary
  • If it adds a significant user-facing change, is it documented in the CHANGELOG?
  • Attach the PR to the Trello card

@CatalinVoineag CatalinVoineag force-pushed the cv/fix-one-login-sandbox branch from 2e9a4e9 to bccd20f Compare January 24, 2025 12:40
@CatalinVoineag CatalinVoineag changed the title Cv/fix one login sandbox Fix sandbox page redirect Jan 24, 2025
@CatalinVoineag CatalinVoineag marked this pull request as ready for review January 24, 2025 12:45
@CatalinVoineag CatalinVoineag self-assigned this Jan 24, 2025
@CatalinVoineag CatalinVoineag requested a review from a team January 24, 2025 12:45
On sandbox, when we log out the user. We redirect them to
contents#sandbox.

This controller is not properly setup to deal with one login, especially
when the one login feature is turned off.

Redirects from the page of this controller hit the Authentication. The
concern returns true that the candidate is signed but with db backed
session, not devise. So the user is stuck in a loop because devise
thinks the user is not signed in but our DB backed session concern says
it is.

Clearing the session fixes this issue.

In reality this should not happen because we don't attempt to use the db
session login when one login is off, in any of our other controllers.
But requests from this controller accesses the Authentication concern.

This commit tries to fix this by just clearing the session if one login
feature is not enabled, this will fix this issue. Ideally we would want
to not have this controller send requests to the Authentication concern
if one login is not enabled.
@CatalinVoineag CatalinVoineag merged commit a8517a0 into main Jan 24, 2025
23 of 24 checks passed
@CatalinVoineag CatalinVoineag deleted the cv/fix-one-login-sandbox branch January 24, 2025 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants