Bugfix/#1190 UnitController vulnerabilities

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

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'
+    }
   }
 };

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,13 +76,20 @@ 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'])
   @Post('/')
   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);
     }
   }
 }

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,32 +112,74 @@ 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);
       res.body.unitCreator.profile.lastName.should.equal(courseAdmin.profile.lastName);
       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 ] }});
       const course = await Course.findOne({lectures: { $in: [ lecture._id ] }});
       const courseAdmin = await User.findOne({_id: course.courseAdmin});
       (<ICodeKataModel>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 ] }});
+      (<ICodeKataModel>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);
+    });
   });
 });