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

BAU: session management fix #121

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nuwan-samarasinghe
Copy link
Collaborator

@nuwan-samarasinghe nuwan-samarasinghe commented Jan 28, 2025

Change description

In this change it consists of following fixes for the form runner

  1. When user logged in and going through the forms we store session in Redis cache and to retrieve the cache are using a session key, previously we have generated that key using following mechanism

<request.yar.id>:<form-name>:<application_id>

but in this method there is an issue if there is a user which is trying to go through the form in a deployment time then request.yar.id changing and then next request it cannot see the cache so in this new implementation we have introdused account id for the cache key

<request.auth.credentials.accountId>:<form-name>:<application_id>

So, this change will guarantee that. user will retrieve the session from cache

@nuwan-samarasinghe nuwan-samarasinghe enabled auto-merge (squash) January 28, 2025 13:23
.run/DOCKER-DEBUG.run.xml Outdated Show resolved Hide resolved
Comment on lines +108 to +114
let id: string;
if (request.auth && request.auth.credentials && request.auth.credentials.accountId) {
id = `${request.auth.credentials.accountId}:${request.params.id}`;
request.logger.info(`[ACTIVATE-SESSION] get user account id for the session key ${id}`);
} else {
id = `${request.yar.id}:${request.params.id}`;
}
Copy link
Contributor

@samuelhwilliams samuelhwilliams Jan 28, 2025

Choose a reason for hiding this comment

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

this feels like an unrelated change?

edit: ah I thought this PR was just for hot reload fix. This should be two PRs - let's keep separate changes as separate PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

also - I'm not convinced we do actually need this after some more investigation, see thread in forms tech 👀

Copy link
Collaborator Author

@nuwan-samarasinghe nuwan-samarasinghe Jan 28, 2025

Choose a reason for hiding this comment

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

broke down to 2 pr's #122

@nuwan-samarasinghe nuwan-samarasinghe changed the title BAU: session management fix and reload code after a change fix BAU: session management fix Jan 28, 2025
@nuwan-samarasinghe nuwan-samarasinghe force-pushed the bau/session-management-fix branch from 011e579 to 1439361 Compare January 28, 2025 14:35
Copy link

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.

3 participants