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

Remove support for legacy user sessions #4540

Merged
merged 6 commits into from
Sep 17, 2024

Conversation

zoldar
Copy link
Contributor

@zoldar zoldar commented Sep 6, 2024

Changes

This PR removes support for legacy sessions entirely. They are treated the same as "no valid token" case.

Additionally, on password reset, all user's sessions are deleted and their LV sockets are disconnected (note: this has limited effect when the pubsub is not connected between the nodes in multi-node scenario).

Tests

  • Automated tests have been added

@zoldar zoldar force-pushed the remove-legacy-user-sessions-support branch 2 times, most recently from f07bb80 to 1281509 Compare September 12, 2024 09:13
@zoldar zoldar marked this pull request as ready for review September 12, 2024 10:14
@zoldar zoldar requested a review from a team September 12, 2024 10:14
Copy link
Member

@aerosol aerosol left a comment

Choose a reason for hiding this comment

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

great

def revoke_all_user_sessions(user) do
{_count, tokens} =
Repo.delete_all(
from us in Auth.UserSession, where: us.user_id == ^user.id, select: us.token
Copy link
Member

Choose a reason for hiding this comment

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

select is probably unnecessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

select here makes the DELETE query return tokens without selecting them separately - they are then iterated over to broadcast "disconnect LV" message.

)

Enum.each(tokens, fn token ->
PlausibleWeb.Endpoint.broadcast(live_socket_id(token), "disconnect", %{})
Copy link
Member

Choose a reason for hiding this comment

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

nice! awaiting empd integration 🤘 is this a built-in message that's handled elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zoldar zoldar added the preview label Sep 12, 2024
Copy link

Preview environment👷🏼‍♀️🏗️
PR-4540

@zoldar
Copy link
Contributor Author

zoldar commented Sep 12, 2024

@plausible/developers is 77d1a10 the proper way to force dashboard refresh when any API request returns with 404 or should that be accomplished differently?

Reasoning for the change:

Currently, when the token expires, until dashboard page is fully refreshed, the user has no feedback. Failed API requests either don't change anything (/current-visitors query which is run periodically) or get stuck in loading/spinning state (filter suggestions for instance). Hence the idea to treat 404 error differently by force reloading the dashboard so that changed token state (or removed site?) is accounted for sooner without user wondering why everything is half-broken.

The change got reverted. We are going to address this separately together with a proper server error response handling on FE side.

@zoldar zoldar force-pushed the remove-legacy-user-sessions-support branch from 77d1a10 to c7bb860 Compare September 16, 2024 12:17
@zoldar zoldar added this pull request to the merge queue Sep 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 17, 2024
@zoldar zoldar added this pull request to the merge queue Sep 17, 2024
Merged via the queue into master with commit f7bbbf8 Sep 17, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants