From 378e16547b357f076b468e65e02a2f23db1e6779 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 00:59:35 +0100
Subject: [PATCH 01/24] Refactor CodeKataUnit POST unit tests with TestHelper

---
 api/test/integration/unit/codeKataUnit.ts | 35 ++++++-----------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/api/test/integration/unit/codeKataUnit.ts b/api/test/integration/unit/codeKataUnit.ts
index 1d5b81e7e..3badbe1d7 100644
--- a/api/test/integration/unit/codeKataUnit.ts
+++ b/api/test/integration/unit/codeKataUnit.ts
@@ -1,7 +1,7 @@
 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';
@@ -11,10 +11,9 @@ 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 +35,9 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
     '\n' +
     '}'
   };
-  // Before each test we reset the database
+
   beforeEach(async () => {
-    await fixtureLoader.load();
+    await testHelper.resetForNextTest();
   });
 
   describe(`POST ${BASE_URL}`, () => {
@@ -54,12 +53,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 +62,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 +72,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 +94,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);

From eacab377961b53d605d1614c953f0a2f69c6bef1 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 01:01:18 +0100
Subject: [PATCH 02/24] Fix CodeKataUnit PUT route test describe section

---
 api/test/integration/unit/codeKataUnit.ts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/api/test/integration/unit/codeKataUnit.ts b/api/test/integration/unit/codeKataUnit.ts
index 3badbe1d7..bc052bdf0 100644
--- a/api/test/integration/unit/codeKataUnit.ts
+++ b/api/test/integration/unit/codeKataUnit.ts
@@ -101,7 +101,9 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
       res.body.unitCreator.profile.lastName.should.equal(courseAdmin.profile.lastName);
       res.body.unitCreator.profile.firstName.should.equal(courseAdmin.profile.firstName);
     });
+  });
 
+  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 ] }});

From 7c6948623bac6102b1d3ce63d663699a832b0436 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 01:03:22 +0100
Subject: [PATCH 03/24] Refactor CodeKataUnit PUT unit test with TestHelper

---
 api/test/integration/unit/codeKataUnit.ts | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/api/test/integration/unit/codeKataUnit.ts b/api/test/integration/unit/codeKataUnit.ts
index bc052bdf0..aa2d03257 100644
--- a/api/test/integration/unit/codeKataUnit.ts
+++ b/api/test/integration/unit/codeKataUnit.ts
@@ -2,7 +2,6 @@ import * as chai from 'chai';
 import chaiHttp = require('chai-http');
 import {TestHelper} from '../../TestHelper';
 import {Server} from '../../../src/server';
-import {JwtUtils} from '../../../src/security/JwtUtils';
 import {User} from '../../../src/models/User';
 import {Lecture} from '../../../src/models/Lecture';
 import {Course} from '../../../src/models/Course';
@@ -111,11 +110,7 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
       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');
     });

From e5f696dde95778fa5e78561036c28b16489b978c Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 18:22:32 +0100
Subject: [PATCH 04/24] Add UnitController GET success unit test

---
 api/test/integration/unit/codeKataUnit.ts | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/api/test/integration/unit/codeKataUnit.ts b/api/test/integration/unit/codeKataUnit.ts
index aa2d03257..e2d009737 100644
--- a/api/test/integration/unit/codeKataUnit.ts
+++ b/api/test/integration/unit/codeKataUnit.ts
@@ -39,6 +39,17 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
     await testHelper.resetForNextTest();
   });
 
+  describe(`GET ${BASE_URL}`, () => {
+    it('should get unit data', 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.commonUserGetRequest(courseAdmin, `/${unit.id}`);
+      res.status.should.be.equal(200);
+    });
+  });
+
   describe(`POST ${BASE_URL}`, () => {
     it('should fail with wrong authorization', async () => {
       const res = await chai.request(app)

From 6a8108157f42ece572b608066a46d9893bdc62c9 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 18:27:32 +0100
Subject: [PATCH 05/24] Add UnitController DELETE success unit test

---
 api/test/integration/unit/codeKataUnit.ts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/api/test/integration/unit/codeKataUnit.ts b/api/test/integration/unit/codeKataUnit.ts
index e2d009737..1293de010 100644
--- a/api/test/integration/unit/codeKataUnit.ts
+++ b/api/test/integration/unit/codeKataUnit.ts
@@ -126,4 +126,18 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
       res.body.test.should.string('// Test if we can edit a Kata');
     });
   });
