From 6fec2a93fc112d8cb6ed9d578a0912f95c53fb31 Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Mon, 27 Sep 2021 17:46:53 +0100 Subject: [PATCH] Server: Only disable API access when an account is disabled --- .../src/middleware/ownerHandler.test.ts | 17 +--------------- .../server/src/middleware/ownerHandler.ts | 6 ------ packages/server/src/models/BaseModel.ts | 7 +++++++ packages/server/src/models/UserModel.ts | 1 - packages/server/src/routes/api/items.test.ts | 15 +++++++++++++- packages/server/src/utils/routeUtils.ts | 9 ++++++++- .../server/src/utils/testing/testUtils.ts | 7 ++++++- readme/spec/server_user_status.md | 20 +++++++++++++++++++ 8 files changed, 56 insertions(+), 26 deletions(-) create mode 100644 readme/spec/server_user_status.md diff --git a/packages/server/src/middleware/ownerHandler.test.ts b/packages/server/src/middleware/ownerHandler.test.ts index ddd76da2aa..e567f5f087 100644 --- a/packages/server/src/middleware/ownerHandler.test.ts +++ b/packages/server/src/middleware/ownerHandler.test.ts @@ -1,5 +1,4 @@ -import { ErrorForbidden } from '../utils/errors'; -import { createUserAndSession, beforeAllDb, afterAllTests, beforeEachDb, koaAppContext, koaNext, models, expectHttpError } from '../utils/testing/testUtils'; +import { createUserAndSession, beforeAllDb, afterAllTests, beforeEachDb, koaAppContext, koaNext } from '../utils/testing/testUtils'; import ownerHandler from './ownerHandler'; describe('ownerHandler', function() { @@ -45,18 +44,4 @@ describe('ownerHandler', function() { expect(!!context.joplin.owner).toBe(false); }); - test('should not login if the user has been disabled', async function() { - const { user, session } = await createUserAndSession(1); - - await models().user().save({ id: user.id, enabled: 0 }); - - const context = await koaAppContext({ - sessionId: session.id, - }); - - context.joplin.owner = null; - - await expectHttpError(async () => ownerHandler(context, koaNext), ErrorForbidden.httpCode); - }); - }); diff --git a/packages/server/src/middleware/ownerHandler.ts b/packages/server/src/middleware/ownerHandler.ts index f422f3545a..10af240ec9 100644 --- a/packages/server/src/middleware/ownerHandler.ts +++ b/packages/server/src/middleware/ownerHandler.ts @@ -1,15 +1,9 @@ import { AppContext, KoaNext } from '../utils/types'; import { contextSessionId } from '../utils/requestUtils'; -import { ErrorForbidden } from '../utils/errors'; -import { cookieSet } from '../utils/cookies'; export default async function(ctx: AppContext, next: KoaNext): Promise { const sessionId = contextSessionId(ctx, false); const owner = sessionId ? await ctx.joplin.models.session().sessionUser(sessionId) : null; - if (owner && !owner.enabled) { - cookieSet(ctx, 'sessionId', ''); // Clear cookie, otherwise the user cannot login at all anymore - throw new ErrorForbidden('This user account is disabled. Please contact support.'); - } ctx.joplin.owner = owner; return next(); } diff --git a/packages/server/src/models/BaseModel.ts b/packages/server/src/models/BaseModel.ts index cb5f9c851e..710299c297 100644 --- a/packages/server/src/models/BaseModel.ts +++ b/packages/server/src/models/BaseModel.ts @@ -211,6 +211,13 @@ export default abstract class BaseModel { return rows as T[]; } + public async count(): Promise { + const r = await this + .db(this.tableName) + .count('*', { as: 'item_count' }); + return r[0].item_count; + } + public fromApiInput(object: T): T { const blackList = ['updated_time', 'created_time', 'owner_id']; const whiteList = Object.keys(databaseSchema[this.tableName]); diff --git a/packages/server/src/models/UserModel.ts b/packages/server/src/models/UserModel.ts index 53ab49572c..5216ceac16 100644 --- a/packages/server/src/models/UserModel.ts +++ b/packages/server/src/models/UserModel.ts @@ -118,7 +118,6 @@ export default class UserModel extends BaseModel { public async login(email: string, password: string): Promise { const user = await this.loadByEmail(email); if (!user) return null; - if (!user.enabled) throw new ErrorForbidden('This account is disabled. Please contact support if you need to re-activate it.'); if (!auth.checkPassword(password, user.password)) return null; return user; } diff --git a/packages/server/src/routes/api/items.test.ts b/packages/server/src/routes/api/items.test.ts index c4c7105af0..b402617337 100644 --- a/packages/server/src/routes/api/items.test.ts +++ b/packages/server/src/routes/api/items.test.ts @@ -1,4 +1,4 @@ -import { beforeAllDb, afterAllTests, beforeEachDb, createUserAndSession, models, createItem, makeTempFileWithContent, makeNoteSerializedBody, createItemTree, expectHttpError, createNote, expectNoHttpError } from '../../utils/testing/testUtils'; +import { beforeAllDb, afterAllTests, beforeEachDb, createUserAndSession, models, createItem, makeTempFileWithContent, makeNoteSerializedBody, createItemTree, expectHttpError, createNote, expectNoHttpError, getItem } from '../../utils/testing/testUtils'; import { NoteEntity } from '@joplin/lib/services/database/types'; import { ModelType } from '@joplin/lib/BaseModel'; import { deleteApi, getApi, putApi } from '../../utils/testing/apiUtils'; @@ -272,6 +272,19 @@ describe('api_items', function() { expect(item.jop_share_id).toBe(share.id); }); + test('should not upload or download items if the account is disabled', async function() { + const { session, user } = await createUserAndSession(1); + + // Should work + await createItem(session.id, 'root:/test1.txt:', 'test1'); + expect(await getItem(session.id, 'root:/test1.txt:')).toBe('test1'); + + // Should no longer work + await models().user().save({ id: user.id, enabled: 0 }); + await expectHttpError(async () => createItem(session.id, 'root:/test2.txt:', 'test2'), ErrorForbidden.httpCode); + await expectHttpError(async () => getItem(session.id, 'root:/test1.txt:'), ErrorForbidden.httpCode); + }); + test('should check permissions - only share participants can associate an item with a share', async function() { const { session: session1 } = await createUserAndSession(1); const { session: session2 } = await createUserAndSession(2); diff --git a/packages/server/src/utils/routeUtils.ts b/packages/server/src/utils/routeUtils.ts index 1eb731dcac..bdc78d9026 100644 --- a/packages/server/src/utils/routeUtils.ts +++ b/packages/server/src/utils/routeUtils.ts @@ -1,5 +1,5 @@ import config, { baseUrl } from '../config'; -import { Item, ItemAddressingType, Uuid } from '../services/database/types'; +import { Item, ItemAddressingType, User, Uuid } from '../services/database/types'; import { ErrorBadRequest, ErrorForbidden, ErrorNotFound } from './errors'; import Router from './Router'; import { AppContext, HttpMethod, RouteType } from './types'; @@ -182,6 +182,12 @@ export function routeResponseFormat(context: AppContext): RouteResponseFormat { return path.indexOf('api') === 0 || path.indexOf('/api') === 0 ? RouteResponseFormat.Json : RouteResponseFormat.Html; } +function disabledAccountCheck(route: MatchedRoute, user: User) { + if (!user || user.enabled) return; + + if (route.subPath.schema.startsWith('api/')) throw new ErrorForbidden(`This account is disabled. Please login to ${config().baseUrl} for more information.`); +} + export async function execRequest(routes: Routers, ctx: AppContext) { const match = findMatchingRoute(ctx.path, routes); if (!match) throw new ErrorNotFound(); @@ -197,6 +203,7 @@ export async function execRequest(routes: Routers, ctx: AppContext) { if (!isPublicRoute && !ctx.joplin.owner) throw new ErrorForbidden(); await csrfCheck(ctx, isPublicRoute); + disabledAccountCheck(match, ctx.joplin.owner); return endPoint.handler(match.subPath, ctx); } diff --git a/packages/server/src/utils/testing/testUtils.ts b/packages/server/src/utils/testing/testUtils.ts index 4eff7e2d2a..203daeb2b3 100644 --- a/packages/server/src/utils/testing/testUtils.ts +++ b/packages/server/src/utils/testing/testUtils.ts @@ -15,7 +15,7 @@ import * as fs from 'fs-extra'; import * as jsdom from 'jsdom'; import setupAppContext from '../setupAppContext'; import { ApiError } from '../errors'; -import { putApi } from './apiUtils'; +import { getApi, putApi } from './apiUtils'; import { FolderEntity, NoteEntity, ResourceEntity } from '@joplin/lib/services/database/types'; import { ModelType } from '@joplin/lib/BaseModel'; import { initializeJoplinUtils } from '../joplinUtils'; @@ -327,6 +327,11 @@ export async function createItemTree3(userId: Uuid, parentFolderId: string, shar } } +export async function getItem(sessionId: string, path: string): Promise { + const item: Buffer = await getApi(sessionId, `items/${path}/content`); + return item.toString(); +} + export async function createItem(sessionId: string, path: string, content: string | Buffer): Promise { const tempFilePath = await makeTempFileWithContent(content); const item: Item = await putApi(sessionId, `items/${path}/content`, null, { filePath: tempFilePath }); diff --git a/readme/spec/server_user_status.md b/readme/spec/server_user_status.md new file mode 100644 index 0000000000..612834de55 --- /dev/null +++ b/readme/spec/server_user_status.md @@ -0,0 +1,20 @@ +# Joplin Server user status + +## User flags + +User flags are used to indicate problem conditions with a particular account. They are usually automatically set by various services, for example when an account go over the limit, or when a payment fails. Likewise they are removed automatically when the condition changes. + +The list of flags is defined in `UserFlagType`. + +## User status + +A user can have various status that affects the possible actions they can do. **User statuses are derived from user flags**. + +| Status | Values | Description | +| --- | --- | --- | +| can_upload | 0 or 1 | Whether the user can upload items, such as notes or tags, to the server. +| enabled | 0 or 1 | A disabled user cannot upload or download data from the server API anymore. However, they can still login to the website, make change to their profile, etc. + +Perhaps a third status: "blocked" could be created. It would be like `enabled = 0`, except they won't be able to login to the website either. + +These status should only be set as a results of user flags. In other words, the application should not directly set `enabled` to 0 or 1 but instead set a user flag that would indicate the issue. A script will then process the user flags and set the status as a result.