diff --git a/packages/server/modules/core/domain/streams/operations.ts b/packages/server/modules/core/domain/streams/operations.ts index cefcbc052..f2c7c6f7c 100644 --- a/packages/server/modules/core/domain/streams/operations.ts +++ b/packages/server/modules/core/domain/streams/operations.ts @@ -66,6 +66,8 @@ export type GetStreamCollaborators = ( type?: StreamRoles ) => Promise> +export type GetUserDeletableStreams = (userId: string) => Promise> + export type LegacyGetStreamCollaborators = (params: { streamId: string }) => Promise< { role: string @@ -95,7 +97,7 @@ export type CanUserFavoriteStream = (params: { streamId: string }) => Promise -export type DeleteStreamRecords = (streamId: string) => Promise +export type DeleteStreamRecord = (streamId: string) => Promise export type GetOnboardingBaseStream = (version: string) => Promise> diff --git a/packages/server/modules/core/domain/users/operations.ts b/packages/server/modules/core/domain/users/operations.ts index 70e0e3bcd..8d599194b 100644 --- a/packages/server/modules/core/domain/users/operations.ts +++ b/packages/server/modules/core/domain/users/operations.ts @@ -45,8 +45,12 @@ export type UpdateUser = ( }> ) => Promise> +export type DeleteUserRecord = (userId: string) => Promise + export type CountAdminUsers = () => Promise +export type IsLastAdminUser = (userId: string) => Promise + export type StoreUserAcl = (params: { acl: ServerAclRecord }) => Promise @@ -107,3 +111,5 @@ export type ValidateUserPassword = (params: { email: string password: string }) => Promise + +export type DeleteUser = (id: string) => Promise diff --git a/packages/server/modules/core/graph/resolvers/users.js b/packages/server/modules/core/graph/resolvers/users.js index 100ab8ae3..d8e72a714 100644 --- a/packages/server/modules/core/graph/resolvers/users.js +++ b/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, diff --git a/packages/server/modules/core/repositories/streams.ts b/packages/server/modules/core/repositories/streams.ts index 6536ad72b..eda624feb 100644 --- a/packages/server/modules/core/repositories/streams.ts +++ b/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 } } + +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) + } diff --git a/packages/server/modules/core/repositories/users.ts b/packages/server/modules/core/repositories/users.ts index 85727330e..14c2d90b7 100644 --- a/packages/server/modules/core/repositories/users.ts +++ b/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) => { diff --git a/packages/server/modules/core/services/streams/management.ts b/packages/server/modules/core/services/streams/management.ts index a40316ff1..ec006a403 100644 --- a/packages/server/modules/core/services/streams/management.ts +++ b/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 diff --git a/packages/server/modules/core/services/users.js b/packages/server/modules/core/services/users.js index b7ee086df..c902c8c9b 100644 --- a/packages/server/modules/core/services/users.js +++ b/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}`) diff --git a/packages/server/modules/core/services/users/management.ts b/packages/server/modules/core/services/users/management.ts index 31adbfa61..b7b5aa73d 100644 --- a/packages/server/modules/core/services/users/management.ts +++ b/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) + } diff --git a/packages/server/modules/core/tests/users.spec.js b/packages/server/modules/core/tests/users.spec.js index d6bf56adf..7493b533a 100644 --- a/packages/server/modules/core/tests/users.spec.js +++ b/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( diff --git a/packages/server/modules/core/tests/usersAdmin.spec.js b/packages/server/modules/core/tests/usersAdmin.spec.js index 3082b769a..6f0175a30 100644 --- a/packages/server/modules/core/tests/usersAdmin.spec.js +++ b/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) }) diff --git a/packages/server/modules/workspaces/services/management.ts b/packages/server/modules/workspaces/services/management.ts index e4273e9c4..335869668 100644 --- a/packages/server/modules/workspaces/services/management.ts +++ b/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 }) =>