+
+  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);
+    });
+  });
 });

From 9068197c17a96e32581fd4254b1d9e54e176910f Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 18:40:51 +0100
Subject: [PATCH 06/24] Add UnitController 403 unit tests

These will fail until the corresponding vulnerabilities are fixed.
---
 api/test/integration/unit/codeKataUnit.ts | 42 +++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/api/test/integration/unit/codeKataUnit.ts b/api/test/integration/unit/codeKataUnit.ts
index 1293de010..69c94c8c1 100644
--- a/api/test/integration/unit/codeKataUnit.ts
+++ b/api/test/integration/unit/codeKataUnit.ts
@@ -48,6 +48,15 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
       const res = await testHelper.commonUserGetRequest(courseAdmin, `/${unit.id}`);
       res.status.should.be.equal(200);
     });
+
+    it('should deny access to unit data if the user is unauthorized', async () => {
+      const unit = await Unit.findOne({__t: 'code-kata'});
+      const course = await Course.findById(unit._course);
+      const unauthorizedUser = await FixtureUtils.getUnauthorizedStudentForCourse(course);
+
+      const res = await testHelper.commonUserGetRequest(unauthorizedUser, `/${unit.id}`);
+      res.status.should.be.equal(403);
+    });
   });
 
   describe(`POST ${BASE_URL}`, () => {
@@ -111,6 +120,16 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
       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}`, () => {
@@ -125,6 +144,17 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
       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}`, () => {
@@ -139,5 +169,17 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
       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 unauthorizedTeacher = await FixtureUtils.getUnauthorizedTeacherForCourse(course);
+
+      const res = await testHelper.commonUserDeleteRequest(unauthorizedTeacher, `/${unit.id}`);
+      res.status.should.be.equal(403);
+
+      const res2 = await testHelper.commonUserGetRequest(unauthorizedTeacher, `/${unit.id}`);
+      res2.status.should.be.equal(200);
+    });
   });
 });

From 3260618c543874bff5e35bfd0950a30e5089db0e Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 18:52:24 +0100
Subject: [PATCH 07/24] Factor out shared UnitController GET unit test code

---
 api/test/integration/unit/codeKataUnit.ts | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/api/test/integration/unit/codeKataUnit.ts b/api/test/integration/unit/codeKataUnit.ts
index 69c94c8c1..a6ffd81a6 100644
--- a/api/test/integration/unit/codeKataUnit.ts
+++ b/api/test/integration/unit/codeKataUnit.ts
@@ -4,7 +4,7 @@ import {TestHelper} from '../../TestHelper';
 import {Server} from '../../../src/server';
 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';
