Merge pull request #3288 from specklesystems/fabians/core-ioc-62

chore(server): core IoC #62 - deleteUserFactory

packages/server/modules/core/domain/streams/operations.ts

@@ -66,6 +66,8 @@ export type GetStreamCollaborators = (
   type?: StreamRoles
 ) => Promise<Array<LimitedUserWithStreamRole>>
 
+export type GetUserDeletableStreams = (userId: string) => Promise<Array<string>>
+
 export type LegacyGetStreamCollaborators = (params: { streamId: string }) => Promise<
   {
     role: string
@@ -95,7 +97,7 @@ export type CanUserFavoriteStream = (params: {
   streamId: string
 }) => Promise<boolean>
 
-export type DeleteStreamRecords = (streamId: string) => Promise<number>
+export type DeleteStreamRecord = (streamId: string) => Promise<number>
 
 export type GetOnboardingBaseStream = (version: string) => Promise<Optional<Stream>>
 

packages/server/modules/core/domain/users/operations.ts

@@ -45,8 +45,12 @@ export type UpdateUser = (
   }>
 ) => Promise<Nullable<User>>
 
+export type DeleteUserRecord = (userId: string) => Promise<boolean>
+
 export type CountAdminUsers = () => Promise<number>
 
+export type IsLastAdminUser = (userId: string) => Promise<boolean>
+
 export type StoreUserAcl = (params: {
   acl: ServerAclRecord
 }) => Promise<ServerAclRecord>
@@ -107,3 +111,5 @@ export type ValidateUserPassword = (params: {
   email: string
   password: string
 }) => Promise<boolean>
+
+export type DeleteUser = (id: string) => Promise<boolean>

packages/server/modules/core/graph/resolvers/users.js

@@ -1,6 +1,5 @@
 const {
   getUserRole,
-  deleteUser,
   searchUsers,
   changeUserRole
 } = require('@/modules/core/services/users')
@@ -17,7 +16,9 @@ const {
   legacyGetUserFactory,
   legacyGetUserByEmailFactory,
   getUserFactory,
-  updateUserFactory
+  updateUserFactory,
+  isLastAdminUserFactory,
+  deleteUserRecordFactory
 } = require('@/modules/core/repositories/users')
 const { UsersMeta } = require('@/modules/core/dbSchema')
 const { getServerInfo } = require('@/modules/core/services/generic')
@@ -31,11 +32,17 @@ const db = require('@/db/knex')
 const { BadRequestError } = require('@/modules/shared/errors')
 const { saveActivityFactory } = require('@/modules/activitystream/repositories')
 const {
-  updateUserAndNotifyFactory
+  updateUserAndNotifyFactory,
+  deleteUserFactory
 } = require('@/modules/core/services/users/management')
 const {
   addUserUpdatedActivityFactory
 } = require('@/modules/activitystream/services/userActivity')
+const {
+  deleteStreamFactory,
+  getUserDeletableStreamsFactory
+} = require('@/modules/core/repositories/streams')
+const { dbLogger } = require('@/logging/logging')
 
 const getUser = legacyGetUserFactory({ db })
 const getUserByEmail = legacyGetUserByEmailFactory({ db })
@@ -48,6 +55,15 @@ const updateUserAndNotify = updateUserAndNotifyFactory({
   })
 })
 
