Merge pull request #3471 from gradido/fix_humhub_sync

fix(backend): humhub sync on edge cases

backend/src/apis/humhub/ExportUsers.ts

@@ -26,7 +26,7 @@ function getUsersPage(page: number, limit: number): Promise<[User[], number]> {
 
 /**
  * @param client
- * @returns user map indices with email
+ * @returns user map indices with username
  */
 async function loadUsersFromHumHub(client: HumHubClient): Promise<Map<string, GetUser>> {
   const start = new Date().getTime()
@@ -42,7 +42,7 @@ async function loadUsersFromHumHub(client: HumHubClient): Promise<Map<string, Ge
     usersPage.results.forEach((user) => {
       // deleted users have empty emails
       if (user.account.email) {
-        humhubUsers.set(user.account.email.trim(), user)
+        humhubUsers.set(user.account.username, user)
       } else {
         skippedUsersCount++
       }
@@ -52,6 +52,7 @@ async function loadUsersFromHumHub(client: HumHubClient): Promise<Map<string, Ge
       `load users from humhub: ${humhubUsers.size}/${usersPage.total}, skipped: ${skippedUsersCount}\r`,
     )
   } while (usersPage && usersPage.results.length === HUMHUB_BULK_SIZE)
+  process.stdout.write('\n')
 
   const elapsed = new Date().getTime() - start
   logger.info('load users from humhub', {
@@ -87,23 +88,29 @@ async function main() {
   const humhubUsers = await loadUsersFromHumHub(humHubClient)
 
   let dbUserCount = 0
-  const executedHumhubActionsCount = [0, 0, 0, 0]
+  const executedHumhubActionsCount = [0, 0, 0, 0, 0]
 
   do {
-    const [users, totalUsers] = await getUsersPage(page, USER_BULK_SIZE)
-    dbUserCount += users.length
-    userCount = users.length
-    page++
-    const promises: Promise<ExecutedHumhubAction>[] = []
-    users.forEach((user: User) => promises.push(syncUser(user, humhubUsers)))
-    const executedActions = await Promise.all(promises)
-    executedActions.forEach((executedAction: ExecutedHumhubAction) => {
-      executedHumhubActionsCount[executedAction as number]++
-    })
-    // using process.stdout.write here so that carriage-return is working analog to c
-    // printf("\rchecked user: %d/%d", dbUserCount, totalUsers);
-    process.stdout.write(`checked user: ${dbUserCount}/${totalUsers}\r`)
+    try {
+      const [users, totalUsers] = await getUsersPage(page, USER_BULK_SIZE)
+      dbUserCount += users.length
+      userCount = users.length
+      page++
+      const promises: Promise<ExecutedHumhubAction>[] = []
+      users.forEach((user: User) => promises.push(syncUser(user, humhubUsers)))
+      const executedActions = await Promise.all(promises)
+      executedActions.forEach((executedAction: ExecutedHumhubAction) => {
+        executedHumhubActionsCount[executedAction as number]++
+      })
+      // using process.stdout.write here so that carriage-return is working analog to c
+      // printf("\rchecked user: %d/%d", dbUserCount, totalUsers);
+      process.stdout.write(`checked user: ${dbUserCount}/${totalUsers}\r`)
+    } catch (e) {
+      process.stdout.write('\n')
+      throw e
+    }
   } while (userCount === USER_BULK_SIZE)
+  process.stdout.write('\n')
 
   await con.destroy()
   const elapsed = new Date().getTime() - start
@@ -114,12 +121,13 @@ async function main() {
     updatedCount: executedHumhubActionsCount[ExecutedHumhubAction.UPDATE],
     skippedCount: executedHumhubActionsCount[ExecutedHumhubAction.SKIP],
     deletedCount: executedHumhubActionsCount[ExecutedHumhubAction.DELETE],
+    validationErrorCount: executedHumhubActionsCount[ExecutedHumhubAction.VALIDATION_ERROR],
   })
 }
 
 main().catch((e) => {
   // eslint-disable-next-line no-console
-  console.error(e)
+  console.error(JSON.stringify(e, null, 2))
   // eslint-disable-next-line n/no-process-exit
   process.exit(1)
 })

backend/src/apis/humhub/__mocks__/syncUser.ts

@@ -2,6 +2,7 @@ import { User } from '@entity/User'
 
 import { isHumhubUserIdenticalToDbUser } from '@/apis/humhub/compareHumhubUserDbUser'
 import { GetUser } from '@/apis/humhub/model/GetUser'
+import { PostUser } from '@/apis/humhub/model/PostUser'
 
 export enum ExecutedHumhubAction {
   UPDATE,
@@ -26,7 +27,8 @@ export async function syncUser(
   user: User,
   humhubUsers: Map<string, GetUser>,
 ): Promise<ExecutedHumhubAction> {
-  const humhubUser = humhubUsers.get(user.emailContact.email.trim())
+  const postUser = new PostUser(user)
+  const humhubUser = humhubUsers.get(postUser.account.username)
   if (humhubUser) {
     if (!user.humhubAllowed) {
       return Promise.resolve(ExecutedHumhubAction.DELETE)

backend/src/apis/humhub/compareHumhubUserDbUser.test.ts

@@ -1,4 +1,7 @@
 /* eslint-disable prettier/prettier */
+// eslint-disable-next-line import/no-unassigned-import
+import 'reflect-metadata'
+import { PublishNameType } from '@/graphql/enum/PublishNameType'
 import { communityDbUser } from '@/util/communityUser'
 
 import { isHumhubUserIdenticalToDbUser } from './compareHumhubUserDbUser'
@@ -13,6 +16,7 @@ describe('isHumhubUserIdenticalToDbUser', () => {
         defaultUser.alias = 'alias'
         defaultUser.emailContact.email = 'email@gmail.com'
         defaultUser.language = 'en'
+        defaultUser.gradidoID = 'gradidoID'
     })
 
     it('Should return true because humhubUser was created from entity user', () => {
@@ -21,6 +25,20 @@ describe('isHumhubUserIdenticalToDbUser', () => {
         expect(result).toBe(true)
     })
 
+    it('Should return false, because last name differ because of publish name type', () => {
+        const humhubUser = new GetUser(defaultUser, 1)
+        defaultUser.humhubPublishName = PublishNameType.PUBLISH_NAME_FIRST
+        const result = isHumhubUserIdenticalToDbUser(humhubUser, defaultUser)
+        expect(result).toBe(false)
+    })
+
+    it('Should return true, even if alias is empty', () => {
+        defaultUser.alias = ''
+        const humhubUser = new GetUser(defaultUser, 1)
+        const result = isHumhubUserIdenticalToDbUser(humhubUser, defaultUser)
+        expect(result).toBe(true)
+    })
+
     it('Should return false because first name differ', () => {
         const humhubUser = new GetUser(defaultUser, 1)
         humhubUser.profile.firstname = 'changed first name'

backend/src/apis/humhub/syncUser.test.ts

@@ -28,7 +28,8 @@ describe('syncUser function', () => {
 
   it('When humhubUser exists and user.humhubAllowed is false, should return DELETE action', async () => {
     const humhubUsers = new Map<string, GetUser>()
-    humhubUsers.set(defaultUser.emailContact.email, new GetUser(defaultUser, 1))
+    const humhubUser = new GetUser(defaultUser, 1)
+    humhubUsers.set(humhubUser.account.username, humhubUser)
 
     defaultUser.humhubAllowed = false
     const result = await syncUser(defaultUser, humhubUsers)
@@ -39,8 +40,8 @@ describe('syncUser function', () => {
   it('When humhubUser exists and user.humhubAllowed is true and there are changes in user data, should return UPDATE action', async () => {
     const humhubUsers = new Map<string, GetUser>()
     const humhubUser = new GetUser(defaultUser, 1)
+    humhubUsers.set(humhubUser.account.username, humhubUser)
     humhubUser.account.username = 'test username'
-    humhubUsers.set(defaultUser.emailContact.email, humhubUser)
 
     defaultUser.humhubAllowed = true
     const result = await syncUser(defaultUser, humhubUsers)
@@ -51,7 +52,7 @@ describe('syncUser function', () => {
   it('When humhubUser exists and user.humhubAllowed is true and there are no changes in user data, should return SKIP action', async () => {
     const humhubUsers = new Map<string, GetUser>()
     const humhubUser = new GetUser(defaultUser, 1)
-    humhubUsers.set(defaultUser.emailContact.email, humhubUser)
+    humhubUsers.set(humhubUser.account.username, humhubUser)
 
     defaultUser.humhubAllowed = true
     const result = await syncUser(defaultUser, humhubUsers)

backend/src/apis/humhub/syncUser.ts

@@ -1,6 +1,7 @@
 import { User } from '@entity/User'
 
 import { LogError } from '@/server/LogError'
+import { backendLogger as logger } from '@/server/logger'
 
 import { isHumhubUserIdenticalToDbUser } from './compareHumhubUserDbUser'
 import { HumHubClient } from './HumHubClient'
@@ -12,7 +13,22 @@ export enum ExecutedHumhubAction {
   CREATE,
   SKIP,
   DELETE,
+  VALIDATION_ERROR,
 }
+
+// todo: replace with full validation (schema)
+function isValid(postUser: PostUser, userId: number): boolean {
+  if (postUser.profile.firstname.length > 20) {
+    logger.error('firstname too long for humhub, for user with id:', userId)
+    return false
+  }
+  if (postUser.profile.lastname.length > 20) {
+    logger.error('lastname too long for humhub, for user with id:', userId)
+    return false
+  }
+  return true
+}
+
 /**
  * Trigger action according to conditions
  * | User exist on humhub | export to humhub allowed | changes in user data | ACTION
@@ -21,9 +37,8 @@ export enum ExecutedHumhubAction {
  * |      true            |         true             |        false         | SKIP
  * |      false           |         false            |       ignored        | SKIP
  * |      false           |         true             |       ignored        | CREATE
- * @param user
- * @param humHubClient
- * @param humhubUsers
+ * @param user user entity
+ * @param humhubUsers user map indices with username
  * @returns
  */
 export async function syncUser(
@@ -31,7 +46,10 @@ export async function syncUser(
   humhubUsers: Map<string, GetUser>,
 ): Promise<ExecutedHumhubAction> {
   const postUser = new PostUser(user)
-  const humhubUser = humhubUsers.get(user.emailContact.email.trim())
+  if (!isValid(postUser, user.id)) {
+    return ExecutedHumhubAction.VALIDATION_ERROR
+  }
+  const humhubUser = humhubUsers.get(postUser.account.username)
   const humHubClient = HumHubClient.getInstance()
   if (!humHubClient) {
     throw new LogError('Error creating humhub client')

backend/src/data/PublishName.logic.ts

@@ -35,12 +35,16 @@ export class PublishNameLogic {
    * @returns user.firstName for PUBLISH_NAME_FIRST, PUBLISH_NAME_FIRST_INITIAL or PUBLISH_NAME_FULL
    */
   public getFirstName(publishNameType: PublishNameType): string {
+    let firstName = ''
+    if (this.user && typeof this.user.firstName === 'string') {
+      firstName = this.user.firstName
+    }
     return [
       PublishNameType.PUBLISH_NAME_FIRST,
       PublishNameType.PUBLISH_NAME_FIRST_INITIAL,
       PublishNameType.PUBLISH_NAME_FULL,
     ].includes(publishNameType)
-      ? this.user.firstName
+      ? firstName.slice(0, 20)
       : ''
   }
 
@@ -51,10 +55,14 @@ export class PublishNameLogic {
    *   first initial from user.lastName for PUBLISH_NAME_FIRST_INITIAL
    */
   public getLastName(publishNameType: PublishNameType): string {
+    let lastName = ''
+    if (this.user && typeof this.user.lastName === 'string') {
+      lastName = this.user.lastName
+    }
     return publishNameType === PublishNameType.PUBLISH_NAME_FULL
-      ? this.user.lastName
-      : publishNameType === PublishNameType.PUBLISH_NAME_FIRST_INITIAL
-      ? this.user.lastName.charAt(0)
+      ? lastName.slice(0, 20)
+      : publishNameType === PublishNameType.PUBLISH_NAME_FIRST_INITIAL && lastName.length > 0
+      ? lastName.charAt(0)
       : ''
   }
 

backend/src/graphql/resolver/UserResolver.ts

@@ -37,6 +37,7 @@ import { UpdateUserInfosArgs } from '@arg/UpdateUserInfosArgs'
 import { OptInType } from '@enum/OptInType'
 import { Order } from '@enum/Order'
 import { PasswordEncryptionType } from '@enum/PasswordEncryptionType'
+import { PublishNameType } from '@enum/PublishNameType'
 import { UserContactType } from '@enum/UserContactType'
 import { SearchAdminUsersResult } from '@model/AdminUser'
 // import { Location } from '@model/Location'
@@ -54,6 +55,7 @@ import { subscribe } from '@/apis/KlicktippController'
 import { encode } from '@/auth/JWT'
 import { RIGHTS } from '@/auth/RIGHTS'
 import { CONFIG } from '@/config'
+import { PublishNameLogic } from '@/data/PublishName.logic'
 import {
   sendAccountActivationEmail,
   sendAccountMultiRegistrationEmail,
@@ -246,12 +248,12 @@ export class UserResolver {
       try {
         const result = await humhubUserPromise
         user.humhubAllowed = result?.result?.account.status === 1
-        if (user.humhubAllowed) {
+        if (user.humhubAllowed && result?.result?.account?.username) {
           let spaceId = null
           if (projectBranding) {
             spaceId = projectBranding.spaceId
           }
-          void syncHumhub(null, dbUser, spaceId)
+          await syncHumhub(null, dbUser, result.result.account.username, spaceId)
         }
       } catch (e) {
         logger.error("couldn't reach out to humhub, disable for now", e)
@@ -448,7 +450,11 @@ export class UserResolver {
       if (projectBranding) {
         spaceId = projectBranding.spaceId
       }
-      void syncHumhub(null, dbUser, spaceId)
+      try {
+        await syncHumhub(null, dbUser, dbUser.gradidoID, spaceId)
+      } catch (e) {
+        logger.error("createUser: couldn't reach out to humhub, disable for now", e)
+      }
     }
 
     if (redeemCode) {
@@ -659,6 +665,10 @@ export class UserResolver {
     )
     const user = getUser(context)
     const updateUserInGMS = compareGmsRelevantUserSettings(user, updateUserInfosArgs)
+    const publishNameLogic = new PublishNameLogic(user)
+    const oldHumhubUsername = publishNameLogic.getUserIdentifier(
+      user.humhubPublishName as PublishNameType,
+    )
 
     // try {
     if (firstName) {
@@ -764,7 +774,7 @@ export class UserResolver {
     }
     try {
       if (CONFIG.HUMHUB_ACTIVE) {
-        await syncHumhub(updateUserInfosArgs, user)
+        await syncHumhub(updateUserInfosArgs, user, oldHumhubUsername)
       }
     } catch (e) {
       logger.error('error sync user with humhub', e)
@@ -839,7 +849,7 @@ export class UserResolver {
     }
     const humhubUserAccount = new HumhubAccount(dbUser)
     const autoLoginUrlPromise = humhubClient.createAutoLoginUrl(humhubUserAccount.username, project)
-    const humhubUser = await syncHumhub(null, dbUser)
+    const humhubUser = await syncHumhub(null, dbUser, humhubUserAccount.username)
     if (!humhubUser) {
       throw new LogError("user don't exist (any longer) on humhub and couldn't be created")
     }

backend/src/graphql/resolver/util/syncHumhub.test.ts

@@ -32,7 +32,7 @@ describe('syncHumhub', () => {
   })
 
   it('Should not sync if no relevant changes', async () => {
-    await syncHumhub(mockUpdateUserInfosArg, new User())
+    await syncHumhub(mockUpdateUserInfosArg, new User(), 'username')
     expect(HumHubClient.getInstance).not.toBeCalled()
     // language logging from some other place
     expect(logger.debug).toBeCalledTimes(5)
@@ -42,7 +42,7 @@ describe('syncHumhub', () => {
   it('Should retrieve user from humhub and sync if relevant changes', async () => {
     mockUpdateUserInfosArg.firstName = 'New' // Relevant changes
     mockUser.firstName = 'New'
-    await syncHumhub(mockUpdateUserInfosArg, mockUser)
+    await syncHumhub(mockUpdateUserInfosArg, mockUser, 'username')
     expect(logger.debug).toHaveBeenCalledTimes(8) // Four language logging calls, two debug calls in function, one for not syncing
     expect(logger.info).toHaveBeenLastCalledWith('finished sync user with humhub', {
       localId: mockUser.id,

backend/src/graphql/resolver/util/syncHumhub.ts

@@ -2,10 +2,9 @@ import { User } from '@entity/User'
 
 import { HumHubClient } from '@/apis/humhub/HumHubClient'
 import { GetUser } from '@/apis/humhub/model/GetUser'
+import { PostUser } from '@/apis/humhub/model/PostUser'
 import { ExecutedHumhubAction, syncUser } from '@/apis/humhub/syncUser'
-import { PublishNameLogic } from '@/data/PublishName.logic'
 import { UpdateUserInfosArgs } from '@/graphql/arg/UpdateUserInfosArgs'
-import { PublishNameType } from '@/graphql/enum/PublishNameType'
 import { backendLogger as logger } from '@/server/logger'
 
 /**
@@ -17,6 +16,7 @@ import { backendLogger as logger } from '@/server/logger'
 export async function syncHumhub(
   updateUserInfosArg: UpdateUserInfosArgs | null,
   user: User,
+  oldHumhubUsername: string,
   spaceId?: number | null,
 ): Promise<GetUser | null | undefined> {
   // check for humhub relevant changes
@@ -38,15 +38,13 @@ export async function syncHumhub(
     return
   }
   logger.debug('retrieve user from humhub')
-  const userNameLogic = new PublishNameLogic(user)
-  const username = userNameLogic.getUserIdentifier(user.humhubPublishName as PublishNameType)
-  let humhubUser = await humhubClient.userByUsername(username)
+  let humhubUser = await humhubClient.userByUsername(oldHumhubUsername)
   if (!humhubUser) {
     humhubUser = await humhubClient.userByEmail(user.emailContact.email)
   }
   const humhubUsers = new Map<string, GetUser>()
   if (humhubUser) {
-    humhubUsers.set(user.emailContact.email, humhubUser)
+    humhubUsers.set(humhubUser.account.username, humhubUser)
   }
   logger.debug('update user at humhub')
   const result = await syncUser(user, humhubUsers)
@@ -63,7 +61,8 @@ export async function syncHumhub(
     logger.debug(`user added to space ${spaceId}`)
   }
   if (result !== ExecutedHumhubAction.SKIP) {
-    return await humhubClient.userByUsername(username)
+    const getUser = new PostUser(user)
+    return await humhubClient.userByUsername(getUser.account.username)
   }
   return humhubUser
 }

backend/test/testSetup.ts

@@ -6,6 +6,8 @@ import { backendLogger as logger } from '@/server/logger'
 
 CONFIG.EMAIL = true
 CONFIG.EMAIL_TEST_MODUS = false
+CONFIG.HUMHUB_ACTIVE = false
+CONFIG.GMS_ACTIVE = false
 
 jest.setTimeout(1000000)