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

O3-1444: Expired login session keeps the location page stuck #580

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

jnsereko
Copy link
Contributor

@jnsereko jnsereko commented Dec 11, 2022

Requirements

  • This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.

For changes to apps

If applicable

  • My work includes tests or is validated by existing tests.
  • I have updated the esm-framework mock to reflect any API changes I have made.

Summary

The session is saved as a global store using developit/unistore (https://github.com/developit/unistore). This session object is only updated either by clearing the session store (https://github.com/openmrs/openmrs-esm-core/blob/main/packages/framework/esm-api/src/shared-api-objects/current-user.ts#L188), or by refetching the current user (https://github.com/openmrs/openmrs-esm-core/blob/main/packages/framework/esm-api/src/shared-api-objects/current-user.ts#L163).

However, clearing or refetching the session store depends on only timeout and if the store is loaded, but doesn't check for the availability the JSESSIONID cookie. At the same time, this cookie is an HTTP-ONLY cookie so it ain't available via document.cookie.

to make this work, i manually opted to verifying that the user is logged in twice. As this is working, it might be not working wright.

cc @brandones @denniskigen @vasharma05 @ibacher @hadijahkyampeire

Screenshots

to.github.mov

Related Issue

https://issues.openmrs.org/browse/O3-1444

Other

@jnsereko
Copy link
Contributor Author

AFAIK, If there is any way we can add a check at https://github.com/openmrs/openmrs-esm-core/blob/main/packages/framework/esm-api/src/shared-api-objects/current-user.ts#L107-L108 so that the session object is refetched with absence of the JSESSIONID cookie, it would really be the simplest alternative.

  if (
    lastFetchTimeMillis < Date.now() - 1000 * 60 ||
    !sessionStore.getState().loaded  || 
    document.cookie.split('; ').find((row) => row.startsWith('JSESSIONID'))
  ) {
    refetchCurrentUser();
  }

@hadijahkyampeire
Copy link
Contributor

@jnsereko I just checked on dev3 and the behavior is the same, when I remove the session cookie, the user gets redirected to the login, it is confusing whether we still have this issue or not

@jnsereko
Copy link
Contributor Author

jnsereko commented Jan 4, 2023

@jnsereko I just checked on dev3 and the behavior is the same, when I remove the session cookie, the user gets redirected to the login, it is confusing whether we still have this issue or not

Do you mind trying again @hadijahkyampeire. I know this might sound silly but this issue sometimes responds differently mostly basing on how fast or slow your internet is. It happens so fast that the location page ain't displayed but the choose location URL appears in the Address Bar.
So, while am testing this, i actually ignore the displayed components and focus on the Address bar's URL changes. After deleting the cookie, it should only redirect to /login and not /login/location first, then /login later on

Copy link
Contributor

@hadijahkyampeire hadijahkyampeire left a comment

Choose a reason for hiding this comment

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

Works well, thanks @jnsereko

@hadijahkyampeire hadijahkyampeire merged commit 9591164 into openmrs:main Jan 4, 2023
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