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

UX: Reloading pages forces the users to login #37

Closed
janaka opened this issue Jun 28, 2023 · 10 comments · Fixed by #111
Closed

UX: Reloading pages forces the users to login #37

janaka opened this issue Jun 28, 2023 · 10 comments · Fixed by #111
Assignees
Labels
bug Something isn't working

Comments

@janaka
Copy link
Contributor

janaka commented Jun 28, 2023

Describe the bug
After login if you hit refresh in the browser you have to login.

Clicking on these manage documents links also open in a new window and force the user to login again

image

To Reproduce
Steps to reproduce the behavior:

  1. click on General Chat
  2. Login
  3. Hit refresh in your browser

Expected behavior
User remains logged in until the session expires or explicit logout action. Refresh any page shout not force a user to login. It should reload the page as expected.

Screenshots
If applicable, add screenshots to help explain your problem.

Environment (please complete the following information):

  • OS: [e.g. Ubuntu 22.04, macOS 11.4]
  • Code Version main branch latest (27 June 2023) commit c4314d2
  • Other Runtime Versions (please list below)

Additional context
Add any other context about the problem here.

@janaka janaka added the bug Something isn't working label Jun 28, 2023
@janaka
Copy link
Contributor Author

janaka commented Jun 30, 2023

@osala-eng once PR #36 is merged can you take a look at this one please.

@osala-eng
Copy link
Contributor

@osala-eng once PR #36 is merged can you take a look at this one please.

@cwang @janaka

Currently the auth_state is stored in the st.session_state which is tied to the socket connection that gets reset on page refresh forcing a re-login.

To persist the auth state we could make use of the browsers local storage to store the auth_state.
The library extra-streamlit-components https://pypi.org/project/extra-streamlit-components/ already has a module to interact with the local storage that we could use.

Sould I go ahead with this?

@cwang
Copy link
Contributor

cwang commented Jul 3, 2023

Probably needs a server-side component to make sure it's validated instead of trusting the client-side alone.

Or something like https://pypi.org/project/streamlit-cookies-manager/ where we could encrypt values

@osala-eng
Copy link
Contributor

Probably needs a server-side component to make sure it's validated instead of trusting the client-side alone.

Or something like https://pypi.org/project/streamlit-cookies-manager/ where we could encrypt values

Okay, Let me use this.

@janaka
Copy link
Contributor Author

janaka commented Jul 3, 2023

this still feels like a bug with with the way we are using this. I've not looked into it so might be wrong.

@janaka
Copy link
Contributor Author

janaka commented Jul 3, 2023

Seems like their naming and implementation here is a bit poor.

https://discuss.streamlit.io/t/why-session-state-is-not-persisting-between-refresh/32020

Some other options:
image

@cwang
Copy link
Contributor

cwang commented Jul 3, 2023

st.cache is the only viable option as the other two are deprecated.
This could be the reason to steer us towards FastAPI and then custom-built frontend, depending on whether we want to tackle it now or later.

@janaka
Copy link
Contributor Author

janaka commented Jul 3, 2023

Doesn't feel bad enough so far to justify a rebuild now (given where we are). But we should start introducing a FastAPI in parallel for other uses cases (like chrome ext) with a view of moving the web UI later.

Note just learnt this is also the cause of login screen when navigating links adding in MD.

@janaka
Copy link
Contributor Author

janaka commented Jul 31, 2023

@osala-eng using JWT would probably avoid having to read/write user context from the database.

@osala-eng osala-eng linked a pull request Aug 3, 2023 that will close this issue
20 tasks
@janaka
Copy link
Contributor Author

janaka commented Sep 10, 2023

@osala-eng so it looks like the following two are the 3rd party components for cookie management. Forum chat seems to imply that they are secure. Also, the first one looked like it sets the secure and samesite flags. iirc you said doesn't work on HTTPS?

We should be able to mitigate replay attacks pretty easily I think by adding a timestamp or previous token and checking etc. but we can address that in a separate PR.

Feature toggling is the short term mitigation.

FWIW - I did dig into the streamlit server and runtime code. I was also not able to see a way to set headers server side. The web server instance singleton is simply not exposed, even internally. The server init method doesn't even return and instance of the tornado webserver.

@janaka janaka mentioned this issue Sep 10, 2023
20 tasks
@osala-eng osala-eng removed a link to a pull request Sep 11, 2023
20 tasks
@osala-eng osala-eng linked a pull request Sep 11, 2023 that will close this issue
20 tasks
@osala-eng osala-eng linked a pull request Sep 27, 2023 that will close this issue
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants