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: deletes all sessions for user on logout #2071

Merged
merged 2 commits into from
Sep 23, 2022

Conversation

chriswk
Copy link
Contributor

@chriswk chriswk commented Sep 19, 2022

To ensure we clean up sessions on logout this PR does two things:

  1. When users changes password, it deletes all sessions for user
  2. When logging out, it deletes all sessions for user, not just the current one in the request.

Will add a test before moving from draft

@vercel
Copy link

vercel bot commented Sep 19, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Ignored Deployments
Name Status Preview Updated
unleash ⬜️ Ignored (Inspect) Sep 19, 2022 at 9:07AM (UTC)
unleash-docs ⬜️ Ignored (Inspect) Sep 19, 2022 at 9:07AM (UTC)
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Sep 19, 2022 at 9:07AM (UTC)

@github-actions
Copy link

github-actions bot commented Sep 19, 2022

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
88.27% (-3.1% 🔻)
7052/7989
🟡 Branches 78.72% 1099/1396
🟢 Functions
81.89% (-4.34% 🔻)
1985/2424
🟢 Lines
88.57% (-2.74% 🔻)
6538/7382

⚠️ Details were not displayed: the report size has exceeded the limit.

Test suite run success

1159 tests passing in 191 suites.

Report generated by 🧪jest coverage report action from 180855a

@chriswk chriswk marked this pull request as ready for review September 19, 2022 09:08
@chriswk
Copy link
Contributor Author

chriswk commented Sep 19, 2022

tests added both for invalidating sessions when changing password and for logging out

app.use('/logout', new LogoutController(config, { sessionService }).router);
await supertest(app).get('/logout').expect(302);
let activeSessions = await sessionStore.getSessionsForUser(1);
expect(activeSessions).toHaveLength(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Contributor

@gardleopard gardleopard left a comment

Choose a reason for hiding this comment

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

LGTM

.post(`/api/admin/user-admin/${user.id}/change-password`)
.send({ password: 'simple123-_ASsad' })
.expect(200);
expect(spy).toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

nice:D

@chriswk chriswk merged commit 667fb9a into main Sep 23, 2022
@chriswk chriswk deleted the task/killAllSessionsOnLogout branch September 23, 2022 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants