Skip to content

Commit

Permalink
fix: deletes all sessions for user on logout (#2071)
Browse files Browse the repository at this point in the history 
* fix: deletes all sessions for user on logout
  • Loading branch information
Christopher Kolstad committed Sep 23, 2022
1 parent a358534 commit 667fb9a
Show file tree
Hide file tree
Showing 5 changed files with 154 additions and 17 deletions.
2 changes: 1 addition & 1 deletion src/lib/routes/index.ts
View file
Expand Up @@ -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,
Expand Down
137 changes: 127 additions & 10 deletions src/lib/routes/logout.test.ts
View file
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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`);

Expand All @@ -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`);

Expand All @@ -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`);

Expand All @@ -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);
});
19 changes: 17 additions & 2 deletions src/lib/routes/logout.ts
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -41,14 +49,21 @@ 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);

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}/`);
}

Expand Down
5 changes: 3 additions & 2 deletions src/lib/services/user-service.ts
View file
Expand Up @@ -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> {
Expand Down
8 changes: 6 additions & 2 deletions src/test/e2e/api/admin/user-admin.e2e.test.ts
View file
Expand Up @@ -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;
Expand All @@ -21,6 +22,7 @@ let app;
let userStore: IUserStore;
let eventStore: IEventStore;
let roleStore: IRoleStore;
let sessionStore: ISessionStore;
let editorRole: IRole;
let adminRole: IRole;

Expand All @@ -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);
Expand Down Expand Up @@ -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 () => {
Expand Down

0 comments on commit 667fb9a

Please sign in to comment.