Merge pull request #10008 from weseek/imprv/166722-modify-new-access-token-parser-to-optionally-accept-legacy-api-tokens

imprv: Modify new accessTokenParser to optionally accept legacy api tokens

apps/app/src/server/middlewares/access-token-parser/access-token.integ.ts

@@ -38,9 +38,8 @@ describe('access-token-parser middleware for access token with scopes', () => {
       user: undefined,
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
-    await parserForAccessToken([])(reqMock, resMock, nextMock);
+    await parserForAccessToken([])(reqMock, resMock);
 
     expect(reqMock.user).toBeUndefined();
   });
@@ -51,7 +50,6 @@ describe('access-token-parser middleware for access token with scopes', () => {
       user: undefined,
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
     expect(reqMock.user).toBeUndefined();
 
@@ -71,7 +69,7 @@ describe('access-token-parser middleware for access token with scopes', () => {
 
     // act
     reqMock.query.access_token = token;
-    await parserForAccessToken([])(reqMock, resMock, nextMock);
+    await parserForAccessToken([])(reqMock, resMock);
 
     // assert
     expect(reqMock.user).toBeUndefined();
@@ -84,7 +82,6 @@ describe('access-token-parser middleware for access token with scopes', () => {
       user: undefined,
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
     expect(reqMock.user).toBeUndefined();
 
@@ -105,7 +102,7 @@ describe('access-token-parser middleware for access token with scopes', () => {
 
     // act
     reqMock.query.access_token = token;
-    await parserForAccessToken([SCOPE.READ.USER_SETTINGS.INFO])(reqMock, resMock, nextMock);
+    await parserForAccessToken([SCOPE.READ.USER_SETTINGS.INFO])(reqMock, resMock);
 
     // assert
     expect(reqMock.user).toBeDefined();
@@ -119,7 +116,6 @@ describe('access-token-parser middleware for access token with scopes', () => {
       user: undefined,
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
     expect(reqMock.user).toBeUndefined();
 
@@ -141,7 +137,7 @@ describe('access-token-parser middleware for access token with scopes', () => {
 
     // act - try to access with write:user:info scope
     reqMock.query.access_token = token;
-    await parserForAccessToken([SCOPE.WRITE.USER_SETTINGS.INFO])(reqMock, resMock, nextMock);
+    await parserForAccessToken([SCOPE.WRITE.USER_SETTINGS.INFO])(reqMock, resMock);
 
     // // assert
     expect(reqMock.user).toBeUndefined();
@@ -154,7 +150,6 @@ describe('access-token-parser middleware for access token with scopes', () => {
       user: undefined,
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
     expect(reqMock.user).toBeUndefined();
 
@@ -175,7 +170,7 @@ describe('access-token-parser middleware for access token with scopes', () => {
 
     // act - try to access with read:user:info scope
     reqMock.query.access_token = token;
-    await parserForAccessToken([SCOPE.READ.USER_SETTINGS.INFO])(reqMock, resMock, nextMock);
+    await parserForAccessToken([SCOPE.READ.USER_SETTINGS.INFO])(reqMock, resMock);
 
     // assert
     expect(reqMock.user).toBeDefined();
@@ -189,7 +184,6 @@ describe('access-token-parser middleware for access token with scopes', () => {
       user: undefined,
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
     // prepare a user
     const targetUser = await User.create({
@@ -208,7 +202,7 @@ describe('access-token-parser middleware for access token with scopes', () => {
 
     // act - try to access with read:user:info scope
     reqMock.query.access_token = token;
-    await parserForAccessToken([SCOPE.READ.USER_SETTINGS.INFO, SCOPE.READ.USER_SETTINGS.API.ACCESS_TOKEN])(reqMock, resMock, nextMock);
+    await parserForAccessToken([SCOPE.READ.USER_SETTINGS.INFO, SCOPE.READ.USER_SETTINGS.API.ACCESS_TOKEN])(reqMock, resMock);
 
     // assert
     expect(reqMock.user).toBeDefined();

apps/app/src/server/middlewares/access-token-parser/access-token.ts

@@ -1,6 +1,6 @@
 import type { IUserHasId } from '@growi/core/dist/interfaces';
 import { serializeUserSecurely } from '@growi/core/dist/models/serializers';
-import type { NextFunction, Response } from 'express';
+import type { Response } from 'express';
 
 import type { Scope } from '~/interfaces/scope';
 import { AccessToken } from '~/server/models/access-token';
@@ -11,7 +11,7 @@ import type { AccessTokenParserReq } from './interfaces';
 const logger = loggerFactory('growi:middleware:access-token-parser:access-token');
 
 export const parserForAccessToken = (scopes: Scope[]) => {
-  return async(req: AccessTokenParserReq, res: Response, next: NextFunction): Promise<void> => {
+  return async(req: AccessTokenParserReq, res: Response): Promise<void> => {
 
     const accessToken = req.query.access_token ?? req.body.access_token;
     if (accessToken == null || typeof accessToken !== 'string') {

apps/app/src/server/middlewares/access-token-parser/api-token.integ.ts

@@ -38,12 +38,11 @@ describe('access-token-parser middleware', () => {
       user: undefined,
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
     expect(reqMock.user).toBeUndefined();
 
     // act
-    await parserForApiToken(reqMock, resMock, nextMock);
+    await parserForApiToken(reqMock, resMock);
 
     // assert
     expect(reqMock.user).toBeUndefined();
@@ -56,13 +55,12 @@ describe('access-token-parser middleware', () => {
       user: undefined,
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
     expect(reqMock.user).toBeUndefined();
 
     // act
     reqMock.query.access_token = 'invalidToken';
-    await parserForApiToken(reqMock, resMock, nextMock);
+    await parserForApiToken(reqMock, resMock);
 
     // assert
     expect(reqMock.user).toBeUndefined();
@@ -90,7 +88,7 @@ describe('access-token-parser middleware', () => {
 
     // act
     reqMock.query.access_token = targetUser.apiToken;
-    await parserForApiToken(reqMock, resMock, nextMock);
+    await parserForApiToken(reqMock, resMock);
 
     // assert
     expect(reqMock.user).toBeDefined();
@@ -104,7 +102,6 @@ describe('access-token-parser middleware', () => {
       user: undefined,
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
     expect(reqMock.user).toBeUndefined();
 
@@ -119,7 +116,7 @@ describe('access-token-parser middleware', () => {
 
     // act
     reqMock.body.access_token = targetUser.apiToken;
-    await parserForApiToken(reqMock, resMock, nextMock);
+    await parserForApiToken(reqMock, resMock);
 
     // assert
     expect(reqMock.user).toBeDefined();
@@ -136,7 +133,6 @@ describe('access-token-parser middleware', () => {
       },
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
     expect(reqMock.user).toBeUndefined();
 
@@ -151,13 +147,12 @@ describe('access-token-parser middleware', () => {
 
     // act
     reqMock.headers.authorization = `Bearer ${targetUser.apiToken}`;
-    await accessTokenParser(reqMock, resMock, nextMock);
+    await parserForApiToken(reqMock, resMock);
 
     // assert
     expect(reqMock.user).toBeDefined();
     expect(reqMock.user?._id).toStrictEqual(targetUser._id);
     expect(serializeUserSecurely).toHaveBeenCalledOnce();
-    expect(nextMock).toHaveBeenCalled();
   });
 
   it('should ignore non-Bearer Authorization header', async() => {
@@ -169,7 +164,6 @@ describe('access-token-parser middleware', () => {
       },
     });
     const resMock = mock<Response>();
-    const nextMock = vi.fn();
 
     expect(reqMock.user).toBeUndefined();
 
@@ -178,12 +172,11 @@ describe('access-token-parser middleware', () => {
 
     // act
     reqMock.headers.authorization = `Basic ${randomString}`; // Basic auth header with random string
-    await accessTokenParser(reqMock, resMock, nextMock);
+    await parserForApiToken(reqMock, resMock);
 
     // assert
     expect(reqMock.user).toBeUndefined();
     expect(serializeUserSecurely).not.toHaveBeenCalled();
-    expect(nextMock).toHaveBeenCalled();
   });
 
 });

apps/app/src/server/middlewares/access-token-parser/api-token.ts

@@ -23,7 +23,7 @@ const extractBearerToken = (authHeader: string | undefined): string | null => {
 };
 
 
-export const parserForApiToken = async(req: AccessTokenParserReq, res: Response, next: NextFunction): Promise<void> => {
+export const parserForApiToken = async(req: AccessTokenParserReq, res: Response): Promise<void> => {
   // Extract token from Authorization header first
   const bearerToken = extractBearerToken(req.headers.authorization);
 

apps/app/src/server/middlewares/access-token-parser/index.ts

@@ -6,14 +6,18 @@ import { parserForAccessToken } from './access-token';
 import { parserForApiToken } from './api-token';
 import type { AccessTokenParserReq } from './interfaces';
 
-export const accessTokenParser = (scopes?: Scope[]) => {
+export const accessTokenParser = (scopes?: Scope[], opts?: {acceptLegacy: boolean}) => {
   return async(req: AccessTokenParserReq, res: Response, next: NextFunction): Promise<void> => {
     // TODO: comply HTTP header of RFC6750 / Authorization: Bearer
+    if (scopes == null || scopes.length === 0) {
+      return next();
+    }
+
+    await parserForAccessToken(scopes)(req, res);
 
-    if (scopes != null) {
-      await parserForAccessToken(scopes)(req, res, next);
+    if (opts?.acceptLegacy) {
+      await parserForApiToken(req, res);
     }
-    await parserForApiToken(req, res, next);
 
     return next();
   };

apps/app/src/server/routes/apiv3/personal-setting/index.js

@@ -458,12 +458,12 @@ module.exports = (crowi) => {
    *         200:
    *           description: succded to get access token
    *           content:
-   *           application/json:
-   *             schema:
-   *               properties:
-   *                 accessTokens:
-   *                   type: objet
-   *                   description: array of access tokens
+   *             application/json:
+   *               schema:
+   *                 properties:
+   *                   accessTokens:
+   *                     type: object
+   *                     description: array of access tokens
    */
   router.get('/access-token', accessTokenParser([SCOPE.READ.USER_SETTINGS.API.ACCESS_TOKEN]), getAccessTokenHandlerFactory(crowi));
 
@@ -489,14 +489,16 @@ module.exports = (crowi) => {
    *                     type: string
    *                     description: access token
    *                   expiredAt:
-   *                     type: Date
+   *                     type: string
    *                     description: expired date
    *                   description:
    *                     type: string
    *                     description: description of access token
    *                   scope:
-   *                     type: string[]
+   *                     type: array
    *                     description: scope of access token
+   *                     items:
+   *                      type: string
    */
   router.post('/access-token', accessTokenParser([SCOPE.WRITE.USER_SETTINGS.API.ACCESS_TOKEN]), generateAccessTokenHandlerFactory(crowi));
 
@@ -504,13 +506,13 @@ module.exports = (crowi) => {
    * @swagger
    *   /personal-setting/access-token/:
    *     delete:
-   *     tags: [GeneralSetting]
-   *     operationId: deleteAccessToken
-   *     summary: /personal-setting/access-token
-   *     description: Delete access token
-   *     responses:
-   *       200:
-   *         description: succeded to delete access token
+   *       tags: [GeneralSetting]
+   *       operationId: deleteAccessToken
+   *       summary: /personal-setting/access-token
+   *       description: Delete access token
+   *       responses:
+   *         200:
+   *           description: succeded to delete access token
    *
    */
   router.delete('/access-token', accessTokenParser([SCOPE.WRITE.USER_SETTINGS.API.ACCESS_TOKEN]), deleteAccessTokenHandlersFactory(crowi));