Merge pull request #937 from hedgedoc/feature/forbiddenNoteIds

This commit is contained in:
David Mehren 2021-03-14 16:06:48 +01:00 committed by GitHub
commit 99439af25e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
18 changed files with 231 additions and 22 deletions

View file

@ -23,6 +23,8 @@ import { HistoryEntry } from '../../../history/history-entry.entity';
import { NoteGroupPermission } from '../../../permissions/note-group-permission.entity';
import { NoteUserPermission } from '../../../permissions/note-user-permission.entity';
import { Group } from '../../../groups/group.entity';
import { ConfigModule } from '@nestjs/config';
import appConfigMock from '../../../config/app.config.mock';
describe('Me Controller', () => {
let controller: MeController;
@ -30,7 +32,16 @@ describe('Me Controller', () => {
beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
controllers: [MeController],
imports: [UsersModule, HistoryModule, NotesModule, LoggerModule],
imports: [
UsersModule,
HistoryModule,
NotesModule,
LoggerModule,
ConfigModule.forRoot({
isGlobal: true,
load: [appConfigMock],
}),
],
})
.overrideProvider(getRepositoryToken(User))
.useValue({})

View file

@ -26,6 +26,8 @@ import { NoteGroupPermission } from '../../../permissions/note-group-permission.
import { NoteUserPermission } from '../../../permissions/note-user-permission.entity';
import { Group } from '../../../groups/group.entity';
import { GroupsModule } from '../../../groups/groups.module';
import { ConfigModule } from '@nestjs/config';
import appConfigMock from '../../../config/app.config.mock';
describe('Notes Controller', () => {
let controller: NotesController;
@ -51,6 +53,10 @@ describe('Notes Controller', () => {
LoggerModule,
PermissionsModule,
HistoryModule,
ConfigModule.forRoot({
isGlobal: true,
load: [appConfigMock],
}),
],
})
.overrideProvider(getRepositoryToken(Note))

View file

@ -21,6 +21,7 @@ import {
} from '@nestjs/common';
import {
AlreadyInDBError,
ForbiddenIdError,
NotInDBError,
PermissionsUpdateInconsistentError,
} from '../../../errors/errors';
@ -86,6 +87,9 @@ export class NotesController {
if (e instanceof NotInDBError) {
throw new NotFoundException(e.message);
}
if (e instanceof ForbiddenIdError) {
throw new BadRequestException(e.message);
}
throw e;
}
if (!this.permissionsService.mayRead(req.user, note)) {
@ -114,6 +118,9 @@ export class NotesController {
if (e instanceof AlreadyInDBError) {
throw new BadRequestException(e.message);
}
if (e instanceof ForbiddenIdError) {
throw new BadRequestException(e.message);
}
throw e;
}
}
@ -137,6 +144,9 @@ export class NotesController {
if (e instanceof NotInDBError) {
throw new NotFoundException(e.message);
}
if (e instanceof ForbiddenIdError) {
throw new BadRequestException(e.message);
}
throw e;
}
}
@ -161,6 +171,9 @@ export class NotesController {
if (e instanceof NotInDBError) {
throw new NotFoundException(e.message);
}
if (e instanceof ForbiddenIdError) {
throw new BadRequestException(e.message);
}
throw e;
}
}
@ -182,6 +195,9 @@ export class NotesController {
if (e instanceof NotInDBError) {
throw new NotFoundException(e.message);
}
if (e instanceof ForbiddenIdError) {
throw new BadRequestException(e.message);
}
throw e;
}
}
@ -205,6 +221,9 @@ export class NotesController {
if (e instanceof PermissionsUpdateInconsistentError) {
throw new BadRequestException(e.message);
}
if (e instanceof ForbiddenIdError) {
throw new BadRequestException(e.message);
}
throw e;
}
}
@ -228,6 +247,9 @@ export class NotesController {
if (e instanceof NotInDBError) {
throw new NotFoundException(e.message);
}
if (e instanceof ForbiddenIdError) {
throw new BadRequestException(e.message);
}
throw e;
}
}
@ -253,6 +275,9 @@ export class NotesController {
if (e instanceof NotInDBError) {
throw new NotFoundException(e.message);
}
if (e instanceof ForbiddenIdError) {
throw new BadRequestException(e.message);
}
throw e;
}
}
@ -276,6 +301,9 @@ export class NotesController {
if (e instanceof NotInDBError) {
throw new NotFoundException(e.message);
}
if (e instanceof ForbiddenIdError) {
throw new BadRequestException(e.message);
}
throw e;
}
}

View file

