Feature/58379 allow space in pupil password (#2492)

* Trim school pin before authenticating * clarify comment for org upload * MInor / patch updates * revert helmet upgrade * add test for space in password --------- Co-authored-by: Mohsen Qureshi <[email protected]>
DFEAGILEDEVOPS · Apr 26, 2023 · 627729e · 627729e
1 parent 183cc9f
commit 627729e
Show file tree

Hide file tree

Showing 7 changed files with 174 additions and 83 deletions.
diff --git a/pupil-api/package.json b/pupil-api/package.json
@@ -28,7 +28,7 @@
     "express": "^4.18.2",
     "express-winston": "^4.0.2",
     "feature-toggles": "^1.4.0",
-    "helmet": "^6.0.1",
+    "helmet": "6.0.1",
     "ioredis": "^5.3.1",
     "moment": "^2.29.4",
     "morgan": "^1.9.0",
@@ -66,5 +66,8 @@
     "node-mocks-http": "^1.7.0",
     "ts-jest": "^29.0.5",
     "typescript": "4.3.5"
+  },
+  "moduleNameMapper": {
+    "helmet": "<root>/node_modules/helmet/index.cjs"
   }
 }
diff --git a/pupil-api/src/controllers/redis.auth.controller.spec.ts b/pupil-api/src/controllers/redis.auth.controller.spec.ts
@@ -4,9 +4,11 @@ import logger from '../services/log.service'
 import type { IPupilAuthenticationService } from '../services/redis-pupil-auth.service'
 import type { Request } from 'express'
 
-const RedisPupilAuthServiceMock = jest.fn<IPupilAuthenticationService, any>(() => ({
-  authenticate: jest.fn()
-}))
+class RedisPupilAuthServiceMock implements IPupilAuthenticationService {
+  async authenticate (): Promise<object | undefined> {
+    return undefined
+  }
+}
 
 let req: Request
 let res: any
@@ -20,6 +22,11 @@ describe('redis auth controller', () => {
     res = httpMocks.createResponse()
     redisPupilAuthService = new RedisPupilAuthServiceMock()
     authController = new RedisAuthController(redisPupilAuthService)
+    jest.spyOn(redisPupilAuthService, 'authenticate')
+  })
+
+  afterEach(() => {
+    jest.restoreAllMocks()
   })
 
   test('returns an 400 error if the request is not JSON', async () => {
@@ -93,7 +100,8 @@ describe('redis auth controller', () => {
     jest.spyOn(logger, 'error').mockImplementation()
     req.body = {
       pupilPin: '123',
-      schoolPin: '1234'
+      schoolPin: 'def4ger'
+      // buildVersion: '42' // we do not want a build version
     }
     await authController.postAuth(req, res)
     expect(redisPupilAuthService.authenticate).not.toHaveBeenCalled()
@@ -118,6 +126,45 @@ describe('redis auth controller', () => {
     const data = JSON.parse(res._getData())
     expect(data.error).toBe('Unauthorised')
   })
+
+  test('trims leading whitespace from the schoolPin', async () => {
+    req.body = {
+      pupilPin: '123',
+      schoolPin: ' abc',
+      buildVersion: '123'
+    }
+    const redisPupilAuthServiceMock = jest.spyOn(redisPupilAuthService, 'authenticate').mockResolvedValue({})
+    await authController.postAuth(req, res)
+    expect(res.statusCode).toBe(200) // it succeeds even though the leading space is superflous
+    const authArgs = redisPupilAuthServiceMock.mock.calls[0]
+    expect(authArgs[0]).toBe('abc') // leading space removed
+  })
+
+  test('trims trailing whitespace from the schoolPin', async () => {
+    req.body = {
+      pupilPin: '123',
+      schoolPin: 'def   ',
+      buildVersion: '123'
+    }
+    const redisPupilAuthServiceMock = jest.spyOn(redisPupilAuthService, 'authenticate').mockResolvedValue({})
+    await authController.postAuth(req, res)
+    expect(res.statusCode).toBe(200) // it succeeds even though the trailing space is superflous
+    const authArgs = redisPupilAuthServiceMock.mock.calls[0]
+    expect(authArgs[0]).toBe('def') // trailing space removed
+  })
+
+  test('trims leading and trailing whitespace from the schoolPin', async () => {
+    req.body = {
+      pupilPin: '123',
+      schoolPin: '  xyz   ',
+      buildVersion: '123'
+    }
+    const redisPupilAuthServiceMock = jest.spyOn(redisPupilAuthService, 'authenticate').mockResolvedValue({})
+    await authController.postAuth(req, res)
+    expect(res.statusCode).toBe(200) // it succeeds even though the trailing space is superflous
+    const authArgs = redisPupilAuthServiceMock.mock.calls[0]
+    expect(authArgs[0]).toBe('xyz') // trailing space removed
+  })
 })
 
 function createMockRequest (contentType: string): any {

diff --git a/pupil-api/src/controllers/redis.auth.controller.ts b/pupil-api/src/controllers/redis.auth.controller.ts
@@ -26,7 +26,7 @@ export class RedisAuthController implements IAuthController {
     if (schoolPin === undefined || pupilPin === undefined || buildVersion === undefined) return apiResponse.unauthorised(res)
 
     try {
-      const data = await this.redisAuthService.authenticate(schoolPin, pupilPin, buildVersion)
+      const data = await this.redisAuthService.authenticate(schoolPin.trim(), pupilPin, buildVersion)
       if (data === undefined) {
         return apiResponse.unauthorised(res)
       }