@@ -40,22 +40,21 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
   });
 
   describe(`GET ${BASE_URL}`, () => {
-    it('should get unit data', async () => {
+    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 User.findById(course.courseAdmin);
+      const courseAdmin = await getUserForCourseFunc(course);
 
       const res = await testHelper.commonUserGetRequest(courseAdmin, `/${unit.id}`);
-      res.status.should.be.equal(200);
+      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 () => {
-      const unit = await Unit.findOne({__t: 'code-kata'});
-      const course = await Course.findById(unit._course);
-      const unauthorizedUser = await FixtureUtils.getUnauthorizedStudentForCourse(course);
-
-      const res = await testHelper.commonUserGetRequest(unauthorizedUser, `/${unit.id}`);
-      res.status.should.be.equal(403);
+      await commonGetTest(FixtureUtils.getUnauthorizedStudentForCourse, 403);
     });
   });
 

From f486ad44022507abb65dc68fe01bc88884aa547a Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 18:58:08 +0100
Subject: [PATCH 08/24] Secure UnitController GET via course.checkPrivileges

---
 api/src/controllers/UnitController.ts | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index c72e857c1..fff47b37e 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 {Course} from '../models/Course';
 import {Lecture} from '../models/Lecture';
 import {IUnitModel, Unit} from '../models/units/Unit';
 import {IUser} from '../../../shared/models/IUser';
@@ -36,11 +37,15 @@ export class UnitController {
    *     }
    */
   @Get('/:id')
-  async getUnit(@Param('id') id: string) {
+  async getUnit(@Param('id') id: string, @CurrentUser() currentUser: IUser) {
     const unit = await Unit.findById(id);
     if (!unit) {
       throw new NotFoundError();
     }
+    const course = await Course.findById(unit._course);
+    if (!course.checkPrivileges(currentUser).userCanViewCourse) {
+      throw new ForbiddenError();
+    }
     return unit.toObject();
   }
 

From 93ec3c770550f78afc541b42d2a32c926f2b3e0e Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 19:03:00 +0100
Subject: [PATCH 09/24] Secure UnitController POST via course.checkPrivileges

---
 api/src/controllers/UnitController.ts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index fff47b37e..bfcdcb93c 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -87,6 +87,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 {

From 1a4880a48be3bc1c7aa165a611bff5b2664d223d Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 19:04:43 +0100
Subject: [PATCH 10/24] Secure UnitController PUT via course.checkPrivileges

---
 api/src/controllers/UnitController.ts | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index bfcdcb93c..27affb29e 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -140,13 +140,18 @@ export class UnitController {
    */
   @Authorized(['teacher', 'admin'])
   @Put('/:id')
-  async updateUnit(@Param('id') id: string, @Body() data: any) {
+  async updateUnit(@Param('id') id: string, @Body() data: any, @CurrentUser() currentUser: IUser) {
     const oldUnit: IUnitModel = await Unit.findById(id);
 
     if (!oldUnit) {
       throw new NotFoundError();
     }
 
+    const course = await Course.findById(oldUnit._course);
+    if (!course.checkPrivileges(currentUser).userCanEditCourse) {
+      throw new ForbiddenError();
+    }
+
     try {
       oldUnit.set(data);
       const updatedUnit: IUnitModel = await oldUnit.save();

From aeab1d8ad6e3803d4153f55d0326b8e6712c7001 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 19:08:37 +0100
Subject: [PATCH 11/24] Refactor UnitController DELETE to use async/await

---
 api/src/controllers/UnitController.ts | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index 27affb29e..a97a8c39b 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -185,18 +185,16 @@ export class UnitController {
    */
   @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) {
+    const unit = await Unit.findById(id);
+
+    if (!unit) {
+      throw new NotFoundError();
+    }
 
-      return Lecture.updateMany({}, {$pull: {units: id}})
-        .then(() => unit.remove())
-        .then(() => {
-          return {result: true};
-        });
-    });
+    await Lecture.updateMany({}, {$pull: {units: id}});
+    await unit.remove();
+    return {result: true};
   }
 
   protected pushToLecture(lectureId: string, unit: any) {

From 127440c0168479d3a33e0cae958c0a19b4017dae Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 19:10:57 +0100
Subject: [PATCH 12/24] Fix UnitController DELETE 403 unit test

---
 api/test/integration/unit/codeKataUnit.ts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/api/test/integration/unit/codeKataUnit.ts b/api/test/integration/unit/codeKataUnit.ts
index a6ffd81a6..e0318d7be 100644
--- a/api/test/integration/unit/codeKataUnit.ts
+++ b/api/test/integration/unit/codeKataUnit.ts
@@ -172,12 +172,13 @@ describe(`CodeKataUnit ${BASE_URL}`, () => {
     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(unauthorizedTeacher, `/${unit.id}`);
+      const res2 = await testHelper.commonUserGetRequest(courseAdmin, `/${unit.id}`);
       res2.status.should.be.equal(200);
     });
   });

From 5d99e4436eb0c8e728610b7e67482efc00faa2f5 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 19:11:17 +0100
Subject: [PATCH 13/24] Secure UnitController DELETE via course.checkPrivileges

---
 api/src/controllers/UnitController.ts | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index a97a8c39b..f37c22919 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -185,13 +185,18 @@ export class UnitController {
    */
   @Authorized(['teacher', 'admin'])
   @Delete('/:id')
-  async deleteUnit(@Param('id') id: string) {
+  async deleteUnit(@Param('id') id: string, @CurrentUser() currentUser: IUser) {
     const unit = await Unit.findById(id);
 
     if (!unit) {
       throw new NotFoundError();
     }
 
+    const course = await Course.findById(unit._course);
+    if (!course.checkPrivileges(currentUser).userCanEditCourse) {
+      throw new ForbiddenError();
+    }
+
     await Lecture.updateMany({}, {$pull: {units: id}});
     await unit.remove();
     return {result: true};

From 1e1aab2fb91abb175e181afc95fda16fc0a0917b Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 19:20:30 +0100
Subject: [PATCH 14/24] Factor out shared UnitController code as getUnitFor

---
 api/src/controllers/UnitController.ts | 45 ++++++++++-----------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index f37c22919..bf1265d6d 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -13,6 +13,20 @@ import {IUser} from '../../../shared/models/IUser';
 @UseBefore(passportJwtMiddleware)
 export class UnitController {
 
+  static async getUnitFor (unitId: string, currentUser: IUser, privilege: 'userCanViewCourse' | 'userCanEditCourse') {
+    const unit = await Unit.findById(unitId);
+    if (!unit) {
+      throw new NotFoundError();
+    }
+
+    const course = await Course.findById(unit._course);
+    if (!course.checkPrivileges(currentUser)[privilege]) {
+      throw new ForbiddenError();
+    }
+
+    return unit;
+  }
+
   /**
    * @api {get} /api/units/:id Request unit
    * @apiName GetUnit
@@ -38,14 +52,7 @@ export class UnitController {
    */
   @Get('/:id')
   async getUnit(@Param('id') id: string, @CurrentUser() currentUser: IUser) {
-    const unit = await Unit.findById(id);
-    if (!unit) {
-      throw new NotFoundError();
-    }
-    const course = await Course.findById(unit._course);
-    if (!course.checkPrivileges(currentUser).userCanViewCourse) {
-      throw new ForbiddenError();
-    }
+    const unit = await UnitController.getUnitFor(id, currentUser, 'userCanViewCourse');
     return unit.toObject();
   }
 
@@ -141,16 +148,7 @@ export class UnitController {
   @Authorized(['teacher', 'admin'])
   @Put('/:id')
   async updateUnit(@Param('id') id: string, @Body() data: any, @CurrentUser() currentUser: IUser) {
-    const oldUnit: IUnitModel = await Unit.findById(id);
-
-    if (!oldUnit) {
-      throw new NotFoundError();
-    }
-
-    const course = await Course.findById(oldUnit._course);
-    if (!course.checkPrivileges(currentUser).userCanEditCourse) {
-      throw new ForbiddenError();
-    }
+    const oldUnit = await UnitController.getUnitFor(id, currentUser, 'userCanEditCourse');
 
     try {
       oldUnit.set(data);
@@ -186,16 +184,7 @@ export class UnitController {
   @Authorized(['teacher', 'admin'])
   @Delete('/:id')
   async deleteUnit(@Param('id') id: string, @CurrentUser() currentUser: IUser) {
-    const unit = await Unit.findById(id);
-
-    if (!unit) {
-      throw new NotFoundError();
-    }
-
-    const course = await Course.findById(unit._course);
-    if (!course.checkPrivileges(currentUser).userCanEditCourse) {
-      throw new ForbiddenError();
-    }
+    const unit = await UnitController.getUnitFor(id, currentUser, 'userCanEditCourse');
 
     await Lecture.updateMany({}, {$pull: {units: id}});
     await unit.remove();

From 72287baa32b57d48ad76bf35b405d7060d0b515a Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 19:21:30 +0100
Subject: [PATCH 15/24] Add missing UnitController @apiErrors

---
 api/src/controllers/UnitController.ts | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index bf1265d6d..afe0abf1e 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -49,6 +49,9 @@ export class UnitController {
    *         "type": "free-text",
    *         "__v": 0
    *     }
+   *
+   * @apiError NotFoundError
+   * @apiError ForbiddenError
    */
   @Get('/:id')
   async getUnit(@Param('id') id: string, @CurrentUser() currentUser: IUser) {
@@ -87,6 +90,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'])
@@ -140,9 +144,10 @@ 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'])
@@ -180,6 +185,7 @@ export class UnitController {
    *     }
    *
    * @apiError NotFoundError
+   * @apiError ForbiddenError
    */
   @Authorized(['teacher', 'admin'])
   @Delete('/:id')

From a0e28d37befe573f124b65d92d43e2c2841be94c Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 19:50:33 +0100
Subject: [PATCH 16/24] Refactor UnitController POST route errorCodes

---
 api/src/config/errorCodes.ts          | 14 ++++++++++++++
 api/src/controllers/UnitController.ts |  8 ++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

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 afe0abf1e..d3acf85e8 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -3,7 +3,7 @@ import {
   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';
@@ -216,15 +216,15 @@ export class UnitController {
 
   protected 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);
     }
   }
 }

From 66adf6e1d720d77e35cfba2df127bfffdcc3f65e Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 20:24:17 +0100
Subject: [PATCH 17/24] Change UnitController DELETE response to {}

---
 api/src/controllers/UnitController.ts | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index d3acf85e8..412f5c058 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -177,12 +177,10 @@ 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
@@ -194,7 +192,7 @@ export class UnitController {
 
     await Lecture.updateMany({}, {$pull: {units: id}});
     await unit.remove();
-    return {result: true};
+    return {};
   }
 
   protected pushToLecture(lectureId: string, unit: any) {

From c8636b225e7d10b296120a2990cf76e5ce718ad5 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Thu, 21 Mar 2019 20:32:23 +0100
Subject: [PATCH 18/24] CHANGELOG: Add entries for issue 1190

---
 CHANGELOG.md | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6ed4495a2..132168f37 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,8 @@ 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)
@@ -30,6 +32,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ### Security
 - Closed `ProgressController` vulnerabilities. [#1116](https://github.com/geli-lms/geli/issues/1116)
+- Closed `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

From 255daa142240ceff81d2f26b949ee97c0f783c9a Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Fri, 22 Mar 2019 14:51:30 +0100
Subject: [PATCH 19/24] Mark UnitController.getUnitFor as protected

---
 api/src/controllers/UnitController.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index 412f5c058..64f7a98f9 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -13,7 +13,7 @@ import {IUser} from '../../../shared/models/IUser';
 @UseBefore(passportJwtMiddleware)
 export class UnitController {
 
-  static async getUnitFor (unitId: string, currentUser: IUser, privilege: 'userCanViewCourse' | 'userCanEditCourse') {
+  protected static async getUnitFor (unitId: string, currentUser: IUser, privilege: 'userCanViewCourse' | 'userCanEditCourse') {
     const unit = await Unit.findById(unitId);
     if (!unit) {
       throw new NotFoundError();

From 745263724cc3e299325d12695ece70882b10f485 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Fri, 22 Mar 2019 14:54:20 +0100
Subject: [PATCH 20/24] Refactor UnitController using orFail for unit 404

---
 api/src/controllers/UnitController.ts | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index 64f7a98f9..4226d315e 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -14,10 +14,7 @@ import {IUser} from '../../../shared/models/IUser';
 export class UnitController {
 
   protected static async getUnitFor (unitId: string, currentUser: IUser, privilege: 'userCanViewCourse' | 'userCanEditCourse') {
-    const unit = await Unit.findById(unitId);
-    if (!unit) {
-      throw new NotFoundError();
-    }
+    const unit = await Unit.findById(unitId).orFail(new NotFoundError());
 
     const course = await Course.findById(unit._course);
     if (!course.checkPrivileges(currentUser)[privilege]) {

From 0ec4611ef3b37c3ea69b86e57484c9233ce0cc68 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Fri, 22 Mar 2019 18:34:56 +0100
Subject: [PATCH 21/24] Refactor UnitController methods to non-static & private

---
 api/src/controllers/UnitController.ts | 35 +++++++++++++++------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index 4226d315e..c18d8fa98 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -13,17 +13,6 @@ import {IUser} from '../../../shared/models/IUser';
 @UseBefore(passportJwtMiddleware)
 export class UnitController {
 
-  protected static 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 unit;
-  }
-
   /**
    * @api {get} /api/units/:id Request unit
    * @apiName GetUnit
@@ -52,7 +41,7 @@ export class UnitController {
    */
   @Get('/:id')
   async getUnit(@Param('id') id: string, @CurrentUser() currentUser: IUser) {
-    const unit = await UnitController.getUnitFor(id, currentUser, 'userCanViewCourse');
+    const unit = await this.getUnitFor(id, currentUser, 'userCanViewCourse');
     return unit.toObject();
   }
 
@@ -150,7 +139,7 @@ export class UnitController {
   @Authorized(['teacher', 'admin'])
   @Put('/:id')
   async updateUnit(@Param('id') id: string, @Body() data: any, @CurrentUser() currentUser: IUser) {
-    const oldUnit = await UnitController.getUnitFor(id, currentUser, 'userCanEditCourse');
+    const oldUnit = await this.getUnitFor(id, currentUser, 'userCanEditCourse');
 
     try {
       oldUnit.set(data);
@@ -185,14 +174,28 @@ export class UnitController {
   @Authorized(['teacher', 'admin'])
   @Delete('/:id')
   async deleteUnit(@Param('id') id: string, @CurrentUser() currentUser: IUser) {
-    const unit = await UnitController.getUnitFor(id, currentUser, 'userCanEditCourse');
+    const unit = await this.getUnitFor(id, currentUser, 'userCanEditCourse');
 
     await Lecture.updateMany({}, {$pull: {units: id}});
     await unit.remove();
     return {};
   }
 
-  protected pushToLecture(lectureId: string, unit: any) {
+  private async getUnitFor (unitId: string, currentUser: IUser, privilege: 'userCanViewCourse' | 'userCanEditCourse') {
+    const unit = await Unit.findById(unitId);
+    if (!unit) {
+      throw new NotFoundError();
+    }
+
+    const course = await Course.findById(unit._course);
+    if (!course.checkPrivileges(currentUser)[privilege]) {
+      throw new ForbiddenError();
+    }
+
+    return unit;
+  }
+
+  private pushToLecture(lectureId: string, unit: any) {
     return Lecture.findById(lectureId)
       .then((lecture) => {
         lecture.units.push(unit);
@@ -209,7 +212,7 @@ export class UnitController {
       });
   }
 
-  protected checkPostParam(data: any) {
+  private checkPostParam(data: any) {
     if (!data.lectureId) {
       throw new BadRequestError(errorCodes.unit.postMissingLectureId.text);
     }

From b55a7726c96a77364e5a9c02bc84ac0c2fea30b4 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Fri, 22 Mar 2019 20:52:53 +0100
Subject: [PATCH 22/24] Re-refactor UnitController.getUnitFor with orFail

---
 api/src/controllers/UnitController.ts | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/api/src/controllers/UnitController.ts b/api/src/controllers/UnitController.ts
index c18d8fa98..c2c1c3054 100644
--- a/api/src/controllers/UnitController.ts
+++ b/api/src/controllers/UnitController.ts
@@ -182,10 +182,7 @@ export class UnitController {
   }
 
   private async getUnitFor (unitId: string, currentUser: IUser, privilege: 'userCanViewCourse' | 'userCanEditCourse') {
-    const unit = await Unit.findById(unitId);
-    if (!unit) {
-      throw new NotFoundError();
-    }
+    const unit = await Unit.findById(unitId).orFail(new NotFoundError());
 
     const course = await Course.findById(unit._course);
     if (!course.checkPrivileges(currentUser)[privilege]) {

From 96137cc53d7091480e883761b60ef5f2c3e6bbd7 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Fri, 22 Mar 2019 21:02:04 +0100
Subject: [PATCH 23/24] CHANGELOG: Adjust tense of [Unreleased] entries

---
 CHANGELOG.md | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 132168f37..87f9c7927 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,8 +18,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - `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
@@ -31,8 +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)
-- Closed `UnitController` vulnerabilities. [#1190](https://github.com/geli-lms/geli/issues/1190)
+- Closes `ProgressController` vulnerabilities. [#1116](https://github.com/geli-lms/geli/issues/1116)
+- Closes `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

From ea91a7c8d965ec566c2fffbdeca0afa674ab9e87 Mon Sep 17 00:00:00 2001
From: torss <37229901+torss@users.noreply.github.com>
Date: Fri, 22 Mar 2019 21:04:01 +0100
Subject: [PATCH 24/24] CHANGELOG: Change "Closes" to "Close" (imperative)

---
 CHANGELOG.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 87f9c7927..95ebb40e9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -31,8 +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
-- Closes `ProgressController` vulnerabilities. [#1116](https://github.com/geli-lms/geli/issues/1116)
-- Closes `UnitController` vulnerabilities. [#1190](https://github.com/geli-lms/geli/issues/1190)
+- 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