feat(auth): check that token value matches during rotation & revocati…

…on (#860)
interledger · Dec 20, 2022 · 4db6f8e · 4db6f8e
1 parent 468e5ad
commit 4db6f8e
Show file tree

Hide file tree

Showing 10 changed files with 143 additions and 60 deletions.
diff --git a/packages/auth/src/accessToken/routes.test.ts b/packages/auth/src/accessToken/routes.test.ts
@@ -238,12 +238,30 @@ describe('Access Token Routes', (): void => {
       url = `/token/${managementId}`
     })
 
-    test('Returns status 204 even if token does not exist', async (): Promise<void> => {
+    test('Returns status 204 even if management id does not exist', async (): Promise<void> => {
       managementId = v4()
       const ctx = createContext(
         {
           headers: {
-            Accept: 'application/json'
+            Accept: 'application/json',
+            Authorization: `GNAP ${token.value}`
+          },
+          url: `/token/${managementId}`,
+          method
+        },
+        { id: managementId }
+      )
+
+      await accessTokenRoutes.revoke(ctx)
+      expect(ctx.response.status).toBe(204)
+    })
+
+    test('Returns status 204 even if token does not exist', async (): Promise<void> => {
+      const ctx = createContext(
+        {
+          headers: {
+            Accept: 'application/json',
+            Authorization: `GNAP ${v4()}`
           },
           url: `/token/${managementId}`,
           method
@@ -259,7 +277,8 @@ describe('Access Token Routes', (): void => {
       const ctx = createContext(
         {
           headers: {
-            Accept: 'application/json'
+            Accept: 'application/json',
+            Authorization: `GNAP ${token.value}`
           },
           url,
           method
@@ -281,7 +300,8 @@ describe('Access Token Routes', (): void => {
       const ctx = createContext(
         {
           headers: {
-            Accept: 'application/json'
+            Accept: 'application/json',
+            Authorization: `GNAP ${token.value}`
           },
           url,
           method
@@ -325,11 +345,33 @@ describe('Access Token Routes', (): void => {
       jestOpenAPI(openApi.authServerSpec)
     })
 
-    test('Cannot rotate nonexistent token', async (): Promise<void> => {
+    test('Cannot rotate nonexistent token management id', async (): Promise<void> => {
       managementId = v4()
       const ctx = createContext(
         {
-          headers: { Accept: 'application/json' },
+          headers: {
+            Accept: 'application/json',
+            Authorization: `GNAP ${token.value}`
+          },
+          method: 'POST',
+          url: `/token/${managementId}`
+        },
+        { id: managementId }
+      )
+
+      await expect(accessTokenRoutes.rotate(ctx)).rejects.toMatchObject({
+        status: 404,
+        message: 'token not found'
+      })
+    })
+
+    test('Cannot rotate nonexistent token', async (): Promise<void> => {
+      const ctx = createContext(
+        {
+          headers: {
+            Accept: 'application/json',
+            Authorization: `GNAP ${v4()}`
+          },
           method: 'POST',
           url: `/token/${managementId}`
         },
@@ -345,7 +387,10 @@ describe('Access Token Routes', (): void => {
     test('Can rotate token', async (): Promise<void> => {
       const ctx = createContext(
         {
-          headers: { Accept: 'application/json' },
+          headers: {
+            Accept: 'application/json',
+            Authorization: `GNAP ${token.value}`
+          },
           url: `/token/${token.id}`,
           method: 'POST'
         },
@@ -376,7 +421,10 @@ describe('Access Token Routes', (): void => {
     test('Can rotate an expired token', async (): Promise<void> => {
       const ctx = createContext(
         {
-          headers: { Accept: 'application/json' },
+          headers: {
+            Accept: 'application/json',
+            Authorization: `GNAP ${token.value}`
+          },
           url: `/token/${token.id}`,
           method: 'POST'
         },

diff --git a/packages/auth/src/accessToken/routes.ts b/packages/auth/src/accessToken/routes.ts
@@ -91,18 +91,19 @@ async function revokeToken(
   deps: ServiceDependencies,
   ctx: RevokeContext
 ): Promise<void> {
+  const token = (ctx.headers['authorization'] ?? '').replace('GNAP ', '')
   const { id: managementId } = ctx.params
-  await deps.accessTokenService.revoke(managementId)
+  await deps.accessTokenService.revoke(managementId, token)
   ctx.status = 204
 }
 
 async function rotateToken(
   deps: ServiceDependencies,
   ctx: RotateContext
 ): Promise<void> {
-  // TODO: verify Authorization: GNAP ${accessToken} contains correct token value
   const { id: managementId } = ctx.params
-  const result = await deps.accessTokenService.rotate(managementId)
+  const token = (ctx.headers['authorization'] ?? '').replace('GNAP ', '')
+  const result = await deps.accessTokenService.rotate(managementId, token)
   if (result.success == true) {
     ctx.status = 200
     ctx.body = {

diff --git a/packages/auth/src/accessToken/service.test.ts b/packages/auth/src/accessToken/service.test.ts
@@ -209,23 +209,29 @@ describe('Access Token Service', (): void => {
     })
     test('Can revoke un-expired token', async (): Promise<void> => {
       await token.$query(trx).patch({ expiresIn: 1000000 })
-      const result = await accessTokenService.revoke(token.managementId)
+      const result = await accessTokenService.revoke(
+        token.managementId,
+        token.value
+      )
       expect(result).toBeUndefined()
       await expect(
         AccessToken.query(trx).findById(token.id)
       ).resolves.toBeUndefined()
     })
     test('Can revoke even if token has already expired', async (): Promise<void> => {
       await token.$query(trx).patch({ expiresIn: -1 })
-      const result = await accessTokenService.revoke(token.managementId)
+      const result = await accessTokenService.revoke(
+        token.managementId,
+        token.value
+      )
       expect(result).toBeUndefined()
       await expect(
         AccessToken.query(trx).findById(token.id)
       ).resolves.toBeUndefined()
     })
     test('Can revoke even if token has already been revoked', async (): Promise<void> => {
       await token.$query(trx).delete()
-      const result = await accessTokenService.revoke(token.id)
+      const result = await accessTokenService.revoke(token.id, token.value)
       expect(result).toBeUndefined()
       await expect(
         AccessToken.query(trx).findById(token.id)
@@ -262,22 +268,32 @@ describe('Access Token Service', (): void => {
 
     test('Can rotate un-expired token', async (): Promise<void> => {
       await token.$query(trx).patch({ expiresIn: 1000000 })
-      const result = await accessTokenService.rotate(token.managementId)
+      const result = await accessTokenService.rotate(
+        token.managementId,
+        token.value
+      )
       expect(result.success).toBe(true)
       expect(result.success && result.value).not.toBe(originalTokenValue)
     })
     test('Can rotate expired token', async (): Promise<void> => {
       await token.$query(trx).patch({ expiresIn: -1 })
-      const result = await accessTokenService.rotate(token.managementId)
+      const result = await accessTokenService.rotate(
+        token.managementId,
+        token.value
+      )
       expect(result.success).toBe(true)
       const rotatedToken = await AccessToken.query(trx).findOne({
         managementId: result.success && result.managementId
       })
       expect(rotatedToken).toBeDefined()
       expect(rotatedToken?.value).not.toBe(originalTokenValue)
     })
-    test('Cannot rotate nonexistent token', async (): Promise<void> => {
-      const result = await accessTokenService.rotate(v4())
+    test('Cannot rotate token with incorrect management id', async (): Promise<void> => {
+      const result = await accessTokenService.rotate(v4(), token.value)
+      expect(result.success).toBe(false)
+    })
+    test('Cannot rotate token with incorrect value', async (): Promise<void> => {
+      const result = await accessTokenService.rotate(token.managementId, v4())
       expect(result.success).toBe(false)
     })
   })

diff --git a/packages/auth/src/accessToken/service.ts b/packages/auth/src/accessToken/service.ts
@@ -14,9 +14,9 @@ export interface AccessTokenService {
   get(token: string): Promise<AccessToken>
   getByManagementId(managementId: string): Promise<AccessToken>
   introspect(token: string): Promise<Introspection | undefined>
-  revoke(id: string): Promise<void>
+  revoke(id: string, tokenValue: string): Promise<void>
   create(grantId: string, opts?: AccessTokenOpts): Promise<AccessToken>
-  rotate(managementId: string): Promise<Rotation>
+  rotate(managementId: string, tokenValue: string): Promise<Rotation>
 }
 
 interface ServiceDependencies extends BaseService {
@@ -76,10 +76,11 @@ export async function createAccessTokenService({
     getByManagementId: (managementId: string) =>
       getByManagementId(managementId),
     introspect: (token: string) => introspect(deps, token),
-    revoke: (id: string) => revoke(deps, id),
+    revoke: (id: string, tokenValue: string) => revoke(deps, id, tokenValue),
     create: (grantId: string, opts?: AccessTokenOpts) =>
       createAccessToken(deps, grantId, opts),
-    rotate: (managementId: string) => rotate(deps, managementId)
+    rotate: (managementId: string, tokenValue: string) =>
+      rotate(deps, managementId, tokenValue)
   }
 }
 
@@ -137,11 +138,17 @@ async function introspect(
   }
 }
 
-async function revoke(deps: ServiceDependencies, id: string): Promise<void> {
-  const token = await AccessToken.query(deps.knex).findOne({ managementId: id })
-  if (token) {
-    await token.$query(deps.knex).delete()
-  }
+async function revoke(
+  deps: ServiceDependencies,
+  id: string,
+  tokenValue: string
+): Promise<void> {
+  await AccessToken.query()
+    .findOne({
+      managementId: id,
+      value: tokenValue
+    })
+    .delete()
 }
 
 async function createAccessToken(
@@ -159,31 +166,47 @@ async function createAccessToken(
 
 async function rotate(
   deps: ServiceDependencies,
-  managementId: string
+  managementId: string,
+  tokenValue: string
 ): Promise<Rotation> {
-  let token = await AccessToken.query(deps.knex).findOne({ managementId })
-  if (token) {
-    await token.$query(deps.knex).delete()
-    token = await AccessToken.query(deps.knex).insertAndFetch({
-      value: crypto.randomBytes(8).toString('hex').toUpperCase(),
-      grantId: token.grantId,
-      expiresIn: token.expiresIn,
-      managementId: v4()
-    })
-    const access = await Access.query(deps.knex).where({
-      grantId: token.grantId
+  try {
+    return await AccessToken.transaction(async (trx) => {
+      const oldToken = await AccessToken.query(trx)
+        .delete()
+        .returning('*')
+        .findOne({
+          managementId,
+          value: tokenValue
+        })
+      if (oldToken) {
+        const token = await AccessToken.query(trx).insertAndFetch({
+          value: crypto.randomBytes(8).toString('hex').toUpperCase(),
+          grantId: oldToken.grantId,
+          expiresIn: oldToken.expiresIn,
+          managementId: v4()
+        })
+        const access = await Access.query(trx).where({
+          grantId: token.grantId
+        })
+
+        return {
+          success: true,
+          access,
+          value: token.value,
+          managementId: token.managementId,
+          expiresIn: token.expiresIn
+        }
+      } else {
+        return {
+          success: false,
+          error: new Error('token not found')
+        }
+      }
     })
-    return {
-      success: true,
-      access,
-      value: token.value,
-      managementId: token.managementId,
-      expiresIn: token.expiresIn
-    }
-  } else {
+  } catch (error) {
     return {
       success: false,
-      error: new Error('token not found')
+      error
     }
   }
 }
diff --git a/packages/auth/src/app.ts b/packages/auth/src/app.ts
@@ -72,7 +72,6 @@ export interface AppServices {
   knex: Promise<Knex>
   closeEmitter: Promise<EventEmitter>
   config: Promise<IAppConfig>
-  // TODO: Add GNAP-related services
   clientService: Promise<ClientService>
   grantService: Promise<GrantService>
   accessTokenRoutes: Promise<AccessTokenRoutes>
@@ -123,7 +122,6 @@ export class App {
       session(
         {
           key: 'sessionId',
-          // TODO: make this time shorter?
           maxAge: 60 * 1000,
           signed: true
         },
@@ -272,7 +270,6 @@ export class App {
     )
 
     /* Front Channel Routes */
-    // TODO: update front-channel routes to have /frontend prefix here and in openapi spec
 
     // Interaction start
     this.publicRouter.get(

diff --git a/packages/auth/src/config/app.ts b/packages/auth/src/config/app.ts
@@ -35,7 +35,7 @@ export const Config = {
   authServerDomain: envString(
     'AUTH_SERVER_DOMAIN',
     `http://localhost:${envInt('PORT', 3006)}`
-  ), // TODO: replace this with whatever frontend port ends up being
+  ),
   waitTimeSeconds: envInt('WAIT_SECONDS', 5),
   cookieKey: envString('COOKIE_KEY', crypto.randomBytes(32).toString('hex')),
   accessTokenExpirySeconds: envInt('ACCESS_TOKEN_EXPIRY_SECONDS', 10 * 60), // Default 10 minutes

diff --git a/packages/auth/src/grant/routes.test.ts b/packages/auth/src/grant/routes.test.ts
@@ -266,6 +266,7 @@ describe('Grant Routes', (): void => {
   })
 
   // TODO: validate that routes satisfy API spec
+  // https://github.com/interledger/rafiki/issues/841
   describe('interaction', (): void => {
     beforeEach(async (): Promise<void> => {
       const openApi = await deps.use('openApi')

diff --git a/packages/auth/src/grant/routes.ts b/packages/auth/src/grant/routes.ts
@@ -273,7 +273,6 @@ async function startInteraction(
     ctx.throw(401, { error: 'unknown_request' })
   }
 
-  // TODO: also establish session in redis with short expiry
   ctx.session.nonce = grant.interactNonce
 
   const interactionUrl = new URL(config.identityServerDomain)
@@ -286,11 +285,11 @@ async function startInteraction(
 }
 
 // TODO: allow idp to specify the reason for rejection
+// https://github.com/interledger/rafiki/issues/886
 async function handleGrantChoice(
   deps: ServiceDependencies,
   ctx: ChooseContext
 ): Promise<void> {
-  // TODO: check redis for a session
   const { id: interactId, nonce, choice } = ctx.params
   const { config, grantService } = deps
 

diff --git a/packages/auth/src/grant/service.ts b/packages/auth/src/grant/service.ts
@@ -139,8 +139,10 @@ async function create(
       interactId: interact ? v4() : undefined,
       interactRef: interact ? v4() : undefined,
       interactNonce: interact
-        ? crypto.randomBytes(8).toString('hex').toUpperCase()
-        : undefined, // TODO: factor out nonce generation
+        ? // TODO: factor out nonce generation
+          // https://github.com/interledger/rafiki/issues/887
+          crypto.randomBytes(8).toString('hex').toUpperCase()
+        : undefined,
       continueId: v4(),
       continueToken: crypto.randomBytes(8).toString('hex').toUpperCase()
     })