diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ed4495a2..95ebb40e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,10 +14,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added - Translatable SnackBarService. [#922](https://github.com/geli-lms/geli/issues/922) - `ProgressController` `GET` unit tests & access denial tests in general. [#1116](https://github.com/geli-lms/geli/issues/1116) +- `UnitController` `GET` & `DELETE` route unit tests for status code `200`. [#1190](https://github.com/geli-lms/geli/issues/1190) +- `UnitController` status code `403` (not authorized to view / edit course) unit tests for all routes. [#1190](https://github.com/geli-lms/geli/issues/1190) ### Changed -- Extended `ProgressController` `PUT` route to handle both creation and updates. [#1116](https://github.com/geli-lms/geli/issues/1116) -- Refactored `ProgressController` unit tests in general. [#1116](https://github.com/geli-lms/geli/issues/1116) +- Extend `ProgressController` `PUT` route to handle both creation and updates. [#1116](https://github.com/geli-lms/geli/issues/1116) +- Refactor `ProgressController` unit tests in general. [#1116](https://github.com/geli-lms/geli/issues/1116) - Instead of a list of progress data, the `ProgressController` `GET` route now responds with a single progress object or an empty object if no data can be found. [#1116](https://github.com/geli-lms/geli/issues/1116) ### Removed @@ -29,7 +31,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `CodeKataComponent` `progress.code` loading. [#1116](https://github.com/geli-lms/geli/issues/1116) ### Security -- Closed `ProgressController` vulnerabilities. [#1116](https://github.com/geli-lms/geli/issues/1116) +- Close `ProgressController` vulnerabilities. [#1116](https://github.com/geli-lms/geli/issues/1116) +- Close `UnitController` vulnerabilities. [#1190](https://github.com/geli-lms/geli/issues/1190) ## [[0.8.4](https://github.com/geli-lms/geli/releases/tag/v0.8.4)] - 2018-12-20 - WS 18/19 ❄️-Release ### Added diff --git a/api/src/config/errorCodes.ts b/api/src/config/errorCodes.ts index 60dd2f8ca..b4812fd30 100644 --- a/api/src/config/errorCodes.ts +++ b/api/src/config/errorCodes.ts @@ -175,5 +175,19 @@ export const errorCodes = { code: 'pastDeadline', text: 'Past deadline, no further update possible' } + }, + unit: { + postMissingLectureId: { + code: 'postMissingLectureId', + text: 'No lecture ID was submitted.' + }, + postMissingUnit: { + code: 'postMissingUnit', + text: 'No unit was submitted.' + }, + postMissingCourse: { + code: 'postMissingCourse', + text: 'Unit has no _course set' + } } }; diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts index c72e857c1..c2c1c3054 100644 --- a/api/src/controllers/UnitController.ts +++ b/api/src/controllers/UnitController.ts @@ -1,9 +1,10 @@ import { Body, Get, Put, Delete, Param, JsonController, UseBefore, NotFoundError, BadRequestError, Post, - Authorized, CurrentUser + Authorized, CurrentUser, ForbiddenError } from 'routing-controllers'; import passportJwtMiddleware from '../security/passportJwtMiddleware'; - +import {errorCodes} from '../config/errorCodes'; +import {Course} from '../models/Course'; import {Lecture} from '../models/Lecture'; import {IUnitModel, Unit} from '../models/units/Unit'; import {IUser} from '../../../shared/models/IUser'; @@ -34,13 +35,13 @@ export class UnitController { * "type": "free-text", * "__v": 0 * } + * + * @apiError NotFoundError + * @apiError ForbiddenError */ @Get('/:id') - async getUnit(@Param('id') id: string) { - const unit = await Unit.findById(id); - if (!unit) { - throw new NotFoundError(); - } + async getUnit(@Param('id') id: string, @CurrentUser() currentUser: IUser) { + const unit = await this.getUnitFor(id, currentUser, 'userCanViewCourse'); return unit.toObject(); } @@ -75,6 +76,7 @@ export class UnitController { * @apiError BadRequestError No unit was submitted. * @apiError BadRequestError Unit has no _course set. * @apiError BadRequestError + * @apiError ForbiddenError * @apiError ValidationError */ @Authorized(['teacher', 'admin']) @@ -82,6 +84,12 @@ export class UnitController { async addUnit(@Body() data: any, @CurrentUser() currentUser: IUser) { // discard invalid requests this.checkPostParam(data); + + const course = await Course.findById(data.model._course); + if (!course.checkPrivileges(currentUser).userCanEditCourse) { + throw new ForbiddenError(); + } + // Set current user as creator, old unit's dont have a creator data.model.unitCreator = currentUser._id; try { @@ -122,19 +130,16 @@ export class UnitController { * "__v": 0 * } * - * @apiError NotFoundError * @apiError BadRequestError Invalid combination of file upload and unit data. * @apiError BadRequestError + * @apiError NotFoundError + * @apiError ForbiddenError * @apiError ValidationError */ @Authorized(['teacher', 'admin']) @Put('/:id') - async updateUnit(@Param('id') id: string, @Body() data: any) { - const oldUnit: IUnitModel = await Unit.findById(id); - - if (!oldUnit) { - throw new NotFoundError(); - } + async updateUnit(@Param('id') id: string, @Body() data: any, @CurrentUser() currentUser: IUser) { + const oldUnit = await this.getUnitFor(id, currentUser, 'userCanEditCourse'); try { oldUnit.set(data); @@ -158,32 +163,36 @@ export class UnitController { * * @apiParam {String} id Unit ID. * - * @apiSuccess {Boolean} result Confirmation of deletion. + * @apiSuccess {Object} result Empty object. * * @apiSuccessExample {json} Success-Response: - * { - * "result": true - * } + * {} * * @apiError NotFoundError + * @apiError ForbiddenError */ @Authorized(['teacher', 'admin']) @Delete('/:id') - deleteUnit(@Param('id') id: string) { - return Unit.findById(id).then((unit) => { - if (!unit) { - throw new NotFoundError(); - } + async deleteUnit(@Param('id') id: string, @CurrentUser() currentUser: IUser) { + const unit = await this.getUnitFor(id, currentUser, 'userCanEditCourse'); + + await Lecture.updateMany({}, {$pull: {units: id}}); + await unit.remove(); + return {}; + } + + private async getUnitFor (unitId: string, currentUser: IUser, privilege: 'userCanViewCourse' | 'userCanEditCourse') { + const unit = await Unit.findById(unitId).orFail(new NotFoundError()); + + const course = await Course.findById(unit._course); + if (!course.checkPrivileges(currentUser)[privilege]) { + throw new ForbiddenError(); + } - return Lecture.updateMany({}, {$pull: {units: id}}) - .then(() => unit.remove()) - .then(() => { - return {result: true}; - }); - }); + return unit; } - protected pushToLecture(lectureId: string, unit: any) { + private pushToLecture(lectureId: string, unit: any) { return Lecture.findById(lectureId) .then((lecture) => { lecture.units.push(unit); @@ -200,17 +209,17 @@ export class UnitController { }); } - protected checkPostParam(data: any) { + private checkPostParam(data: any) { if (!data.lectureId) { - throw new BadRequestError('No lecture ID was submitted.'); + throw new BadRequestError(errorCodes.unit.postMissingLectureId.text); } if (!data.model) { - throw new BadRequestError('No unit was submitted.'); + throw new BadRequestError(errorCodes.unit.postMissingUnit.text); } if (!data.model._course) { - throw new BadRequestError('Unit has no _course set'); + throw new BadRequestError(errorCodes.unit.postMissingCourse.text); } } } diff --git a/api/test/integration/unit/codeKataUnit.ts b/api/test/integration/unit/codeKataUnit.ts index 1d5b81e7e..e0318d7be 100644 --- a/api/test/integration/unit/codeKataUnit.ts +++ b/api/test/integration/unit/codeKataUnit.ts @@ -1,20 +1,18 @@ import * as chai from 'chai'; import chaiHttp = require('chai-http'); +import {TestHelper} from '../../TestHelper'; import {Server} from '../../../src/server'; -import {FixtureLoader} from '../../../fixtures/FixtureLoader'; -import {JwtUtils} from '../../../src/security/JwtUtils'; import {User} from '../../../src/models/User'; import {Lecture} from '../../../src/models/Lecture'; -import {Course} from '../../../src/models/Course'; +import {Course, ICourseModel} from '../../../src/models/Course'; import {FixtureUtils} from '../../../fixtures/FixtureUtils'; import {Unit} from '../../../src/models/units/Unit'; import {ICodeKataModel} from '../../../src/models/units/CodeKataUnit'; chai.use(chaiHttp); -const should = chai.should(); const app = new Server().app; const BASE_URL = '/api/units'; -const fixtureLoader = new FixtureLoader(); +const testHelper = new TestHelper(BASE_URL); describe(`CodeKataUnit ${BASE_URL}`, () => { const model = { @@ -36,9 +34,28 @@ describe(`CodeKataUnit ${BASE_URL}`, () => { '\n' + '}' }; - // Before each test we reset the database + beforeEach(async () => { - await fixtureLoader.load(); + await testHelper.resetForNextTest(); + }); + + describe(`GET ${BASE_URL}`, () => { + async function commonGetTest (getUserForCourseFunc: Function, status: number) { + const unit = await Unit.findOne({__t: 'code-kata'}); + const course = await Course.findById(unit._course); + const courseAdmin = await getUserForCourseFunc(course); + + const res = await testHelper.commonUserGetRequest(courseAdmin, `/${unit.id}`); + res.status.should.be.equal(status); + } + + it('should get unit data', async () => { + await commonGetTest(async (course: ICourseModel) => await User.findById(course.courseAdmin), 200); + }); + + it('should deny access to unit data if the user is unauthorized', async () => { + await commonGetTest(FixtureUtils.getUnauthorizedStudentForCourse, 403); + }); }); describe(`POST ${BASE_URL}`, () => { @@ -54,12 +71,7 @@ describe(`CodeKataUnit ${BASE_URL}`, () => { it('should fail with BadRequest (missing lectureId)', async () => { const teacher = await FixtureUtils.getRandomTeacher(); - const res = await chai.request(app) - .post(BASE_URL) - .set('Cookie', `token=${JwtUtils.generateToken(teacher)}`) - .send({model: model}) - .catch(err => err.response); - + const res = await testHelper.commonUserPostRequest(teacher, '', {model}); res.status.should.be.equal(400); }); @@ -68,12 +80,7 @@ describe(`CodeKataUnit ${BASE_URL}`, () => { const course = await Course.findOne({lectures: { $in: [ lecture._id ] }}); const courseAdmin = await User.findOne({_id: course.courseAdmin}); - const res = await chai.request(app) - .post(BASE_URL) - .set('Cookie', `token=${JwtUtils.generateToken(courseAdmin)}`) - .send({lectureId: lecture._id}) - .catch(err => err.response); - + const res = await testHelper.commonUserPostRequest(courseAdmin, '', {lectureId: lecture._id}); res.status.should.be.equal(400); }); @@ -83,11 +90,7 @@ describe(`CodeKataUnit ${BASE_URL}`, () => { const courseAdmin = await User.findOne({_id: course.courseAdmin}); model._course = course._id; - const res = await chai.request(app) - .post(BASE_URL) - .set('Cookie', `token=${JwtUtils.generateToken(courseAdmin)}`) - .send({lectureId: lecture._id, model: model}); - + const res = await testHelper.commonUserPostRequest(courseAdmin, '', {lectureId: lecture._id, model}); res.status.should.be.equal(200); res.body.name.should.equal(model.name); res.body.description.should.equal(model.description); @@ -109,11 +112,7 @@ describe(`CodeKataUnit ${BASE_URL}`, () => { model.definition = undefined; model.test = undefined; - const res = await chai.request(app) - .post(BASE_URL) - .set('Cookie', `token=${JwtUtils.generateToken(courseAdmin)}`) - .send({lectureId: lecture._id, model: model}); - + const res = await testHelper.commonUserPostRequest(courseAdmin, '', {lectureId: lecture._id, model}); res.status.should.be.equal(200); res.body.name.should.equal(model.name); res.body.description.should.equal(model.description); @@ -121,6 +120,18 @@ describe(`CodeKataUnit ${BASE_URL}`, () => { res.body.unitCreator.profile.firstName.should.equal(courseAdmin.profile.firstName); }); + it('should fail to create a new unit for an unauthorized teacher', async () => { + const lecture = await FixtureUtils.getRandomLecture(); + const course = await Course.findOne({lectures: { $in: [ lecture._id ] }}); + const unauthorizedTeacher = await FixtureUtils.getUnauthorizedTeacherForCourse(course); + model._course = course._id; + + const res = await testHelper.commonUserPostRequest(unauthorizedTeacher, '', {lectureId: lecture._id, model}); + res.status.should.be.equal(403); + }); + }); + + describe(`PUT ${BASE_URL}`, () => { it('should update a codeKata', async () => { const unit = await Unit.findOne({__t: 'code-kata'}); const lecture = await Lecture.findOne({units: { $in: [ unit._id ] }}); @@ -128,13 +139,47 @@ describe(`CodeKataUnit ${BASE_URL}`, () => { const courseAdmin = await User.findOne({_id: course.courseAdmin}); (unit).test += '\n// Test if we can edit a Kata'; - const res = await chai.request(app) - .put(BASE_URL + '/' + unit.id) - .set('Cookie', `token=${JwtUtils.generateToken(courseAdmin)}`) - .send(unit.toObject()); - + const res = await testHelper.commonUserPutRequest(courseAdmin, `/${unit.id}`, unit.toObject()); res.status.should.be.equal(200); res.body.test.should.string('// Test if we can edit a Kata'); }); + + it('should fail to update a unit for an unauthorized teacher', async () => { + const unit = await Unit.findOne({__t: 'code-kata'}); + const lecture = await Lecture.findOne({units: { $in: [ unit._id ] }}); + const course = await Course.findOne({lectures: { $in: [ lecture._id ] }}); + (unit).test += '\n// Test if we can edit a Kata'; + const unauthorizedTeacher = await FixtureUtils.getUnauthorizedTeacherForCourse(course); + + const res = await testHelper.commonUserPutRequest(unauthorizedTeacher, `/${unit.id}`, unit.toObject()); + res.status.should.be.equal(403); + }); + }); + + describe(`DELETE ${BASE_URL}`, () => { + it('should delete unit', async () => { + const unit = await Unit.findOne({__t: 'code-kata'}); + const course = await Course.findById(unit._course); + const courseAdmin = await User.findById(course.courseAdmin); + + const res = await testHelper.commonUserDeleteRequest(courseAdmin, `/${unit.id}`); + res.status.should.be.equal(200); + + const res2 = await testHelper.commonUserGetRequest(courseAdmin, `/${unit.id}`); + res2.status.should.be.equal(404); + }); + + it('should fail to delete unit for an unauthorized teacher', async () => { + const unit = await Unit.findOne({__t: 'code-kata'}); + const course = await Course.findById(unit._course); + const courseAdmin = await User.findById(course.courseAdmin); + const unauthorizedTeacher = await FixtureUtils.getUnauthorizedTeacherForCourse(course); + + const res = await testHelper.commonUserDeleteRequest(unauthorizedTeacher, `/${unit.id}`); + res.status.should.be.equal(403); + + const res2 = await testHelper.commonUserGetRequest(courseAdmin, `/${unit.id}`); + res2.status.should.be.equal(200); + }); }); });