@ -8,4 +8,5 @@ import { registerAs } from '@nestjs/config';
export default registerAs('appConfig', () => ({
port: 3000,
forbiddenNoteIds: ['forbiddenNoteId'],
}));

View file

@ -7,12 +7,13 @@
import { registerAs } from '@nestjs/config';
import * as Joi from 'joi';
import { Loglevel } from './loglevel.enum';
import { buildErrorMessage } from './utils';
import { buildErrorMessage, toArrayConfig } from './utils';
export interface AppConfig {
domain: string;
port: number;
loglevel: Loglevel;
forbiddenNoteIds: string[];
}
const schema = Joi.object({
@ -23,6 +24,10 @@ const schema = Joi.object({
.default(Loglevel.WARN)
.optional()
.label('HD_LOGLEVEL'),
forbiddenNoteIds: Joi.string()
.optional()
.default([])
.label('HD_FORBIDDEN_NOTE_IDS'),
});
export default registerAs('appConfig', () => {
@ -31,6 +36,7 @@ export default registerAs('appConfig', () => {
domain: process.env.HD_DOMAIN,
port: parseInt(process.env.PORT) || undefined,
loglevel: process.env.HD_LOGLEVEL,
forbiddenNoteIds: toArrayConfig(process.env.HD_FORBIDDEN_NOTE_IDS, ','),
},
{
abortEarly: false,

View file

@ -12,6 +12,10 @@ export class AlreadyInDBError extends Error {
name = 'AlreadyInDBError';
}
export class ForbiddenIdError extends Error {
name = 'ForbiddenIdError';
}
export class ClientError extends Error {
name = 'ClientError';
}

View file

@ -11,6 +11,7 @@ import { TypeOrmModule } from '@nestjs/typeorm';
import { HistoryEntry } from './history-entry.entity';
import { UsersModule } from '../users/users.module';
import { NotesModule } from '../notes/notes.module';
import { ConfigModule } from '@nestjs/config';
@Module({
providers: [HistoryService],
@ -20,6 +21,7 @@ import { NotesModule } from '../notes/notes.module';
TypeOrmModule.forFeature([HistoryEntry]),
UsersModule,
NotesModule,
ConfigModule,
],
})
export class HistoryModule {}

View file

@ -30,6 +30,8 @@ import { NotInDBError } from '../errors/errors';
import { NoteGroupPermission } from '../permissions/note-group-permission.entity';
import { NoteUserPermission } from '../permissions/note-user-permission.entity';
import { Group } from '../groups/group.entity';
import { ConfigModule } from '@nestjs/config';
import appConfigMock from '../config/app.config.mock';
describe('HistoryService', () => {
let service: HistoryService;
@ -45,7 +47,15 @@ describe('HistoryService', () => {
useClass: Repository,
},
],
imports: [LoggerModule, UsersModule, NotesModule],
imports: [
LoggerModule,
UsersModule,
NotesModule,
ConfigModule.forRoot({
isGlobal: true,
load: [appConfigMock],
}),
],
})
.overrideProvider(getRepositoryToken(User))
.useValue({})

View file

@ -34,6 +34,7 @@ import { ClientError, NotInDBError, PermissionError } from '../errors/errors';
import { NoteGroupPermission } from '../permissions/note-group-permission.entity';
import { NoteUserPermission } from '../permissions/note-user-permission.entity';
import { Group } from '../groups/group.entity';
import appConfigMock from '../../src/config/app.config.mock';
describe('MediaService', () => {
let service: MediaService;
@ -54,7 +55,7 @@ describe('MediaService', () => {
imports: [
ConfigModule.forRoot({
isGlobal: true,
load: [mediaConfigMock],
load: [mediaConfigMock, appConfigMock],
}),
LoggerModule,
NotesModule,

View file

@ -16,6 +16,7 @@ import { Tag } from './tag.entity';
import { NoteGroupPermission } from '../permissions/note-group-permission.entity';
import { NoteUserPermission } from '../permissions/note-user-permission.entity';
import { GroupsModule } from '../groups/groups.module';
import { ConfigModule } from '@nestjs/config';
@Module({
imports: [
@ -30,6 +31,7 @@ import { GroupsModule } from '../groups/groups.module';
UsersModule,
GroupsModule,
LoggerModule,
ConfigModule,
],
controllers: [],
providers: [NotesService],

View file

@ -26,6 +26,8 @@ import { NotesService } from './notes.service';
import { Repository } from 'typeorm';
import { Tag } from './tag.entity';
import {
AlreadyInDBError,
ForbiddenIdError,
NotInDBError,
PermissionsUpdateInconsistentError,
} from '../errors/errors';
@ -37,6 +39,8 @@ import { NoteGroupPermission } from '../permissions/note-group-permission.entity
import { NoteUserPermission } from '../permissions/note-user-permission.entity';
import { GroupsModule } from '../groups/groups.module';
import { Group } from '../groups/group.entity';
import { ConfigModule, ConfigService } from '@nestjs/config';
import appConfigMock from '../config/app.config.mock';
describe('NotesService', () => {
let service: NotesService;
@ -44,6 +48,7 @@ describe('NotesService', () => {
let revisionRepo: Repository<Revision>;
let userRepo: Repository<User>;
let groupRepo: Repository<Group>;
let forbiddenNoteId: string;
beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
@ -58,7 +63,16 @@ describe('NotesService', () => {
useClass: Repository,
},
],
imports: [LoggerModule, UsersModule, GroupsModule, RevisionsModule],
imports: [
ConfigModule.forRoot({
isGlobal: true,
load: [appConfigMock],
}),
LoggerModule,
UsersModule,
GroupsModule,
RevisionsModule,
],
})
.overrideProvider(getRepositoryToken(Note))
.useClass(Repository)
@ -84,6 +98,8 @@ describe('NotesService', () => {
.useClass(Repository)
.compile();
const config = module.get<ConfigService>(ConfigService);
forbiddenNoteId = config.get('appConfig').forbiddenNoteIds[0];
service = module.get<NotesService>(NotesService);
noteRepo = module.get<Repository<Note>>(getRepositoryToken(Note));
revisionRepo = module.get<Repository<Revision>>(
@ -124,10 +140,10 @@ describe('NotesService', () => {
});
describe('createNote', () => {
const user = User.create('hardcoded', 'Testy') as User;
const alias = 'alias';
const content = 'testContent';
describe('works', () => {
const user = User.create('hardcoded', 'Testy') as User;
const alias = 'alias';
const content = 'testContent';
beforeEach(() => {
jest
.spyOn(noteRepo, 'save')
@ -184,6 +200,26 @@ describe('NotesService', () => {
expect(newNote.alias).toEqual(alias);
});
});
describe('fails:', () => {
it('alias is forbidden', async () => {
try {
await service.createNote(content, forbiddenNoteId);
} catch (e) {
expect(e).toBeInstanceOf(ForbiddenIdError);
}
});
it('alias is already used', async () => {
jest.spyOn(noteRepo, 'save').mockImplementationOnce(async () => {
throw new Error();
});
try {
await service.createNote(content, alias);
} catch (e) {
expect(e).toBeInstanceOf(AlreadyInDBError);
}
});
});
});
describe('getNoteContent', () => {
@ -241,13 +277,23 @@ describe('NotesService', () => {
const foundNote = await service.getNoteByIdOrAlias('noteThatExists');
expect(foundNote).toEqual(note);
});
it('fails: no note found', async () => {
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(undefined);
try {
await service.getNoteByIdOrAlias('noteThatDoesNoteExist');
} catch (e) {
expect(e).toBeInstanceOf(NotInDBError);
}
describe('fails:', () => {
it('no note found', async () => {
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(undefined);
try {
await service.getNoteByIdOrAlias('noteThatDoesNoteExist');
} catch (e) {
expect(e).toBeInstanceOf(NotInDBError);
}
});
it('id is forbidden', async () => {
jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(undefined);
try {
await service.getNoteByIdOrAlias(forbiddenNoteId);
} catch (e) {
expect(e).toBeInstanceOf(ForbiddenIdError);
}
});
});
});

View file

@ -9,6 +9,7 @@ import { InjectRepository } from '@nestjs/typeorm';
import { Repository } from 'typeorm';
import {
AlreadyInDBError,
ForbiddenIdError,
NotInDBError,
PermissionsUpdateInconsistentError,
} from '../errors/errors';
@ -30,6 +31,7 @@ import { NoteUserPermission } from '../permissions/note-user-permission.entity';
import { NoteGroupPermission } from '../permissions/note-group-permission.entity';
import { GroupsService } from '../groups/groups.service';
import { checkArrayForDuplicates } from '../utils/arrayDuplicatCheck';
import appConfiguration, { AppConfig } from '../config/app.config';
@Injectable()
export class NotesService {
@ -41,6 +43,8 @@ export class NotesService {
@Inject(GroupsService) private groupsService: GroupsService,
@Inject(forwardRef(() => RevisionsService))
private revisionsService: RevisionsService,
@Inject(appConfiguration.KEY)
private appConfig: AppConfig,
) {
this.logger.setContext(NotesService.name);
}
@ -88,6 +92,15 @@ export class NotesService {
]);
if (alias) {
newNote.alias = alias;
if (this.appConfig.forbiddenNoteIds.includes(alias)) {
this.logger.debug(
`Creating a note with the alias '${alias}' is forbidden by the administrator.`,
'createNote',
);
throw new ForbiddenIdError(
`Creating a note with the alias '${alias}' is forbidden by the administrator.`,
);
}
}
if (owner) {
newNote.historyEntries = [HistoryEntry.create(owner)];
@ -148,6 +161,15 @@ export class NotesService {
`Trying to find note '${noteIdOrAlias}'`,
'getNoteByIdOrAlias',
);
if (this.appConfig.forbiddenNoteIds.includes(noteIdOrAlias)) {
this.logger.debug(
`Accessing a note with the alias '${noteIdOrAlias}' is forbidden by the administrator.`,
'getNoteByIdOrAlias',
);
throw new ForbiddenIdError(
`Accessing a note with the alias '${noteIdOrAlias}' is forbidden by the administrator.`,
);
}
const note = await this.noteRepository.findOne({
where: [
{

View file

@ -28,6 +28,8 @@ import { NoteGroupPermission } from './note-group-permission.entity';
import { NoteUserPermission } from './note-user-permission.entity';
import { PermissionsModule } from './permissions.module';
import { GuestPermission, PermissionsService } from './permissions.service';
import { ConfigModule } from '@nestjs/config';
import appConfigMock from '../config/app.config.mock';
describe('PermissionsService', () => {
let permissionsService: PermissionsService;
@ -35,7 +37,16 @@ describe('PermissionsService', () => {
beforeAll(async () => {
const module: TestingModule = await Test.createTestingModule({
providers: [PermissionsService],
imports: [PermissionsModule, UsersModule, LoggerModule, NotesModule],
imports: [
PermissionsModule,
UsersModule,
LoggerModule,
NotesModule,
ConfigModule.forRoot({
isGlobal: true,
load: [appConfigMock],
}),
],
})
.overrideProvider(getRepositoryToken(User))
.useValue({})

View file

@ -11,12 +11,14 @@ import { NotesModule } from '../notes/notes.module';
import { Authorship } from './authorship.entity';
import { Revision } from './revision.entity';
import { RevisionsService } from './revisions.service';
import { ConfigModule } from '@nestjs/config';
@Module({
imports: [
TypeOrmModule.forFeature([Revision, Authorship]),
forwardRef(() => NotesModule),
LoggerModule,
ConfigModule,
],
providers: [RevisionsService],
exports: [RevisionsService],

View file

@ -20,6 +20,8 @@ import { Tag } from '../notes/tag.entity';
import { NoteGroupPermission } from '../permissions/note-group-permission.entity';
import { NoteUserPermission } from '../permissions/note-user-permission.entity';
import { Group } from '../groups/group.entity';
import { ConfigModule } from '@nestjs/config';
import appConfigMock from '../config/app.config.mock';
describe('RevisionsService', () => {
let service: RevisionsService;
@ -33,7 +35,14 @@ describe('RevisionsService', () => {
useValue: {},
},
],
imports: [NotesModule, LoggerModule],
imports: [
NotesModule,
LoggerModule,
ConfigModule.forRoot({
isGlobal: true,
load: [appConfigMock],
}),
],
})
.overrideProvider(getRepositoryToken(Authorship))
.useValue({})

View file

@ -31,6 +31,7 @@ import { UsersModule } from '../../src/users/users.module';
import { HistoryModule } from '../../src/history/history.module';
import { ConfigModule } from '@nestjs/config';
import mediaConfigMock from '../../src/config/media.config.mock';
import appConfigMock from '../../src/config/app.config.mock';
import { User } from '../../src/users/user.entity';
// TODO Tests have to be reworked using UserService functions
@ -47,7 +48,7 @@ describe('Notes', () => {
imports: [
ConfigModule.forRoot({
isGlobal: true,
load: [mediaConfigMock],
load: [mediaConfigMock, appConfigMock],
}),
PublicApiModule,
NotesModule,

View file

@ -17,6 +17,7 @@ import { promises as fs } from 'fs';
import * as request from 'supertest';
import { PublicApiModule } from '../../src/api/public/public-api.module';
import mediaConfigMock from '../../src/config/media.config.mock';
import appConfigMock from '../../src/config/app.config.mock';
import { GroupsModule } from '../../src/groups/groups.module';
import { LoggerModule } from '../../src/logger/logger.module';
import { NestConsoleLoggerService } from '../../src/logger/nest-console-logger.service';
@ -40,7 +41,7 @@ describe('Notes', () => {
imports: [
ConfigModule.forRoot({
isGlobal: true,
load: [mediaConfigMock],
load: [mediaConfigMock, appConfigMock],
}),
PublicApiModule,
MediaModule,

View file

@ -10,12 +10,13 @@
*/
import { INestApplication } from '@nestjs/common';
import { ConfigModule } from '@nestjs/config';
import { ConfigModule, ConfigService } from '@nestjs/config';
import { Test } from '@nestjs/testing';
import { TypeOrmModule } from '@nestjs/typeorm';
import * as request from 'supertest';
import { PublicApiModule } from '../../src/api/public/public-api.module';
import mediaConfigMock from '../../src/config/media.config.mock';
import appConfigMock from '../../src/config/app.config.mock';
import { NotInDBError } from '../../src/errors/errors';
import { GroupsModule } from '../../src/groups/groups.module';
import { LoggerModule } from '../../src/logger/logger.module';
@ -34,13 +35,14 @@ describe('Notes', () => {
let notesService: NotesService;
let user: User;
let content: string;
let forbiddenNoteId: string;
beforeAll(async () => {
const moduleRef = await Test.createTestingModule({
imports: [
ConfigModule.forRoot({
isGlobal: true,
load: [mediaConfigMock],
load: [mediaConfigMock, appConfigMock],
}),
PublicApiModule,
NotesModule,
@ -62,6 +64,8 @@ describe('Notes', () => {
.useClass(MockAuthGuard)
.compile();
const config = moduleRef.get<ConfigService>(ConfigService);
forbiddenNoteId = config.get('appConfig').forbiddenNoteIds[0];
app = moduleRef.createNestApplication();
await app.init();
notesService = moduleRef.get(NotesService);
@ -120,6 +124,15 @@ describe('Notes', () => {
).toEqual(content);
});
it('fails with a forbidden alias', async () => {
await request(app.getHttpServer())
.post(`/notes/${forbiddenNoteId}`)
.set('Content-Type', 'text/markdown')
.send(content)
.expect('Content-Type', /json/)
.expect(400);
});
it('fails with a existing alias', async () => {
await request(app.getHttpServer())
.post('/notes/test2')
@ -138,6 +151,11 @@ describe('Notes', () => {
new NotInDBError("Note with id/alias 'test3' not found."),
);
});
it('fails with a forbidden alias', async () => {
await request(app.getHttpServer())
.delete(`/notes/${forbiddenNoteId}`)
.expect(400);
});
it('fails with a non-existing alias', async () => {
await request(app.getHttpServer())
.delete('/notes/i_dont_exist')
@ -161,6 +179,13 @@ describe('Notes', () => {
).toEqual(changedContent);
expect(response.body.content).toEqual(changedContent);
});
it('fails with a forbidden alias', async () => {
await request(app.getHttpServer())
.put(`/notes/${forbiddenNoteId}`)
.set('Content-Type', 'text/markdown')
.send(changedContent)
.expect(400);
});
it('fails with a non-existing alias', async () => {
await request(app.getHttpServer())
.put('/notes/i_dont_exist')
@ -195,6 +220,12 @@ describe('Notes', () => {
expect(metadata.body.editedBy).toEqual([]);
});
it('fails with a forbidden alias', async () => {
await request(app.getHttpServer())
.get(`/notes/${forbiddenNoteId}/metadata`)
.expect(400);
});
it('fails with non-existing alias', async () => {
// check if a missing note correctly returns 404
await request(app.getHttpServer())
@ -230,6 +261,12 @@ describe('Notes', () => {
expect(response.body).toHaveLength(1);
});
it('fails with a forbidden alias', async () => {
await request(app.getHttpServer())
.get(`/notes/${forbiddenNoteId}/revisions`)
.expect(400);
});
it('fails with non-existing alias', async () => {
// check if a missing note correctly returns 404
await request(app.getHttpServer())
@ -249,6 +286,11 @@ describe('Notes', () => {
.expect(200);
expect(response.body.content).toEqual(content);
});
it('fails with a forbidden alias', async () => {
await request(app.getHttpServer())
.get(`/notes/${forbiddenNoteId}/revisions/1`)
.expect(400);
});
it('fails with non-existing alias', async () => {
// check if a missing note correctly returns 404
await request(app.getHttpServer())
@ -266,7 +308,11 @@ describe('Notes', () => {
.expect(200);
expect(response.text).toEqual(content);
});
it('fails with a forbidden alias', async () => {
await request(app.getHttpServer())
.get(`/notes/${forbiddenNoteId}/content`)
.expect(400);
});
it('fails with non-existing alias', async () => {
// check if a missing note correctly returns 404
await request(app.getHttpServer())