diff --git a/packages/server/src/models/BaseModel.ts b/packages/server/src/models/BaseModel.ts index e4a879951c..4c8b157371 100644 --- a/packages/server/src/models/BaseModel.ts +++ b/packages/server/src/models/BaseModel.ts @@ -102,16 +102,57 @@ export default abstract class BaseModel { return false; } - protected async withTransaction(fn: Function): Promise { + // When using withTransaction, make sure any database call uses an instance + // of `this.db()` that was accessed within the `fn` callback, otherwise the + // transaction will be stuck! + // + // This for example, would result in a stuck transaction: + // + // const query = this.db(this.tableName).where('id', '=', id); + // + // this.withTransaction(async () => { + // await query.delete(); + // }); + // + // This is because withTransaction is going to swap the value of "this.db()" + // for as long as the transaction is active. So if the query is started + // outside the transaction, it will use the regular db connection and wait + // for the newly created transaction to finish, which will never happen. + // + // This is a bit of a leaky abstraction, which ideally should be improved + // but for now one just has to be aware of the caveat. + // + // The `name` argument is only for debugging, so that any stuck transaction + // can be more easily identified. + protected async withTransaction(fn: Function, name: string = null): Promise { + const debugTransaction = false; + + const debugTimerId = debugTransaction ? setTimeout(() => { + console.info('Transaction did not complete:', name, txIndex); + }, 5000) : null; + const txIndex = await this.transactionHandler_.start(); + if (debugTransaction) console.info('START', name, txIndex); + try { await fn(); } catch (error) { await this.transactionHandler_.rollback(txIndex); + + if (debugTransaction) { + console.info('ROLLBACK', name, txIndex); + clearTimeout(debugTimerId); + } + throw error; } + if (debugTransaction) { + console.info('COMMIT', name, txIndex); + clearTimeout(debugTimerId); + } + await this.transactionHandler_.commit(txIndex); } @@ -197,7 +238,7 @@ export default abstract class BaseModel { // Sanity check: if (updatedCount !== 1) throw new ErrorBadRequest(`one row should have been updated, but ${updatedCount} row(s) were updated`); } - }); + }, 'BaseModel::save'); return toSave; } @@ -220,11 +261,6 @@ export default abstract class BaseModel { if (!ids.length) throw new Error('no id provided'); - const query = this.db(this.tableName).where({ id: ids[0] }); - for (let i = 1; i < ids.length; i++) { - await query.orWhere({ id: ids[i] }); - } - const trackChanges = this.trackChanges; let itemsWithParentIds: AnyItemType[] = null; @@ -233,13 +269,18 @@ export default abstract class BaseModel { } await this.withTransaction(async () => { + const query = this.db(this.tableName).where({ id: ids[0] }); + for (let i = 1; i < ids.length; i++) { + await query.orWhere({ id: ids[i] }); + } + const deletedCount = await query.del(); if (deletedCount !== ids.length) throw new Error(`${ids.length} row(s) should have been deleted by ${deletedCount} row(s) were deleted`); if (trackChanges) { for (const item of itemsWithParentIds) await this.handleChangeTracking({}, item, ChangeType.Delete); } - }); + }, 'BaseModel::delete'); } } diff --git a/packages/server/src/models/FileModel.ts b/packages/server/src/models/FileModel.ts index b640d98545..486833b234 100644 --- a/packages/server/src/models/FileModel.ts +++ b/packages/server/src/models/FileModel.ts @@ -87,7 +87,7 @@ export default class FileModel extends BaseModel { output[item.id] = segments.length ? (`root:/${segments.join('/')}:`) : 'root'; } - }); + }, 'FileModel::itemFullPaths'); return output; } @@ -404,7 +404,7 @@ export default class FileModel extends BaseModel { for (const childId of childrenIds) { await this.delete(childId); } - }); + }, 'FileModel::deleteChildren'); } public async delete(id: string, options: DeleteOptions = {}): Promise { @@ -427,7 +427,7 @@ export default class FileModel extends BaseModel { } await super.delete(id); - }); + }, 'FileModel::delete'); } } diff --git a/packages/server/src/models/SessionModel.ts b/packages/server/src/models/SessionModel.ts index 3a03bfee23..da1486b848 100644 --- a/packages/server/src/models/SessionModel.ts +++ b/packages/server/src/models/SessionModel.ts @@ -29,4 +29,9 @@ export default class SessionModel extends BaseModel { return this.createUserSession(user.id); } + public async logout(sessionId: string) { + if (!sessionId) return; + await this.delete(sessionId); + } + } diff --git a/packages/server/src/models/UserModel.ts b/packages/server/src/models/UserModel.ts index 7187f4558b..006e1efea7 100644 --- a/packages/server/src/models/UserModel.ts +++ b/packages/server/src/models/UserModel.ts @@ -97,7 +97,7 @@ export default class UserModel extends BaseModel { const rootFile = await fileModel.userRootFile(); await fileModel.delete(rootFile.id, { validationRules: { canDeleteRoot: true } }); await super.delete(id); - }); + }, 'UserModel::delete'); } public async save(object: User, options: SaveOptions = {}): Promise { @@ -114,7 +114,7 @@ export default class UserModel extends BaseModel { const fileModel = this.models().file({ userId: newUser.id }); await fileModel.createRootFile(); } - }); + }, 'UserModel::save'); return newUser; } diff --git a/packages/server/src/routes/index/logout.test.ts b/packages/server/src/routes/index/logout.test.ts new file mode 100644 index 0000000000..015fc0e803 --- /dev/null +++ b/packages/server/src/routes/index/logout.test.ts @@ -0,0 +1,37 @@ +import routeHandler from '../../middleware/routeHandler'; +import { beforeAllDb, afterAllTests, beforeEachDb, koaAppContext, models, createUserAndSession } from '../../utils/testing/testUtils'; + +describe('index_logout', function() { + + beforeAll(async () => { + await beforeAllDb('index_logout'); + }); + + afterAll(async () => { + await afterAllTests(); + }); + + beforeEach(async () => { + await beforeEachDb(); + }); + + test('should logout', async function() { + const { session } = await createUserAndSession(); + + const context = await koaAppContext({ + sessionId: session.id, + request: { + method: 'POST', + url: '/logout', + }, + }); + + expect(context.cookies.get('sessionId')).toBe(session.id); + expect(!!(await models().session().load(session.id))).toBe(true); + await routeHandler(context); + + expect(!context.cookies.get('sessionId')).toBe(true); + expect(!!(await models().session().load(session.id))).toBe(false); + }); + +}); diff --git a/packages/server/src/routes/index/logout.ts b/packages/server/src/routes/index/logout.ts index 331628d37d..3f839d44ab 100644 --- a/packages/server/src/routes/index/logout.ts +++ b/packages/server/src/routes/index/logout.ts @@ -2,13 +2,15 @@ import { SubPath, Route, redirect } from '../../utils/routeUtils'; import { ErrorMethodNotAllowed } from '../../utils/errors'; import { AppContext } from '../../utils/types'; import { baseUrl } from '../../config'; +import { contextSessionId } from '../../utils/requestUtils'; const route: Route = { exec: async function(_path: SubPath, ctx: AppContext) { if (ctx.method === 'POST') { - // TODO: also delete the session from the database + const sessionId = contextSessionId(ctx, false); ctx.cookies.set('sessionId', ''); + await ctx.models.session().logout(sessionId); return redirect(ctx, `${baseUrl()}/login`); }