+const deleteUser = deleteUserFactory({
+  deleteStream: deleteStreamFactory({ db }),
+  logger: dbLogger,
+  isLastAdminUser: isLastAdminUserFactory({ db }),
+  getUserDeletableStreams: getUserDeletableStreamsFactory({ db }),
+  deleteAllUserInvites: deleteAllUserInvitesFactory({ db }),
+  deleteUserRecord: deleteUserRecordFactory({ db })
+})
+
 /** @type {import('@/modules/core/graph/generated/graphql').Resolvers} */
 module.exports = {
   Query: {
@@ -182,9 +198,7 @@ module.exports = {
       const user = await getUserByEmail({ email: args.userConfirmation.email })
       if (!user) return false
 
-      await deleteUser({
-        deleteAllUserInvites: deleteAllUserInvitesFactory({ db })
-      })(user.id)
+      await deleteUser(user.id)
       return true
     },
 
@@ -201,9 +215,7 @@ module.exports = {
       await throwForNotHavingServerRole(context, Roles.Server.Guest)
       await validateScopes(context.scopes, Scopes.Profile.Delete)
 
-      await deleteUser({
-        deleteAllUserInvites: deleteAllUserInvitesFactory({ db })
-      })(context.userId, args.user)
+      await deleteUser(context.userId, args.user)
 
       await saveActivityFactory({ db })({
         streamId: null,

packages/server/modules/core/repositories/streams.ts

@@ -70,7 +70,7 @@ import {
   GetStream,
   GetStreamCollaborators,
   GetStreams,
-  DeleteStreamRecords,
+  DeleteStreamRecord,
   UpdateStreamRecord,
   RevokeStreamPermissions,
   GrantStreamPermissions,
@@ -97,7 +97,8 @@ import {
   GetUserStreamsCount,
   MarkBranchStreamUpdated,
   MarkCommitStreamUpdated,
-  MarkOnboardingBaseStream
+  MarkOnboardingBaseStream,
+  GetUserDeletableStreams
 } from '@/modules/core/domain/streams/operations'
 export type { StreamWithOptionalRole, StreamWithCommitId }
 
@@ -880,7 +881,7 @@ export const getUserStreamCountsFactory =
   }
 
 export const deleteStreamFactory =
-  (deps: { db: Knex }): DeleteStreamRecords =>
+  (deps: { db: Knex }): DeleteStreamRecord =>
   async (streamId: string) => {
     // Delete stream commits (not automatically cascaded)
     await deps.db.raw(
@@ -1306,3 +1307,31 @@ export const legacyGetStreamsFactory =
       cursorDate: cursorDate as Nullable<Date>
     }
   }
+
+export const getUserDeletableStreamsFactory =
+  (deps: { db: Knex }): GetUserDeletableStreams =>
+  async (id) => {
+    const streams = (await deps.db.raw(
+      `
+      -- Get the stream ids with only this user as owner
+      SELECT "resourceId" as id
+      FROM (
+        -- Compute (streamId, ownerCount) table for streams on which the user is owner
+        SELECT acl."resourceId", count(*) as cnt
+        FROM stream_acl acl
+        INNER JOIN
+          (
+          -- Get streams ids on which the user is owner
+          SELECT "resourceId" FROM stream_acl
+          WHERE role = '${Roles.Stream.Owner}' AND "userId" = ?
+          ) AS us ON acl."resourceId" = us."resourceId"
+        WHERE acl.role = '${Roles.Stream.Owner}'
+        GROUP BY (acl."resourceId")
+      ) AS soc
+      WHERE cnt = 1
+      `,
+      [id]
+    )) as { rows: { id: string }[] }
+
+    return streams.rows.map((s) => s.id)
+  }

packages/server/modules/core/repositories/users.ts

@@ -12,10 +12,12 @@ import { markUserEmailAsVerifiedFactory } from '@/modules/core/services/users/em
 import { UserWithOptionalRole } from '@/modules/core/domain/users/types'
 import {
   CountAdminUsers,
+  DeleteUserRecord,
   GetUser,
   GetUserByEmail,
   GetUserParams,
   GetUsers,
+  IsLastAdminUser,
   LegacyGetPaginatedUsers,
   LegacyGetPaginatedUsersCount,
   LegacyGetUser,
@@ -382,6 +384,29 @@ export const countAdminUsersFactory =
     return parseInt(count as string)
   }
 
+export const isLastAdminUserFactory =
+  (deps: { db: Knex }): IsLastAdminUser =>
+  async (userId) => {
+    if ((await countAdminUsersFactory(deps)()) === 1) {
+      const currentAdmin = await tables
+        .serverAcl(deps.db)
+        .where({ role: Roles.Server.Admin })
+        .first()
+      if (currentAdmin && currentAdmin.userId === userId) {
+        return true
+      }
+    }
+
+    return false
+  }
+
+export const deleteUserRecordFactory =
+  (deps: { db: Knex }): DeleteUserRecord =>
+  async (id) => {
+    const res = await tables.users(deps.db).where({ id }).del()
+    return !!res
+  }
+
 export const storeUserAclFactory =
   (deps: { db: Knex }): StoreUserAcl =>
   async (params) => {

packages/server/modules/core/services/streams/management.ts

@@ -33,7 +33,7 @@ import {
   AddOrUpdateStreamCollaborator,
   CreateStream,
   DeleteStream,
-  DeleteStreamRecords,
+  DeleteStreamRecord,
   GetStream,
   IsStreamCollaborator,
   LegacyCreateStream,
@@ -138,7 +138,7 @@ export const legacyCreateStreamFactory =
  */
 export const deleteStreamAndNotifyFactory =
   (deps: {
-    deleteStream: DeleteStreamRecords
+    deleteStream: DeleteStreamRecord
     authorizeResolver: AuthorizeResolver
     addStreamDeletedActivity: AddStreamDeletedActivity
     deleteAllResourceInvites: DeleteAllResourceInvites

packages/server/modules/core/services/users.js

@@ -11,11 +11,8 @@ const Acl = () => ServerAclSchema.knex()
 
 const { LIMITED_USER_FIELDS } = require('@/modules/core/helpers/userHelper')
 const { omit } = require('lodash')
-const { dbLogger } = require('@/logging/logging')
 const { UserInputError } = require('@/modules/core/errors/userinput')
 const { Roles } = require('@speckle/shared')
-const { db } = require('@/db/knex')
-const { deleteStreamFactory } = require('@/modules/core/repositories/streams')
 
 const _changeUserRole = async ({ userId, role }) =>
   await Acl().where({ userId }).update({ role })
@@ -76,49 +73,6 @@ module.exports = {
     }
   },
 
-  /**
-   * TODO: this should be moved to repository
-   * @param {{ deleteAllUserInvites: import('@/modules/serverinvites/domain/operations').DeleteAllUserInvites }} param0
-   */
-  deleteUser({ deleteAllUserInvites }) {
-    const deleteStream = deleteStreamFactory({ db })
-    return async (id) => {
-      //TODO: check for the last admin user to survive
-      dbLogger.info('Deleting user ' + id)
-      await _ensureAtleastOneAdminRemains(id)
-      const streams = await knex.raw(
-        `
-      -- Get the stream ids with only this user as owner
-      SELECT "resourceId" as id
-      FROM (
-        -- Compute (streamId, ownerCount) table for streams on which the user is owner
-        SELECT acl."resourceId", count(*) as cnt
-        FROM stream_acl acl
-        INNER JOIN
-          (
-          -- Get streams ids on which the user is owner
-          SELECT "resourceId" FROM stream_acl
-          WHERE role = '${Roles.Stream.Owner}' AND "userId" = ?
-          ) AS us ON acl."resourceId" = us."resourceId"
-        WHERE acl.role = '${Roles.Stream.Owner}'
-        GROUP BY (acl."resourceId")
-      ) AS soc
-      WHERE cnt = 1
-      `,
-        [id]
-      )
-      for (const i in streams.rows) {
-        await deleteStream(streams.rows[i].id)
-      }
-
-      // Delete all invites (they don't have a FK, so we need to do this manually)
-      // THIS REALLY SHOULD BE A REACTION TO THE USER DELETED EVENT EMITTED HER
-      await deleteAllUserInvites(id)
-
-      return await Users().where({ id }).del()
-    }
-  },
-
   async changeUserRole({ userId, role, guestModeEnabled = false }) {
     if (!Object.values(Roles.Server).includes(role))
       throw new UserInputError(`Invalid role specified: ${role}`)

packages/server/modules/core/services/users/management.ts

@@ -3,9 +3,12 @@ import {
   ChangeUserPassword,
   CountAdminUsers,
   CreateValidatedUser,
+  DeleteUser,
+  DeleteUserRecord,
   FindOrCreateValidatedUser,
   GetUser,
   GetUserByEmail,
+  IsLastAdminUser,
   StoreUser,
   StoreUserAcl,
   UpdateUser,
@@ -28,6 +31,12 @@ import {
   ValidateAndCreateUserEmail
 } from '@/modules/core/domain/userEmails/operations'
 import { UsersEvents, UsersEventsEmitter } from '@/modules/core/events/usersEmitter'
+import {
+  DeleteStreamRecord,
+  GetUserDeletableStreams
+} from '@/modules/core/domain/streams/operations'
+import { Logger } from '@/logging/logging'
+import { DeleteAllUserInvites } from '@/modules/serverinvites/domain/operations'
 
 export const MINIMUM_PASSWORD_LENGTH = 8
 
@@ -232,3 +241,31 @@ export const findOrCreateUserFactory =
       isNewUser: true
     }
   }
+
+export const deleteUserFactory =
+  (deps: {
+    deleteStream: DeleteStreamRecord
+    logger: Logger
+    isLastAdminUser: IsLastAdminUser
+    getUserDeletableStreams: GetUserDeletableStreams
+    deleteAllUserInvites: DeleteAllUserInvites
+    deleteUserRecord: DeleteUserRecord
+  }): DeleteUser =>
+  async (id) => {
+    deps.logger.info('Deleting user ' + id)
+    const isLastAdmin = await deps.isLastAdminUser(id)
+    if (isLastAdmin) {
+      throw new UserInputError('Cannot remove the last admin role from the server')
+    }
+
+    const streamIds = await deps.getUserDeletableStreams(id)
+    for (const id of streamIds) {
+      await deps.deleteStream(id)
+    }
+
+    // Delete all invites (they don't have a FK, so we need to do this manually)
+    // THIS REALLY SHOULD BE A REACTION TO THE USER DELETED EVENT EMITTED HER
+    await deps.deleteAllUserInvites(id)
+
+    return await deps.deleteUserRecord(id)
+  }

packages/server/modules/core/tests/users.spec.js

@@ -2,7 +2,7 @@
 const expect = require('chai').expect
 const assert = require('assert')
 
-const { changeUserRole, searchUsers, deleteUser } = require('../services/users')
+const { changeUserRole, searchUsers } = require('../services/users')
 const {
   createPersonalAccessToken,
   revokeToken,
@@ -39,7 +39,9 @@ const {
   getStreamFactory,
   createStreamFactory,
   grantStreamPermissionsFactory,
-  markCommitStreamUpdatedFactory
+  markCommitStreamUpdatedFactory,
+  deleteStreamFactory,
+  getUserDeletableStreamsFactory
 } = require('@/modules/core/repositories/streams')
 const { VersionsEmitter } = require('@/modules/core/events/versionsEmitter')
 const { getObjectFactory } = require('@/modules/core/repositories/objects')
@@ -57,7 +59,8 @@ const {
   findUserByTargetFactory,
   insertInviteAndDeleteOldFactory,
   deleteServerOnlyInvitesFactory,
-  updateAllInviteTargetsFactory
+  updateAllInviteTargetsFactory,
+  deleteAllUserInvitesFactory
 } = require('@/modules/serverinvites/repositories/serverInvites')
 const {
   collectAndValidateCoreTargetsFactory
@@ -84,7 +87,9 @@ const {
   storeUserAclFactory,
   legacyGetUserByEmailFactory,
   updateUserFactory,
-  getUserByEmailFactory
+  getUserByEmailFactory,
+  isLastAdminUserFactory,
+  deleteUserRecordFactory
 } = require('@/modules/core/repositories/users')
 const {
   findEmailFactory,
@@ -106,7 +111,8 @@ const {
   findOrCreateUserFactory,
   updateUserAndNotifyFactory,
   changePasswordFactory,
-  validateUserPasswordFactory
+  validateUserPasswordFactory,
+  deleteUserFactory
 } = require('@/modules/core/services/users/management')
 const {
   validateAndCreateUserEmailFactory
@@ -118,6 +124,7 @@ const { UsersEmitter } = require('@/modules/core/events/usersEmitter')
 const {
   addUserUpdatedActivityFactory
 } = require('@/modules/activitystream/services/userActivity')
+const { dbLogger } = require('@/logging/logging')
 
 const getUser = legacyGetUserFactory({ db })
 const getUsers = getUsersFactory({ db })
@@ -228,6 +235,15 @@ const validateUserPassword = validateUserPasswordFactory({
   getUserByEmail: getUserByEmailFactory({ db })
 })
 
+const deleteUser = deleteUserFactory({
+  deleteStream: deleteStreamFactory({ db }),
+  logger: dbLogger,
+  isLastAdminUser: isLastAdminUserFactory({ db }),
+  getUserDeletableStreams: getUserDeletableStreamsFactory({ db }),
+  deleteAllUserInvites: deleteAllUserInvitesFactory({ db }),
+  deleteUserRecord: deleteUserRecordFactory({ db })
+})
+
 describe('Actors & Tokens @user-services', () => {
   const myTestActor = {
     name: 'Dimitrie Stefanescu',
@@ -347,7 +363,7 @@ describe('Actors & Tokens @user-services', () => {
         })
       ).id
 
-      await deleteUser({ deleteAllUserInvites: async () => true })(ballmerUserId)
+      await deleteUser(ballmerUserId)
 
       if ((await getStream({ streamId: soloOwnerStream.id })) !== undefined) {
         assert.fail('user stream not deleted')
@@ -381,7 +397,7 @@ describe('Actors & Tokens @user-services', () => {
 
     it('Should not delete the last admin user', async () => {
       try {
-        await deleteUser({ deleteAllUserInvites: async () => true })(myTestActor.id)
+        await deleteUser(myTestActor.id)
         assert.fail('boom')
       } catch (err) {
         expect(err.message).to.equal(

packages/server/modules/core/tests/usersAdmin.spec.js

@@ -1,11 +1,7 @@
 const expect = require('chai').expect
 const assert = require('assert')
 
-const {
-  deleteUser,
-  changeUserRole,
-  getUserRole
-} = require('@/modules/core/services/users')
+const { changeUserRole, getUserRole } = require('@/modules/core/services/users')
 const { beforeEachContext } = require('@/test/hooks')
 const { Roles } = require('@speckle/shared')
 const cryptoRandomString = require('crypto-random-string')
@@ -15,7 +11,9 @@ const {
   getUserFactory,
   storeUserFactory,
   countAdminUsersFactory,
-  storeUserAclFactory
+  storeUserAclFactory,
+  isLastAdminUserFactory,
+  deleteUserRecordFactory
 } = require('@/modules/core/repositories/users')
 const { db } = require('@/db/knex')
 const {
@@ -32,7 +30,10 @@ const {
 } = require('@/modules/emails/repositories')
 const { renderEmail } = require('@/modules/emails/services/emailRendering')
 const { sendEmail } = require('@/modules/emails/services/sending')
-const { createUserFactory } = require('@/modules/core/services/users/management')
+const {
+  createUserFactory,
+  deleteUserFactory
+} = require('@/modules/core/services/users/management')
 const {
   validateAndCreateUserEmailFactory
 } = require('@/modules/core/services/userEmails')
@@ -41,9 +42,15 @@ const {
 } = require('@/modules/serverinvites/services/processing')
 const {
   deleteServerOnlyInvitesFactory,
-  updateAllInviteTargetsFactory
+  updateAllInviteTargetsFactory,
+  deleteAllUserInvitesFactory
 } = require('@/modules/serverinvites/repositories/serverInvites')
 const { UsersEmitter } = require('@/modules/core/events/usersEmitter')
+const {
+  deleteStreamFactory,
+  getUserDeletableStreamsFactory
+} = require('@/modules/core/repositories/streams')
+const { dbLogger } = require('@/logging/logging')
 
 const getUsers = legacyGetPaginatedUsersFactory({ db })
 const countUsers = legacyGetPaginatedUsersCountFactory({ db })
@@ -75,6 +82,14 @@ const createUser = createUserFactory({
   }),
   usersEventsEmitter: UsersEmitter.emit
 })
+const deleteUser = deleteUserFactory({
+  deleteStream: deleteStreamFactory({ db }),
+  logger: dbLogger,
+  isLastAdminUser: isLastAdminUserFactory({ db }),
+  getUserDeletableStreams: getUserDeletableStreamsFactory({ db }),
+  deleteAllUserInvites: deleteAllUserInvitesFactory({ db }),
+  deleteUserRecord: deleteUserRecordFactory({ db })
+})
 
 describe('User admin @user-services', () => {
   const myTestActor = {
@@ -111,7 +126,7 @@ describe('User admin @user-services', () => {
 
     expect(await countUsers()).to.equal(2)
 
-    await deleteUser({ deleteAllUserInvites: async () => true })(actorId)
+    await deleteUser(actorId)
     expect(await countUsers()).to.equal(1)
   })
 

packages/server/modules/workspaces/services/management.ts

@@ -63,7 +63,7 @@ import { chunk, isEmpty, omit } from 'lodash'
 import { userEmailsCompliantWithWorkspaceDomains } from '@/modules/workspaces/domain/logic'
 import { workspaceRoles as workspaceRoleDefinitions } from '@/modules/workspaces/roles'
 import { blockedDomains } from '@speckle/shared'
-import { DeleteStreamRecords } from '@/modules/core/domain/streams/operations'
+import { DeleteStreamRecord } from '@/modules/core/domain/streams/operations'
 
 type WorkspaceCreateArgs = {
   userId: string
@@ -278,7 +278,7 @@ export const deleteWorkspaceFactory =
     deleteAllResourceInvites
   }: {
     deleteWorkspace: DeleteWorkspace
-    deleteProject: DeleteStreamRecords
+    deleteProject: DeleteStreamRecord
     queryAllWorkspaceProjects: QueryAllWorkspaceProjects
     deleteAllResourceInvites: DeleteAllResourceInvites
   }) =>