fix: deletes all sessions for user on logout (#2071)

* fix: deletes all sessions for user on logout

src/lib/routes/index.ts

@@ -18,7 +18,7 @@ class IndexRouter extends Controller {
 
         this.use('/health', new HealthCheckController(config, services).router);
         this.use('/internal-backstage', new BackstageController(config).router);
-        this.use('/logout', new LogoutController(config).router);
+        this.use('/logout', new LogoutController(config, services).router);
         this.use(
             '/auth/simple',
             new SimplePasswordProvider(config, services).router,

src/lib/routes/logout.test.ts

@@ -4,12 +4,26 @@ import { createTestConfig } from '../../test/config/test-config';
 
 import LogoutController from './logout';
 import { IAuthRequest } from './unleash-types';
+import SessionService from '../services/session-service';
+import FakeSessionStore from '../../test/fixtures/fake-session-store';
+import noLogger from '../../test/fixtures/no-logger';
+import { addDays } from 'date-fns';
 
 test('should redirect to "/" after logout', async () => {
     const baseUriPath = '';
     const app = express();
     const config = createTestConfig({ server: { baseUriPath } });
-    app.use('/logout', new LogoutController(config).router);
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+    app.use(
+        '/logout',
+        new LogoutController(config, {
+            sessionService,
+        }).router,
+    );
     const request = supertest(app);
     expect.assertions(0);
     await request
@@ -22,7 +36,12 @@ test('should redirect to "/basePath" after logout when baseUriPath is set', asyn
     const baseUriPath = '/basePath';
     const app = express();
     const config = createTestConfig({ server: { baseUriPath } });
-    app.use('/logout', new LogoutController(config).router);
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+    app.use('/logout', new LogoutController(config, { sessionService }).router);
     const request = supertest(app);
     expect.assertions(0);
     await request
@@ -35,7 +54,13 @@ test('should set "Clear-Site-Data" header', async () => {
     const baseUriPath = '';
     const app = express();
     const config = createTestConfig({ server: { baseUriPath } });
-    app.use('/logout', new LogoutController(config).router);
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+
+    app.use('/logout', new LogoutController(config, { sessionService }).router);
     const request = supertest(app);
     expect.assertions(0);
     await request
@@ -51,7 +76,13 @@ test('should not set "Clear-Site-Data" header', async () => {
         server: { baseUriPath },
         session: { clearSiteDataOnLogout: false },
     });
-    app.use('/logout', new LogoutController(config).router);
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+
+    app.use('/logout', new LogoutController(config, { sessionService }).router);
     const request = supertest(app);
     expect.assertions(1);
     await request
@@ -66,7 +97,14 @@ test('should clear "unleash-session" cookies', async () => {
     const baseUriPath = '';
     const app = express();
     const config = createTestConfig({ server: { baseUriPath } });
-    app.use('/logout', new LogoutController(config).router);
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+
+    app.use('/logout', new LogoutController(config, { sessionService }).router);
+
     const request = supertest(app);
     expect.assertions(0);
     await request
@@ -85,7 +123,14 @@ test('should clear "unleash-session" cookie even when disabled clear site data',
         server: { baseUriPath },
         session: { clearSiteDataOnLogout: false },
     });
-    app.use('/logout', new LogoutController(config).router);
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+
+    app.use('/logout', new LogoutController(config, { sessionService }).router);
+
     const request = supertest(app);
     expect.assertions(0);
     await request
@@ -108,7 +153,14 @@ test('should call destroy on session', async () => {
         req.session = fakeSession;
         next();
     });
-    app.use('/logout', new LogoutController(config).router);
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+
+    app.use('/logout', new LogoutController(config, { sessionService }).router);
+
     const request = supertest(app);
     await request.get(`${baseUriPath}/logout`);
 
@@ -125,7 +177,14 @@ test('should handle req.logout with callback function', async () => {
         req.logout = logoutFunction;
         next();
     });
-    app.use('/logout', new LogoutController(config).router);
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+
+    app.use('/logout', new LogoutController(config, { sessionService }).router);
+
     const request = supertest(app);
     await request.get(`${baseUriPath}/logout`);
 
@@ -143,7 +202,14 @@ test('should handle req.logout without callback function', async () => {
         req.logout = logoutFunction;
         next();
     });
-    app.use('/logout', new LogoutController(config).router);
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+
+    app.use('/logout', new LogoutController(config, { sessionService }).router);
+
     const request = supertest(app);
     await request.get(`${baseUriPath}/logout`);
 
@@ -162,10 +228,61 @@ test('should redirect to alternative logoutUrl', async () => {
         req.session = fakeSession;
         next();
     });
-    app.use('/logout', new LogoutController(config).router);
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+
+    app.use('/logout', new LogoutController(config, { sessionService }).router);
+
     const request = supertest(app);
     await request
         .get('/logout')
         .expect(302)
         .expect('Location', '/some-other-path');
 });
+
+test('Should destroy sessions for user', async () => {
+    const app = express();
+    const config = createTestConfig();
+    const fakeSession = {
+        destroy: jest.fn(),
+        user: {
+            id: 1,
+        },
+    };
+    app.use((req: IAuthRequest, res, next) => {
+        req.session = fakeSession;
+        next();
+    });
+    const sessionStore = new FakeSessionStore();
+    const sessionService = new SessionService(
+        { sessionStore },
+        { getLogger: noLogger },
+    );
+    await sessionStore.insertSession({
+        sid: '1',
+        sess: {
+            user: {
+                id: 1,
+            },
+        },
+        expired: addDays(new Date(), 2),
+    });
+    await sessionStore.insertSession({
+        sid: '2',
+        sess: {
+            user: {
+                id: 1,
+            },
+        },
+        expired: addDays(new Date(), 2),
+    });
+    let activeSessionsBeforeLogout = await sessionStore.getSessionsForUser(1);
+    expect(activeSessionsBeforeLogout).toHaveLength(2);
+    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);
+});

src/lib/routes/logout.ts

@@ -3,6 +3,8 @@ import { promisify } from 'util';
 import { IUnleashConfig } from '../types/option';
 import Controller from './controller';
 import { IAuthRequest } from './unleash-types';
+import { IUnleashServices } from '../types';
+import SessionService from '../services/session-service';
 
 class LogoutController extends Controller {
     private clearSiteDataOnLogout: boolean;
@@ -11,8 +13,14 @@ class LogoutController extends Controller {
 
     private baseUri: string;
 
-    constructor(config: IUnleashConfig) {
+    private sessionService: SessionService;
+
+    constructor(
+        config: IUnleashConfig,
+        { sessionService }: Pick<IUnleashServices, 'sessionService'>,
+    ) {
         super(config);
+        this.sessionService = sessionService;
         this.baseUri = config.server.baseUriPath;
         this.clearSiteDataOnLogout = config.session.clearSiteDataOnLogout;
         this.cookieName = config.session.cookieName;
@@ -41,6 +49,11 @@ class LogoutController extends Controller {
         }
 
         if (req.session) {
+            if (req.session.user?.id) {
+                await this.sessionService.deleteSessionsForUser(
+                    req.session.user.id,
+                );
+            }
             req.session.destroy();
         }
         res.clearCookie(this.cookieName);
@@ -48,7 +61,9 @@ class LogoutController extends Controller {
         if (this.clearSiteDataOnLogout) {
             res.set('Clear-Site-Data', '"cookies", "storage"');
         }
-
+        if (req.user?.id) {
+            await this.sessionService.deleteSessionsForUser(req.user.id);
+        }
         res.redirect(`${this.baseUri}/`);
     }
 

src/lib/services/user-service.ts

@@ -340,14 +340,15 @@ class UserService {
                 throw e;
             }
         }
-        this.store.successfullyLogin(user);
+        await this.store.successfullyLogin(user);
         return user;
     }
 
     async changePassword(userId: number, password: string): Promise<void> {
         this.validatePassword(password);
         const passwordHash = await bcrypt.hash(password, saltRounds);
-        return this.store.setPasswordHash(userId, passwordHash);
+        await this.store.setPasswordHash(userId, passwordHash);
+        await this.sessionService.deleteSessionsForUser(userId);
     }
 
     async getUserForToken(token: string): Promise<TokenUserSchema> {

src/test/e2e/api/admin/user-admin.e2e.test.ts

@@ -13,6 +13,7 @@ import { RoleName } from '../../../../lib/types/model';
 import { IRoleStore } from 'lib/types/stores/role-store';
 import { randomId } from '../../../../lib/util/random-id';
 import { omitKeys } from '../../../../lib/util/omit-keys';
+import { ISessionStore } from '../../../../lib/types/stores/session-store';
 
 let stores;
 let db;
@@ -21,6 +22,7 @@ let app;
 let userStore: IUserStore;
 let eventStore: IEventStore;
 let roleStore: IRoleStore;
+let sessionStore: ISessionStore;
 let editorRole: IRole;
 let adminRole: IRole;
 
@@ -32,6 +34,7 @@ beforeAll(async () => {
     userStore = stores.userStore;
     eventStore = stores.eventStore;
     roleStore = stores.roleStore;
+    sessionStore = stores.sessionStore;
     const roles = await roleStore.getRootRoles();
     editorRole = roles.find((r) => r.name === RoleName.EDITOR);
     adminRole = roles.find((r) => r.name === RoleName.ADMIN);
@@ -231,11 +234,12 @@ test('validator should accept strong password', async () => {
 
 test('should change password', async () => {
     const user = await userStore.insert({ email: 'some@mail.com' });
-
-    return app.request
+    const spy = jest.spyOn(sessionStore, 'deleteSessionsForUser');
+    await app.request
         .post(`/api/admin/user-admin/${user.id}/change-password`)
         .send({ password: 'simple123-_ASsad' })
         .expect(200);
+    expect(spy).toHaveBeenCalled();
 });
 
 test('should search for users', async () => {