Require auth token but skip verification for tasks queue functions. (#1073)

Previously, we allowed TQ functions to either:

1) Not have any auth token
2) Have admin SDK verified auth tokens.

Since TQ functions are guarded by IAM, we instead implement the following behavior:

1) MUST have auth token
2) We do not verify auth token - instead we just decode it and pass it to the handler

Author: taeold

CHANGELOG.md

@@ -0,0 +1 @@
+- Improve authorization for tasks. (#1073)

spec/common/providers/tasks.spec.ts

@@ -21,14 +21,12 @@
 // SOFTWARE.
 
 import { expect } from 'chai';
-import * as sinon from 'sinon';
 import * as firebase from 'firebase-admin';
 
 import { checkAuthContext, runHandler } from '../../helper';
 import {
   generateIdToken,
   generateUnsignedIdToken,
-  mockFetchPublicKeys,
   mockRequest,
 } from '../../fixtures/mockrequest';
 import {
@@ -39,7 +37,6 @@ import {
 import { apps as appsNamespace } from '../../../src/apps';
 import * as mocks from '../../fixtures/credential/key.json';
 import * as https from '../../../src/common/providers/https';
-import * as debug from '../../../src/common/debug';
 
 /** Represents a test case for a Task Queue Function */
 interface TaskTest {
@@ -83,6 +80,14 @@ export async function runTaskTest(test: TaskTest): Promise<any> {
 describe('onEnqueueHandler', () => {
   let app: firebase.app.App;
 
+  function mockEnqueueRequest(
+    data: unknown,
+    contentType: string = 'application/json',
+    context: { authorization: string } = { authorization: 'Bearer abc' }
+  ): ReturnType<typeof mockRequest> {
+    return mockRequest(data, contentType, context);
+  }
+
   before(() => {
     const credential = {
       getAccessToken: () => {
@@ -111,7 +116,7 @@ describe('onEnqueueHandler', () => {
 
   it('should handle success', () => {
     return runTaskTest({
-      httpRequest: mockRequest({ foo: 'bar' }),
+      httpRequest: mockEnqueueRequest({ foo: 'bar' }),
       expectedData: { foo: 'bar' },
       expectedStatus: 204,
     });
@@ -129,22 +134,22 @@ describe('onEnqueueHandler', () => {
 
   it('should ignore charset', () => {
     return runTaskTest({
-      httpRequest: mockRequest(null, 'application/json; charset=utf-8'),
+      httpRequest: mockEnqueueRequest(null, 'application/json; charset=utf-8'),
       expectedData: null,
       expectedStatus: 204,
     });
   });
 
   it('should reject bad content type', () => {
     return runTaskTest({
-      httpRequest: mockRequest(null, 'text/plain'),
+      httpRequest: mockEnqueueRequest(null, 'text/plain'),
       expectedData: null,
       expectedStatus: 400,
     });
   });
 
   it('should reject extra body fields', () => {
-    const req = mockRequest(null);
+    const req = mockEnqueueRequest(null);
     req.body.extra = 'bad';
     return runTaskTest({
       httpRequest: req,
@@ -155,7 +160,7 @@ describe('onEnqueueHandler', () => {
 
   it('should handle unhandled error', () => {
     return runTaskTest({
-      httpRequest: mockRequest(null),
+      httpRequest: mockEnqueueRequest(null),
       expectedData: null,
       taskFunction: (data, context) => {
         throw new Error(`ceci n'est pas une error`);
@@ -169,7 +174,7 @@ describe('onEnqueueHandler', () => {
 
   it('should handle unknown error status', () => {
     return runTaskTest({
-      httpRequest: mockRequest(null),
+      httpRequest: mockEnqueueRequest(null),
       expectedData: null,
       taskFunction: (data, context) => {
         throw new https.HttpsError('THIS_IS_NOT_VALID' as any, 'nope');
@@ -183,7 +188,7 @@ describe('onEnqueueHandler', () => {
 
   it('should handle well-formed error', () => {
     return runTaskTest({
-      httpRequest: mockRequest(null),
+      httpRequest: mockEnqueueRequest(null),
       expectedData: null,
       taskFunction: (data, context) => {
         throw new https.HttpsError('not-found', 'i am error');
@@ -196,11 +201,10 @@ describe('onEnqueueHandler', () => {
   });
 
   it('should handle auth', async () => {
-    const mock = mockFetchPublicKeys();
     const projectId = appsNamespace().admin.options.projectId;
     const idToken = generateIdToken(projectId);
     await runTaskTest({
-      httpRequest: mockRequest(null, 'application/json', {
+      httpRequest: mockEnqueueRequest(null, 'application/json', {
         authorization: 'Bearer ' + idToken,
       }),
       expectedData: null,
@@ -214,49 +218,25 @@ describe('onEnqueueHandler', () => {
       },
       expectedStatus: 204,
     });
-    mock.done();
   });
 
-  it('should reject bad auth', async () => {
+  it('should accept unsigned auth too', async () => {
     const projectId = appsNamespace().admin.options.projectId;
     const idToken = generateUnsignedIdToken(projectId);
     await runTaskTest({
-      httpRequest: mockRequest(null, 'application/json', {
+      httpRequest: mockEnqueueRequest(null, 'application/json', {
         authorization: 'Bearer ' + idToken,
       }),
       expectedData: null,
-      expectedStatus: 401,
-    });
-  });
-
-  describe('skip token verification debug mode support', () => {
-    before(() => {
-      sinon
-        .stub(debug, 'isDebugFeatureEnabled')
-        .withArgs('skipTokenVerification')
-        .returns(true);
-    });
-
-    after(() => {
-      sinon.verifyAndRestore();
-    });
-
-    it('should skip auth token verification', async () => {
-      const projectId = appsNamespace().admin.options.projectId;
-      const idToken = generateUnsignedIdToken(projectId);
-      await runTaskTest({
-        httpRequest: mockRequest(null, 'application/json', {
-          authorization: 'Bearer ' + idToken,
-        }),
-        expectedData: null,
-        taskFunction: (data, context) => {
-          checkAuthContext(context, projectId, mocks.user_id);
-        },
-        taskFunction2: (request) => {
-          checkAuthContext(request, projectId, mocks.user_id);
-        },
-        expectedStatus: 204,
-      });
+      taskFunction: (data, context) => {
+        checkAuthContext(context, projectId, mocks.user_id);
+        return null;
+      },
+      taskFunction2: (request) => {
+        checkAuthContext(request, projectId, mocks.user_id);
+        return null;
+      },
+      expectedStatus: 204,
     });
   });
 });

spec/v1/providers/tasks.spec.ts

@@ -146,6 +146,7 @@ describe('#onDispatch', () => {
       },
       {
         'content-type': 'application/json',
+        authorization: 'Bearer abc',
       }
     );
     req.method = 'POST';

spec/v2/providers/tasks.spec.ts

@@ -149,16 +149,9 @@ describe('onTaskDispatched', () => {
   });
 
   it('has a .run method', async () => {
-    const request: any = {
-      data: 'data',
-      auth: {
-        uid: 'abc',
-        token: 'token',
-      },
-    };
+    const request: any = { data: 'data' };
     const cf = onTaskDispatched((r) => {
       expect(r.data).to.deep.equal(request.data);
-      expect(r.auth).to.deep.equal(request.auth);
     });
 
     await cf.run(request);
@@ -173,6 +166,7 @@ describe('onTaskDispatched', () => {
       },
       {
         'content-type': 'application/json',
+        authorization: 'Bearer abc',
         origin: 'example.com',
       }
     );

src/common/providers/tasks.ts

@@ -110,12 +110,20 @@ export function onDispatchHandler<Req = any>(
         throw new https.HttpsError('invalid-argument', 'Bad Request');
       }
 
-      const context: TaskContext = {};
-      const status = await https.checkAuthToken(req, context);
+      const authHeader = req.header('Authorization') || '';
+      const token = authHeader.match(/^Bearer (.*)$/)?.[1];
       // Note: this should never happen since task queue functions are guarded by IAM.
-      if (status === 'INVALID') {
+      if (!token) {
         throw new https.HttpsError('unauthenticated', 'Unauthenticated');
       }
+      // We skip authenticating the token since tq functions are guarded by IAM.
+      const authToken = await https.unsafeDecodeIdToken(token);
+      const context: TaskContext = {
+        auth: {
+          uid: authToken.uid,
+          token: authToken,
+        },
+      };
 
       const data: Req = https.decode(req.body.data);
       if (handler.length === 2) {