From 0d9954af70dfb5ae8b028cb56a5f02905512abf9 Mon Sep 17 00:00:00 2001 From: Pranav C Date: Mon, 26 Dec 2022 20:05:15 +0530 Subject: [PATCH 1/4] fix(gui): allow attachment upload if user have permission re #4694 Signed-off-by: Pranav C --- packages/nocodb-sdk/src/lib/enums.ts | 7 ++++ .../nocodb/src/lib/meta/api/attachmentApis.ts | 35 ++++++++++++++++--- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/packages/nocodb-sdk/src/lib/enums.ts b/packages/nocodb-sdk/src/lib/enums.ts index c5b1888256..cabdc2ff90 100644 --- a/packages/nocodb-sdk/src/lib/enums.ts +++ b/packages/nocodb-sdk/src/lib/enums.ts @@ -3,3 +3,10 @@ export enum OrgUserRoles { CREATOR = 'org-level-creator', VIEWER = 'org-level-viewer', } +export enum ProjectRoles { + OWNER = 'owner', + CREATOR = 'creator', + EDITOR = 'editor', + COMMENTER = 'commenter', + VIEWER = 'viewer', +} diff --git a/packages/nocodb/src/lib/meta/api/attachmentApis.ts b/packages/nocodb/src/lib/meta/api/attachmentApis.ts index 4eb3f7597e..535d227da0 100644 --- a/packages/nocodb/src/lib/meta/api/attachmentApis.ts +++ b/packages/nocodb/src/lib/meta/api/attachmentApis.ts @@ -2,15 +2,41 @@ import { Request, Response, Router } from 'express'; import multer from 'multer'; import { nanoid } from 'nanoid'; +import { OrgUserRoles, ProjectRoles } from 'nocodb-sdk'; import path from 'path'; import slash from 'slash'; +import Noco from '../../Noco'; +import { MetaTable } from '../../utils/globals'; import mimetypes, { mimeIcons } from '../../utils/mimeTypes'; import { Tele } from 'nc-help'; -import ncMetaAclMw from '../helpers/ncMetaAclMw'; -import catchError from '../helpers/catchError'; +import extractProjectIdAndAuthenticate from '../helpers/extractProjectIdAndAuthenticate'; +import catchError, { NcError } from '../helpers/catchError'; import NcPluginMgrv2 from '../helpers/NcPluginMgrv2'; import { NC_ATTACHMENT_FIELD_SIZE } from '../../constants'; +const isUploadAllowed = async (req: Request, _res: Response, next: any) => { + try { + // check user is super admin or creator + if ( + req['user']?.roles?.includes(OrgUserRoles.SUPER_ADMIN) || + req['user']?.roles?.includes(OrgUserRoles.CREATOR) || + // if viewer then check at-least one project have editor or higher role + // todo: cache + !!(await Noco.ncMeta + .knex(MetaTable.PROJECT_USERS) + .where(function () { + this.where('roles', ProjectRoles.OWNER); + this.orWhere('roles', ProjectRoles.CREATOR); + this.orWhere('roles', ProjectRoles.EDITOR); + }) + .andWhere('fk_user_id', req['user']?.id) + .first()) + ) + return next(); + } catch {} + NcError.badRequest('Upload not allowed'); +}; + // const storageAdapter = new Local(); export async function upload(req: Request, res: Response) { const filePath = sanitizeUrlPath( @@ -156,11 +182,12 @@ router.post( fieldSize: NC_ATTACHMENT_FIELD_SIZE, }, }).any(), - ncMetaAclMw(upload, 'upload') + [extractProjectIdAndAuthenticate, isUploadAllowed, catchError(upload)] ); router.post( '/api/v1/db/storage/upload-by-url', - ncMetaAclMw(uploadViaURL, 'uploadViaURL') + + [extractProjectIdAndAuthenticate, isUploadAllowed, catchError(uploadViaURL)] ); router.get(/^\/download\/(.+)$/, catchError(fileRead)); From 1f17da4b95113ce069fc3cd3bdc4d4aaf01e5e1e Mon Sep 17 00:00:00 2001 From: Pranav C Date: Tue, 27 Dec 2022 13:09:56 +0530 Subject: [PATCH 2/4] fix(nocodb): attachment api middleware corrections Signed-off-by: Pranav C --- .../nocodb/src/lib/meta/api/attachmentApis.ts | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/nocodb/src/lib/meta/api/attachmentApis.ts b/packages/nocodb/src/lib/meta/api/attachmentApis.ts index 535d227da0..53506414cf 100644 --- a/packages/nocodb/src/lib/meta/api/attachmentApis.ts +++ b/packages/nocodb/src/lib/meta/api/attachmentApis.ts @@ -15,11 +15,15 @@ import NcPluginMgrv2 from '../helpers/NcPluginMgrv2'; import { NC_ATTACHMENT_FIELD_SIZE } from '../../constants'; const isUploadAllowed = async (req: Request, _res: Response, next: any) => { + if (!req['user']?.id) { + NcError.unauthorized('Unauthorized'); + } + try { // check user is super admin or creator if ( - req['user']?.roles?.includes(OrgUserRoles.SUPER_ADMIN) || - req['user']?.roles?.includes(OrgUserRoles.CREATOR) || + req['user'].roles?.includes(OrgUserRoles.SUPER_ADMIN) || + req['user'].roles?.includes(OrgUserRoles.CREATOR) || // if viewer then check at-least one project have editor or higher role // todo: cache !!(await Noco.ncMeta @@ -29,7 +33,7 @@ const isUploadAllowed = async (req: Request, _res: Response, next: any) => { this.orWhere('roles', ProjectRoles.CREATOR); this.orWhere('roles', ProjectRoles.EDITOR); }) - .andWhere('fk_user_id', req['user']?.id) + .andWhere('fk_user_id', req['user'].id) .first()) ) return next(); @@ -182,12 +186,20 @@ router.post( fieldSize: NC_ATTACHMENT_FIELD_SIZE, }, }).any(), - [extractProjectIdAndAuthenticate, isUploadAllowed, catchError(upload)] + [ + extractProjectIdAndAuthenticate, + catchError(isUploadAllowed), + catchError(upload), + ] ); router.post( '/api/v1/db/storage/upload-by-url', - [extractProjectIdAndAuthenticate, isUploadAllowed, catchError(uploadViaURL)] + [ + extractProjectIdAndAuthenticate, + catchError(isUploadAllowed), + catchError(uploadViaURL), + ] ); router.get(/^\/download\/(.+)$/, catchError(fileRead)); From 016480962990c09357984e64df96a3e00b72ab3a Mon Sep 17 00:00:00 2001 From: Pranav C Date: Tue, 27 Dec 2022 13:10:59 +0530 Subject: [PATCH 3/4] test: add unit tests for attachment apis Signed-off-by: Pranav C --- packages/nocodb/tests/unit/rest/index.test.ts | 2 + .../tests/unit/rest/tests/attachment.test.ts | 188 ++++++++++++++++++ 2 files changed, 190 insertions(+) create mode 100644 packages/nocodb/tests/unit/rest/tests/attachment.test.ts diff --git a/packages/nocodb/tests/unit/rest/index.test.ts b/packages/nocodb/tests/unit/rest/index.test.ts index b95637a096..8227a1b5e4 100644 --- a/packages/nocodb/tests/unit/rest/index.test.ts +++ b/packages/nocodb/tests/unit/rest/index.test.ts @@ -6,6 +6,7 @@ import columnTypeSpecificTests from './tests/columnTypeSpecific.test'; import tableTests from './tests/table.test'; import tableRowTests from './tests/tableRow.test'; import viewRowTests from './tests/viewRow.test'; +import attachmentTests from './tests/attachment.test'; function restTests() { authTests(); @@ -15,6 +16,7 @@ function restTests() { tableRowTests(); viewRowTests(); columnTypeSpecificTests(); + attachmentTests(); } export default function () { diff --git a/packages/nocodb/tests/unit/rest/tests/attachment.test.ts b/packages/nocodb/tests/unit/rest/tests/attachment.test.ts new file mode 100644 index 0000000000..5ff984d03d --- /dev/null +++ b/packages/nocodb/tests/unit/rest/tests/attachment.test.ts @@ -0,0 +1,188 @@ +import { expect } from 'chai' +import fs from 'fs' +import { OrgUserRoles, ProjectRoles } from 'nocodb-sdk' +import path from 'path' +import 'mocha' +import request from 'supertest' +import { createProject } from '../../factory/project' +import init from '../../init' + +const FILE_PATH = path.join(__dirname, 'test.txt') + +function attachmentTests() { + let context + + + beforeEach(async function() { + context = await init() + fs.writeFileSync(FILE_PATH, 'test', `utf-8`) + context = await init() + }) + + + afterEach(function() { + fs.unlinkSync(FILE_PATH) + }) + + + it('Upload file - Super admin', async () => { + const response = await request(context.app) + .post('/api/v1/db/storage/upload') + .attach('files', FILE_PATH) + // .set('xc-auth', context.token) + .expect(200) + + + const attachments = response.body + expect(attachments).to.be.an('array') + expect(attachments[0].title).to.be.eq(path.basename(FILE_PATH)) + }) + + it('Upload file - Without token', async () => { + const response = await request(context.app) + .post('/api/v1/db/storage/upload') + .attach('files', FILE_PATH) + .expect(401) + + const msg = response.body.msg + expect(msg).to.be.eq('Unauthorized') + }) + + it('Upload file - Super admin', async () => { + const response = await request(context.app) + .post('/api/v1/db/storage/upload') + .attach('files', FILE_PATH) + .set('xc-auth', context.token) + .expect(200) + + const attachments = response.body + expect(attachments).to.be.an('array') + expect(attachments[0].title).to.be.eq(path.basename(FILE_PATH)) + }) + + it('Upload file - Org level viewer', async () => { + + // signup a user + const args = { + email: 'dummyuser@example.com', + password: 'A1234abh2@dsad', + } + + const signupResponse = await request(context.app) + .post('/api/v1/auth/user/signup') + .send(args) + .expect(200) + + const response = await request(context.app) + .post('/api/v1/db/storage/upload') + .attach('files', FILE_PATH) + .set('xc-auth', signupResponse.body.token) + .expect(400) + + const msg = response.body.msg + expect(msg).to.be.eq('Upload not allowed') + }) + + + it('Upload file - Org level creator', async () => { + + // signup a user + const args = { + email: 'dummyuser@example.com', + password: 'A1234abh2@dsad', + } + + + await request(context.app) + .post('/api/v1/auth/user/signup') + .send(args) + .expect(200) + + // update user role to creator + const usersListResponse = await request(context.app) + .get('/api/v1/users') + .set('xc-auth', context.token) + .expect(200) + + const user = usersListResponse.body.list.find(u => u.email === args.email) + + expect(user).to.have.property('roles').to.be.equal(OrgUserRoles.VIEWER) + + + await request(context.app) + .patch('/api/v1/users/' + user.id) + .set('xc-auth', context.token) + .send({ roles: OrgUserRoles.CREATOR }) + .expect(200) + + + const signinResponse = await request(context.app) + .post('/api/v1/auth/user/signin') + // pass empty data in await request + .send(args) + .expect(200) + + const response = await request(context.app) + .post('/api/v1/db/storage/upload') + .attach('files', FILE_PATH) + .set('xc-auth', signinResponse.body.token) + .expect(200) + + const attachments = response.body + expect(attachments).to.be.an('array') + expect(attachments[0].title).to.be.eq(path.basename(FILE_PATH)) + }) + + + it('Upload file - Org level viewer with editor role in a project', async () => { + + // signup a new user + const args = { + email: 'dummyuser@example.com', + password: 'A1234abh2@dsad', + } + + await request(context.app) + .post('/api/v1/auth/user/signup') + .send(args) + .expect(200) + + const newProject = await createProject(context, { + title: 'NewTitle1', + }) + + // invite user to project with editor role + await request(context.app) + .post(`/api/v1/db/meta/projects/${newProject.id}/users`) + .set('xc-auth', context.token) + .send({ + roles: ProjectRoles.EDITOR, + email: args.email, + project_id: newProject.id, + projectName: newProject.title, + }) + .expect(200) + + // signin to get user token + const signinResponse = await request(context.app) + .post('/api/v1/auth/user/signin') + // pass empty data in await request + .send(args) + .expect(200) + + const response = await request(context.app) + .post('/api/v1/db/storage/upload') + .attach('files', FILE_PATH) + .set('xc-auth', signinResponse.body.token) + .expect(200) + + const attachments = response.body + expect(attachments).to.be.an('array') + expect(attachments[0].title).to.be.eq(path.basename(FILE_PATH)) + }) + +} + +export default function() { + describe('Attachment', attachmentTests) +} From a628ff17d5e58a4d3f1f708068505c82122661b7 Mon Sep 17 00:00:00 2001 From: Pranav C Date: Tue, 27 Dec 2022 14:05:53 +0530 Subject: [PATCH 4/4] test: remove duplicate unit test Signed-off-by: Pranav C --- .../tests/unit/rest/tests/attachment.test.ts | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/packages/nocodb/tests/unit/rest/tests/attachment.test.ts b/packages/nocodb/tests/unit/rest/tests/attachment.test.ts index 5ff984d03d..29922c76ba 100644 --- a/packages/nocodb/tests/unit/rest/tests/attachment.test.ts +++ b/packages/nocodb/tests/unit/rest/tests/attachment.test.ts @@ -29,7 +29,7 @@ function attachmentTests() { const response = await request(context.app) .post('/api/v1/db/storage/upload') .attach('files', FILE_PATH) - // .set('xc-auth', context.token) + .set('xc-auth', context.token) .expect(200) @@ -48,18 +48,6 @@ function attachmentTests() { expect(msg).to.be.eq('Unauthorized') }) - it('Upload file - Super admin', async () => { - const response = await request(context.app) - .post('/api/v1/db/storage/upload') - .attach('files', FILE_PATH) - .set('xc-auth', context.token) - .expect(200) - - const attachments = response.body - expect(attachments).to.be.an('array') - expect(attachments[0].title).to.be.eq(path.basename(FILE_PATH)) - }) - it('Upload file - Org level viewer', async () => { // signup a user