feat: simplify access token handling by removing unnecessary scope parameters and improving scope extraction

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

@@ -3,6 +3,7 @@ import { serializeUserSecurely } from '@growi/core/dist/models/serializers';
 import type { Response } from 'express';
 import { mock } from 'vitest-mock-extended';
 
+import { SCOPE } from '~/interfaces/scope';
 import type Crowi from '~/server/crowi';
 import type UserEvent from '~/server/events/user';
 import { AccessToken } from '~/server/models/access-token';
@@ -44,7 +45,7 @@ describe('access-token-parser middleware', () => {
     expect(reqMock.user).toBeUndefined();
 
     // act
-    await accessTokenParser(reqMock, resMock, nextMock);
+    await accessTokenParser()(reqMock, resMock, nextMock);
 
     // assert
     expect(reqMock.user).toBeUndefined();
@@ -64,7 +65,7 @@ describe('access-token-parser middleware', () => {
 
     // act
     reqMock.query.access_token = 'invalidToken';
-    await accessTokenParser(reqMock, resMock, nextMock);
+    await accessTokenParser()(reqMock, resMock, nextMock);
 
     // assert
     expect(reqMock.user).toBeUndefined();
@@ -93,7 +94,7 @@ describe('access-token-parser middleware', () => {
 
     // act
     reqMock.query.access_token = targetUser.apiToken;
-    await accessTokenParser(reqMock, resMock, nextMock);
+    await accessTokenParser()(reqMock, resMock, nextMock);
 
     // assert
     expect(reqMock.user).toBeDefined();
@@ -123,7 +124,7 @@ describe('access-token-parser middleware', () => {
 
     // act
     reqMock.body.access_token = targetUser.apiToken;
-    await accessTokenParser(reqMock, resMock, nextMock);
+    await accessTokenParser()(reqMock, resMock, nextMock);
 
     // assert
     expect(reqMock.user).toBeDefined();
@@ -135,7 +136,7 @@ describe('access-token-parser middleware', () => {
 });
 
 
-describe('access-token-parser middleware for access token', () => {
+describe('access-token-parser middleware for access token with scopes', () => {
 
   let User;
 
@@ -153,7 +154,7 @@ describe('access-token-parser middleware for access token', () => {
     User = userModelFactory(crowiMock);
   });
 
-  it('should set req.user with a valid access token in query', async() => {
+  it('should authenticate with no scopes', async() => {
     // arrange
     const reqMock = mock<AccessTokenParserReq>({
       user: undefined,
@@ -163,7 +164,7 @@ describe('access-token-parser middleware for access token', () => {
 
     expect(reqMock.user).toBeUndefined();
 
-    // prepare a user with an access token
+    // prepare a user
     const targetUser = await User.create({
       name: faker.person.fullName(),
       username: faker.string.uuid(),
@@ -171,10 +172,15 @@ describe('access-token-parser middleware for access token', () => {
       lang: 'en_US',
     });
 
+    // generate token with read:user:info scope
+    const { token } = await AccessToken.generateToken(
+      targetUser._id,
+      new Date(Date.now() + 1000 * 60 * 60 * 24),
+    );
+
     // act
-    const { token } = await AccessToken.generateToken(targetUser._id, new Date(Date.now() + 1000 * 60 * 60 * 24), []);
     reqMock.query.access_token = token;
-    await accessTokenParser(reqMock, resMock, nextMock);
+    await accessTokenParser()(reqMock, resMock, nextMock);
 
     // assert
     expect(reqMock.user).toBeDefined();
@@ -183,7 +189,7 @@ describe('access-token-parser middleware for access token', () => {
     expect(nextMock).toHaveBeenCalled();
   });
 
-  it('should set req.user with a valid access token in body', async() => {
+  it('should authenticate with no scopes', async() => {
     // arrange
     const reqMock = mock<AccessTokenParserReq>({
       user: undefined,
@@ -193,7 +199,7 @@ describe('access-token-parser middleware for access token', () => {
 
     expect(reqMock.user).toBeUndefined();
 
-    // prepare a user with an access token
+    // prepare a user
     const targetUser = await User.create({
       name: faker.person.fullName(),
       username: faker.string.uuid(),
@@ -201,10 +207,122 @@ describe('access-token-parser middleware for access token', () => {
       lang: 'en_US',
     });
 
+    // generate token with read:user:info scope
+    const { token } = await AccessToken.generateToken(
+      targetUser._id,
+      new Date(Date.now() + 1000 * 60 * 60 * 24),
+      [SCOPE.READ.USER.INFO],
+    );
+
     // act
-    const { token } = await AccessToken.generateToken(targetUser._id, new Date(Date.now() + 1000 * 60 * 60 * 24), []);
     reqMock.query.access_token = token;
-    await accessTokenParser(reqMock, resMock, nextMock);
+    await accessTokenParser()(reqMock, resMock, nextMock);
+
+    // assert
+    expect(reqMock.user).toBeDefined();
+    expect(reqMock.user?._id).toStrictEqual(targetUser._id);
+    expect(serializeUserSecurely).toHaveBeenCalledOnce();
+    expect(nextMock).toHaveBeenCalled();
+  });
+
+  it('should reject with insufficient scopes', async() => {
+    // arrange
+    const reqMock = mock<AccessTokenParserReq>({
+      user: undefined,
+    });
+    const resMock = mock<Response>();
+    const nextMock = vi.fn();
+
+    expect(reqMock.user).toBeUndefined();
+
+
+    // prepare a user
+    const targetUser = await User.create({
+      name: faker.person.fullName(),
+      username: faker.string.uuid(),
+      password: faker.internet.password(),
+      lang: 'en_US',
+    });
+
+    // generate token with read:user:info scope
+    const { token } = await AccessToken.generateToken(
+      targetUser._id,
+      new Date(Date.now() + 1000 * 60 * 60 * 24),
+      [SCOPE.READ.USER.INFO],
+    );
+
+    // act - try to access with write:user:info scope
+    reqMock.query.access_token = token;
+    await accessTokenParser([SCOPE.WRITE.USER.INFO])(reqMock, resMock, nextMock);
+
+    // assert
+    expect(reqMock.user).toBeUndefined();
+    expect(serializeUserSecurely).not.toHaveBeenCalled();
+    expect(nextMock).toHaveBeenCalled();
+  });
+
+  it('should authenticate with write scope implying read scope', async() => {
+    // arrange
+    const reqMock = mock<AccessTokenParserReq>({
+      user: undefined,
+    });
+    const resMock = mock<Response>();
+    const nextMock = vi.fn();
+
+    expect(reqMock.user).toBeUndefined();
+
+    // prepare a user
+    const targetUser = await User.create({
+      name: faker.person.fullName(),
+      username: faker.string.uuid(),
+      password: faker.internet.password(),
+      lang: 'en_US',
+    });
+
+    // generate token with write:user:info scope
+    const { token } = await AccessToken.generateToken(
+      targetUser._id,
+      new Date(Date.now() + 1000 * 60 * 60 * 24),
+      [SCOPE.WRITE.USER.INFO],
+    );
+
+    // act - try to access with read:user:info scope
+    reqMock.query.access_token = token;
+    await accessTokenParser([SCOPE.READ.USER.INFO])(reqMock, resMock, nextMock);
+
+    // assert
+    expect(reqMock.user).toBeDefined();
+    expect(reqMock.user?._id).toStrictEqual(targetUser._id);
+    expect(serializeUserSecurely).toHaveBeenCalledOnce();
+    expect(nextMock).toHaveBeenCalled();
+  });
+
+  it('should authenticate with wildcard scope', async() => {
+    // arrange
+    const reqMock = mock<AccessTokenParserReq>({
+      user: undefined,
+    });
+    const resMock = mock<Response>();
+    const nextMock = vi.fn();
+
+    // prepare a user
+    const targetUser = await User.create({
+      name: faker.person.fullName(),
+      username: faker.string.uuid(),
+      password: faker.internet.password(),
+      lang: 'en_US',
+    });
+
+    // generate token with read:user:* scope
+    const { token } = await AccessToken.generateToken(
+      targetUser._id,
+      new Date(Date.now() + 1000 * 60 * 60 * 24),
+      [SCOPE.READ.USER.ALL],
+    );
+
+    // act - try to access with read:user:info scope
+    reqMock.query.access_token = token;
+    await accessTokenParser([SCOPE.READ.USER.INFO])(reqMock, resMock, nextMock);
 
     // assert
     expect(reqMock.user).toBeDefined();

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

@@ -25,7 +25,7 @@ export const accessTokenParser = (scopes?: Scope[]) => {
 
     // check the api token is valid
     const User = mongoose.model<HydratedDocument<IUser>, { findUserByApiToken }>('User');
-    const userByApiToken: IUserHasId = await User.findUserByApiToken(accessToken, scopes);
+    const userByApiToken: IUserHasId = await User.findUserByApiToken(accessToken);
     if (userByApiToken != null) {
       req.user = serializeUserSecurely(userByApiToken);
       logger.debug('API token parsed.');

apps/app/src/server/models/access-token.ts

@@ -12,6 +12,7 @@ import type { Scope } from '~/interfaces/scope';
 import loggerFactory from '~/utils/logger';
 
 import { getOrCreateModel } from '../util/mongoose-utils';
+import { extractScopes } from '../util/scope-utils';
 
 const logger = loggerFactory('growi:models:access-token');
 
@@ -63,17 +64,18 @@ accessTokenSchema.plugin(uniqueValidator);
 
 accessTokenSchema.statics.generateToken = async function(userId: Types.ObjectId | string, expiredAt: Date, scope?: Scope[], description?: string) {
 
+  const extractedScopes = extractScopes(scope ?? []);
   const token = crypto.randomBytes(32).toString('hex');
   const tokenHash = generateTokenHash(token);
 
   try {
     const { _id } = await this.create({
-      user: userId, tokenHash, expiredAt, scope, description,
+      user: userId, tokenHash, expiredAt, scope: extractedScopes, description,
     });
 
     logger.debug('Token generated');
     return {
-      token, _id, expiredAt, scope, description,
+      token, _id, expiredAt, scope: extractedScopes, description,
     };
   }
   catch (err) {
@@ -103,7 +105,10 @@ accessTokenSchema.statics.deleteExpiredToken = async function() {
 accessTokenSchema.statics.findUserIdByToken = async function(token: string, requiredScopes?: Scope[]) {
   const tokenHash = generateTokenHash(token);
   const now = new Date();
-  return this.findOne({ tokenHash, expiredAt: { $gt: now }, scope: { $all: requiredScopes ?? [] } }).select('user');
+  if (requiredScopes != null && requiredScopes.length > 0) {
+    return this.findOne({ tokenHash, expiredAt: { $gt: now }, scope: { $all: requiredScopes } }).select('user');
+  }
+  return this.findOne({ tokenHash, expiredAt: { $gt: now } }).select('user');
 };
 
 accessTokenSchema.statics.findTokenByUserId = async function(userId: Types.ObjectId | string) {

apps/app/src/server/routes/apiv3/personal-setting/generate-access-token.ts

@@ -10,7 +10,7 @@ import type { Scope } from '~/interfaces/scope';
 import type Crowi from '~/server/crowi';
 import { generateAddActivityMiddleware } from '~/server/middlewares/add-activity';
 import { AccessToken } from '~/server/models/access-token';
-import { extractScopes, isValidScope } from '~/server/util/scope-utils';
+import { isValidScope } from '~/server/util/scope-utils';
 import loggerFactory from '~/utils/logger';
 
 import { apiV3FormValidator } from '../../../middlewares/apiv3-form-validator';
@@ -91,7 +91,7 @@ export const generateAccessTokenHandlerFactory: GenerateAccessTokenHandlerFactor
       const { expiredAt, description, scope } = body;
 
       try {
-        const tokenData = await AccessToken.generateToken(user._id, expiredAt, extractScopes(scope), description);
+        const tokenData = await AccessToken.generateToken(user._id, expiredAt, scope, description);
 
         const parameters = { action: SupportedAction.ACTION_USER_ACCESS_TOKEN_CREATE };
         activityEvent.emit('update', res.locals.activity._id, parameters);

apps/app/src/server/routes/apiv3/personal-setting/get-access-tokens.ts

@@ -25,7 +25,7 @@ export const getAccessTokenHandlerFactory: GetAccessTokenHandlerFactory = (crowi
   const addActivity = generateAddActivityMiddleware();
 
   return [
-    accessTokenParser([SCOPE.READ.USER.API.ACCESS_TOKEN]),
+    accessTokenParser(),
     loginRequiredStrictly,
     addActivity,
     async(req: GetAccessTokenRequest, res: ApiV3Response) => {

apps/app/src/server/util/scope-utils.ts

@@ -45,7 +45,7 @@ const getAllScopeValues = (scopeObj: any): Scope[] => {
  * For example, WRITE permission implies READ permission
  */
 const getImpliedScopes = (scope: Scope): Scope[] => {
-  const scopeParts = scope.split(':').map(x => (x.toUpperCase()));
+  const scopeParts = scope.split(':');
   if (scopeParts[0] === ACTION.READ) {
     return [scope];
   }