fix(plugin-common): fix wrong authorization

build/jest.ts

@@ -9,16 +9,18 @@ const [,, argv2, argv3] = process.argv
 if (argv2 && !argv2.startsWith('-')) {
   args.push(argv2)
   if (argv3 && !argv3.startsWith('-')) {
-    args.push('--collectCoverageFrom', `**/${argv3}/**/*.ts`, ...process.argv.slice(3))
+    args.push('--collectCoverageFrom')
+    if (argv3.endsWith('.ts')) {
+      args.push(`**/${argv3}`)
+    } else {
+      args.push(`**/${argv3}/**/*.ts`)
+    }
+    args.push(...process.argv.slice(4))
   } else {
     args.push(...process.argv.slice(3))
   }
 } else {
   args.push('packages/.+\\.spec\\.ts', ...process.argv.slice(2))
 }
 
-const child = spawn('npx', args, { stdio: 'inherit' })
-
-child.on('close', () => {
-  open(resolve(__dirname, '../coverage/lcov-report/index.html'))
-})
+spawn('npx', args, { stdio: 'inherit' })

packages/plugin-common/src/authorize.ts

@@ -13,17 +13,24 @@ interface AuthorizeInfo {
 export default function apply (ctx: Context, config: AuthorizeOptions = {}) {
   const { app, database } = ctx
   const { authorizeUser = {}, authorizeGroup = {} } = config
+  const logger = ctx.logger('authorize')
 
   /**
    * array of `AuthorizeInfo`
    */
   const authorizeInfoList: AuthorizeInfo[] = []
 
   /**
-   * a map of users' authority (buffered)
+   * a map of users' new authority
    * to make sure every user gets maximum possible authority
    */
-  const userAuthorityMap: Record<number, number> = {}
+  const newAuthorityMap = new Map<number, number>()
+
+  /**
+   * a map of users' old authority
+   * to prevent from duplicate data fetching
+   */
+  const oldAuthorityMap = new Map<number, number>()
 
   /**
    * inversion of `config.authorizeUser`
@@ -40,31 +47,38 @@ export default function apply (ctx: Context, config: AuthorizeOptions = {}) {
   }
 
   async function updateAuthorizeInfo (authority: number, ids: number[]) {
-    const users = await database.getUsers(ids, ['id', 'authority'])
+    const idsToFetch = ids.filter(id => !oldAuthorityMap.has(id))
+    const users = await database.getUsers(idsToFetch, ['id', 'authority'])
+    users.forEach((user) => oldAuthorityMap.set(user.id, user.authority))
+
     const info = authorizeInfoList[authority] || (authorizeInfoList[authority] = {
       insert: new Set(),
       update: new Set(),
     })
+
     for (const id of ids) {
-      const oldAuthority = userAuthorityMap[id]
-      if (oldAuthority) {
-        if (oldAuthority >= authority) continue
-        authorizeInfoList[oldAuthority].insert.delete(id)
-        authorizeInfoList[oldAuthority].update.delete(id)
+      const newAuthority = newAuthorityMap.get(id)
+      if (newAuthority) {
+        if (newAuthority >= authority) continue
+        authorizeInfoList[newAuthority].insert.delete(id)
+        authorizeInfoList[newAuthority].update.delete(id)
       }
-      userAuthorityMap[id] = authority
-      const user = users.find(u => u.id === id)
-      if (!user) {
+      newAuthorityMap.set(id, authority)
+
+      const oldAuthority = oldAuthorityMap.get(id)
+      if (!oldAuthority) {
         info.insert.add(id)
-      } else if (user.authority !== authority) {
+      } else if (oldAuthority < authority) {
         info.update.add(id)
       }
     }
   }
 
   app.receiver.once('ready', async () => {
     await Promise.all([
-      ...Object.keys(inversedUserMap).map(key => updateAuthorizeInfo(+key, inversedUserMap[+key])),
+      ...Object.keys(inversedUserMap).map(async (key) => {
+        await updateAuthorizeInfo(+key, inversedUserMap[+key])
+      }),
       ...Object.entries(authorizeGroup).map(async ([key, value]) => {
         const groupId = +key
         const config = typeof value === 'number' ? { member: value } : value
@@ -99,6 +113,10 @@ export default function apply (ctx: Context, config: AuthorizeOptions = {}) {
       }),
     ])
 
+    authorizeInfoList.forEach((info, authority) => {
+      logger.info(info)
+    })
+
     for (const key in authorizeInfoList) {
       const authority = +key
       const { insert, update } = authorizeInfoList[key]

packages/plugin-common/tests/authorize.spec.ts

@@ -1,110 +1,205 @@
-import { MockedApp } from 'koishi-test-utils'
+import { App } from 'koishi-test-utils'
 import { sleep } from 'koishi-utils'
-import { createUser, Meta } from 'koishi-core'
+import { createUser } from 'koishi-core'
 import { authorize, AuthorizeOptions } from '../src'
 import 'koishi-database-memory'
 
-const app = new MockedApp({ database: { memory: {} } })
-
-// make coverage happy
-app.plugin(authorize)
-app.plugin<AuthorizeOptions>(authorize, {
-  authorizeUser: {
-    123: 2,
-    231: 2,
-    312: 2,
-  },
-  authorizeGroup: {
-    123: 1,
-    231: {
-      admin: 2,
-      owner: 3,
+let counter = 0
+
+function createApp () {
+  return new App({ database: { memory: { identifier: ++counter } } })
+}
+
+test('authorize user', async () => {
+  const app = createApp()
+
+  // make coverage happy
+  app.plugin(authorize)
+  app.plugin<AuthorizeOptions>(authorize, {
+    authorizeUser: {
+      123: 2,
+      231: 2,
+      312: 2,
     },
-    312: 3,
-  },
-})
+  })
 
-// mock group
-app.setResponse('get_group_member_list', (params) => {
-  let data = []
-  if (params.group_id === 123) {
-    data = [
-      { user_id: 564, role: 'member' },
-    ]
-  } else if (params.group_id === 231) {
-    data = [
-      { user_id: 123, role: 'member' },
-      { user_id: 564, role: 'admin' },
-      { user_id: 645, role: 'owner' },
-    ]
-  }
-  return { data }
-})
+  app.receiver.on('connect', () => {
+    app.database.memory.store.user[231] = createUser(231, 3)
+    app.database.memory.store.user[312] = createUser(312, 2)
+  })
+
+  await app.start()
+  await sleep(0)
 
-app.receiver.on('connect', () => {
-  app.database.memory.store.user[231] = createUser(231, 3)
-  app.database.memory.store.user[312] = createUser(312, 2)
+  await expect(app.database.getUser(123)).resolves.toHaveProperty('authority', 2)
+  await expect(app.database.getUser(231)).resolves.toHaveProperty('authority', 3)
+  await expect(app.database.getUser(312)).resolves.toHaveProperty('authority', 2)
 })
 
-beforeAll(async () => {
+test('authorize group 1', async () => {
+  const app = createApp()
+
+  app.plugin<AuthorizeOptions>(authorize, {
+    authorizeGroup: {
+      456: 2,
+    },
+  })
+
+  app.receiver.on('connect', () => {
+    app.database.memory.store.user[231] = createUser(231, 3)
+  })
+
+  app.setResponse('get_group_member_list', [
+    { userId: 123, role: 'member' },
+    { userId: 231, role: 'member' },
+  ])
+
   await app.start()
   await sleep(0)
-})
 
-test('basic support', async () => {
+  await expect(app.database.getGroup(456)).resolves.toHaveProperty('assignee', app.selfId)
   await expect(app.database.getUser(123)).resolves.toHaveProperty('authority', 2)
-  await expect(app.database.getUser(231)).resolves.toHaveProperty('authority', 2)
-  await expect(app.database.getUser(312)).resolves.toHaveProperty('authority', 2)
-  await expect(app.database.getGroup(123)).resolves.toHaveProperty('assignee', app.selfId)
-  await expect(app.database.getGroup(231)).resolves.toHaveProperty('assignee', app.selfId)
-  await expect(app.database.getGroup(312)).resolves.toHaveProperty('assignee', app.selfId)
-  await expect(app.database.getUser(564)).resolves.toHaveProperty('authority', 2)
-  await expect(app.database.getUser(645)).resolves.toHaveProperty('authority', 3)
+  await expect(app.database.getUser(231)).resolves.toHaveProperty('authority', 3)
 })
 
-const createGroupIncrease = (userId: number, groupId: number): Meta => ({
-  postType: 'notice',
-  noticeType: 'group_increase',
-  subType: 'invite',
-  userId,
-  groupId,
+test('authorize group 2', async () => {
+  const app = createApp()
+
+  app.plugin<AuthorizeOptions>(authorize, {
+    authorizeGroup: {
+      456: { admin: 2, owner: 3 },
+    },
+  })
+
+  app.setResponse('get_group_member_list', [
+    { userId: 123, role: 'member' },
+    { userId: 231, role: 'admin' },
+    { userId: 312, role: 'owner' },
+  ])
+
+  await app.start()
+  await sleep(0)
+
+  await expect(app.database.getGroup(456)).resolves.toHaveProperty('assignee', app.selfId)
+  await expect(app.database.getUser(123)).resolves.toHaveProperty('authority', 1)
+  await expect(app.database.getUser(231)).resolves.toHaveProperty('authority', 2)
+  await expect(app.database.getUser(312)).resolves.toHaveProperty('authority', 3)
 })
 
-describe('handle group_increase', () => {
+describe('handle group increase', () => {
+  const app = createApp()
+
+  app.plugin<AuthorizeOptions>(authorize, {
+    authorizeGroup: {
+      456: 2,
+    },
+  })
+
+  app.receiver.on('connect', () => {
+    app.database.memory.store.user[231] = createUser(231, 3)
+    app.database.memory.store.user[312] = createUser(312, 1)
+  })
+
+  app.setResponse('get_group_member_list', [])
+
+  beforeAll(async () => {
+    await app.start()
+    await sleep(0)
+  })
+
   test('create new user', async () => {
-    app.receive(createGroupIncrease(456, 231))
+    app.receiveGroupIncrease('approve', 123, 456)
     await sleep(0)
-    await expect(app.database.getUser(456)).resolves.toHaveProperty('authority', 1)
+    await expect(app.database.getUser(123)).resolves.toHaveProperty('authority', 2)
   })
 
   test('not affect higher authority', async () => {
-    app.receive(createGroupIncrease(312, 231))
+    app.receiveGroupIncrease('approve', 231, 456)
     await sleep(0)
-    await expect(app.database.getUser(312)).resolves.toHaveProperty('authority', 2)
+    await expect(app.database.getUser(231)).resolves.toHaveProperty('authority', 3)
   })
 
   test('overwrite lower authority', async () => {
-    app.receive(createGroupIncrease(564, 312))
+    app.receiveGroupIncrease('approve', 312, 456)
     await sleep(0)
-    await expect(app.database.getUser(564)).resolves.toHaveProperty('authority', 3)
+    await expect(app.database.getUser(312)).resolves.toHaveProperty('authority', 2)
   })
 
   test('skip unregistered groups', async () => {
-    app.receive(createGroupIncrease(789, 789))
+    app.receiveGroupIncrease('approve', 789, 789)
     await sleep(0)
     await expect(app.database.getUser(789)).resolves.toHaveProperty('authority', 0)
   })
 })
 
-test('handle group-admin/set', async () => {
-  app.receive({
-    postType: 'notice',
-    noticeType: 'group_admin',
-    subType: 'set',
-    userId: 456,
-    groupId: 231,
+describe('handle group admin set', () => {
+  const app = createApp()
+
+  app.plugin<AuthorizeOptions>(authorize, {
+    authorizeGroup: {
+      456: { admin: 2 },
+    },
+  })
+
+  app.receiver.on('connect', () => {
+    app.database.memory.store.user[231] = createUser(231, 3)
+    app.database.memory.store.user[312] = createUser(312, 1)
   })
 
+  app.setResponse('get_group_member_list', [
+    { userId: 123, role: 'member' },
+    { userId: 231, role: 'member' },
+  ])
+
+  beforeAll(async () => {
+    await app.start()
+    await sleep(0)
+  })
+
+  test('not affect higher authority', async () => {
+    app.receiveGroupAdmin('set', 231, 456)
+    await sleep(0)
+    await expect(app.database.getUser(231)).resolves.toHaveProperty('authority', 3)
+  })
+
+  test('overwrite lower authority', async () => {
+    app.receiveGroupAdmin('set', 123, 456)
+    await sleep(0)
+    await expect(app.database.getUser(123)).resolves.toHaveProperty('authority', 2)
+  })
+
+  test('skip unregistered groups', async () => {
+    app.receiveGroupAdmin('set', 312, 789)
+    await sleep(0)
+    await expect(app.database.getUser(312)).resolves.toHaveProperty('authority', 1)
+  })
+})
+
+test('mixed usage', async () => {
+  const app = createApp()
+
+  app.plugin<AuthorizeOptions>(authorize, {
+    authorizeUser: {
+      123: 2,
+    },
+    authorizeGroup: {
+      456: 1,
+      564: 3,
+    },
+  })
+
+  app.receiver.on('connect', () => {
+    app.database.memory.store.user[231] = createUser(231, 2)
+  })
+
+  app.setResponse('get_group_member_list', [
+    { userId: 123, role: 'member' },
+    { userId: 231, role: 'member' },
+  ])
+
+  await app.start()
   await sleep(0)
-  await expect(app.database.getUser(456)).resolves.toHaveProperty('authority', 2)
+
+  await expect(app.database.getUser(123)).resolves.toHaveProperty('authority', 3)
+  await expect(app.database.getUser(231)).resolves.toHaveProperty('authority', 3)
 })