From 314b03e90e053f1ee5429af27e7127d59208ea32 Mon Sep 17 00:00:00 2001 From: fflorent Date: Fri, 2 Aug 2024 17:49:19 +0200 Subject: [PATCH 01/22] Start of UsersManager tests --- test/gen-server/lib/homedb/UsersManager.ts | 370 +++++++++++++++++++++ 1 file changed, 370 insertions(+) create mode 100644 test/gen-server/lib/homedb/UsersManager.ts diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts new file mode 100644 index 0000000000..078f753bc6 --- /dev/null +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -0,0 +1,370 @@ +import { User } from 'app/gen-server/entity/User'; +import { AclRuleOrg } from 'app/gen-server/entity/AclRule'; +import { Group } from 'app/gen-server/entity/Group'; +import { Organization } from 'app/gen-server/entity/Organization'; +import { SUPPORT_EMAIL, UsersManager } from 'app/gen-server/lib/homedb/UsersManager'; +import { NonGuestGroup, Resource } from 'app/gen-server/lib/homedb/Interfaces'; +import { tmpdir } from 'os'; +import path from 'path'; +import { prepareDatabase } from 'test/server/lib/helpers/PrepareDatabase'; +import { prepareFilesystemDirectoryForTests } from 'test/server/lib/helpers/PrepareFilesystemDirectoryForTests'; +import { EnvironmentSnapshot } from 'test/server/testUtils'; +import { getDatabase } from 'test/testUtils'; +import { Workspace } from 'app/gen-server/entity/Workspace'; +import { Document } from 'app/gen-server/entity/Document'; +import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; +import Sinon, { SinonSandbox } from 'sinon'; +import { assert } from 'chai'; +import { addSeedData } from 'test/gen-server/seed'; +import { FullUser } from 'app/common/LoginSessionAPI'; +import { ANONYMOUS_USER_EMAIL } from 'app/common/UserAPI'; +import { Login } from 'app/gen-server/entity/Login'; +import { Pref } from 'app/gen-server/entity/Pref'; + +const username = process.env.USER || "nobody"; +const tmpDir = path.join(tmpdir(), `grist_test_${username}_userendpoint`); + +describe('UsersManager', function () { + this.timeout(30000); + let env: EnvironmentSnapshot; + let db: HomeDBManager; + + before(async function () { + env = new EnvironmentSnapshot(); + await prepareFilesystemDirectoryForTests(tmpDir); + await prepareDatabase(tmpDir); + db = await getDatabase(); + await addSeedData(db.connection); + }); + + after(async function () { + env.restore(); + }); + + function* makeIdxIterator() { + for (let i = 0; i < 500; i++) { + yield i; + } + } + + function makeUsers(nbUsers: number, idxIt = makeIdxIterator()) { + return Array(nbUsers).fill(null).map(() => { + const user = new User(); + const index = idxIt.next(); + if (index.done) { + throw new Error('Excessive number of users created'); + } + user.id = index.value; + user.name = `User ${index.value}`; + return user; + }); + } + + function makeUsersList(usersListLength: number) { + const nbUsersByGroups = 5; // arbitrary value + const idxIt = makeIdxIterator(); + return Array(usersListLength).fill(null).map(_ => makeUsers(nbUsersByGroups, idxIt)); + } + + describe('getResourceUsers', function () { + function populateResourceWithMembers(res: Resource, members: User[], groupName?: string) { + const aclRule = new AclRuleOrg(); + const group = new Group(); + if (groupName) { + group.name = groupName; + } + group.memberUsers = members; + aclRule.group = group; + res.aclRules = [ + aclRule + ]; + } + + it('should return all users from a single organization ACL', function () { + const resource = new Organization(); + const users = makeUsers(5); + populateResourceWithMembers(resource, users); + const result = UsersManager.getResourceUsers(resource); + assert.deepEqual(result, users); + }); + + it('should return all users from all resources ACL', function () { + const resources: Resource[] = [new Organization(), new Workspace(), new Document()]; + const usersList = makeUsersList(3); + for (const [idx, resource] of resources.entries()) { + populateResourceWithMembers(resource, usersList[idx]); + } + const result = UsersManager.getResourceUsers(resources); + assert.deepEqual(result, usersList.flat()); + }); + + it('should return users matching group names from all resources ACL', function () { + const resources: Resource[] = [new Organization(), new Workspace(), new Document()]; + const usersList = makeUsersList(3); + const allGroupNames = ['Grp1', 'Grp2', 'Grp3']; + const filteredGroupNames = allGroupNames.slice(1); + for (const [idx, resource] of resources.entries()) { + populateResourceWithMembers(resource, usersList[idx], allGroupNames[idx]); + } + const result = UsersManager.getResourceUsers(resources, filteredGroupNames); + assert.deepEqual(result, usersList.slice(1).flat(), 'should discard the users from the first resource'); + }); + }); + + describe('getUsersWithRole', function () { + function makeGroups(groupDefinition: {[k in NonGuestGroup['name']]?: User[] | undefined}){ + return (Object.entries(groupDefinition) as [NonGuestGroup['name'], User[] | undefined][]) + .map(([groupName, users], index) => { + const group = new Group() as NonGuestGroup; + group.id = index; + group.name = groupName; + if (users) { + group.memberUsers = users; + } + return group; + }); + } + + it('should retrieve no users if passed groups do not contain any', function () { + const groups = makeGroups({ + 'members': undefined + }); + assert.deepEqual(UsersManager.getUsersWithRole(groups), new Map([['members', undefined]] as any)); + }); + + it('should retrieve users of passed groups', function () { + const idxIt = makeIdxIterator(); + const groupsUsersMap = { + 'editors': makeUsers(3, idxIt), + 'owners': makeUsers(4, idxIt), + 'members': makeUsers(5, idxIt), + 'viewers': [] + }; + const groups = makeGroups(groupsUsersMap); + assert.deepEqual(UsersManager.getUsersWithRole(groups), new Map(Object.entries(groupsUsersMap))); + }); + + it('should exclude users of given IDs', function () { + const groupUsersMap = { + 'editors': makeUsers(5), + }; + const excludedUsersId = [1, 2, 3, 4]; + const expectedUsers = [groupUsersMap.editors[0]]; + const groups = makeGroups(groupUsersMap); + + assert.deepEqual( + UsersManager.getUsersWithRole(groups, excludedUsersId), + new Map([ + ['editors', expectedUsers] + ]) + ); + }); + }); + + describe('Special User Ids', function () { + const ANONYMOUS_USER_ID = 6; + const PREVIEWER_USER_ID = 7; + const EVERYONE_USER_ID = 8; + const SUPPORT_USER_ID = 5; + it('getAnonymousUserId() should retrieve anonymous user id', function () { + assert.strictEqual(db.getAnonymousUserId(), ANONYMOUS_USER_ID); + }); + + it('getPreviewerUserId() should retrieve previewer user id', function () { + assert.strictEqual(db.getPreviewerUserId(), PREVIEWER_USER_ID); + }); + + it("getEveryoneUserId() should retrieve 'everyone' user id", function () { + assert.strictEqual(db.getEveryoneUserId(), EVERYONE_USER_ID); + }); + + it("getSupportUserId() should retrieve 'support' user id", function () { + assert.strictEqual(db.getSupportUserId(), SUPPORT_USER_ID); + }); + + describe('Without special id initialization', function () { + let sandbox: SinonSandbox; + before(function () { + sandbox = Sinon.createSandbox(); + }); + after(function () { + sandbox.restore(); + }); + + async function createDatabaseWithoutSpecialIds() { + const stub = sandbox.stub(HomeDBManager.prototype, 'initializeSpecialIds').resolves(undefined); + const localDb = await getDatabase('without-special-ids'); + stub.restore(); + return localDb; + } + + it('should throw an error', async function () { + const localDb = await createDatabaseWithoutSpecialIds(); + assert.throws(() => localDb.getAnonymousUserId(), "Anonymous user not available"); + assert.throws(() => localDb.getPreviewerUserId(), "Previewer user not available"); + assert.throws(() => localDb.getEveryoneUserId(), "'everyone' user not available"); + assert.throws(() => localDb.getSupportUserId(), "'support' user not available"); + }); + }); + }); + + describe('getUserByKey', function () { + it('should return the user given their API Key', async function () { + const user = await db.getUserByKey('api_key_for_chimpy'); + assert.strictEqual(user?.name, 'Chimpy', 'should retrieve Chimpy by their API key'); + assert.strictEqual(user?.logins?.[0].email, 'chimpy@getgrist.com'); + }); + + it('should return undefined if no user matches the API key', async function () { + const user = await db.getUserByKey('non-existing API key'); + assert.strictEqual(user, undefined); + }); + }); + + describe('getUser', async function () { + + const userWithPrefsEmail = 'userwithprefs@getgrist.com'; + async function getOrCreateUserWithPrefs() { + const user = await db.getUserByLogin(userWithPrefsEmail); + if (!user) { + throw new Error('getOrCreateUserWithPrefs: User not created!'); + } + return user; + } + + it('should retrieve a user by their ID', async function () { + const user = await db.getUser(db.getSupportUserId()); + assert.ok(user, 'Should have returned a user'); + assert.strictEqual(user!.name, 'Support'); + assert.strictEqual(user!.loginEmail, SUPPORT_EMAIL); + assert.ok(!user!.prefs, "should not have retrieved user's prefs"); + }); + + it('should retrieve a user along with their prefs with `includePrefs` set to true', async function () { + const expectedUser = await getOrCreateUserWithPrefs(); + const user = await db.getUser(expectedUser.id, {includePrefs: true}); + assert.ok(user, "Should have retrieved the user"); + assert.ok(user!.loginEmail); + assert.strictEqual(user!.loginEmail, expectedUser.loginEmail); + assert.ok(Array.isArray(user!.prefs), "should not have retrieved user's prefs"); + assert.deepEqual(user!.prefs, [{ + userId: expectedUser.id, + orgId: expectedUser.personalOrg.id, + prefs: { showGristTour: true } as any + }]); + }); + }); + + describe('getFullUser()', function () { + it('should return the support user', async function () { + const supportId = db.getSupportUserId(); + const user = await db.getFullUser(supportId); + const expectedResult: FullUser = { + isSupport: true, + email: SUPPORT_EMAIL, + id: supportId, + name: 'Support' + }; + assert.deepInclude(user, expectedResult); + assert.ok(!user.anonymous, 'anonymous property should be falsy'); + }); + + it('should return the anonymous user', async function () { + const anonId = db.getAnonymousUserId(); + const user = await db.getFullUser(anonId); + const expectedResult: FullUser = { + anonymous: true, + email: ANONYMOUS_USER_EMAIL, + id: anonId, + name: 'Anonymous', + }; + assert.deepInclude(user, expectedResult); + assert.ok(!user.isSupport, 'support property should be falsy'); + }); + + it('should throw when user is not found', async function () { + await assert.isRejected(db.getFullUser(1337), "unable to find user"); + }); + }); + + describe('makeFullUser()', function () { + const someUserDisplayEmail = 'SomeUser@getgrist.com'; + const normalizedSomeUserEmail = 'someuser@getgrist.com'; + const someUserLocale = 'en-US'; + const SOME_USER_ID = 42; + const prefWithOrg: Pref = { + prefs: {placeholder: 'pref-with-org'}, + orgId: 43, + user: new User(), + userId: SOME_USER_ID, + }; + const prefWithoutOrg: Pref = { + prefs: {placeholder: 'pref-without-org'}, + orgId: null, + user: new User(), + userId: SOME_USER_ID + } + + + function makeSomeUser() { + return User.create({ + id: SOME_USER_ID, + ref: 'some ref', + name: 'some user', + picture: 'https://grist.com/mypic', + options: { + locale: someUserLocale + }, + logins: [ + Login.create({ + userId: SOME_USER_ID, + email: normalizedSomeUserEmail, + displayEmail: someUserDisplayEmail, + }), + ], + prefs: [ + prefWithOrg, + prefWithoutOrg + ] + }); + } + + it('creates a FullUser from a User entity', function () { + const input = makeSomeUser(); + const fullUser = db.makeFullUser(input); + assert.deepEqual(fullUser, { + id: SOME_USER_ID, + email: someUserDisplayEmail, + loginEmail: normalizedSomeUserEmail, + name: input.name, + picture: input.picture, + ref: input.ref, + locale: someUserLocale, + prefs: prefWithoutOrg.prefs + }); + }); + + it('sets `anonymous` property to true for anon@getgrist.com', function () { + const anon = db.getAnonymousUser(); + const fullUser = db.makeFullUser(anon); + assert.isTrue(fullUser.anonymous, "`anonymous` property should be set to true"); + assert.ok(!fullUser.isSupport, "`isSupport` should be falsy"); + }); + + it('sets `isSupport` property to true for support account', async function () { + const support = await db.getUser(db.getSupportUserId()); + const fullUser = db.makeFullUser(support!); + assert.isTrue(fullUser.isSupport, "`isSupport` property should be set to true"); + assert.ok(!fullUser.anonymous, "`anonymouse` should be falsy"); + }); + + it('should throw when no displayEmail exist for this user', function () { + const input = makeSomeUser(); + input.logins[0].displayEmail = ''; + assert.throws(() => db.makeFullUser(input), "unable to find mandatory user email"); + input.logins = []; + assert.throws(() => db.makeFullUser(input), "unable to find mandatory user email"); + }); + }); + +}); From 2f0594308496e2d3d8f218af196dcfee769d6c83 Mon Sep 17 00:00:00 2001 From: fflorent Date: Fri, 2 Aug 2024 19:15:33 +0200 Subject: [PATCH 02/22] Test ensureExternalUser() --- test/gen-server/lib/homedb/UsersManager.ts | 118 ++++++++++++++++++--- 1 file changed, 105 insertions(+), 13 deletions(-) diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 078f753bc6..1d651b35e6 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -13,13 +13,14 @@ import { getDatabase } from 'test/testUtils'; import { Workspace } from 'app/gen-server/entity/Workspace'; import { Document } from 'app/gen-server/entity/Document'; import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; -import Sinon, { SinonSandbox } from 'sinon'; +import Sinon, { SinonSandbox, SinonSpy } from 'sinon'; import { assert } from 'chai'; import { addSeedData } from 'test/gen-server/seed'; -import { FullUser } from 'app/common/LoginSessionAPI'; +import { FullUser, UserProfile } from 'app/common/LoginSessionAPI'; import { ANONYMOUS_USER_EMAIL } from 'app/common/UserAPI'; import { Login } from 'app/gen-server/entity/Login'; import { Pref } from 'app/gen-server/entity/Pref'; +import { EntityManager } from 'typeorm'; const username = process.env.USER || "nobody"; const tmpDir = path.join(tmpdir(), `grist_test_${username}_userendpoint`); @@ -28,19 +29,32 @@ describe('UsersManager', function () { this.timeout(30000); let env: EnvironmentSnapshot; let db: HomeDBManager; + let sandbox: SinonSandbox; + + function getTmpDatabase(typeormDb: string) { + return getDatabase(path.join(tmpDir, typeormDb)); + } before(async function () { env = new EnvironmentSnapshot(); await prepareFilesystemDirectoryForTests(tmpDir); await prepareDatabase(tmpDir); db = await getDatabase(); - await addSeedData(db.connection); }); after(async function () { env.restore(); }); + beforeEach(function () { + sandbox = Sinon.createSandbox(); + }); + + afterEach(function () { + sandbox.restore(); + }); + + function* makeIdxIterator() { for (let i = 0; i < 500; i++) { yield i; @@ -183,17 +197,9 @@ describe('UsersManager', function () { }); describe('Without special id initialization', function () { - let sandbox: SinonSandbox; - before(function () { - sandbox = Sinon.createSandbox(); - }); - after(function () { - sandbox.restore(); - }); - async function createDatabaseWithoutSpecialIds() { const stub = sandbox.stub(HomeDBManager.prototype, 'initializeSpecialIds').resolves(undefined); - const localDb = await getDatabase('without-special-ids'); + const localDb = await getTmpDatabase('without-special-ids.db'); stub.restore(); return localDb; } @@ -303,7 +309,7 @@ describe('UsersManager', function () { orgId: null, user: new User(), userId: SOME_USER_ID - } + }; function makeSomeUser() { @@ -367,4 +373,90 @@ describe('UsersManager', function () { }); }); + describe('ensureExternalUser()', function () { + let managerSaveSpy: SinonSpy; + let userSaveSpy: SinonSpy; + let localDb: HomeDBManager; + + before(async function () { + localDb = await getTmpDatabase('ensureExternalUser.db'); + await addSeedData(localDb.connection); + }); + beforeEach(function () { + managerSaveSpy = sandbox.spy(EntityManager.prototype, 'save'); + userSaveSpy = sandbox.spy(User.prototype, 'save'); + + }); + + function makeProfile(uuid: string): UserProfile { + return { + email: `${uuid}@getgrist.com`, + name: `NewUser ${uuid}`, + connectId: `ConnectId-${uuid}`, + picture: `https://mypic.com/${uuid}.png` + }; + } + + async function checkUserInfo(profile: UserProfile) { + const user = await localDb.getExistingUserByLogin(profile.email); + assert.exists(user, "the new user should be in database"); + assert.deepInclude(user, { + isFirstTimeUser: false, + name: profile.name, + picture: profile.picture + }); + assert.exists(user!.logins?.[0]); + assert.deepInclude(user!.logins[0], { + email: profile.email.toLowerCase(), + displayEmail: profile.email + }); + } + + it('should not do anything if the user already exists and is up to date', async function () { + await localDb.ensureExternalUser({ + name: 'Chimpy', + email: 'chimpy@getgrist.com', + }); + assert.isFalse(userSaveSpy.called, 'user.save() should not have been called'); + assert.isFalse(managerSaveSpy.called, 'manager.save() should not have been called'); + }); + + it('should save an unknown user', async function () { + const profile = makeProfile('f6fce1ec-892e-493a-9bd7-2c4b0480e7f5'); + await localDb.ensureExternalUser(profile); + assert.isTrue(userSaveSpy.called, 'user.save() should have been called'); + assert.isTrue(managerSaveSpy.called, 'manager.save() should have been called'); + + await checkUserInfo(profile); + }); + + it('should update a user if they already exist in database', async function () { + const oldProfile = makeProfile('b48b3c95-a808-43e1-9b78-9171fb6c58fd'); + await localDb.ensureExternalUser(oldProfile); + + let oldUser = await localDb.getExistingUserByLogin(oldProfile.email); + assert.exists(oldUser); + + const newProfile = { + ...makeProfile('b6755ae0-0cb5-4a40-a54c-764d4b187c4c'), + connectId: oldProfile.connectId, + }; + + await localDb.ensureExternalUser(newProfile); + oldUser = await localDb.getExistingUserByLogin(oldProfile.email); + assert.notExists(oldUser, 'we should not retrieve the user given their old email address'); + + await checkUserInfo(newProfile); + }); + + it('should normalize email address', async function() { + const profile = makeProfile('92AF6F0F-03C0-414B-9012-2C282D89512A'); + await localDb.ensureExternalUser(profile); + await checkUserInfo(profile); + }); + + it('FIXME', function () { + throw new Error('TODO: only use `db` and no localDb variable anymore'); + }); + }); }); From 48f5d33f73e1456cc89e8155fe3e986a6851b973 Mon Sep 17 00:00:00 2001 From: fflorent Date: Sat, 3 Aug 2024 16:26:21 +0200 Subject: [PATCH 03/22] new progress on UsersManager unit tests --- app/gen-server/ApiServer.ts | 2 +- app/gen-server/lib/homedb/HomeDBManager.ts | 8 +- app/gen-server/lib/homedb/Interfaces.ts | 6 +- app/gen-server/lib/homedb/UsersManager.ts | 20 +- test/gen-server/lib/homedb/UsersManager.ts | 305 ++++++++++++++++++--- 5 files changed, 285 insertions(+), 56 deletions(-) diff --git a/app/gen-server/ApiServer.ts b/app/gen-server/ApiServer.ts index 7d283f2f4e..1db45714d6 100644 --- a/app/gen-server/ApiServer.ts +++ b/app/gen-server/ApiServer.ts @@ -429,7 +429,7 @@ export class ApiServer { throw new ApiError('Name expected in the body', 400); } const name = req.body.name; - await this._dbManager.updateUserName(userId, name); + await this._dbManager.updateUser(userId, { name }); res.sendStatus(200); })); diff --git a/app/gen-server/lib/homedb/HomeDBManager.ts b/app/gen-server/lib/homedb/HomeDBManager.ts index a4f897211f..b61c5375ab 100644 --- a/app/gen-server/lib/homedb/HomeDBManager.ts +++ b/app/gen-server/lib/homedb/HomeDBManager.ts @@ -461,10 +461,6 @@ export class HomeDBManager extends EventEmitter { } } - public async updateUserName(userId: number, name: string) { - return this._usersManager.updateUserName(userId, name); - } - public async updateUserOptions(userId: number, props: Partial) { return this._usersManager.updateUserOptions(userId, props); } @@ -473,14 +469,14 @@ export class HomeDBManager extends EventEmitter { * @see UsersManager.prototype.getUserByLoginWithRetry */ public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise { - return this._usersManager.getUserByLoginWithRetry(email, options); + return await this._usersManager.getUserByLoginWithRetry(email, options) ?? undefined; } /** * @see UsersManager.prototype.getUserByLogin */ public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise { - return this._usersManager.getUserByLogin(email, options); + return await this._usersManager.getUserByLogin(email, options) ?? undefined; } /** diff --git a/app/gen-server/lib/homedb/Interfaces.ts b/app/gen-server/lib/homedb/Interfaces.ts index 0802d96ae0..22eec24969 100644 --- a/app/gen-server/lib/homedb/Interfaces.ts +++ b/app/gen-server/lib/homedb/Interfaces.ts @@ -34,7 +34,7 @@ export type NonGuestGroup = Group & { name: roles.NonGuestRole }; export type Resource = Organization|Workspace|Document; -export type RunInTransaction = ( +export type RunInTransaction = ( transaction: EntityManager|undefined, - op: ((manager: EntityManager) => Promise) -) => Promise; + op: ((manager: EntityManager) => Promise) +) => Promise; diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index e070273ac5..c917faea02 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -286,13 +286,7 @@ export class UsersManager { return { user, isWelcomed }; } - public async updateUserName(userId: number, name: string) { - const user = await User.findOne({where: {id: userId}}); - if (!user) { throw new ApiError("unable to find user", 400); } - user.name = name; - await user.save(); - } - + // TODO: rather use the updateUser() method, if that makes sense? public async updateUserOptions(userId: number, props: Partial) { const user = await User.findOne({where: {id: userId}}); if (!user) { throw new ApiError("unable to find user", 400); } @@ -321,16 +315,16 @@ export class UsersManager { // for an email key conflict failure. This is in case our transaction conflicts with a peer // doing the same thing. This is quite likely if the first page visited by a previously // unseen user fires off multiple api calls. - public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise { + public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise { try { - return await this.getUserByLogin(email, options); + return this.getUserByLogin(email, options); } catch (e) { if (e.name === 'QueryFailedError' && e.detail && e.detail.match(/Key \(email\)=[^ ]+ already exists/)) { // This is a postgres-specific error message. This problem cannot arise in sqlite, // because we have to serialize sqlite transactions in any case to get around a typeorm // limitation. - return await this.getUserByLogin(email, options); + return this.getUserByLogin(email, options); } throw e; } @@ -361,10 +355,10 @@ export class UsersManager { * unset/outdated fields of an existing record. * */ - public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise { + public async getUserByLogin(email: string, options: GetUserOptions = {}) { const {manager: transaction, profile, userOptions} = options; const normalizedEmail = normalizeEmail(email); - const userByLogin = await this._runInTransaction(transaction, async manager => { + return this._runInTransaction(transaction, async manager => { let needUpdate = false; const userQuery = manager.createQueryBuilder() .select('user') @@ -439,6 +433,7 @@ export class UsersManager { const startOfDay = timestamp - (timestamp % 86400 /*24h*/); // start of a day in seconds since epoc return startOfDay; }; + console.log("user.lastConnectionAt = ", user.lastConnectionAt); if (!user.lastConnectionAt || getTimestampStartOfDay(user.lastConnectionAt) !== getTimestampStartOfDay(nowish)) { user.lastConnectionAt = nowish; needUpdate = true; @@ -475,7 +470,6 @@ export class UsersManager { } return user; }); - return userByLogin; } /** diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 1d651b35e6..7aa6132fc4 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -3,7 +3,7 @@ import { AclRuleOrg } from 'app/gen-server/entity/AclRule'; import { Group } from 'app/gen-server/entity/Group'; import { Organization } from 'app/gen-server/entity/Organization'; import { SUPPORT_EMAIL, UsersManager } from 'app/gen-server/lib/homedb/UsersManager'; -import { NonGuestGroup, Resource } from 'app/gen-server/lib/homedb/Interfaces'; +import { GetUserOptions, NonGuestGroup, Resource } from 'app/gen-server/lib/homedb/Interfaces'; import { tmpdir } from 'os'; import path from 'path'; import { prepareDatabase } from 'test/server/lib/helpers/PrepareDatabase'; @@ -15,17 +15,19 @@ import { Document } from 'app/gen-server/entity/Document'; import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; import Sinon, { SinonSandbox, SinonSpy } from 'sinon'; import { assert } from 'chai'; -import { addSeedData } from 'test/gen-server/seed'; import { FullUser, UserProfile } from 'app/common/LoginSessionAPI'; -import { ANONYMOUS_USER_EMAIL } from 'app/common/UserAPI'; +import { ANONYMOUS_USER_EMAIL, EVERYONE_EMAIL, UserOptions } from 'app/common/UserAPI'; import { Login } from 'app/gen-server/entity/Login'; import { Pref } from 'app/gen-server/entity/Pref'; import { EntityManager } from 'typeorm'; +import log from 'app/server/lib/log'; +import winston from 'winston'; const username = process.env.USER || "nobody"; const tmpDir = path.join(tmpdir(), `grist_test_${username}_userendpoint`); describe('UsersManager', function () { + const NON_EXISTING_USER_ID = 10001337; this.timeout(30000); let env: EnvironmentSnapshot; let db: HomeDBManager; @@ -35,6 +37,34 @@ describe('UsersManager', function () { return getDatabase(path.join(tmpDir, typeormDb)); } + /** + * Works around lacks of type narrowing after asserting the value is defined. + * This is fixed in latest version of @types/chai + * FIXME: once using @types/chai > 4.3.17, remove this function + */ + function assertExists(value?: T, message?: string): asserts value is T { + assert.exists(value, message); + } + + /** + * TODO: Get rid of this function after having forced getUserByLogin returing a Promise + * and not a Promise + */ + async function getOrCreateUser(uuid: string, options?: GetUserOptions) { + const user = await db.getUserByLogin(makeEmail(uuid), options); + assertExists(user, 'new user should be created'); + return user; + } + + function makeEmail(uuid: string) { + return uuid + '@getgrist.com'; + } + + function disableLogging(method: T) { + return sandbox.stub(log, method); + } + + before(async function () { env = new EnvironmentSnapshot(); await prepareFilesystemDirectoryForTests(tmpDir); @@ -240,20 +270,20 @@ describe('UsersManager', function () { it('should retrieve a user by their ID', async function () { const user = await db.getUser(db.getSupportUserId()); - assert.ok(user, 'Should have returned a user'); - assert.strictEqual(user!.name, 'Support'); - assert.strictEqual(user!.loginEmail, SUPPORT_EMAIL); - assert.ok(!user!.prefs, "should not have retrieved user's prefs"); + assertExists(user, 'Should have returned a user'); + assert.strictEqual(user.name, 'Support'); + assert.strictEqual(user.loginEmail, SUPPORT_EMAIL); + assertExists(!user.prefs, "should not have retrieved user's prefs"); }); it('should retrieve a user along with their prefs with `includePrefs` set to true', async function () { const expectedUser = await getOrCreateUserWithPrefs(); const user = await db.getUser(expectedUser.id, {includePrefs: true}); - assert.ok(user, "Should have retrieved the user"); - assert.ok(user!.loginEmail); - assert.strictEqual(user!.loginEmail, expectedUser.loginEmail); - assert.ok(Array.isArray(user!.prefs), "should not have retrieved user's prefs"); - assert.deepEqual(user!.prefs, [{ + assertExists(user, "Should have retrieved the user"); + assertExists(user.loginEmail); + assert.strictEqual(user.loginEmail, expectedUser.loginEmail); + assertExists(Array.isArray(user.prefs), "should not have retrieved user's prefs"); + assert.deepEqual(user.prefs, [{ userId: expectedUser.id, orgId: expectedUser.personalOrg.id, prefs: { showGristTour: true } as any @@ -289,7 +319,7 @@ describe('UsersManager', function () { }); it('should throw when user is not found', async function () { - await assert.isRejected(db.getFullUser(1337), "unable to find user"); + await assert.isRejected(db.getFullUser(NON_EXISTING_USER_ID), "unable to find user"); }); }); @@ -376,21 +406,20 @@ describe('UsersManager', function () { describe('ensureExternalUser()', function () { let managerSaveSpy: SinonSpy; let userSaveSpy: SinonSpy; - let localDb: HomeDBManager; - before(async function () { - localDb = await getTmpDatabase('ensureExternalUser.db'); - await addSeedData(localDb.connection); - }); beforeEach(function () { managerSaveSpy = sandbox.spy(EntityManager.prototype, 'save'); userSaveSpy = sandbox.spy(User.prototype, 'save'); }); + /** + * Make a user profile. + * @param uuid A hard-coded UUID (so it is unique and still can be easily retrieved in database for diagnosis) + */ function makeProfile(uuid: string): UserProfile { return { - email: `${uuid}@getgrist.com`, + email: makeEmail(uuid), name: `NewUser ${uuid}`, connectId: `ConnectId-${uuid}`, picture: `https://mypic.com/${uuid}.png` @@ -398,22 +427,22 @@ describe('UsersManager', function () { } async function checkUserInfo(profile: UserProfile) { - const user = await localDb.getExistingUserByLogin(profile.email); - assert.exists(user, "the new user should be in database"); + const user = await db.getExistingUserByLogin(profile.email); + assertExists(user, "the new user should be in database"); assert.deepInclude(user, { isFirstTimeUser: false, name: profile.name, picture: profile.picture }); - assert.exists(user!.logins?.[0]); - assert.deepInclude(user!.logins[0], { + assert.exists(user.logins?.[0]); + assert.deepInclude(user.logins[0], { email: profile.email.toLowerCase(), displayEmail: profile.email }); } it('should not do anything if the user already exists and is up to date', async function () { - await localDb.ensureExternalUser({ + await db.ensureExternalUser({ name: 'Chimpy', email: 'chimpy@getgrist.com', }); @@ -423,7 +452,7 @@ describe('UsersManager', function () { it('should save an unknown user', async function () { const profile = makeProfile('f6fce1ec-892e-493a-9bd7-2c4b0480e7f5'); - await localDb.ensureExternalUser(profile); + await db.ensureExternalUser(profile); assert.isTrue(userSaveSpy.called, 'user.save() should have been called'); assert.isTrue(managerSaveSpy.called, 'manager.save() should have been called'); @@ -432,18 +461,18 @@ describe('UsersManager', function () { it('should update a user if they already exist in database', async function () { const oldProfile = makeProfile('b48b3c95-a808-43e1-9b78-9171fb6c58fd'); - await localDb.ensureExternalUser(oldProfile); + await db.ensureExternalUser(oldProfile); - let oldUser = await localDb.getExistingUserByLogin(oldProfile.email); - assert.exists(oldUser); + let oldUser = await db.getExistingUserByLogin(oldProfile.email); + assertExists(oldUser); const newProfile = { ...makeProfile('b6755ae0-0cb5-4a40-a54c-764d4b187c4c'), connectId: oldProfile.connectId, }; - await localDb.ensureExternalUser(newProfile); - oldUser = await localDb.getExistingUserByLogin(oldProfile.email); + await db.ensureExternalUser(newProfile); + oldUser = await db.getExistingUserByLogin(oldProfile.email); assert.notExists(oldUser, 'we should not retrieve the user given their old email address'); await checkUserInfo(newProfile); @@ -451,12 +480,222 @@ describe('UsersManager', function () { it('should normalize email address', async function() { const profile = makeProfile('92AF6F0F-03C0-414B-9012-2C282D89512A'); - await localDb.ensureExternalUser(profile); + await db.ensureExternalUser(profile); await checkUserInfo(profile); }); + }); + + describe('updateUser()', function () { + let emitSpy: SinonSpy; + + before(function () { + emitSpy = Sinon.spy(); + db.on('firstLogin', emitSpy); + }); + + after(function () { + db.off('firstLogin', emitSpy); + }); + + afterEach(function () { + emitSpy.resetHistory(); + }); + + function checkNoEventEmitted() { + assert.equal(emitSpy.callCount, 0, 'No event should have been emitted'); + } + + it('should reject when user is not found', async function () { + disableLogging('debug'); + const promise = db.updateUser(NON_EXISTING_USER_ID, {name: 'foobar'}); + await assert.isRejected(promise, 'unable to find user'); + }); + + it('should update a user name', async function () { + const uuid = 'b7afd9fc-02e1-4206-93f1-b6b923319aed'; + const newUser = await getOrCreateUser(uuid); + assert.equal(newUser.name, ''); + const userName = 'user name'; + await db.updateUser(newUser.id, {name: userName}); + checkNoEventEmitted(); + const updatedUser = await getOrCreateUser(uuid); + assert.equal(updatedUser.name, userName); + }); + + it('should not emit any event when isFirstTimeUser value has not changed', async function () { + const uuid = 'f94fb688-1a62-4423-b852-cbe7f7ceab27'; + const newUser = await getOrCreateUser(uuid); + assert.equal(newUser.isFirstTimeUser, true); + + await db.updateUser(newUser.id, {isFirstTimeUser: true}); + checkNoEventEmitted(); + }); + + it('should emit "firstLogin" event when isFirstTimeUser value has been toggled to false', async function () { + const uuid = 'f04080c3-98e6-4f4d-a428-20aca006ad52'; + const userName = 'user name'; + const newUser = await getOrCreateUser(uuid); + assert.equal(newUser.isFirstTimeUser, true); + + await db.updateUser(newUser.id, {isFirstTimeUser: false, name: userName}); + assert.equal(emitSpy.callCount, 1, '"firstLogin" event should have been emitted'); + + const fullUserFromEvent = emitSpy.firstCall.args[0]; + assertExists(fullUserFromEvent, 'a FullUser object should be passed with the "firstLogin" event'); + assert.equal(fullUserFromEvent.name, userName); + assert.equal(fullUserFromEvent.email, makeEmail(uuid)); + + const updatedUser = await getOrCreateUser(uuid); + assert.equal(updatedUser.isFirstTimeUser, false); + }); + }); + + describe('updateUserOptions()', function () { + it('should reject when user is not found', async function () { + disableLogging('debug'); + const promise = db.updateUserOptions(NON_EXISTING_USER_ID, {}); + await assert.isRejected(promise, 'unable to find user'); + }); + + it('should update user options', async function () { + const uuid = 'ca8a6812-12c3-473d-a063-500df4827f64'; + const newUser = await getOrCreateUser(uuid); + + assert.notExists(newUser.options); + + const options: UserOptions = {locale: 'fr', authSubject: 'subject', isConsultant: true, allowGoogleLogin: true}; + await db.updateUserOptions(newUser.id, options); + + const updatedUser = await getOrCreateUser(uuid); + assertExists(updatedUser); + assertExists(updatedUser.options); + assert.deepEqual(updatedUser.options, options); + }); + }); + + describe('getExistingUserByLogin()', function () { + it('should return an existing user', async function () { + const uuid = '3fcc580a-567b-4786-ba5b-139c36653598'; + const newUser = await getOrCreateUser(uuid); + + const retrievedUser = await db.getExistingUserByLogin(newUser.loginEmail!); + assertExists(retrievedUser); + + assert.equal(retrievedUser.id, newUser.id); + assert.equal(retrievedUser.name, newUser.name); + }); + + it('should normalize the passed user email', async function () { + const uuid = '35413bfd-961a-4429-91e0-7b3fb7c4e09f'; + const newUser = await getOrCreateUser(uuid); + + const retrievedUser = await db.getExistingUserByLogin(newUser.loginEmail!.toUpperCase()); + assertExists(retrievedUser); + }); + + it('should return undefined when the user is not found', async function () { + const nonExistingEmail = 'i-dont-exist@getgrist.com'; + const retrievedUser = await db.getExistingUserByLogin(nonExistingEmail); + assert.isUndefined(retrievedUser); + }); + }); + + describe('getUserByLogin', function () { + const callGetUserByLogin = getOrCreateUser; + + it('should create a user when none exist with the corresponding email', async function () { + const uuid = 'a091767e-1ff9-4018-aff3-a19258bf51ac'; + const email = makeEmail(uuid); + sandbox.useFakeTimers(42_000); + assert.notExists(await db.getExistingUserByLogin(email)); + + const user = await callGetUserByLogin(uuid.toUpperCase()); + + assert.isTrue(user.isFirstTimeUser, 'should be marked as first time user'); + assert.equal(user.loginEmail, email); + assert.equal(user.logins[0].displayEmail, makeEmail(uuid.toUpperCase())); + assert.equal(user.name, ''); + // FIXME: why is user.lastConnectionAt actually a string and not a Date? + // FIXME: why is user.lastConnectionAt updated here and ont firstLoginAt? + assert.equal(String(user.lastConnectionAt), '1970-01-01 00:00:42.000'); + }); + + it('should create a personnal organization for the new user', async function () { + const uuid = '66e36443-027e-4a6a-bc58-dafd8d0cda5c'; - it('FIXME', function () { - throw new Error('TODO: only use `db` and no localDb variable anymore'); + const user = await callGetUserByLogin(uuid); + + const org = await db.getOrg({userId: user.id, showAll: true}, user.personalOrg.id); + assertExists(org.data, 'should have retrieved personnal org data'); + assert.equal(org.data.name, 'Personal'); + }); + + it('should not create organizations for non-login emails', async function () { + const user = await db.getUserByLogin(EVERYONE_EMAIL); + assertExists(user); + assert.notOk(user.personalOrg); + }); + + it('should not update user information when no profile is passed', async function () { + const uuid = '330eb9dd-5aba-4d65-9397-88634fe448a4'; + const userFirstCall = await callGetUserByLogin(uuid); + const userSecondCall = await callGetUserByLogin(uuid); + assert.deepEqual(userFirstCall, userSecondCall); + }); + + it('should update lastConnectionAt only for different days', async function () { + const fakeTimer = sandbox.useFakeTimers(0); + const uuid = '174509c4-f671-4241-9c17-47df54cdc4e0'; + let user = await callGetUserByLogin(uuid); + const epochDateTime = '1970-01-01 00:00:00.000'; + assert.equal(String(user.lastConnectionAt), epochDateTime); + + await fakeTimer.tickAsync(42_000); + user = await callGetUserByLogin(uuid); + assert.equal(String(user.lastConnectionAt), epochDateTime); + + // await fakeTimer.tickAsync('1d'); + // user = await callGetUserByLogin(uuid); + // assert.match(String(user.lastConnectionAt), /^1970-01-02/); + }); + + describe('when passing information to update (using `profile`)', function () { + const makeProfile = (newEmail: string): UserProfile => ({ + name: 'my user name', + email: newEmail, + picture: 'https://mypic.com/foo.png', + connectId: 'new-connect-id', + }); + + it('should populate the firstTimeLogin and deduce the name from the email', async function () { + sandbox.useFakeTimers(42_000); + const uuid = '59c4fd04-46ca-4a29-aeca-61809359f19d'; + const user = await callGetUserByLogin(uuid, {profile: {name: '', email: makeEmail(uuid)}}); + assert.equal(user.name, uuid); + // FIXME: why is user.firstLoginAt actually a string and not a Date? + assert.equal(String(user.firstLoginAt), '1970-01-01 00:00:42.000'); + }); + + it('should populate user with any passed information', async function () { + const uuid = 'eea38c2d-f470-4d4f-9fee-e7c8564292d1'; + await callGetUserByLogin(uuid); + + const profile = makeProfile(makeEmail('5d3aa491-e8ae-457c-96d1-7022998148db')); + const userOptions: UserOptions = {authSubject: 'my-auth-subject'}; + + const updatedUser = await callGetUserByLogin(uuid, { profile, userOptions }); + assert.deepInclude(updatedUser, { + name: profile.name, + connectId: profile.connectId, + picture: profile.picture, + }); + assert.deepInclude(updatedUser.logins[0], { + displayEmail: profile.email, + }); + assert.deepInclude(updatedUser.options, { + authSubject: userOptions.authSubject, + }); + }); }); }); }); From a6bd18a39f2c68adbc95fcadde2f4f11bcb3cd0c Mon Sep 17 00:00:00 2001 From: fflorent Date: Sat, 3 Aug 2024 21:55:44 +0200 Subject: [PATCH 04/22] test deleteUser() --- app/gen-server/lib/homedb/UsersManager.ts | 1 - test/gen-server/lib/homedb/UsersManager.ts | 99 ++++++++++++++++++++-- 2 files changed, 92 insertions(+), 8 deletions(-) diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index c917faea02..cb9adcd93b 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -433,7 +433,6 @@ export class UsersManager { const startOfDay = timestamp - (timestamp % 86400 /*24h*/); // start of a day in seconds since epoc return startOfDay; }; - console.log("user.lastConnectionAt = ", user.lastConnectionAt); if (!user.lastConnectionAt || getTimestampStartOfDay(user.lastConnectionAt) !== getTimestampStartOfDay(nowish)) { user.lastConnectionAt = nowish; needUpdate = true; diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 7aa6132fc4..69daed89b5 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -60,11 +60,14 @@ describe('UsersManager', function () { return uuid + '@getgrist.com'; } + async function getPersonalOrg(user: User) { + return db.getOrg({userId: user.id}, user.personalOrg.id); + } + function disableLogging(method: T) { return sandbox.stub(log, method); } - before(async function () { env = new EnvironmentSnapshot(); await prepareFilesystemDirectoryForTests(tmpDir); @@ -625,7 +628,7 @@ describe('UsersManager', function () { const user = await callGetUserByLogin(uuid); - const org = await db.getOrg({userId: user.id, showAll: true}, user.personalOrg.id); + const org = await getPersonalOrg(user); assertExists(org.data, 'should have retrieved personnal org data'); assert.equal(org.data.name, 'Personal'); }); @@ -643,7 +646,8 @@ describe('UsersManager', function () { assert.deepEqual(userFirstCall, userSecondCall); }); - it('should update lastConnectionAt only for different days', async function () { + // FIXME: why using Sinon.useFakeTimers() makes user.lastConnectionAt a string instead of a Date + it.skip('should update lastConnectionAt only for different days', async function () { const fakeTimer = sandbox.useFakeTimers(0); const uuid = '174509c4-f671-4241-9c17-47df54cdc4e0'; let user = await callGetUserByLogin(uuid); @@ -654,9 +658,9 @@ describe('UsersManager', function () { user = await callGetUserByLogin(uuid); assert.equal(String(user.lastConnectionAt), epochDateTime); - // await fakeTimer.tickAsync('1d'); - // user = await callGetUserByLogin(uuid); - // assert.match(String(user.lastConnectionAt), /^1970-01-02/); + await fakeTimer.tickAsync('1d'); + user = await callGetUserByLogin(uuid); + assert.match(String(user.lastConnectionAt), /^1970-01-02/); }); describe('when passing information to update (using `profile`)', function () { @@ -672,7 +676,7 @@ describe('UsersManager', function () { const uuid = '59c4fd04-46ca-4a29-aeca-61809359f19d'; const user = await callGetUserByLogin(uuid, {profile: {name: '', email: makeEmail(uuid)}}); assert.equal(user.name, uuid); - // FIXME: why is user.firstLoginAt actually a string and not a Date? + // FIXME: why using Sinon.useFakeTimers() makes user.firstLoginAt a string instead of a Date assert.equal(String(user.firstLoginAt), '1970-01-01 00:00:42.000'); }); @@ -698,4 +702,85 @@ describe('UsersManager', function () { }); }); }); + + describe('deleteUser()', function () { + function userHasPrefs(userId: number, manager: EntityManager) { + return manager.exists(Pref, { where: { userId: userId }}); + } + + function userHasGroupUsers(userId: number, manager: EntityManager) { + return manager.exists('group_users', { where: {user_id: userId} }); + } + + it('should refuse to delete the account of someone else', async function () { + const uuid = 'e5c22c05-fb3c-4e9b-b410-b9a3b72e1c03'; + const userToDelete = await getOrCreateUser(uuid); + + const promise = db.deleteUser({userId: 2}, userToDelete.id); + await assert.isRejected(promise, 'not permitted to delete this user'); + }); + + it('should refuse to delete a non existing account', async function () { + disableLogging('debug'); + const promise = db.deleteUser({userId: NON_EXISTING_USER_ID}, NON_EXISTING_USER_ID); + await assert.isRejected(promise, 'user not found'); + }); + + it('should refuse to delete the account if the passed name is not matching', async function () { + disableLogging('debug'); + const uuid = '317fed70-c68e-46d1-b806-92c47894911d'; + const userToDelete = await getOrCreateUser(uuid, { + profile: { + name: 'someone to delete', + email: makeEmail(uuid), + } + }); + + const promise = db.deleteUser({userId: userToDelete.id}, userToDelete.id, 'wrong name'); + + await assert.isRejected(promise); + await promise.catch(e => assert.match(e.message, /user name did not match/)); + }); + + it('should remove the user and cleanup their info and personal organization', async function () { + const uuid = '226f8de2-347d-4816-8eb6-ecdc362deb18'; + const userToDelete = await getOrCreateUser(uuid, { + profile: { + name: 'someone to delete', + email: makeEmail(uuid), + } + }); + + assertExists(await getPersonalOrg(userToDelete)); + + await db.connection.transaction(async (manager) => { + assert.isTrue(await userHasGroupUsers(userToDelete.id, manager)); + assert.isTrue(await userHasPrefs(userToDelete.id, manager)); + }); + + await db.deleteUser({userId: userToDelete.id}, userToDelete.id); + + assert.notExists(await db.getUser(userToDelete.id)); + assert.deepEqual(await getPersonalOrg(userToDelete), { errMessage: 'organization not found', status: 404 }); + + await db.connection.transaction(async (manager) => { + assert.isFalse(await userHasGroupUsers(userToDelete.id, manager)); + assert.isFalse(await userHasPrefs(userToDelete.id, manager)); + }); + }); + + it("should remove the user when passed name corresponds to the user's name", async function () { + const uuid = '48a2ae1f-743a-45cc-943f-c8eb6c11ee75'; + const userName = 'someone to delete'; + const userToDelete = await getOrCreateUser(uuid, { + profile: { + name: userName, + email: makeEmail(uuid), + } + }); + const promise = db.deleteUser({userId: userToDelete.id}, userToDelete.id, userName); + await assert.isFulfilled(promise); + }); + + }); }); From d2b185c214abae5d388dd68a285578b0882379f1 Mon Sep 17 00:00:00 2001 From: fflorent Date: Sun, 4 Aug 2024 00:06:33 +0200 Subject: [PATCH 05/22] Tests for getLoginWithRetry --- test/gen-server/lib/homedb/UsersManager.ts | 61 ++++++++++++++++++++-- 1 file changed, 56 insertions(+), 5 deletions(-) diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 69daed89b5..3d1ecc62e0 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -113,7 +113,7 @@ describe('UsersManager', function () { return Array(usersListLength).fill(null).map(_ => makeUsers(nbUsersByGroups, idxIt)); } - describe('getResourceUsers', function () { + describe('getResourceUsers()', function () { function populateResourceWithMembers(res: Resource, members: User[], groupName?: string) { const aclRule = new AclRuleOrg(); const group = new Group(); @@ -158,7 +158,7 @@ describe('UsersManager', function () { }); }); - describe('getUsersWithRole', function () { + describe('getUsersWithRole()', function () { function makeGroups(groupDefinition: {[k in NonGuestGroup['name']]?: User[] | undefined}){ return (Object.entries(groupDefinition) as [NonGuestGroup['name'], User[] | undefined][]) .map(([groupName, users], index) => { @@ -247,7 +247,7 @@ describe('UsersManager', function () { }); }); - describe('getUserByKey', function () { + describe('getUserByKey()', function () { it('should return the user given their API Key', async function () { const user = await db.getUserByKey('api_key_for_chimpy'); assert.strictEqual(user?.name, 'Chimpy', 'should retrieve Chimpy by their API key'); @@ -260,7 +260,7 @@ describe('UsersManager', function () { }); }); - describe('getUser', async function () { + describe('getUser()', async function () { const userWithPrefsEmail = 'userwithprefs@getgrist.com'; async function getOrCreateUserWithPrefs() { @@ -603,7 +603,7 @@ describe('UsersManager', function () { }); }); - describe('getUserByLogin', function () { + describe('getUserByLogin()', function () { const callGetUserByLogin = getOrCreateUser; it('should create a user when none exist with the corresponding email', async function () { @@ -703,6 +703,57 @@ describe('UsersManager', function () { }); }); + describe('getUserByLoginWithRetry()', async function () { + async function ensureGetUserByLoginWithRetryWorks(uuid: string) { + const email = makeEmail(uuid); + const user = await db.getUserByLoginWithRetry(email); + assertExists(user); + assert.equal(user.loginEmail, email); + } + + function makeQueryFailedError() { + const error = new Error() as any; + error.name = 'QueryFailedError'; + error.detail = 'Key (email)=whatever@getgrist.com already exists'; + return error; + } + + it('should work just like getUserByLogin', async function () { + await ensureGetUserByLoginWithRetryWorks( '55bde435-2f61-477a-881a-20076232a941'); + }); + + it('should make a second attempt on special error', async function () { + sandbox.stub(UsersManager.prototype, 'getUserByLogin') + .onFirstCall().throws(makeQueryFailedError()) + .callThrough(); + await ensureGetUserByLoginWithRetryWorks( '47c42c26-e1e0-4dbb-a042-862107eb28ce'); + }); + + it('should reject after 3 attempts', async function () { + const secondError = makeQueryFailedError(); + sandbox.stub(UsersManager.prototype, 'getUserByLogin') + .onFirstCall().throws(makeQueryFailedError()) + .onSecondCall().throws(secondError) + .callThrough(); + + const email = makeEmail('cb5ff3d2-9d84-4b6a-b329-1b80d40e4edf'); + const promise = db.getUserByLoginWithRetry(email); + await assert.isRejected(promise); + await promise.catch(err => assert.equal(err, secondError)); + }); + + it('should reject immediately if the error is not a QueryFailedError', async function () { + const errorMsg = 'my error'; + sandbox.stub(UsersManager.prototype, 'getUserByLogin') + .rejects(new Error(errorMsg)) + .callThrough(); + + const email = makeEmail('cb5ff3d2-9d84-4b6a-b329-1b80d40e4edf'); + const promise = db.getUserByLoginWithRetry(email); + await assert.isRejected(promise, errorMsg); + }); + }); + describe('deleteUser()', function () { function userHasPrefs(userId: number, manager: EntityManager) { return manager.exists(Pref, { where: { userId: userId }}); From 71e0599a6d12f03b1bfb4f76624fc2ff98bf1c7d Mon Sep 17 00:00:00 2001 From: fflorent Date: Sun, 4 Aug 2024 16:39:32 +0200 Subject: [PATCH 06/22] initializeSpecialIds and completeProfiles() --- app/gen-server/lib/homedb/HomeDBManager.ts | 13 ++- app/gen-server/lib/homedb/UsersManager.ts | 104 +++++++++++---------- app/server/lib/dbUtils.ts | 35 ++++--- test/gen-server/lib/homedb/UsersManager.ts | 94 ++++++++++++++++++- test/server/lib/helpers/PrepareDatabase.ts | 4 +- 5 files changed, 184 insertions(+), 66 deletions(-) diff --git a/app/gen-server/lib/homedb/HomeDBManager.ts b/app/gen-server/lib/homedb/HomeDBManager.ts index b61c5375ab..56a8ebe055 100644 --- a/app/gen-server/lib/homedb/HomeDBManager.ts +++ b/app/gen-server/lib/homedb/HomeDBManager.ts @@ -56,7 +56,7 @@ import { readJson } from 'app/gen-server/sqlUtils'; import {appSettings} from 'app/server/lib/AppSettings'; -import {getOrCreateConnection} from 'app/server/lib/dbUtils'; +import {createNewConnection, getOrCreateConnection} from 'app/server/lib/dbUtils'; import {makeId} from 'app/server/lib/idUtils'; import log from 'app/server/lib/log'; import {Permit} from 'app/server/lib/Permit'; @@ -248,7 +248,6 @@ export type BillingOptions = Partial { this._connection = await getOrCreateConnection(); - this._dbType = this._connection.driver.options.type; + } + + public async createNewConnection(name?: string): Promise { + this._connection = await createNewConnection(name); } // make sure special users and workspaces are available @@ -4358,7 +4364,6 @@ export class HomeDBManager extends EventEmitter { }); return verifyEntity(orgQuery); } - } // Return a QueryResult reflecting the output of a query builder. diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index cb9adcd93b..8e96ca99b0 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -513,6 +513,63 @@ export class UsersManager { }; } + public async initializeSpecialIds(): Promise { + await this._maybeCreateSpecialUserId({ + email: ANONYMOUS_USER_EMAIL, + name: "Anonymous" + }); + await this._maybeCreateSpecialUserId({ + email: PREVIEWER_EMAIL, + name: "Preview" + }); + await this._maybeCreateSpecialUserId({ + email: EVERYONE_EMAIL, + name: "Everyone" + }); + await this._maybeCreateSpecialUserId({ + email: SUPPORT_EMAIL, + name: "Support" + }); + } + + /** + * + * Take a list of user profiles coming from the client's session, correlate + * them with Users and Logins in the database, and construct full profiles + * with user ids, standardized display emails, pictures, and anonymous flags. + * + */ + public async completeProfiles(profiles: UserProfile[]): Promise { + if (profiles.length === 0) { return []; } + const qb = this._connection.createQueryBuilder() + .select('logins') + .from(Login, 'logins') + .leftJoinAndSelect('logins.user', 'user') + .where('logins.email in (:...emails)', {emails: profiles.map(profile => normalizeEmail(profile.email))}); + const completedProfiles: {[email: string]: FullUser} = {}; + for (const login of await qb.getMany()) { + completedProfiles[login.email] = { + id: login.user.id, + email: login.displayEmail, + name: login.user.name, + picture: login.user.picture, + anonymous: login.user.id === this.getAnonymousUserId(), + locale: login.user.options?.locale + }; + } + return profiles.map(profile => completedProfiles[normalizeEmail(profile.email)]) + .filter(fullProfile => fullProfile); + } + + /** + * ================================== + * + * Below methods are public but not exposed by HomeDBManager + * + * They are meant to be used internally (i.e. by homedb/ modules) + * + */ + // Looks up the emails in the permission delta and adds them to the users map in // the delta object. // Returns a QueryResult based on the validity of the passed in PermissionDelta object. @@ -582,25 +639,6 @@ export class UsersManager { }; } - public async initializeSpecialIds(): Promise { - await this._maybeCreateSpecialUserId({ - email: ANONYMOUS_USER_EMAIL, - name: "Anonymous" - }); - await this._maybeCreateSpecialUserId({ - email: PREVIEWER_EMAIL, - name: "Preview" - }); - await this._maybeCreateSpecialUserId({ - email: EVERYONE_EMAIL, - name: "Everyone" - }); - await this._maybeCreateSpecialUserId({ - email: SUPPORT_EMAIL, - name: "Support" - }); - } - /** * Check for anonymous user, either encoded directly as an id, or as a singular * profile (this case arises during processing of the session/access/all endpoint @@ -677,34 +715,6 @@ export class UsersManager { return members; } - /** - * - * Take a list of user profiles coming from the client's session, correlate - * them with Users and Logins in the database, and construct full profiles - * with user ids, standardized display emails, pictures, and anonymous flags. - * - */ - public async completeProfiles(profiles: UserProfile[]): Promise { - if (profiles.length === 0) { return []; } - const qb = this._connection.createQueryBuilder() - .select('logins') - .from(Login, 'logins') - .leftJoinAndSelect('logins.user', 'user') - .where('logins.email in (:...emails)', {emails: profiles.map(profile => normalizeEmail(profile.email))}); - const completedProfiles: {[email: string]: FullUser} = {}; - for (const login of await qb.getMany()) { - completedProfiles[login.email] = { - id: login.user.id, - email: login.displayEmail, - name: login.user.name, - picture: login.user.picture, - anonymous: login.user.id === this.getAnonymousUserId(), - locale: login.user.options?.locale - }; - } - return profiles.map(profile => completedProfiles[normalizeEmail(profile.email)]) - .filter(profile => profile); - } // For the moment only the support user can add both everyone@ and anon@ to a // resource, since that allows spam. TODO: enhance or remove. diff --git a/app/server/lib/dbUtils.ts b/app/server/lib/dbUtils.ts index 4a82364003..c1da96d04d 100644 --- a/app/server/lib/dbUtils.ts +++ b/app/server/lib/dbUtils.ts @@ -50,8 +50,23 @@ export async function updateDb(connection?: Connection) { * avoid duplication. */ const connectionMutex = new Mutex(); + +async function buildConnection(name?: string) { + const connection = await createConnection({...getTypeORMSettings(), ...(name ? {name} : {})}); + // When using Sqlite, set a busy timeout of 3s to tolerate a little + // interference from connections made by tests. Logging doesn't show + // any particularly slow queries, but bad luck is possible. + // This doesn't affect when Postgres is in use. It also doesn't have + // any impact when there is a single connection to the db, as is the + // case when Grist is run as a single process. + if (connection.driver.options.type === 'sqlite') { + await connection.query('PRAGMA busy_timeout = 3000'); + } + return connection; +} + export async function getOrCreateConnection(): Promise { - return connectionMutex.runExclusive(async() => { + return connectionMutex.runExclusive(async () => { try { // If multiple servers are started within the same process, we // share the database connection. This saves locking trouble @@ -62,21 +77,17 @@ export async function getOrCreateConnection(): Promise { if (!String(e).match(/ConnectionNotFoundError/)) { throw e; } - const connection = await createConnection(getTypeORMSettings()); - // When using Sqlite, set a busy timeout of 3s to tolerate a little - // interference from connections made by tests. Logging doesn't show - // any particularly slow queries, but bad luck is possible. - // This doesn't affect when Postgres is in use. It also doesn't have - // any impact when there is a single connection to the db, as is the - // case when Grist is run as a single process. - if (connection.driver.options.type === 'sqlite') { - await connection.query('PRAGMA busy_timeout = 3000'); - } - return connection; + return buildConnection(); } }); } +export async function createNewConnection(name?: string): Promise { + return connectionMutex.runExclusive(async () => { + return buildConnection(name); + }) +} + export async function runMigrations(connection: Connection) { // on SQLite, migrations fail if we don't temporarily disable foreign key // constraint checking. This is because for sqlite typeorm copies each diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 3d1ecc62e0..4c7695dc50 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -16,12 +16,13 @@ import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; import Sinon, { SinonSandbox, SinonSpy } from 'sinon'; import { assert } from 'chai'; import { FullUser, UserProfile } from 'app/common/LoginSessionAPI'; -import { ANONYMOUS_USER_EMAIL, EVERYONE_EMAIL, UserOptions } from 'app/common/UserAPI'; +import { ANONYMOUS_USER_EMAIL, EVERYONE_EMAIL, PREVIEWER_EMAIL, UserOptions } from 'app/common/UserAPI'; import { Login } from 'app/gen-server/entity/Login'; import { Pref } from 'app/gen-server/entity/Pref'; import { EntityManager } from 'typeorm'; import log from 'app/server/lib/log'; import winston from 'winston'; +import { updateDb } from 'app/server/lib/dbUtils'; const username = process.env.USER || "nobody"; const tmpDir = path.join(tmpdir(), `grist_test_${username}_userendpoint`); @@ -832,6 +833,97 @@ describe('UsersManager', function () { const promise = db.deleteUser({userId: userToDelete.id}, userToDelete.id, userName); await assert.isFulfilled(promise); }); + }); + + describe('completeProfiles()', function () { + it('should return an empty array if no profiles are provided', async function () { + const res = await db.completeProfiles([]); + assert.deepEqual(res, []); + }); + + it("should complete a single user profile with looking by normalized address", async function () { + const uuid = '0f9a8eae-4fcf-4e9b-85e8-227b2ce9cad5'; + const email = makeEmail(uuid); + const profile = { + name: 'complete-profile-email-normalized-user', + email: makeEmail(uuid), + picture: 'https://mypic.com/me.png', + }; + const someLocale = 'fr-FR'; + const userCreated = await getOrCreateUser(uuid, { profile }); + await db.updateUserOptions(userCreated.id, {locale: someLocale}); + + const res = await db.completeProfiles([{name: 'whatever', email: email.toUpperCase()}]); + + assert.deepEqual(res, [{ + ...profile, + id: userCreated.id, + locale: someLocale, + anonymous: false + }]); + }); + + it('should complete several user profiles', async function () { + const uuidPrefix = '0f9a8eae-4fcf-4e9b-85e8-227b2ce9cad5'; + const seq = Array(10).fill(null).map((_, i) => i+1); + const uuids = seq.map(i => `${uuidPrefix}_${i}`); + const usersCreated = await Promise.all( + uuids.map(uuid => getOrCreateUser(uuid)) + ); + + const res = await db.completeProfiles( + uuids.map(uuid => ({name: 'whatever', email: makeEmail(uuid)})) + ); + assert.lengthOf(res, uuids.length); + for (const [index, uuid] of uuids.entries()) { + assert.deepInclude(res[index], { + id: usersCreated[index].id, + email: makeEmail(uuid), + }); + } + }); + }); + + describe('initializeSpecialIds()', function () { + let initSpecIdEnv: EnvironmentSnapshot; + async function withDataBase(dbName: string, cb: (db: HomeDBManager) => Promise) { + // await prepareDatabase(tmpDir, dbName); + process.env.TYPEORM_DATABASE = path.join(tmpDir, dbName); + const localDb = new HomeDBManager(); + await localDb.createNewConnection('special-id'); + await updateDb(localDb.connection); + try { + await cb(localDb); + } finally { + await localDb.connection.destroy(); + } + } + beforeEach(function () { + initSpecIdEnv = new EnvironmentSnapshot(); + }); + afterEach(function () { + initSpecIdEnv.restore(); + }); + + it('should initialize special ids', async function () { + return withDataBase('test-special-ids.db', async (localDb) => { + const specialAccounts = [ + {name: "Support", email: SUPPORT_EMAIL}, + {name: "Anonymous", email: ANONYMOUS_USER_EMAIL}, + {name: "Preview", email: PREVIEWER_EMAIL}, + {name: "Everyone", email: EVERYONE_EMAIL} + ]; + for (const {email} of specialAccounts) { + assert.notExists(await localDb.getExistingUserByLogin(email)); + } + await localDb.initializeSpecialIds(); + for (const {name, email} of specialAccounts) { + const res = await localDb.getExistingUserByLogin(email); + assertExists(res); + assert.equal(res.name, name); + } + }); + }); }); }); diff --git a/test/server/lib/helpers/PrepareDatabase.ts b/test/server/lib/helpers/PrepareDatabase.ts index ab643a60cd..eff2e0bf44 100644 --- a/test/server/lib/helpers/PrepareDatabase.ts +++ b/test/server/lib/helpers/PrepareDatabase.ts @@ -2,13 +2,13 @@ import path from "path"; import * as testUtils from "test/server/testUtils"; import {execFileSync} from "child_process"; -export async function prepareDatabase(tempDirectory: string) { +export async function prepareDatabase(tempDirectory: string, filename: string = 'landing.db') { // Let's create a sqlite db that we can share with servers that run in other processes, hence // not an in-memory db. Running seed.ts directly might not take in account the most recent value // for TYPEORM_DATABASE, because ormconfig.js may already have been loaded with a different // configuration (in-memory for instance). Spawning a process is one way to make sure that the // latest value prevail. - process.env.TYPEORM_DATABASE = path.join(tempDirectory, 'landing.db'); + process.env.TYPEORM_DATABASE = path.join(tempDirectory, filename); const seed = await testUtils.getBuildFile('test/gen-server/seed.js'); execFileSync('node', [seed, 'init'], { env: process.env, From f9148feeb0c2db5783ad74b887e8ba522714d4e6 Mon Sep 17 00:00:00 2001 From: fflorent Date: Mon, 5 Aug 2024 15:25:04 +0200 Subject: [PATCH 07/22] Fix tests for initializeSpecialIds() --- app/gen-server/lib/homedb/HomeDBManager.ts | 5 +- app/server/companion.ts | 4 +- app/server/lib/dbUtils.ts | 23 +++--- test/gen-server/lib/homedb/UsersManager.ts | 92 +++++++++++----------- 4 files changed, 67 insertions(+), 57 deletions(-) diff --git a/app/gen-server/lib/homedb/HomeDBManager.ts b/app/gen-server/lib/homedb/HomeDBManager.ts index 56a8ebe055..db54054211 100644 --- a/app/gen-server/lib/homedb/HomeDBManager.ts +++ b/app/gen-server/lib/homedb/HomeDBManager.ts @@ -70,6 +70,7 @@ import { Brackets, Connection, DatabaseType, + DataSourceOptions, EntityManager, ObjectLiteral, SelectQueryBuilder, @@ -353,8 +354,8 @@ export class HomeDBManager extends EventEmitter { this._connection = await getOrCreateConnection(); } - public async createNewConnection(name?: string): Promise { - this._connection = await createNewConnection(name); + public async createNewConnection(overrideConf?: Partial): Promise { + this._connection = await createNewConnection(overrideConf); } // make sure special users and workspaces are available diff --git a/app/server/companion.ts b/app/server/companion.ts index bad8092ce5..e62d99bba5 100644 --- a/app/server/companion.ts +++ b/app/server/companion.ts @@ -3,7 +3,7 @@ import { version } from 'app/common/version'; import { synchronizeProducts } from 'app/gen-server/entity/Product'; import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; import { applyPatch } from 'app/gen-server/lib/TypeORMPatches'; -import { getMigrations, getOrCreateConnection, getTypeORMSettings, +import { createNewConnection, getMigrations, getTypeORMSettings, undoLastMigration, updateDb } from 'app/server/lib/dbUtils'; import { getDatabaseUrl } from 'app/server/lib/serverUtils'; import { getTelemetryPrefs } from 'app/server/lib/Telemetry'; @@ -194,7 +194,7 @@ export function addDbCommand(program: commander.Command, if (!process.env.TYPEORM_LOGGING) { process.env.TYPEORM_LOGGING = 'true'; } - const connection = reuseConnection || await getOrCreateConnection(); + const connection = reuseConnection || await createNewConnection(); const exitCode = await op(connection); if (exitCode !== 0) { program.error('db command failed', {exitCode}); diff --git a/app/server/lib/dbUtils.ts b/app/server/lib/dbUtils.ts index c1da96d04d..65a23b5e2a 100644 --- a/app/server/lib/dbUtils.ts +++ b/app/server/lib/dbUtils.ts @@ -45,14 +45,19 @@ export async function updateDb(connection?: Connection) { await synchronizeProducts(connection, true); } +function getConnectionName() { + return process.env.TYPEORM_NAME || 'default'; +} + /** * Get a connection to db if one exists, or create one. Serialized to * avoid duplication. */ const connectionMutex = new Mutex(); -async function buildConnection(name?: string) { - const connection = await createConnection({...getTypeORMSettings(), ...(name ? {name} : {})}); +async function buildConnection(overrideConf?: Partial) { + const settings = getTypeORMSettings(overrideConf); + const connection = await createConnection(settings); // When using Sqlite, set a busy timeout of 3s to tolerate a little // interference from connections made by tests. Logging doesn't show // any particularly slow queries, but bad luck is possible. @@ -71,8 +76,7 @@ export async function getOrCreateConnection(): Promise { // If multiple servers are started within the same process, we // share the database connection. This saves locking trouble // with Sqlite. - const connection = getConnection(); - return connection; + return getConnection(getConnectionName()); } catch (e) { if (!String(e).match(/ConnectionNotFoundError/)) { throw e; @@ -82,10 +86,10 @@ export async function getOrCreateConnection(): Promise { }); } -export async function createNewConnection(name?: string): Promise { +export async function createNewConnection(overrideConf?: Partial): Promise { return connectionMutex.runExclusive(async () => { - return buildConnection(name); - }) + return buildConnection(overrideConf); + }); } export async function runMigrations(connection: Connection) { @@ -114,7 +118,7 @@ export async function undoLastMigration(connection: Connection) { // Replace the old janky ormconfig.js file, which was always a source of // pain to use since it wasn't properly integrated into the typescript // project. -export function getTypeORMSettings(): DataSourceOptions { +export function getTypeORMSettings(overrideConf?: Partial): DataSourceOptions { // If we have a redis server available, tell typeorm. Then any queries built with // .cache() called on them will be cached via redis. // We use a separate environment variable for the moment so that we don't have to @@ -131,7 +135,7 @@ export function getTypeORMSettings(): DataSourceOptions { } : undefined; return { - "name": process.env.TYPEORM_NAME || "default", + "name": getConnectionName(), "type": (process.env.TYPEORM_TYPE as any) || "sqlite", // officially, TYPEORM_CONNECTION - // but if we use that, this file will never // be read, and we can't configure @@ -155,5 +159,6 @@ export function getTypeORMSettings(): DataSourceOptions { ], ...JSON.parse(process.env.TYPEORM_EXTRA || "{}"), ...cache, + ...overrideConf, }; } diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 4c7695dc50..af980286d6 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -835,6 +835,51 @@ describe('UsersManager', function () { }); }); + describe('initializeSpecialIds()', function () { + async function withDataBase(dbName: string, cb: (db: HomeDBManager) => Promise) { + // await prepareDatabase(tmpDir, dbName); + const database = path.join(tmpDir, dbName); + const localDb = new HomeDBManager(); + await localDb.createNewConnection({ name: 'special-id', database }); + await updateDb(localDb.connection); + try { + await cb(localDb); + } finally { + await localDb.connection.destroy(); + } + } + + after(async function () { + // HACK: This is a weird case, we have established a different connection + // but the existing connection is also impacted. + // A found workaround consist in destroying and creating again the connection. + // + // TODO: Check whether using DataSource would help and avoid this hack. + await db.connection.destroy(); + await db.createNewConnection(); + }); + + it('should initialize special ids', async function () { + return withDataBase('test-special-ids.db', async (localDb) => { + const specialAccounts = [ + {name: "Support", email: SUPPORT_EMAIL}, + {name: "Anonymous", email: ANONYMOUS_USER_EMAIL}, + {name: "Preview", email: PREVIEWER_EMAIL}, + {name: "Everyone", email: EVERYONE_EMAIL} + ]; + for (const {email} of specialAccounts) { + assert.notExists(await localDb.getExistingUserByLogin(email)); + } + await localDb.initializeSpecialIds(); + for (const {name, email} of specialAccounts) { + const res = await localDb.getExistingUserByLogin(email); + assertExists(res); + assert.equal(res.name, name); + } + }); + }); + }); + describe('completeProfiles()', function () { it('should return an empty array if no profiles are provided', async function () { const res = await db.completeProfiles([]); @@ -872,7 +917,9 @@ describe('UsersManager', function () { ); const res = await db.completeProfiles( - uuids.map(uuid => ({name: 'whatever', email: makeEmail(uuid)})) + uuids.map( + uuid => ({name: 'whatever', email: makeEmail(uuid)}) + ) ); assert.lengthOf(res, uuids.length); for (const [index, uuid] of uuids.entries()) { @@ -883,47 +930,4 @@ describe('UsersManager', function () { } }); }); - - describe('initializeSpecialIds()', function () { - let initSpecIdEnv: EnvironmentSnapshot; - async function withDataBase(dbName: string, cb: (db: HomeDBManager) => Promise) { - // await prepareDatabase(tmpDir, dbName); - process.env.TYPEORM_DATABASE = path.join(tmpDir, dbName); - const localDb = new HomeDBManager(); - await localDb.createNewConnection('special-id'); - await updateDb(localDb.connection); - try { - await cb(localDb); - } finally { - await localDb.connection.destroy(); - } - } - - beforeEach(function () { - initSpecIdEnv = new EnvironmentSnapshot(); - }); - afterEach(function () { - initSpecIdEnv.restore(); - }); - - it('should initialize special ids', async function () { - return withDataBase('test-special-ids.db', async (localDb) => { - const specialAccounts = [ - {name: "Support", email: SUPPORT_EMAIL}, - {name: "Anonymous", email: ANONYMOUS_USER_EMAIL}, - {name: "Preview", email: PREVIEWER_EMAIL}, - {name: "Everyone", email: EVERYONE_EMAIL} - ]; - for (const {email} of specialAccounts) { - assert.notExists(await localDb.getExistingUserByLogin(email)); - } - await localDb.initializeSpecialIds(); - for (const {name, email} of specialAccounts) { - const res = await localDb.getExistingUserByLogin(email); - assertExists(res); - assert.equal(res.name, name); - } - }); - }); - }); }); From 4c0b0872e49e671a129656f1366fcea7c2fc1642 Mon Sep 17 00:00:00 2001 From: fflorent Date: Mon, 5 Aug 2024 17:42:25 +0200 Subject: [PATCH 08/22] Replace uuid with unique local parts --- test/gen-server/lib/homedb/UsersManager.ts | 191 +++++++++++---------- 1 file changed, 103 insertions(+), 88 deletions(-) diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index af980286d6..f541705296 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -33,6 +33,7 @@ describe('UsersManager', function () { let env: EnvironmentSnapshot; let db: HomeDBManager; let sandbox: SinonSandbox; + const uniqueLocalPart = new Set(); function getTmpDatabase(typeormDb: string) { return getDatabase(path.join(tmpDir, typeormDb)); @@ -47,18 +48,31 @@ describe('UsersManager', function () { assert.exists(value, message); } + function ensureUnique(localPart: string) { + if (uniqueLocalPart.has(localPart)) { + throw new Error('passed localPart is already used elsewhere'); + } + uniqueLocalPart.add(localPart); + return localPart; + } + + function createUniqueUser(uniqueEmailLocalPart: string) { + ensureUnique(uniqueEmailLocalPart); + return getOrCreateUser(uniqueEmailLocalPart); + } + /** * TODO: Get rid of this function after having forced getUserByLogin returing a Promise * and not a Promise */ - async function getOrCreateUser(uuid: string, options?: GetUserOptions) { - const user = await db.getUserByLogin(makeEmail(uuid), options); + async function getOrCreateUser(localPart: string, options?: GetUserOptions) { + const user = await db.getUserByLogin(makeEmail(localPart), options); assertExists(user, 'new user should be created'); return user; } - function makeEmail(uuid: string) { - return uuid + '@getgrist.com'; + function makeEmail(localPart: string) { + return localPart + '@getgrist.com'; } async function getPersonalOrg(user: User) { @@ -419,14 +433,15 @@ describe('UsersManager', function () { /** * Make a user profile. - * @param uuid A hard-coded UUID (so it is unique and still can be easily retrieved in database for diagnosis) + * @param localPart A unique local part of the email (also used for the other fields). */ - function makeProfile(uuid: string): UserProfile { + function makeProfile(localPart: string): UserProfile { + ensureUnique(localPart); return { - email: makeEmail(uuid), - name: `NewUser ${uuid}`, - connectId: `ConnectId-${uuid}`, - picture: `https://mypic.com/${uuid}.png` + email: makeEmail(localPart), + name: `NewUser ${localPart}`, + connectId: `ConnectId-${localPart}`, + picture: `https://mypic.com/${localPart}.png` }; } @@ -455,7 +470,7 @@ describe('UsersManager', function () { }); it('should save an unknown user', async function () { - const profile = makeProfile('f6fce1ec-892e-493a-9bd7-2c4b0480e7f5'); + const profile = makeProfile('ensureExternalUser-saves-an-unknown-user'); await db.ensureExternalUser(profile); assert.isTrue(userSaveSpy.called, 'user.save() should have been called'); assert.isTrue(managerSaveSpy.called, 'manager.save() should have been called'); @@ -464,14 +479,14 @@ describe('UsersManager', function () { }); it('should update a user if they already exist in database', async function () { - const oldProfile = makeProfile('b48b3c95-a808-43e1-9b78-9171fb6c58fd'); + const oldProfile = makeProfile('ensureexternaluser-updates-an-existing-user_old'); await db.ensureExternalUser(oldProfile); let oldUser = await db.getExistingUserByLogin(oldProfile.email); assertExists(oldUser); const newProfile = { - ...makeProfile('b6755ae0-0cb5-4a40-a54c-764d4b187c4c'), + ...makeProfile('ensureexternaluser-updates-an-existing-user_new'), connectId: oldProfile.connectId, }; @@ -483,7 +498,7 @@ describe('UsersManager', function () { }); it('should normalize email address', async function() { - const profile = makeProfile('92AF6F0F-03C0-414B-9012-2C282D89512A'); + const profile = makeProfile('ENSUREEXTERNALUSER-NORMALIZES-email-address'); await db.ensureExternalUser(profile); await checkUserInfo(profile); }); @@ -516,29 +531,29 @@ describe('UsersManager', function () { }); it('should update a user name', async function () { - const uuid = 'b7afd9fc-02e1-4206-93f1-b6b923319aed'; - const newUser = await getOrCreateUser(uuid); - assert.equal(newUser.name, ''); + const emailLocalPart = 'updateUser-should-update-user-name'; + const createdUser = await createUniqueUser(emailLocalPart); + assert.equal(createdUser.name, ''); const userName = 'user name'; - await db.updateUser(newUser.id, {name: userName}); + await db.updateUser(createdUser.id, {name: userName}); checkNoEventEmitted(); - const updatedUser = await getOrCreateUser(uuid); + const updatedUser = await getOrCreateUser(emailLocalPart); assert.equal(updatedUser.name, userName); }); it('should not emit any event when isFirstTimeUser value has not changed', async function () { - const uuid = 'f94fb688-1a62-4423-b852-cbe7f7ceab27'; - const newUser = await getOrCreateUser(uuid); - assert.equal(newUser.isFirstTimeUser, true); + const localPart = 'updateuser-should-not-emit-when-isfirsttimeuser-not-changed'; + const createdUser = await createUniqueUser(localPart); + assert.equal(createdUser.isFirstTimeUser, true); - await db.updateUser(newUser.id, {isFirstTimeUser: true}); + await db.updateUser(createdUser.id, {isFirstTimeUser: true}); checkNoEventEmitted(); }); it('should emit "firstLogin" event when isFirstTimeUser value has been toggled to false', async function () { - const uuid = 'f04080c3-98e6-4f4d-a428-20aca006ad52'; + const localPart = 'updateuser-emits-firstlogin'; const userName = 'user name'; - const newUser = await getOrCreateUser(uuid); + const newUser = await createUniqueUser(localPart); assert.equal(newUser.isFirstTimeUser, true); await db.updateUser(newUser.id, {isFirstTimeUser: false, name: userName}); @@ -547,9 +562,9 @@ describe('UsersManager', function () { const fullUserFromEvent = emitSpy.firstCall.args[0]; assertExists(fullUserFromEvent, 'a FullUser object should be passed with the "firstLogin" event'); assert.equal(fullUserFromEvent.name, userName); - assert.equal(fullUserFromEvent.email, makeEmail(uuid)); + assert.equal(fullUserFromEvent.email, makeEmail(localPart)); - const updatedUser = await getOrCreateUser(uuid); + const updatedUser = await getOrCreateUser(localPart); assert.equal(updatedUser.isFirstTimeUser, false); }); }); @@ -562,15 +577,15 @@ describe('UsersManager', function () { }); it('should update user options', async function () { - const uuid = 'ca8a6812-12c3-473d-a063-500df4827f64'; - const newUser = await getOrCreateUser(uuid); + const localPart = 'updateuseroptions-updates-user-options'; + const createdUser = await createUniqueUser(localPart); - assert.notExists(newUser.options); + assert.notExists(createdUser.options); const options: UserOptions = {locale: 'fr', authSubject: 'subject', isConsultant: true, allowGoogleLogin: true}; - await db.updateUserOptions(newUser.id, options); + await db.updateUserOptions(createdUser.id, options); - const updatedUser = await getOrCreateUser(uuid); + const updatedUser = await getOrCreateUser(localPart); assertExists(updatedUser); assertExists(updatedUser.options); assert.deepEqual(updatedUser.options, options); @@ -579,19 +594,19 @@ describe('UsersManager', function () { describe('getExistingUserByLogin()', function () { it('should return an existing user', async function () { - const uuid = '3fcc580a-567b-4786-ba5b-139c36653598'; - const newUser = await getOrCreateUser(uuid); + const localPart = 'getexistinguserbylogin-returns-existing-user'; + const createdUser = await createUniqueUser(localPart); - const retrievedUser = await db.getExistingUserByLogin(newUser.loginEmail!); + const retrievedUser = await db.getExistingUserByLogin(createdUser.loginEmail!); assertExists(retrievedUser); - assert.equal(retrievedUser.id, newUser.id); - assert.equal(retrievedUser.name, newUser.name); + assert.equal(retrievedUser.id, createdUser.id); + assert.equal(retrievedUser.name, createdUser.name); }); it('should normalize the passed user email', async function () { - const uuid = '35413bfd-961a-4429-91e0-7b3fb7c4e09f'; - const newUser = await getOrCreateUser(uuid); + const localPart = 'getexistinguserbylogin-normalizes-user-email'; + const newUser = await createUniqueUser(localPart); const retrievedUser = await db.getExistingUserByLogin(newUser.loginEmail!.toUpperCase()); assertExists(retrievedUser); @@ -608,16 +623,16 @@ describe('UsersManager', function () { const callGetUserByLogin = getOrCreateUser; it('should create a user when none exist with the corresponding email', async function () { - const uuid = 'a091767e-1ff9-4018-aff3-a19258bf51ac'; - const email = makeEmail(uuid); + const localPart = ensureUnique('getuserbylogin-creates-user-when-not-already-exists'); + const email = makeEmail(localPart); sandbox.useFakeTimers(42_000); assert.notExists(await db.getExistingUserByLogin(email)); - const user = await callGetUserByLogin(uuid.toUpperCase()); + const user = await callGetUserByLogin(localPart.toUpperCase()); assert.isTrue(user.isFirstTimeUser, 'should be marked as first time user'); assert.equal(user.loginEmail, email); - assert.equal(user.logins[0].displayEmail, makeEmail(uuid.toUpperCase())); + assert.equal(user.logins[0].displayEmail, makeEmail(localPart.toUpperCase())); assert.equal(user.name, ''); // FIXME: why is user.lastConnectionAt actually a string and not a Date? // FIXME: why is user.lastConnectionAt updated here and ont firstLoginAt? @@ -625,9 +640,9 @@ describe('UsersManager', function () { }); it('should create a personnal organization for the new user', async function () { - const uuid = '66e36443-027e-4a6a-bc58-dafd8d0cda5c'; + const localPart = ensureUnique('getuserbylogin-creates-personnal-org'); - const user = await callGetUserByLogin(uuid); + const user = await callGetUserByLogin(localPart); const org = await getPersonalOrg(user); assertExists(org.data, 'should have retrieved personnal org data'); @@ -641,26 +656,26 @@ describe('UsersManager', function () { }); it('should not update user information when no profile is passed', async function () { - const uuid = '330eb9dd-5aba-4d65-9397-88634fe448a4'; - const userFirstCall = await callGetUserByLogin(uuid); - const userSecondCall = await callGetUserByLogin(uuid); + const localPart = ensureUnique('getuserbylogin-does-not-update-without-profile'); + const userFirstCall = await callGetUserByLogin(localPart); + const userSecondCall = await callGetUserByLogin(localPart); assert.deepEqual(userFirstCall, userSecondCall); }); // FIXME: why using Sinon.useFakeTimers() makes user.lastConnectionAt a string instead of a Date it.skip('should update lastConnectionAt only for different days', async function () { const fakeTimer = sandbox.useFakeTimers(0); - const uuid = '174509c4-f671-4241-9c17-47df54cdc4e0'; - let user = await callGetUserByLogin(uuid); + const localPart = ensureUnique('getuserbylogin-updates-last_connection_at-for-different-days'); + let user = await callGetUserByLogin(localPart); const epochDateTime = '1970-01-01 00:00:00.000'; assert.equal(String(user.lastConnectionAt), epochDateTime); await fakeTimer.tickAsync(42_000); - user = await callGetUserByLogin(uuid); + user = await callGetUserByLogin(localPart); assert.equal(String(user.lastConnectionAt), epochDateTime); await fakeTimer.tickAsync('1d'); - user = await callGetUserByLogin(uuid); + user = await callGetUserByLogin(localPart); assert.match(String(user.lastConnectionAt), /^1970-01-02/); }); @@ -674,21 +689,21 @@ describe('UsersManager', function () { it('should populate the firstTimeLogin and deduce the name from the email', async function () { sandbox.useFakeTimers(42_000); - const uuid = '59c4fd04-46ca-4a29-aeca-61809359f19d'; - const user = await callGetUserByLogin(uuid, {profile: {name: '', email: makeEmail(uuid)}}); - assert.equal(user.name, uuid); + const localPart = ensureUnique('getuserbylogin-with-profile-populates-first_time_login-and-name'); + const user = await callGetUserByLogin(localPart, {profile: {name: '', email: makeEmail(localPart)}}); + assert.equal(user.name, localPart); // FIXME: why using Sinon.useFakeTimers() makes user.firstLoginAt a string instead of a Date assert.equal(String(user.firstLoginAt), '1970-01-01 00:00:42.000'); }); it('should populate user with any passed information', async function () { - const uuid = 'eea38c2d-f470-4d4f-9fee-e7c8564292d1'; - await callGetUserByLogin(uuid); + const localPart = ensureUnique('getuserbylogin-with-profile-populates-user-with-passed-info'); + await callGetUserByLogin(localPart); const profile = makeProfile(makeEmail('5d3aa491-e8ae-457c-96d1-7022998148db')); const userOptions: UserOptions = {authSubject: 'my-auth-subject'}; - const updatedUser = await callGetUserByLogin(uuid, { profile, userOptions }); + const updatedUser = await callGetUserByLogin(localPart, { profile, userOptions }); assert.deepInclude(updatedUser, { name: profile.name, connectId: profile.connectId, @@ -705,8 +720,8 @@ describe('UsersManager', function () { }); describe('getUserByLoginWithRetry()', async function () { - async function ensureGetUserByLoginWithRetryWorks(uuid: string) { - const email = makeEmail(uuid); + async function ensureGetUserByLoginWithRetryWorks(localPart: string) { + const email = makeEmail(localPart); const user = await db.getUserByLoginWithRetry(email); assertExists(user); assert.equal(user.loginEmail, email); @@ -720,24 +735,24 @@ describe('UsersManager', function () { } it('should work just like getUserByLogin', async function () { - await ensureGetUserByLoginWithRetryWorks( '55bde435-2f61-477a-881a-20076232a941'); + await ensureGetUserByLoginWithRetryWorks( ensureUnique('getuserbyloginwithretry-works-like-getuserbylogin')); }); it('should make a second attempt on special error', async function () { sandbox.stub(UsersManager.prototype, 'getUserByLogin') .onFirstCall().throws(makeQueryFailedError()) .callThrough(); - await ensureGetUserByLoginWithRetryWorks( '47c42c26-e1e0-4dbb-a042-862107eb28ce'); + await ensureGetUserByLoginWithRetryWorks( 'getuserbyloginwithretry-makes-a-single-retry'); }); - it('should reject after 3 attempts', async function () { + it('should reject after 2 attempts', async function () { const secondError = makeQueryFailedError(); sandbox.stub(UsersManager.prototype, 'getUserByLogin') .onFirstCall().throws(makeQueryFailedError()) .onSecondCall().throws(secondError) .callThrough(); - const email = makeEmail('cb5ff3d2-9d84-4b6a-b329-1b80d40e4edf'); + const email = makeEmail(ensureUnique('getuserbyloginwithretry-rejects-after-2-attempts')); const promise = db.getUserByLoginWithRetry(email); await assert.isRejected(promise); await promise.catch(err => assert.equal(err, secondError)); @@ -749,7 +764,7 @@ describe('UsersManager', function () { .rejects(new Error(errorMsg)) .callThrough(); - const email = makeEmail('cb5ff3d2-9d84-4b6a-b329-1b80d40e4edf'); + const email = makeEmail(ensureUnique('getuserbyloginwithretry-rejects-immediately-when-not-queryfailederror')); const promise = db.getUserByLoginWithRetry(email); await assert.isRejected(promise, errorMsg); }); @@ -765,8 +780,8 @@ describe('UsersManager', function () { } it('should refuse to delete the account of someone else', async function () { - const uuid = 'e5c22c05-fb3c-4e9b-b410-b9a3b72e1c03'; - const userToDelete = await getOrCreateUser(uuid); + const localPart = ensureUnique('deleteuser-refuses-for-someone-else'); + const userToDelete = await getOrCreateUser(localPart); const promise = db.deleteUser({userId: 2}, userToDelete.id); await assert.isRejected(promise, 'not permitted to delete this user'); @@ -780,11 +795,11 @@ describe('UsersManager', function () { it('should refuse to delete the account if the passed name is not matching', async function () { disableLogging('debug'); - const uuid = '317fed70-c68e-46d1-b806-92c47894911d'; - const userToDelete = await getOrCreateUser(uuid, { + const localPart = ensureUnique('deleteuser-refuses-if-name-not-matching'); + const userToDelete = await getOrCreateUser(localPart, { profile: { name: 'someone to delete', - email: makeEmail(uuid), + email: makeEmail(localPart), } }); @@ -795,11 +810,11 @@ describe('UsersManager', function () { }); it('should remove the user and cleanup their info and personal organization', async function () { - const uuid = '226f8de2-347d-4816-8eb6-ecdc362deb18'; - const userToDelete = await getOrCreateUser(uuid, { + const localPart = ensureUnique('deleteuser-removes-user-and-cleanups-info'); + const userToDelete = await getOrCreateUser(localPart, { profile: { name: 'someone to delete', - email: makeEmail(uuid), + email: makeEmail(localPart), } }); @@ -822,12 +837,12 @@ describe('UsersManager', function () { }); it("should remove the user when passed name corresponds to the user's name", async function () { - const uuid = '48a2ae1f-743a-45cc-943f-c8eb6c11ee75'; + const localPart = ensureUnique('deleteuser-removes-user-when-name-matches'); const userName = 'someone to delete'; - const userToDelete = await getOrCreateUser(uuid, { + const userToDelete = await getOrCreateUser(localPart, { profile: { name: userName, - email: makeEmail(uuid), + email: makeEmail(localPart), } }); const promise = db.deleteUser({userId: userToDelete.id}, userToDelete.id, userName); @@ -887,15 +902,15 @@ describe('UsersManager', function () { }); it("should complete a single user profile with looking by normalized address", async function () { - const uuid = '0f9a8eae-4fcf-4e9b-85e8-227b2ce9cad5'; - const email = makeEmail(uuid); + const localPart = ensureUnique('completeprofiles-with-single-profile'); + const email = makeEmail(localPart); const profile = { name: 'complete-profile-email-normalized-user', - email: makeEmail(uuid), + email: makeEmail(localPart), picture: 'https://mypic.com/me.png', }; const someLocale = 'fr-FR'; - const userCreated = await getOrCreateUser(uuid, { profile }); + const userCreated = await getOrCreateUser(localPart, { profile }); await db.updateUserOptions(userCreated.id, {locale: someLocale}); const res = await db.completeProfiles([{name: 'whatever', email: email.toUpperCase()}]); @@ -909,23 +924,23 @@ describe('UsersManager', function () { }); it('should complete several user profiles', async function () { - const uuidPrefix = '0f9a8eae-4fcf-4e9b-85e8-227b2ce9cad5'; + const localPartPrefix = ensureUnique('completeprofiles-with-several-profiles'); const seq = Array(10).fill(null).map((_, i) => i+1); - const uuids = seq.map(i => `${uuidPrefix}_${i}`); + const localParts = seq.map(i => `${localPartPrefix}_${i}`); const usersCreated = await Promise.all( - uuids.map(uuid => getOrCreateUser(uuid)) + localParts.map(localPart => getOrCreateUser(localPart)) ); const res = await db.completeProfiles( - uuids.map( - uuid => ({name: 'whatever', email: makeEmail(uuid)}) + localParts.map( + localPart => ({name: 'whatever', email: makeEmail(localPart)}) ) ); - assert.lengthOf(res, uuids.length); - for (const [index, uuid] of uuids.entries()) { + assert.lengthOf(res, localParts.length); + for (const [index, localPart] of localParts.entries()) { assert.deepInclude(res[index], { id: usersCreated[index].id, - email: makeEmail(uuid), + email: makeEmail(localPart), }); } }); From ca18de7d636f2d2564c8af51d2e1d637752a726d Mon Sep 17 00:00:00 2001 From: fflorent Date: Mon, 5 Aug 2024 18:15:15 +0200 Subject: [PATCH 09/22] make getUserByLogin return Promise (cannot return Promise) --- app/gen-server/lib/homedb/HomeDBManager.ts | 4 +-- app/gen-server/lib/homedb/UsersManager.ts | 6 ++-- app/server/companion.ts | 4 --- app/server/lib/TestLogin.ts | 6 ++-- stubs/app/server/server.ts | 4 --- test/gen-server/ApiServer.ts | 14 ++++----- test/gen-server/ApiServerAccess.ts | 6 ++-- test/gen-server/ApiServerBugs.ts | 2 +- test/gen-server/lib/homedb/UsersManager.ts | 35 ++++++++-------------- test/gen-server/testUtils.ts | 1 - test/nbrowser/homeUtil.ts | 9 ++---- test/server/fixSiteProducts.ts | 2 +- test/server/lib/GranularAccess.ts | 2 +- 13 files changed, 34 insertions(+), 61 deletions(-) diff --git a/app/gen-server/lib/homedb/HomeDBManager.ts b/app/gen-server/lib/homedb/HomeDBManager.ts index db54054211..3caa079582 100644 --- a/app/gen-server/lib/homedb/HomeDBManager.ts +++ b/app/gen-server/lib/homedb/HomeDBManager.ts @@ -482,8 +482,8 @@ export class HomeDBManager extends EventEmitter { /** * @see UsersManager.prototype.getUserByLogin */ - public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise { - return await this._usersManager.getUserByLogin(email, options) ?? undefined; + public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise { + return await this._usersManager.getUserByLogin(email, options); } /** diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index 8e96ca99b0..5b26196089 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -98,9 +98,7 @@ export class UsersManager { return await this._connection.transaction(async manager => { for (const email of emails) { const user = await this.getUserByLogin(email, {manager}); - if (user) { - await manager.delete(Pref, {userId: user.id}); - } + await manager.delete(Pref, {userId: user.id}); } }); } @@ -467,7 +465,7 @@ export class UsersManager { // In principle this could be optimized, but this is simpler to maintain. user = await userQuery.getOne(); } - return user; + return user!; }); } diff --git a/app/server/companion.ts b/app/server/companion.ts index e62d99bba5..cafc0ed545 100644 --- a/app/server/companion.ts +++ b/app/server/companion.ts @@ -166,10 +166,6 @@ export function addSiteCommand(program: commander.Command, const profile = {email, name: email}; const db = await getHomeDBManager(); const user = await db.getUserByLogin(email, {profile}); - if (!user) { - // This should not happen. - throw new Error('failed to create user'); - } db.unwrapQueryResult(await db.addOrg(user, { name: domain, domain, diff --git a/app/server/lib/TestLogin.ts b/app/server/lib/TestLogin.ts index 12ef87a3c2..ca85b47cab 100644 --- a/app/server/lib/TestLogin.ts +++ b/app/server/lib/TestLogin.ts @@ -25,10 +25,8 @@ export async function getTestLoginSystem(): Promise { if (process.env.TEST_SUPPORT_API_KEY) { const dbManager = gristServer.getHomeDBManager(); const user = await dbManager.getUserByLogin(SUPPORT_EMAIL); - if (user) { - user.apiKey = process.env.TEST_SUPPORT_API_KEY; - await user.save(); - } + user.apiKey = process.env.TEST_SUPPORT_API_KEY; + await user.save(); } return "test-login"; }, diff --git a/stubs/app/server/server.ts b/stubs/app/server/server.ts index 6fbffdf52b..95d3692113 100644 --- a/stubs/app/server/server.ts +++ b/stubs/app/server/server.ts @@ -80,10 +80,6 @@ async function setupDb() { } const profile = {email, name: email}; const user = await db.getUserByLogin(email, {profile}); - if (!user) { - // This should not happen. - throw new Error('failed to create GRIST_DEFAULT_EMAIL user'); - } db.unwrapQueryResult(await db.addOrg(user, { name: org, domain: org, diff --git a/test/gen-server/ApiServer.ts b/test/gen-server/ApiServer.ts index 39aba8c35d..07484e8c2b 100644 --- a/test/gen-server/ApiServer.ts +++ b/test/gen-server/ApiServer.ts @@ -49,9 +49,9 @@ describe('ApiServer', function() { homeUrl = await server.start(['home', 'docs']); dbManager = server.dbManager; - chimpyRef = await dbManager.getUserByLogin(chimpyEmail).then((user) => user!.ref); - kiwiRef = await dbManager.getUserByLogin(kiwiEmail).then((user) => user!.ref); - charonRef = await dbManager.getUserByLogin(charonEmail).then((user) => user!.ref); + chimpyRef = await dbManager.getUserByLogin(chimpyEmail).then((user) => user.ref); + kiwiRef = await dbManager.getUserByLogin(kiwiEmail).then((user) => user.ref); + charonRef = await dbManager.getUserByLogin(charonEmail).then((user) => user.ref); // Listen to user count updates and add them to an array. dbManager.on('userChange', ({org, countBefore, countAfter}: UserChange) => { @@ -2070,8 +2070,7 @@ describe('ApiServer', function() { // create a new user const profile = {email: 'meep@getgrist.com', name: 'Meep'}; const user = await dbManager.getUserByLogin('meep@getgrist.com', {profile}); - assert(user); - const userId = user!.id; + const userId = user.id; // set up an api key await dbManager.connection.query("update users set api_key = 'api_key_for_meep' where id = $1", [userId]); @@ -2111,11 +2110,10 @@ describe('ApiServer', function() { const userBlank = await dbManager.getUserByLogin('blank@getgrist.com', {profile: {email: 'blank@getgrist.com', name: ''}}); - assert(userBlank); - await dbManager.connection.query("update users set api_key = 'api_key_for_blank' where id = $1", [userBlank!.id]); + await dbManager.connection.query("update users set api_key = 'api_key_for_blank' where id = $1", [userBlank.id]); // check that user can delete themselves - resp = await axios.delete(`${homeUrl}/api/users/${userBlank!.id}`, + resp = await axios.delete(`${homeUrl}/api/users/${userBlank.id}`, {data: {name: ""}, ...configForUser("blank")}); assert.equal(resp.status, 200); diff --git a/test/gen-server/ApiServerAccess.ts b/test/gen-server/ApiServerAccess.ts index 33abe90b1d..a95494f5f2 100644 --- a/test/gen-server/ApiServerAccess.ts +++ b/test/gen-server/ApiServerAccess.ts @@ -61,9 +61,9 @@ describe('ApiServerAccess', function() { } ); dbManager = server.dbManager; - chimpyRef = await dbManager.getUserByLogin(chimpyEmail).then((user) => user!.ref); - kiwiRef = await dbManager.getUserByLogin(kiwiEmail).then((user) => user!.ref); - charonRef = await dbManager.getUserByLogin(charonEmail).then((user) => user!.ref); + chimpyRef = await dbManager.getUserByLogin(chimpyEmail).then((user) => user.ref); + kiwiRef = await dbManager.getUserByLogin(kiwiEmail).then((user) => user.ref); + charonRef = await dbManager.getUserByLogin(charonEmail).then((user) => user.ref); // Listen to user count updates and add them to an array. dbManager.on('userChange', ({org, countBefore, countAfter}: UserChange) => { if (countBefore === countAfter) { return; } diff --git a/test/gen-server/ApiServerBugs.ts b/test/gen-server/ApiServerBugs.ts index f8ef21126c..dd1ad183fe 100644 --- a/test/gen-server/ApiServerBugs.ts +++ b/test/gen-server/ApiServerBugs.ts @@ -32,7 +32,7 @@ describe('ApiServerBugs', function() { server = new TestServer(this); homeUrl = await server.start(); dbManager = server.dbManager; - userRef = (email) => server.dbManager.getUserByLogin(email).then((user) => user!.ref); + userRef = (email) => server.dbManager.getUserByLogin(email).then((user) => user.ref); }); after(async function() { diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index f541705296..172676190c 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -61,14 +61,8 @@ describe('UsersManager', function () { return getOrCreateUser(uniqueEmailLocalPart); } - /** - * TODO: Get rid of this function after having forced getUserByLogin returing a Promise - * and not a Promise - */ async function getOrCreateUser(localPart: string, options?: GetUserOptions) { - const user = await db.getUserByLogin(makeEmail(localPart), options); - assertExists(user, 'new user should be created'); - return user; + return db.getUserByLogin(makeEmail(localPart), options); } function makeEmail(localPart: string) { @@ -620,15 +614,13 @@ describe('UsersManager', function () { }); describe('getUserByLogin()', function () { - const callGetUserByLogin = getOrCreateUser; - it('should create a user when none exist with the corresponding email', async function () { const localPart = ensureUnique('getuserbylogin-creates-user-when-not-already-exists'); const email = makeEmail(localPart); sandbox.useFakeTimers(42_000); assert.notExists(await db.getExistingUserByLogin(email)); - const user = await callGetUserByLogin(localPart.toUpperCase()); + const user = await db.getUserByLogin(makeEmail(localPart.toUpperCase())); assert.isTrue(user.isFirstTimeUser, 'should be marked as first time user'); assert.equal(user.loginEmail, email); @@ -642,7 +634,7 @@ describe('UsersManager', function () { it('should create a personnal organization for the new user', async function () { const localPart = ensureUnique('getuserbylogin-creates-personnal-org'); - const user = await callGetUserByLogin(localPart); + const user = await db.getUserByLogin(makeEmail(localPart)); const org = await getPersonalOrg(user); assertExists(org.data, 'should have retrieved personnal org data'); @@ -651,14 +643,13 @@ describe('UsersManager', function () { it('should not create organizations for non-login emails', async function () { const user = await db.getUserByLogin(EVERYONE_EMAIL); - assertExists(user); assert.notOk(user.personalOrg); }); it('should not update user information when no profile is passed', async function () { const localPart = ensureUnique('getuserbylogin-does-not-update-without-profile'); - const userFirstCall = await callGetUserByLogin(localPart); - const userSecondCall = await callGetUserByLogin(localPart); + const userFirstCall = await db.getUserByLogin(makeEmail(localPart)); + const userSecondCall = await db.getUserByLogin(makeEmail(localPart)); assert.deepEqual(userFirstCall, userSecondCall); }); @@ -666,16 +657,16 @@ describe('UsersManager', function () { it.skip('should update lastConnectionAt only for different days', async function () { const fakeTimer = sandbox.useFakeTimers(0); const localPart = ensureUnique('getuserbylogin-updates-last_connection_at-for-different-days'); - let user = await callGetUserByLogin(localPart); + let user = await db.getUserByLogin(makeEmail(localPart)); const epochDateTime = '1970-01-01 00:00:00.000'; assert.equal(String(user.lastConnectionAt), epochDateTime); await fakeTimer.tickAsync(42_000); - user = await callGetUserByLogin(localPart); + user = await db.getUserByLogin(makeEmail(localPart)); assert.equal(String(user.lastConnectionAt), epochDateTime); await fakeTimer.tickAsync('1d'); - user = await callGetUserByLogin(localPart); + user = await db.getUserByLogin(makeEmail(localPart)); assert.match(String(user.lastConnectionAt), /^1970-01-02/); }); @@ -690,20 +681,20 @@ describe('UsersManager', function () { it('should populate the firstTimeLogin and deduce the name from the email', async function () { sandbox.useFakeTimers(42_000); const localPart = ensureUnique('getuserbylogin-with-profile-populates-first_time_login-and-name'); - const user = await callGetUserByLogin(localPart, {profile: {name: '', email: makeEmail(localPart)}}); + const user = await db.getUserByLogin(makeEmail(localPart), {profile: {name: '', email: makeEmail(localPart)}}); assert.equal(user.name, localPart); // FIXME: why using Sinon.useFakeTimers() makes user.firstLoginAt a string instead of a Date assert.equal(String(user.firstLoginAt), '1970-01-01 00:00:42.000'); }); it('should populate user with any passed information', async function () { - const localPart = ensureUnique('getuserbylogin-with-profile-populates-user-with-passed-info'); - await callGetUserByLogin(localPart); + const localPart = ensureUnique('getuserbylogin-with-profile-populates-user-with-passed-info_OLD'); + await db.getUserByLogin(makeEmail(localPart)); - const profile = makeProfile(makeEmail('5d3aa491-e8ae-457c-96d1-7022998148db')); + const profile = makeProfile(makeEmail('getuserbylogin-with-profile-populates-user-with-passed-info_NEW')); const userOptions: UserOptions = {authSubject: 'my-auth-subject'}; - const updatedUser = await callGetUserByLogin(localPart, { profile, userOptions }); + const updatedUser = await db.getUserByLogin(makeEmail(localPart), { profile, userOptions }); assert.deepInclude(updatedUser, { name: profile.name, connectId: profile.connectId, diff --git a/test/gen-server/testUtils.ts b/test/gen-server/testUtils.ts index 3f7707abe5..a942773a57 100644 --- a/test/gen-server/testUtils.ts +++ b/test/gen-server/testUtils.ts @@ -46,7 +46,6 @@ export async function createUser(dbManager: HomeDBManager, name: string): Promis const username = name.toLowerCase(); const email = `${username}@getgrist.com`; const user = await dbManager.getUserByLogin(email, {profile: {email, name}}); - if (!user) { throw new Error('failed to create user'); } user.apiKey = `api_key_for_${username}`; await user.save(); const userHome = (await dbManager.getOrg({userId: user.id}, null)).data; diff --git a/test/nbrowser/homeUtil.ts b/test/nbrowser/homeUtil.ts index 3aa2ff3700..e7b770a3a6 100644 --- a/test/nbrowser/homeUtil.ts +++ b/test/nbrowser/homeUtil.ts @@ -420,7 +420,7 @@ export class HomeUtil { if (this.server.isExternalServer()) { throw new Error('not supported'); } const dbManager = await this.server.getDatabase(); const user = await dbManager.getUserByLogin(email); - if (user) { await dbManager.deleteUser({userId: user.id}, user.id, user.name); } + await dbManager.deleteUser({userId: user.id}, user.id, user.name); } // Set whether this is the user's first time logging in. Requires access to the database. @@ -428,10 +428,8 @@ export class HomeUtil { if (this.server.isExternalServer()) { throw new Error('not supported'); } const dbManager = await this.server.getDatabase(); const user = await dbManager.getUserByLogin(email); - if (user) { - user.isFirstTimeUser = isFirstLogin; - await user.save(); - } + user.isFirstTimeUser = isFirstLogin; + await user.save(); } private async _initShowGristTour(email: string, showGristTour: boolean) { @@ -463,7 +461,6 @@ export class HomeUtil { const dbManager = await this.server.getDatabase(); const user = await dbManager.getUserByLogin(email); - if (!user) { return; } if (user.personalOrg) { const org = await dbManager.getOrg({userId: user.id}, user.personalOrg.id); diff --git a/test/server/fixSiteProducts.ts b/test/server/fixSiteProducts.ts index b42d5a016b..353b4ac8e4 100644 --- a/test/server/fixSiteProducts.ts +++ b/test/server/fixSiteProducts.ts @@ -122,7 +122,7 @@ describe('fixSiteProducts', function() { assert.equal(getDefaultProductNames().teamInitial, 'stub'); const db = server.dbManager; - const user = await db.getUserByLogin(email, {profile}) as any; + const user = await db.getUserByLogin(email, {profile}); const orgId = db.unwrapQueryResult(await db.addOrg(user, { name: 'sanity-check-org', domain: 'sanity-check-org', diff --git a/test/server/lib/GranularAccess.ts b/test/server/lib/GranularAccess.ts index 87225d2d31..c722de3072 100644 --- a/test/server/lib/GranularAccess.ts +++ b/test/server/lib/GranularAccess.ts @@ -3279,7 +3279,7 @@ describe('GranularAccess', function() { cliOwner.flush(); let perm: PermissionDataWithExtraUsers = (await cliOwner.send("getUsersForViewAs", 0)).data; const getId = (name: string) => home.dbManager.testGetId(name) as Promise; - const getRef = (email: string) => home.dbManager.getUserByLogin(email).then(user => user!.ref); + const getRef = (email: string) => home.dbManager.getUserByLogin(email).then(user => user.ref); assert.deepEqual(perm.users, [ { id: await getId('Chimpy'), email: 'chimpy@getgrist.com', name: 'Chimpy', ref: await getRef('chimpy@getgrist.com'), From ad499e343776a8009fac34c25e75cc8a78bb4269 Mon Sep 17 00:00:00 2001 From: fflorent Date: Mon, 5 Aug 2024 18:23:10 +0200 Subject: [PATCH 10/22] Also force getUserByLoginWithRetry to return a Promise --- app/gen-server/lib/homedb/HomeDBManager.ts | 4 ++-- app/gen-server/lib/homedb/UsersManager.ts | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/app/gen-server/lib/homedb/HomeDBManager.ts b/app/gen-server/lib/homedb/HomeDBManager.ts index 3caa079582..bc75d39455 100644 --- a/app/gen-server/lib/homedb/HomeDBManager.ts +++ b/app/gen-server/lib/homedb/HomeDBManager.ts @@ -475,8 +475,8 @@ export class HomeDBManager extends EventEmitter { /** * @see UsersManager.prototype.getUserByLoginWithRetry */ - public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise { - return await this._usersManager.getUserByLoginWithRetry(email, options) ?? undefined; + public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise { + return await this._usersManager.getUserByLoginWithRetry(email, options); } /** diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index 5b26196089..f432e0605e 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -219,9 +219,6 @@ export class UsersManager { profile, manager }); - if (!newUser) { - throw new ApiError("Unable to create user", 500); - } // No need to survey this user. newUser.isFirstTimeUser = false; await newUser.save(); @@ -313,7 +310,7 @@ export class UsersManager { // for an email key conflict failure. This is in case our transaction conflicts with a peer // doing the same thing. This is quite likely if the first page visited by a previously // unseen user fires off multiple api calls. - public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise { + public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise { try { return this.getUserByLogin(email, options); } catch (e) { @@ -736,7 +733,7 @@ export class UsersManager { // user if a bunch of servers start simultaneously and the user doesn't exist // yet. const user = await this.getUserByLoginWithRetry(profile.email, {profile}); - if (user) { id = this._specialUserIds[profile.email] = user.id; } + id = this._specialUserIds[profile.email] = user.id; } if (!id) { throw new Error(`Could not find or create user ${profile.email}`); } return id; From 4ecb24bee5fd71ea4ba4f6a8d1f6448050be1bae Mon Sep 17 00:00:00 2001 From: fflorent Date: Tue, 6 Aug 2024 11:45:39 +0200 Subject: [PATCH 11/22] Self-remarks --- test/gen-server/lib/homedb/UsersManager.ts | 1526 ++++++++++---------- 1 file changed, 780 insertions(+), 746 deletions(-) diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 172676190c..10f5947df2 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -28,149 +28,89 @@ const username = process.env.USER || "nobody"; const tmpDir = path.join(tmpdir(), `grist_test_${username}_userendpoint`); describe('UsersManager', function () { - const NON_EXISTING_USER_ID = 10001337; this.timeout(30000); - let env: EnvironmentSnapshot; - let db: HomeDBManager; - let sandbox: SinonSandbox; - const uniqueLocalPart = new Set(); - - function getTmpDatabase(typeormDb: string) { - return getDatabase(path.join(tmpDir, typeormDb)); - } - - /** - * Works around lacks of type narrowing after asserting the value is defined. - * This is fixed in latest version of @types/chai - * FIXME: once using @types/chai > 4.3.17, remove this function - */ - function assertExists(value?: T, message?: string): asserts value is T { - assert.exists(value, message); - } - - function ensureUnique(localPart: string) { - if (uniqueLocalPart.has(localPart)) { - throw new Error('passed localPart is already used elsewhere'); + + describe('static method', function () { + function* makeIdxIterator() { + for (let i = 0; i < 500; i++) { + yield i; + } } - uniqueLocalPart.add(localPart); - return localPart; - } - - function createUniqueUser(uniqueEmailLocalPart: string) { - ensureUnique(uniqueEmailLocalPart); - return getOrCreateUser(uniqueEmailLocalPart); - } - - async function getOrCreateUser(localPart: string, options?: GetUserOptions) { - return db.getUserByLogin(makeEmail(localPart), options); - } - - function makeEmail(localPart: string) { - return localPart + '@getgrist.com'; - } - - async function getPersonalOrg(user: User) { - return db.getOrg({userId: user.id}, user.personalOrg.id); - } - - function disableLogging(method: T) { - return sandbox.stub(log, method); - } - - before(async function () { - env = new EnvironmentSnapshot(); - await prepareFilesystemDirectoryForTests(tmpDir); - await prepareDatabase(tmpDir); - db = await getDatabase(); - }); - after(async function () { - env.restore(); - }); + function makeUsers(nbUsers: number, idxIt = makeIdxIterator()) { + return Array(nbUsers).fill(null).map(() => { + const user = new User(); + const index = idxIt.next(); + if (index.done) { + throw new Error('Excessive number of users created'); + } + user.id = index.value; + user.name = `User ${index.value}`; + return user; + }); + } - beforeEach(function () { - sandbox = Sinon.createSandbox(); - }); + function makeUsersList(usersListLength: number, nbUsersByGroups: number) { + const idxIt = makeIdxIterator(); + return Array(usersListLength).fill(null).map(_ => makeUsers(nbUsersByGroups, idxIt)); + } - afterEach(function () { - sandbox.restore(); - }); + describe('getResourceUsers()', function () { + function populateResourceWithMembers(res: Resource, members: User[], groupName?: string) { + const aclRule = new AclRuleOrg(); + const group = new Group(); + if (groupName) { + group.name = groupName; + } + group.memberUsers = members; + aclRule.group = group; + res.aclRules = [ + aclRule + ]; + } + it('should return all users from a single organization ACL', function () { + const resource = new Organization(); + const users = makeUsers(5); + populateResourceWithMembers(resource, users); - function* makeIdxIterator() { - for (let i = 0; i < 500; i++) { - yield i; - } - } - - function makeUsers(nbUsers: number, idxIt = makeIdxIterator()) { - return Array(nbUsers).fill(null).map(() => { - const user = new User(); - const index = idxIt.next(); - if (index.done) { - throw new Error('Excessive number of users created'); - } - user.id = index.value; - user.name = `User ${index.value}`; - return user; - }); - } - - function makeUsersList(usersListLength: number) { - const nbUsersByGroups = 5; // arbitrary value - const idxIt = makeIdxIterator(); - return Array(usersListLength).fill(null).map(_ => makeUsers(nbUsersByGroups, idxIt)); - } - - describe('getResourceUsers()', function () { - function populateResourceWithMembers(res: Resource, members: User[], groupName?: string) { - const aclRule = new AclRuleOrg(); - const group = new Group(); - if (groupName) { - group.name = groupName; - } - group.memberUsers = members; - aclRule.group = group; - res.aclRules = [ - aclRule - ]; - } + const result = UsersManager.getResourceUsers(resource); - it('should return all users from a single organization ACL', function () { - const resource = new Organization(); - const users = makeUsers(5); - populateResourceWithMembers(resource, users); - const result = UsersManager.getResourceUsers(resource); - assert.deepEqual(result, users); - }); + assert.deepEqual(result, users); + }); - it('should return all users from all resources ACL', function () { - const resources: Resource[] = [new Organization(), new Workspace(), new Document()]; - const usersList = makeUsersList(3); - for (const [idx, resource] of resources.entries()) { - populateResourceWithMembers(resource, usersList[idx]); - } - const result = UsersManager.getResourceUsers(resources); - assert.deepEqual(result, usersList.flat()); - }); + it('should return all users from all resources ACL', function () { + const resources: Resource[] = [new Organization(), new Workspace(), new Document()]; + const usersList = makeUsersList(3, 5); + for (const [idx, resource] of resources.entries()) { + populateResourceWithMembers(resource, usersList[idx]); + } - it('should return users matching group names from all resources ACL', function () { - const resources: Resource[] = [new Organization(), new Workspace(), new Document()]; - const usersList = makeUsersList(3); - const allGroupNames = ['Grp1', 'Grp2', 'Grp3']; - const filteredGroupNames = allGroupNames.slice(1); - for (const [idx, resource] of resources.entries()) { - populateResourceWithMembers(resource, usersList[idx], allGroupNames[idx]); - } - const result = UsersManager.getResourceUsers(resources, filteredGroupNames); - assert.deepEqual(result, usersList.slice(1).flat(), 'should discard the users from the first resource'); + const result = UsersManager.getResourceUsers(resources); + + assert.deepEqual(result, usersList.flat()); + }); + + it('should return users matching group names from all resources ACL', function () { + const resources: Resource[] = [new Organization(), new Workspace(), new Document()]; + const usersList = makeUsersList(3, 5); + const allGroupNames = ['Grp1', 'Grp2', 'Grp3']; + const filteredGroupNames = [ 'Grp2', 'Grp3' ]; + for (const [idx, resource] of resources.entries()) { + populateResourceWithMembers(resource, usersList[idx], allGroupNames[idx]); + } + + const result = UsersManager.getResourceUsers(resources, filteredGroupNames); + + assert.deepEqual(result, usersList.slice(1).flat(), 'should discard the users from the first resource'); + }); }); - }); - describe('getUsersWithRole()', function () { - function makeGroups(groupDefinition: {[k in NonGuestGroup['name']]?: User[] | undefined}){ - return (Object.entries(groupDefinition) as [NonGuestGroup['name'], User[] | undefined][]) - .map(([groupName, users], index) => { + describe('getUsersWithRole()', function () { + function makeGroups(groupDefinition: {[k in NonGuestGroup['name']]?: User[] | undefined}){ + const entries = Object.entries(groupDefinition) as [NonGuestGroup['name'], User[] | undefined][]; + + return entries.map(([groupName, users], index) => { const group = new Group() as NonGuestGroup; group.id = index; group.name = groupName; @@ -179,761 +119,855 @@ describe('UsersManager', function () { } return group; }); - } + } - it('should retrieve no users if passed groups do not contain any', function () { - const groups = makeGroups({ - 'members': undefined - }); - assert.deepEqual(UsersManager.getUsersWithRole(groups), new Map([['members', undefined]] as any)); - }); + it('should retrieve no users if passed groups do not contain any', function () { + const groups = makeGroups({ + 'members': undefined + }); - it('should retrieve users of passed groups', function () { - const idxIt = makeIdxIterator(); - const groupsUsersMap = { - 'editors': makeUsers(3, idxIt), - 'owners': makeUsers(4, idxIt), - 'members': makeUsers(5, idxIt), - 'viewers': [] - }; - const groups = makeGroups(groupsUsersMap); - assert.deepEqual(UsersManager.getUsersWithRole(groups), new Map(Object.entries(groupsUsersMap))); - }); + const result = UsersManager.getUsersWithRole(groups); - it('should exclude users of given IDs', function () { - const groupUsersMap = { - 'editors': makeUsers(5), - }; - const excludedUsersId = [1, 2, 3, 4]; - const expectedUsers = [groupUsersMap.editors[0]]; - const groups = makeGroups(groupUsersMap); - - assert.deepEqual( - UsersManager.getUsersWithRole(groups, excludedUsersId), - new Map([ - ['editors', expectedUsers] - ]) - ); - }); - }); + assert.deepEqual(result, new Map([['members', undefined]] as any)); + }); - describe('Special User Ids', function () { - const ANONYMOUS_USER_ID = 6; - const PREVIEWER_USER_ID = 7; - const EVERYONE_USER_ID = 8; - const SUPPORT_USER_ID = 5; - it('getAnonymousUserId() should retrieve anonymous user id', function () { - assert.strictEqual(db.getAnonymousUserId(), ANONYMOUS_USER_ID); - }); + it('should retrieve users of passed groups', function () { + const idxIt = makeIdxIterator(); + const groupsUsersMap = { + 'editors': makeUsers(3, idxIt), + 'owners': makeUsers(4, idxIt), + 'members': makeUsers(5, idxIt), + 'viewers': [] + }; + const groups = makeGroups(groupsUsersMap); - it('getPreviewerUserId() should retrieve previewer user id', function () { - assert.strictEqual(db.getPreviewerUserId(), PREVIEWER_USER_ID); - }); + const result = UsersManager.getUsersWithRole(groups); - it("getEveryoneUserId() should retrieve 'everyone' user id", function () { - assert.strictEqual(db.getEveryoneUserId(), EVERYONE_USER_ID); - }); + assert.deepEqual(result, new Map(Object.entries(groupsUsersMap))); + }); - it("getSupportUserId() should retrieve 'support' user id", function () { - assert.strictEqual(db.getSupportUserId(), SUPPORT_USER_ID); - }); + it('should exclude users of given IDs', function () { + const groupUsersMap = { + 'editors': makeUsers(5), + }; + const excludedUsersId = [1, 2, 3, 4]; + const expectedUsers = [groupUsersMap.editors[0]]; + const groups = makeGroups(groupUsersMap); - describe('Without special id initialization', function () { - async function createDatabaseWithoutSpecialIds() { - const stub = sandbox.stub(HomeDBManager.prototype, 'initializeSpecialIds').resolves(undefined); - const localDb = await getTmpDatabase('without-special-ids.db'); - stub.restore(); - return localDb; - } + const result = UsersManager.getUsersWithRole(groups, excludedUsersId); - it('should throw an error', async function () { - const localDb = await createDatabaseWithoutSpecialIds(); - assert.throws(() => localDb.getAnonymousUserId(), "Anonymous user not available"); - assert.throws(() => localDb.getPreviewerUserId(), "Previewer user not available"); - assert.throws(() => localDb.getEveryoneUserId(), "'everyone' user not available"); - assert.throws(() => localDb.getSupportUserId(), "'support' user not available"); + assert.deepEqual( result, new Map([ ['editors', expectedUsers] ])); }); }); }); - describe('getUserByKey()', function () { - it('should return the user given their API Key', async function () { - const user = await db.getUserByKey('api_key_for_chimpy'); - assert.strictEqual(user?.name, 'Chimpy', 'should retrieve Chimpy by their API key'); - assert.strictEqual(user?.logins?.[0].email, 'chimpy@getgrist.com'); - }); + describe('class method', function () { + const NON_EXISTING_USER_ID = 10001337; + let env: EnvironmentSnapshot; + let db: HomeDBManager; + let sandbox: SinonSandbox; + const uniqueLocalPart = new Set(); - it('should return undefined if no user matches the API key', async function () { - const user = await db.getUserByKey('non-existing API key'); - assert.strictEqual(user, undefined); - }); - }); + /** + * Works around lacks of type narrowing after asserting the value is defined. + * This is fixed in latest versions of @types/chai + * + * FIXME: once upgrading @types/chai to 4.3.17 or higher, remove this function which would not be usefull anymore + */ + function assertExists(value?: T, message?: string): asserts value is T { + assert.exists(value, message); + } - describe('getUser()', async function () { + function ensureUnique(localPart: string) { + if (uniqueLocalPart.has(localPart)) { + throw new Error('passed localPart is already used elsewhere'); + } + uniqueLocalPart.add(localPart); + return localPart; + } + + function createUniqueUser(uniqueEmailLocalPart: string) { + ensureUnique(uniqueEmailLocalPart); + return getOrCreateUser(uniqueEmailLocalPart); + } - const userWithPrefsEmail = 'userwithprefs@getgrist.com'; - async function getOrCreateUserWithPrefs() { - const user = await db.getUserByLogin(userWithPrefsEmail); - if (!user) { - throw new Error('getOrCreateUserWithPrefs: User not created!'); + async function getOrCreateUser(localPart: string, options?: GetUserOptions) { + return db.getUserByLogin(makeEmail(localPart), options); + } + + function makeEmail(localPart: string) { + return localPart + '@getgrist.com'; + } + + async function getPersonalOrg(user: User) { + return db.getOrg({userId: user.id}, user.personalOrg.id); + } + + async function withDataBase(dbName: string, cb: (db: HomeDBManager) => Promise) { + const database = path.join(tmpDir, dbName + '.db'); + const localDb = new HomeDBManager(); + await localDb.createNewConnection({ name: dbName, database }); + await updateDb(localDb.connection); + try { + await cb(localDb); + } finally { + await localDb.connection.destroy(); + // HACK: This is a weird case, we have established a different connection + // but the existing connection is also impacted. + // A found workaround consist in destroying and creating again the connection. + // + // TODO: Check whether using DataSource would help and avoid this hack. + await db.connection.destroy(); + await db.createNewConnection(); } - return user; } - it('should retrieve a user by their ID', async function () { - const user = await db.getUser(db.getSupportUserId()); - assertExists(user, 'Should have returned a user'); - assert.strictEqual(user.name, 'Support'); - assert.strictEqual(user.loginEmail, SUPPORT_EMAIL); - assertExists(!user.prefs, "should not have retrieved user's prefs"); - }); + function disableLoggingLevel(method: T) { + return sandbox.stub(log, method); + } - it('should retrieve a user along with their prefs with `includePrefs` set to true', async function () { - const expectedUser = await getOrCreateUserWithPrefs(); - const user = await db.getUser(expectedUser.id, {includePrefs: true}); - assertExists(user, "Should have retrieved the user"); - assertExists(user.loginEmail); - assert.strictEqual(user.loginEmail, expectedUser.loginEmail); - assertExists(Array.isArray(user.prefs), "should not have retrieved user's prefs"); - assert.deepEqual(user.prefs, [{ - userId: expectedUser.id, - orgId: expectedUser.personalOrg.id, - prefs: { showGristTour: true } as any - }]); + before(async function () { + env = new EnvironmentSnapshot(); + await prepareFilesystemDirectoryForTests(tmpDir); + await prepareDatabase(tmpDir); + db = await getDatabase(); }); - }); - describe('getFullUser()', function () { - it('should return the support user', async function () { - const supportId = db.getSupportUserId(); - const user = await db.getFullUser(supportId); - const expectedResult: FullUser = { - isSupport: true, - email: SUPPORT_EMAIL, - id: supportId, - name: 'Support' - }; - assert.deepInclude(user, expectedResult); - assert.ok(!user.anonymous, 'anonymous property should be falsy'); + after(async function () { + env.restore(); }); - it('should return the anonymous user', async function () { - const anonId = db.getAnonymousUserId(); - const user = await db.getFullUser(anonId); - const expectedResult: FullUser = { - anonymous: true, - email: ANONYMOUS_USER_EMAIL, - id: anonId, - name: 'Anonymous', - }; - assert.deepInclude(user, expectedResult); - assert.ok(!user.isSupport, 'support property should be falsy'); + beforeEach(function () { + sandbox = Sinon.createSandbox(); }); - it('should throw when user is not found', async function () { - await assert.isRejected(db.getFullUser(NON_EXISTING_USER_ID), "unable to find user"); + afterEach(function () { + sandbox.restore(); }); - }); - describe('makeFullUser()', function () { - const someUserDisplayEmail = 'SomeUser@getgrist.com'; - const normalizedSomeUserEmail = 'someuser@getgrist.com'; - const someUserLocale = 'en-US'; - const SOME_USER_ID = 42; - const prefWithOrg: Pref = { - prefs: {placeholder: 'pref-with-org'}, - orgId: 43, - user: new User(), - userId: SOME_USER_ID, - }; - const prefWithoutOrg: Pref = { - prefs: {placeholder: 'pref-without-org'}, - orgId: null, - user: new User(), - userId: SOME_USER_ID - }; - - - function makeSomeUser() { - return User.create({ - id: SOME_USER_ID, - ref: 'some ref', - name: 'some user', - picture: 'https://grist.com/mypic', - options: { - locale: someUserLocale - }, - logins: [ - Login.create({ - userId: SOME_USER_ID, - email: normalizedSomeUserEmail, - displayEmail: someUserDisplayEmail, - }), - ], - prefs: [ - prefWithOrg, - prefWithoutOrg - ] + describe('Special User Ids', function () { + const ANONYMOUS_USER_ID = 6; + const PREVIEWER_USER_ID = 7; + const EVERYONE_USER_ID = 8; + const SUPPORT_USER_ID = 5; + it('getAnonymousUserId() should retrieve anonymous user id', function () { + assert.strictEqual(db.getAnonymousUserId(), ANONYMOUS_USER_ID); }); - } - it('creates a FullUser from a User entity', function () { - const input = makeSomeUser(); - const fullUser = db.makeFullUser(input); - assert.deepEqual(fullUser, { - id: SOME_USER_ID, - email: someUserDisplayEmail, - loginEmail: normalizedSomeUserEmail, - name: input.name, - picture: input.picture, - ref: input.ref, - locale: someUserLocale, - prefs: prefWithoutOrg.prefs + it('getPreviewerUserId() should retrieve previewer user id', function () { + assert.strictEqual(db.getPreviewerUserId(), PREVIEWER_USER_ID); + }); + + it("getEveryoneUserId() should retrieve 'everyone' user id", function () { + assert.strictEqual(db.getEveryoneUserId(), EVERYONE_USER_ID); }); - }); - it('sets `anonymous` property to true for anon@getgrist.com', function () { - const anon = db.getAnonymousUser(); - const fullUser = db.makeFullUser(anon); - assert.isTrue(fullUser.anonymous, "`anonymous` property should be set to true"); - assert.ok(!fullUser.isSupport, "`isSupport` should be falsy"); + it("getSupportUserId() should retrieve 'support' user id", function () { + assert.strictEqual(db.getSupportUserId(), SUPPORT_USER_ID); + }); + + describe('Without special id initialization', function () { + it('should throw an error', async function () { + await withDataBase('without-special-ids', async function (localDb) { + assert.throws(() => localDb.getAnonymousUserId(), "Anonymous user not available"); + assert.throws(() => localDb.getPreviewerUserId(), "Previewer user not available"); + assert.throws(() => localDb.getEveryoneUserId(), "'everyone' user not available"); + assert.throws(() => localDb.getSupportUserId(), "'support' user not available"); + }); + }); + }); }); - it('sets `isSupport` property to true for support account', async function () { - const support = await db.getUser(db.getSupportUserId()); - const fullUser = db.makeFullUser(support!); - assert.isTrue(fullUser.isSupport, "`isSupport` property should be set to true"); - assert.ok(!fullUser.anonymous, "`anonymouse` should be falsy"); + describe('getUserByKey()', function () { + it('should return the user given their API Key', async function () { + const user = await db.getUserByKey('api_key_for_chimpy'); + + assert.strictEqual(user?.name, 'Chimpy', 'should retrieve Chimpy by their API key'); + assert.strictEqual(user?.logins?.[0].email, 'chimpy@getgrist.com'); + }); + + it('should return undefined if no user matches the API key', async function () { + const user = await db.getUserByKey('non-existing API key'); + + assert.strictEqual(user, undefined); + }); }); - it('should throw when no displayEmail exist for this user', function () { - const input = makeSomeUser(); - input.logins[0].displayEmail = ''; - assert.throws(() => db.makeFullUser(input), "unable to find mandatory user email"); - input.logins = []; - assert.throws(() => db.makeFullUser(input), "unable to find mandatory user email"); + describe('getUser()', async function () { + it('should retrieve a user by their ID', async function () { + const user = await db.getUser(db.getSupportUserId()); + assertExists(user, 'Should have returned a user'); + assert.strictEqual(user.name, 'Support'); + assert.strictEqual(user.loginEmail, SUPPORT_EMAIL); + assertExists(!user.prefs, "should not have retrieved user's prefs"); + }); + + it('should retrieve a user along with their prefs with `includePrefs` set to true', async function () { + const expectedUser = await createUniqueUser('getuser-userwithprefs'); + const user = await db.getUser(expectedUser.id, {includePrefs: true}); + assertExists(user, "Should have retrieved the user"); + assertExists(user.loginEmail); + assert.strictEqual(user.loginEmail, expectedUser.loginEmail); + assertExists(Array.isArray(user.prefs), "should not have retrieved user's prefs"); + assert.deepEqual(user.prefs, [{ + userId: expectedUser.id, + orgId: expectedUser.personalOrg.id, + prefs: { showGristTour: true } as any + }]); + }); }); - }); - describe('ensureExternalUser()', function () { - let managerSaveSpy: SinonSpy; - let userSaveSpy: SinonSpy; + describe('getFullUser()', function () { + it('should return the support user', async function () { + const supportId = db.getSupportUserId(); - beforeEach(function () { - managerSaveSpy = sandbox.spy(EntityManager.prototype, 'save'); - userSaveSpy = sandbox.spy(User.prototype, 'save'); + const user = await db.getFullUser(supportId); + + const expectedResult: FullUser = { + isSupport: true, + email: SUPPORT_EMAIL, + id: supportId, + name: 'Support' + }; + assert.deepInclude(user, expectedResult); + assert.ok(!user.anonymous, 'anonymous property should be falsy'); + }); + + it('should return the anonymous user', async function () { + const anonId = db.getAnonymousUserId(); + const user = await db.getFullUser(anonId); + + const expectedResult: FullUser = { + anonymous: true, + email: ANONYMOUS_USER_EMAIL, + id: anonId, + name: 'Anonymous', + }; + assert.deepInclude(user, expectedResult); + assert.ok(!user.isSupport, 'support property should be falsy'); + }); + + it('should throw when user is not found', async function () { + await assert.isRejected(db.getFullUser(NON_EXISTING_USER_ID), "unable to find user"); + }); }); - /** - * Make a user profile. - * @param localPart A unique local part of the email (also used for the other fields). - */ - function makeProfile(localPart: string): UserProfile { - ensureUnique(localPart); - return { - email: makeEmail(localPart), - name: `NewUser ${localPart}`, - connectId: `ConnectId-${localPart}`, - picture: `https://mypic.com/${localPart}.png` + describe('makeFullUser()', function () { + const someUserDisplayEmail = 'SomeUser@getgrist.com'; + const normalizedSomeUserEmail = 'someuser@getgrist.com'; + const someUserLocale = 'en-US'; + const SOME_USER_ID = 42; + const prefWithOrg: Pref = { + prefs: {placeholder: 'pref-with-org'}, + orgId: 43, + user: new User(), + userId: SOME_USER_ID, + }; + const prefWithoutOrg: Pref = { + prefs: {placeholder: 'pref-without-org'}, + orgId: null, + user: new User(), + userId: SOME_USER_ID }; - } - async function checkUserInfo(profile: UserProfile) { - const user = await db.getExistingUserByLogin(profile.email); - assertExists(user, "the new user should be in database"); - assert.deepInclude(user, { - isFirstTimeUser: false, - name: profile.name, - picture: profile.picture + + function makeSomeUser() { + return User.create({ + id: SOME_USER_ID, + ref: 'some ref', + name: 'some user', + picture: 'https://grist.com/mypic', + options: { + locale: someUserLocale + }, + logins: [ + Login.create({ + userId: SOME_USER_ID, + email: normalizedSomeUserEmail, + displayEmail: someUserDisplayEmail, + }), + ], + prefs: [ + prefWithOrg, + prefWithoutOrg + ] + }); + } + + it('creates a FullUser from a User entity', function () { + const input = makeSomeUser(); + + const fullUser = db.makeFullUser(input); + + assert.deepEqual(fullUser, { + id: SOME_USER_ID, + email: someUserDisplayEmail, + loginEmail: normalizedSomeUserEmail, + name: input.name, + picture: input.picture, + ref: input.ref, + locale: someUserLocale, + prefs: prefWithoutOrg.prefs + }); }); - assert.exists(user.logins?.[0]); - assert.deepInclude(user.logins[0], { - email: profile.email.toLowerCase(), - displayEmail: profile.email + + it('sets `anonymous` property to true for anon@getgrist.com', function () { + const anon = db.getAnonymousUser(); + + const fullUser = db.makeFullUser(anon); + + assert.isTrue(fullUser.anonymous, "`anonymous` property should be set to true"); + assert.ok(!fullUser.isSupport, "`isSupport` should be falsy"); }); - } - it('should not do anything if the user already exists and is up to date', async function () { - await db.ensureExternalUser({ - name: 'Chimpy', - email: 'chimpy@getgrist.com', + it('sets `isSupport` property to true for support account', async function () { + const support = await db.getUser(db.getSupportUserId()); + + const fullUser = db.makeFullUser(support!); + + assert.isTrue(fullUser.isSupport, "`isSupport` property should be set to true"); + assert.ok(!fullUser.anonymous, "`anonymouse` should be falsy"); }); - assert.isFalse(userSaveSpy.called, 'user.save() should not have been called'); - assert.isFalse(managerSaveSpy.called, 'manager.save() should not have been called'); - }); - it('should save an unknown user', async function () { - const profile = makeProfile('ensureExternalUser-saves-an-unknown-user'); - await db.ensureExternalUser(profile); - assert.isTrue(userSaveSpy.called, 'user.save() should have been called'); - assert.isTrue(managerSaveSpy.called, 'manager.save() should have been called'); + it('should throw when no displayEmail exist for this user', function () { + const input = makeSomeUser(); + input.logins[0].displayEmail = ''; + + assert.throws(() => db.makeFullUser(input), "unable to find mandatory user email"); - await checkUserInfo(profile); + input.logins = []; + assert.throws(() => db.makeFullUser(input), "unable to find mandatory user email"); + }); }); - it('should update a user if they already exist in database', async function () { - const oldProfile = makeProfile('ensureexternaluser-updates-an-existing-user_old'); - await db.ensureExternalUser(oldProfile); + describe('ensureExternalUser()', function () { + let managerSaveSpy: SinonSpy; + let userSaveSpy: SinonSpy; - let oldUser = await db.getExistingUserByLogin(oldProfile.email); - assertExists(oldUser); + beforeEach(function () { + managerSaveSpy = sandbox.spy(EntityManager.prototype, 'save'); + userSaveSpy = sandbox.spy(User.prototype, 'save'); + }); - const newProfile = { - ...makeProfile('ensureexternaluser-updates-an-existing-user_new'), - connectId: oldProfile.connectId, - }; + /** + * Make a user profile. + * @param localPart A unique local part of the email (also used for the other fields). + */ + function makeProfile(localPart: string): UserProfile { + ensureUnique(localPart); + return { + email: makeEmail(localPart), + name: `NewUser ${localPart}`, + connectId: `ConnectId-${localPart}`, + picture: `https://mypic.com/${localPart}.png` + }; + } - await db.ensureExternalUser(newProfile); - oldUser = await db.getExistingUserByLogin(oldProfile.email); - assert.notExists(oldUser, 'we should not retrieve the user given their old email address'); + async function checkUserInfo(profile: UserProfile) { + const user = await db.getExistingUserByLogin(profile.email); + assertExists(user, "the new user should be in database"); + assert.deepInclude(user, { + isFirstTimeUser: false, + name: profile.name, + picture: profile.picture + }); + assert.exists(user.logins?.[0]); + assert.deepInclude(user.logins[0], { + email: profile.email.toLowerCase(), + displayEmail: profile.email + }); + return user; + } - await checkUserInfo(newProfile); - }); + it('should not do anything if the user already exists and is up to date', async function () { + await db.ensureExternalUser({ + name: 'Chimpy', + email: 'chimpy@getgrist.com', + }); - it('should normalize email address', async function() { - const profile = makeProfile('ENSUREEXTERNALUSER-NORMALIZES-email-address'); - await db.ensureExternalUser(profile); - await checkUserInfo(profile); - }); - }); + assert.isFalse(userSaveSpy.called, 'user.save() should not have been called'); + assert.isFalse(managerSaveSpy.called, 'manager.save() should not have been called'); + }); - describe('updateUser()', function () { - let emitSpy: SinonSpy; + it('should save an unknown user', async function () { + const profile = makeProfile('ensureExternalUser-saves-an-unknown-user'); + await db.ensureExternalUser(profile); + assert.isTrue(userSaveSpy.called, 'user.save() should have been called'); + assert.isTrue(managerSaveSpy.called, 'manager.save() should have been called'); - before(function () { - emitSpy = Sinon.spy(); - db.on('firstLogin', emitSpy); - }); + await checkUserInfo(profile); + }); - after(function () { - db.off('firstLogin', emitSpy); - }); + it('should update a user if they already exist in database', async function () { + const oldProfile = makeProfile('ensureexternaluser-updates-an-existing-user_old'); - afterEach(function () { - emitSpy.resetHistory(); - }); + await db.ensureExternalUser(oldProfile); - function checkNoEventEmitted() { - assert.equal(emitSpy.callCount, 0, 'No event should have been emitted'); - } + let oldUser = await db.getExistingUserByLogin(oldProfile.email); + assertExists(oldUser); - it('should reject when user is not found', async function () { - disableLogging('debug'); - const promise = db.updateUser(NON_EXISTING_USER_ID, {name: 'foobar'}); - await assert.isRejected(promise, 'unable to find user'); - }); + const newProfile = { + ...makeProfile('ensureexternaluser-updates-an-existing-user_new'), + connectId: oldProfile.connectId, + }; - it('should update a user name', async function () { - const emailLocalPart = 'updateUser-should-update-user-name'; - const createdUser = await createUniqueUser(emailLocalPart); - assert.equal(createdUser.name, ''); - const userName = 'user name'; - await db.updateUser(createdUser.id, {name: userName}); - checkNoEventEmitted(); - const updatedUser = await getOrCreateUser(emailLocalPart); - assert.equal(updatedUser.name, userName); - }); + await db.ensureExternalUser(newProfile); + + oldUser = await db.getExistingUserByLogin(oldProfile.email); + assert.notExists(oldUser, 'we should not retrieve the user given their old email address'); + + await checkUserInfo(newProfile); + }); + + it('should normalize email address', async function() { + const profile = makeProfile('ENSUREEXTERNALUSER-NORMALIZES-email-address'); - it('should not emit any event when isFirstTimeUser value has not changed', async function () { - const localPart = 'updateuser-should-not-emit-when-isfirsttimeuser-not-changed'; - const createdUser = await createUniqueUser(localPart); - assert.equal(createdUser.isFirstTimeUser, true); + await db.ensureExternalUser(profile); - await db.updateUser(createdUser.id, {isFirstTimeUser: true}); - checkNoEventEmitted(); + const user = await checkUserInfo(profile); + assert.equal(user.logins[0].email, profile.email.toLowerCase(), 'the email should be lowercase'); + assert.equal(user.logins[0].displayEmail, profile.email, 'the display email should keep the original case'); + }); }); - it('should emit "firstLogin" event when isFirstTimeUser value has been toggled to false', async function () { - const localPart = 'updateuser-emits-firstlogin'; - const userName = 'user name'; - const newUser = await createUniqueUser(localPart); - assert.equal(newUser.isFirstTimeUser, true); + describe('updateUser()', function () { + let emitSpy: SinonSpy; - await db.updateUser(newUser.id, {isFirstTimeUser: false, name: userName}); - assert.equal(emitSpy.callCount, 1, '"firstLogin" event should have been emitted'); + before(function () { + emitSpy = Sinon.spy(); + db.on('firstLogin', emitSpy); + }); - const fullUserFromEvent = emitSpy.firstCall.args[0]; - assertExists(fullUserFromEvent, 'a FullUser object should be passed with the "firstLogin" event'); - assert.equal(fullUserFromEvent.name, userName); - assert.equal(fullUserFromEvent.email, makeEmail(localPart)); + after(function () { + db.off('firstLogin', emitSpy); + }); - const updatedUser = await getOrCreateUser(localPart); - assert.equal(updatedUser.isFirstTimeUser, false); - }); - }); + afterEach(function () { + emitSpy.resetHistory(); + }); - describe('updateUserOptions()', function () { - it('should reject when user is not found', async function () { - disableLogging('debug'); - const promise = db.updateUserOptions(NON_EXISTING_USER_ID, {}); - await assert.isRejected(promise, 'unable to find user'); - }); + function checkNoEventEmitted() { + assert.equal(emitSpy.callCount, 0, 'No event should have been emitted'); + } - it('should update user options', async function () { - const localPart = 'updateuseroptions-updates-user-options'; - const createdUser = await createUniqueUser(localPart); + it('should reject when user is not found', async function () { + disableLoggingLevel('debug'); - assert.notExists(createdUser.options); + const promise = db.updateUser(NON_EXISTING_USER_ID, {name: 'foobar'}); - const options: UserOptions = {locale: 'fr', authSubject: 'subject', isConsultant: true, allowGoogleLogin: true}; - await db.updateUserOptions(createdUser.id, options); + await assert.isRejected(promise, 'unable to find user'); + checkNoEventEmitted(); + }); - const updatedUser = await getOrCreateUser(localPart); - assertExists(updatedUser); - assertExists(updatedUser.options); - assert.deepEqual(updatedUser.options, options); - }); - }); + it('should update a user name', async function () { + const emailLocalPart = 'updateUser-should-update-user-name'; + const createdUser = await createUniqueUser(emailLocalPart); + assert.equal(createdUser.name, ''); + const userName = 'user name'; - describe('getExistingUserByLogin()', function () { - it('should return an existing user', async function () { - const localPart = 'getexistinguserbylogin-returns-existing-user'; - const createdUser = await createUniqueUser(localPart); + await db.updateUser(createdUser.id, {name: userName}); - const retrievedUser = await db.getExistingUserByLogin(createdUser.loginEmail!); - assertExists(retrievedUser); + checkNoEventEmitted(); + const updatedUser = await getOrCreateUser(emailLocalPart); + assert.equal(updatedUser.name, userName); + }); - assert.equal(retrievedUser.id, createdUser.id); - assert.equal(retrievedUser.name, createdUser.name); - }); + it('should not emit any event when isFirstTimeUser value has not changed', async function () { + const localPart = 'updateuser-should-not-emit-when-isfirsttimeuser-not-changed'; + const createdUser = await createUniqueUser(localPart); + assert.equal(createdUser.isFirstTimeUser, true); - it('should normalize the passed user email', async function () { - const localPart = 'getexistinguserbylogin-normalizes-user-email'; - const newUser = await createUniqueUser(localPart); + await db.updateUser(createdUser.id, {isFirstTimeUser: true}); - const retrievedUser = await db.getExistingUserByLogin(newUser.loginEmail!.toUpperCase()); - assertExists(retrievedUser); - }); + checkNoEventEmitted(); + }); - it('should return undefined when the user is not found', async function () { - const nonExistingEmail = 'i-dont-exist@getgrist.com'; - const retrievedUser = await db.getExistingUserByLogin(nonExistingEmail); - assert.isUndefined(retrievedUser); - }); - }); + it('should emit "firstLogin" event when isFirstTimeUser value has been toggled to false', async function () { + const localPart = 'updateuser-emits-firstlogin'; + const userName = 'user name'; + const newUser = await createUniqueUser(localPart); + assert.equal(newUser.isFirstTimeUser, true); - describe('getUserByLogin()', function () { - it('should create a user when none exist with the corresponding email', async function () { - const localPart = ensureUnique('getuserbylogin-creates-user-when-not-already-exists'); - const email = makeEmail(localPart); - sandbox.useFakeTimers(42_000); - assert.notExists(await db.getExistingUserByLogin(email)); - - const user = await db.getUserByLogin(makeEmail(localPart.toUpperCase())); - - assert.isTrue(user.isFirstTimeUser, 'should be marked as first time user'); - assert.equal(user.loginEmail, email); - assert.equal(user.logins[0].displayEmail, makeEmail(localPart.toUpperCase())); - assert.equal(user.name, ''); - // FIXME: why is user.lastConnectionAt actually a string and not a Date? - // FIXME: why is user.lastConnectionAt updated here and ont firstLoginAt? - assert.equal(String(user.lastConnectionAt), '1970-01-01 00:00:42.000'); + await db.updateUser(newUser.id, {isFirstTimeUser: false, name: userName}); + assert.equal(emitSpy.callCount, 1, '"firstLogin" event should have been emitted'); + + const fullUserFromEvent = emitSpy.firstCall.args[0]; + assertExists(fullUserFromEvent, 'a FullUser object should be passed with the "firstLogin" event'); + assert.equal(fullUserFromEvent.name, userName); + assert.equal(fullUserFromEvent.email, makeEmail(localPart)); + + const updatedUser = await getOrCreateUser(localPart); + assert.equal(updatedUser.isFirstTimeUser, false, 'the user is not considered as being first time user anymore'); + }); }); - it('should create a personnal organization for the new user', async function () { - const localPart = ensureUnique('getuserbylogin-creates-personnal-org'); + describe('updateUserOptions()', function () { + it('should reject when user is not found', async function () { + disableLoggingLevel('debug'); - const user = await db.getUserByLogin(makeEmail(localPart)); + const promise = db.updateUserOptions(NON_EXISTING_USER_ID, {}); - const org = await getPersonalOrg(user); - assertExists(org.data, 'should have retrieved personnal org data'); - assert.equal(org.data.name, 'Personal'); - }); + await assert.isRejected(promise, 'unable to find user'); + }); - it('should not create organizations for non-login emails', async function () { - const user = await db.getUserByLogin(EVERYONE_EMAIL); - assert.notOk(user.personalOrg); - }); + it('should update user options', async function () { + const localPart = 'updateuseroptions-updates-user-options'; + const createdUser = await createUniqueUser(localPart); - it('should not update user information when no profile is passed', async function () { - const localPart = ensureUnique('getuserbylogin-does-not-update-without-profile'); - const userFirstCall = await db.getUserByLogin(makeEmail(localPart)); - const userSecondCall = await db.getUserByLogin(makeEmail(localPart)); - assert.deepEqual(userFirstCall, userSecondCall); - }); + assert.notExists(createdUser.options); - // FIXME: why using Sinon.useFakeTimers() makes user.lastConnectionAt a string instead of a Date - it.skip('should update lastConnectionAt only for different days', async function () { - const fakeTimer = sandbox.useFakeTimers(0); - const localPart = ensureUnique('getuserbylogin-updates-last_connection_at-for-different-days'); - let user = await db.getUserByLogin(makeEmail(localPart)); - const epochDateTime = '1970-01-01 00:00:00.000'; - assert.equal(String(user.lastConnectionAt), epochDateTime); - - await fakeTimer.tickAsync(42_000); - user = await db.getUserByLogin(makeEmail(localPart)); - assert.equal(String(user.lastConnectionAt), epochDateTime); - - await fakeTimer.tickAsync('1d'); - user = await db.getUserByLogin(makeEmail(localPart)); - assert.match(String(user.lastConnectionAt), /^1970-01-02/); + const options: UserOptions = {locale: 'fr', authSubject: 'subject', isConsultant: true, allowGoogleLogin: true}; + await db.updateUserOptions(createdUser.id, options); + + const updatedUser = await getOrCreateUser(localPart); + assertExists(updatedUser); + assertExists(updatedUser.options); + assert.deepEqual(updatedUser.options, options); + }); }); - describe('when passing information to update (using `profile`)', function () { - const makeProfile = (newEmail: string): UserProfile => ({ - name: 'my user name', - email: newEmail, - picture: 'https://mypic.com/foo.png', - connectId: 'new-connect-id', + describe('getExistingUserByLogin()', function () { + it('should return an existing user', async function () { + const localPart = 'getexistinguserbylogin-returns-existing-user'; + const createdUser = await createUniqueUser(localPart); + + const retrievedUser = await db.getExistingUserByLogin(createdUser.loginEmail!); + assertExists(retrievedUser); + + assert.equal(retrievedUser.id, createdUser.id); + assert.equal(retrievedUser.name, createdUser.name); }); - it('should populate the firstTimeLogin and deduce the name from the email', async function () { + it('should normalize the passed user email', async function () { + const localPart = 'getexistinguserbylogin-normalizes-user-email'; + const newUser = await createUniqueUser(localPart); + + const retrievedUser = await db.getExistingUserByLogin(newUser.loginEmail!.toUpperCase()); + + assertExists(retrievedUser); + }); + + it('should return undefined when the user is not found', async function () { + const nonExistingEmail = 'i-dont-exist@getgrist.com'; + + const retrievedUser = await db.getExistingUserByLogin(nonExistingEmail); + + assert.isUndefined(retrievedUser); + }); + }); + + describe('getUserByLogin()', function () { + it('should create a user when none exist with the corresponding email', async function () { + const localPart = ensureUnique('getuserbylogin-creates-user-when-not-already-exists'); + const email = makeEmail(localPart); sandbox.useFakeTimers(42_000); - const localPart = ensureUnique('getuserbylogin-with-profile-populates-first_time_login-and-name'); - const user = await db.getUserByLogin(makeEmail(localPart), {profile: {name: '', email: makeEmail(localPart)}}); - assert.equal(user.name, localPart); - // FIXME: why using Sinon.useFakeTimers() makes user.firstLoginAt a string instead of a Date - assert.equal(String(user.firstLoginAt), '1970-01-01 00:00:42.000'); + assert.notExists(await db.getExistingUserByLogin(email)); + + const user = await db.getUserByLogin(makeEmail(localPart.toUpperCase())); + + assert.isTrue(user.isFirstTimeUser, 'should be marked as first time user'); + assert.equal(user.loginEmail, email); + assert.equal(user.logins[0].displayEmail, makeEmail(localPart.toUpperCase())); + assert.equal(user.name, ''); + // FIXME: why is user.lastConnectionAt actually a string and not a Date? + // FIXME: is it consistent that user.lastConnectionAt updated here and not firstLoginAt? + assert.equal(String(user.lastConnectionAt), '1970-01-01 00:00:42.000'); }); - it('should populate user with any passed information', async function () { - const localPart = ensureUnique('getuserbylogin-with-profile-populates-user-with-passed-info_OLD'); - await db.getUserByLogin(makeEmail(localPart)); + it('should create a personnal organization for the new user', async function () { + const localPart = ensureUnique('getuserbylogin-creates-personnal-org'); - const profile = makeProfile(makeEmail('getuserbylogin-with-profile-populates-user-with-passed-info_NEW')); - const userOptions: UserOptions = {authSubject: 'my-auth-subject'}; + const user = await db.getUserByLogin(makeEmail(localPart)); - const updatedUser = await db.getUserByLogin(makeEmail(localPart), { profile, userOptions }); - assert.deepInclude(updatedUser, { - name: profile.name, - connectId: profile.connectId, - picture: profile.picture, + const org = await getPersonalOrg(user); + assertExists(org.data, 'should have retrieved personnal org data'); + assert.equal(org.data.name, 'Personal'); + }); + + it('should not create organizations for non-login emails', async function () { + const user = await db.getUserByLogin(EVERYONE_EMAIL); + assert.notExists(user.personalOrg); + }); + + it('should not update user information when no profile is passed', async function () { + const localPart = ensureUnique('getuserbylogin-does-not-update-without-profile'); + + const userFirstCall = await db.getUserByLogin(makeEmail(localPart)); + const userSecondCall = await db.getUserByLogin(makeEmail(localPart)); + + assert.deepEqual(userFirstCall, userSecondCall); + }); + + // FIXME: why using Sinon.useFakeTimers() makes user.lastConnectionAt a string instead of a Date + it.skip('should update lastConnectionAt only for different days', async function () { + const fakeTimer = sandbox.useFakeTimers(0); + const localPart = ensureUnique('getuserbylogin-updates-last_connection_at-for-different-days'); + let user = await db.getUserByLogin(makeEmail(localPart)); + const epochDateTime = '1970-01-01 00:00:00.000'; + assert.equal(String(user.lastConnectionAt), epochDateTime); + + await fakeTimer.tickAsync(42_000); + user = await db.getUserByLogin(makeEmail(localPart)); + assert.equal(String(user.lastConnectionAt), epochDateTime); + + await fakeTimer.tickAsync('1d'); + user = await db.getUserByLogin(makeEmail(localPart)); + assert.match(String(user.lastConnectionAt), /^1970-01-02/); + }); + + describe('when passing information to update (using `profile`)', function () { + const makeProfile = (newEmail: string): UserProfile => ({ + name: 'my user name', + email: newEmail, + picture: 'https://mypic.com/foo.png', + connectId: 'new-connect-id', }); - assert.deepInclude(updatedUser.logins[0], { - displayEmail: profile.email, + + it('should populate the firstTimeLogin and deduce the name from the email', async function () { + sandbox.useFakeTimers(42_000); + const localPart = ensureUnique('getuserbylogin-with-profile-populates-first_time_login-and-name'); + const user = await db.getUserByLogin(makeEmail(localPart), { + profile: {name: '', email: makeEmail(localPart)} + }); + assert.equal(user.name, localPart); + // FIXME: why using Sinon.useFakeTimers() makes user.firstLoginAt a string instead of a Date + assert.equal(String(user.firstLoginAt), '1970-01-01 00:00:42.000'); }); - assert.deepInclude(updatedUser.options, { - authSubject: userOptions.authSubject, + + it('should populate user with any passed information', async function () { + const localPart = ensureUnique('getuserbylogin-with-profile-populates-user-with-passed-info_OLD'); + await db.getUserByLogin(makeEmail(localPart)); + + const profile = makeProfile(makeEmail('getuserbylogin-with-profile-populates-user-with-passed-info_NEW')); + const userOptions: UserOptions = {authSubject: 'my-auth-subject'}; + + const updatedUser = await db.getUserByLogin(makeEmail(localPart), { profile, userOptions }); + assert.deepInclude(updatedUser, { + name: profile.name, + connectId: profile.connectId, + picture: profile.picture, + }); + assert.deepInclude(updatedUser.logins[0], { + displayEmail: profile.email, + }); + assert.deepInclude(updatedUser.options, { + authSubject: userOptions.authSubject, + }); }); }); }); - }); - describe('getUserByLoginWithRetry()', async function () { - async function ensureGetUserByLoginWithRetryWorks(localPart: string) { - const email = makeEmail(localPart); - const user = await db.getUserByLoginWithRetry(email); - assertExists(user); - assert.equal(user.loginEmail, email); - } + describe('getUserByLoginWithRetry()', async function () { + async function ensureGetUserByLoginWithRetryWorks(localPart: string) { + const email = makeEmail(localPart); + const user = await db.getUserByLoginWithRetry(email); + assertExists(user); + assert.equal(user.loginEmail, email); + } - function makeQueryFailedError() { - const error = new Error() as any; - error.name = 'QueryFailedError'; - error.detail = 'Key (email)=whatever@getgrist.com already exists'; - return error; - } + function makeQueryFailedError() { + const error = new Error() as any; + error.name = 'QueryFailedError'; + error.detail = 'Key (email)=whatever@getgrist.com already exists'; + return error; + } - it('should work just like getUserByLogin', async function () { - await ensureGetUserByLoginWithRetryWorks( ensureUnique('getuserbyloginwithretry-works-like-getuserbylogin')); - }); + it('should work just like getUserByLogin', async function () { + await ensureGetUserByLoginWithRetryWorks( ensureUnique('getuserbyloginwithretry-works-like-getuserbylogin')); + }); - it('should make a second attempt on special error', async function () { - sandbox.stub(UsersManager.prototype, 'getUserByLogin') - .onFirstCall().throws(makeQueryFailedError()) - .callThrough(); - await ensureGetUserByLoginWithRetryWorks( 'getuserbyloginwithretry-makes-a-single-retry'); - }); + it('should make a second attempt on special error', async function () { + sandbox.stub(UsersManager.prototype, 'getUserByLogin') + .onFirstCall().throws(makeQueryFailedError()) + .callThrough(); + await ensureGetUserByLoginWithRetryWorks( 'getuserbyloginwithretry-makes-a-single-retry'); + }); - it('should reject after 2 attempts', async function () { - const secondError = makeQueryFailedError(); - sandbox.stub(UsersManager.prototype, 'getUserByLogin') - .onFirstCall().throws(makeQueryFailedError()) - .onSecondCall().throws(secondError) - .callThrough(); - - const email = makeEmail(ensureUnique('getuserbyloginwithretry-rejects-after-2-attempts')); - const promise = db.getUserByLoginWithRetry(email); - await assert.isRejected(promise); - await promise.catch(err => assert.equal(err, secondError)); - }); + it('should reject after 2 attempts', async function () { + const secondError = makeQueryFailedError(); + sandbox.stub(UsersManager.prototype, 'getUserByLogin') + .onFirstCall().throws(makeQueryFailedError()) + .onSecondCall().throws(secondError) + .callThrough(); + + const email = makeEmail(ensureUnique('getuserbyloginwithretry-rejects-after-2-attempts')); + const promise = db.getUserByLoginWithRetry(email); + await assert.isRejected(promise); + await promise.catch(err => assert.equal(err, secondError)); + }); - it('should reject immediately if the error is not a QueryFailedError', async function () { - const errorMsg = 'my error'; - sandbox.stub(UsersManager.prototype, 'getUserByLogin') - .rejects(new Error(errorMsg)) - .callThrough(); + it('should reject immediately if the error is not a QueryFailedError', async function () { + const errorMsg = 'my error'; + sandbox.stub(UsersManager.prototype, 'getUserByLogin') + .rejects(new Error(errorMsg)) + .callThrough(); - const email = makeEmail(ensureUnique('getuserbyloginwithretry-rejects-immediately-when-not-queryfailederror')); - const promise = db.getUserByLoginWithRetry(email); - await assert.isRejected(promise, errorMsg); + const email = makeEmail(ensureUnique('getuserbyloginwithretry-rejects-immediately-when-not-queryfailederror')); + const promise = db.getUserByLoginWithRetry(email); + await assert.isRejected(promise, errorMsg); + }); }); - }); - - describe('deleteUser()', function () { - function userHasPrefs(userId: number, manager: EntityManager) { - return manager.exists(Pref, { where: { userId: userId }}); - } - function userHasGroupUsers(userId: number, manager: EntityManager) { - return manager.exists('group_users', { where: {user_id: userId} }); - } + describe('deleteUser()', function () { + function userHasPrefs(userId: number, manager: EntityManager) { + return manager.exists(Pref, { where: { userId: userId }}); + } - it('should refuse to delete the account of someone else', async function () { - const localPart = ensureUnique('deleteuser-refuses-for-someone-else'); - const userToDelete = await getOrCreateUser(localPart); + function userHasGroupUsers(userId: number, manager: EntityManager) { + return manager.exists('group_users', { where: {user_id: userId} }); + } - const promise = db.deleteUser({userId: 2}, userToDelete.id); - await assert.isRejected(promise, 'not permitted to delete this user'); - }); + it('should refuse to delete the account of someone else', async function () { + const localPart = ensureUnique('deleteuser-refuses-for-someone-else'); + const userToDelete = await getOrCreateUser(localPart); - it('should refuse to delete a non existing account', async function () { - disableLogging('debug'); - const promise = db.deleteUser({userId: NON_EXISTING_USER_ID}, NON_EXISTING_USER_ID); - await assert.isRejected(promise, 'user not found'); - }); + const promise = db.deleteUser({userId: 2}, userToDelete.id); - it('should refuse to delete the account if the passed name is not matching', async function () { - disableLogging('debug'); - const localPart = ensureUnique('deleteuser-refuses-if-name-not-matching'); - const userToDelete = await getOrCreateUser(localPart, { - profile: { - name: 'someone to delete', - email: makeEmail(localPart), - } + await assert.isRejected(promise, 'not permitted to delete this user'); }); - const promise = db.deleteUser({userId: userToDelete.id}, userToDelete.id, 'wrong name'); + it('should refuse to delete a non existing account', async function () { + disableLoggingLevel('debug'); - await assert.isRejected(promise); - await promise.catch(e => assert.match(e.message, /user name did not match/)); - }); + const promise = db.deleteUser({userId: NON_EXISTING_USER_ID}, NON_EXISTING_USER_ID); - it('should remove the user and cleanup their info and personal organization', async function () { - const localPart = ensureUnique('deleteuser-removes-user-and-cleanups-info'); - const userToDelete = await getOrCreateUser(localPart, { - profile: { - name: 'someone to delete', - email: makeEmail(localPart), - } + await assert.isRejected(promise, 'user not found'); }); - assertExists(await getPersonalOrg(userToDelete)); + it('should refuse to delete the account if the passed name is not matching', async function () { + disableLoggingLevel('debug'); + const localPart = ensureUnique('deleteuser-refuses-if-name-not-matching'); + const userToDelete = await getOrCreateUser(localPart, { + profile: { + name: 'someone to delete', + email: makeEmail(localPart), + } + }); + + const promise = db.deleteUser({userId: userToDelete.id}, userToDelete.id, 'wrong name'); - await db.connection.transaction(async (manager) => { - assert.isTrue(await userHasGroupUsers(userToDelete.id, manager)); - assert.isTrue(await userHasPrefs(userToDelete.id, manager)); + await assert.isRejected(promise); + await promise.catch(e => assert.match(e.message, /user name did not match/)); }); - await db.deleteUser({userId: userToDelete.id}, userToDelete.id); + it('should remove the user and cleanup their info and personal organization', async function () { + const localPart = ensureUnique('deleteuser-removes-user-and-cleanups-info'); + const userToDelete = await getOrCreateUser(localPart, { + profile: { + name: 'someone to delete', + email: makeEmail(localPart), + } + }); - assert.notExists(await db.getUser(userToDelete.id)); - assert.deepEqual(await getPersonalOrg(userToDelete), { errMessage: 'organization not found', status: 404 }); + assertExists(await getPersonalOrg(userToDelete)); - await db.connection.transaction(async (manager) => { - assert.isFalse(await userHasGroupUsers(userToDelete.id, manager)); - assert.isFalse(await userHasPrefs(userToDelete.id, manager)); - }); - }); + await db.connection.transaction(async (manager) => { + assert.isTrue(await userHasGroupUsers(userToDelete.id, manager)); + assert.isTrue(await userHasPrefs(userToDelete.id, manager)); + }); - it("should remove the user when passed name corresponds to the user's name", async function () { - const localPart = ensureUnique('deleteuser-removes-user-when-name-matches'); - const userName = 'someone to delete'; - const userToDelete = await getOrCreateUser(localPart, { - profile: { - name: userName, - email: makeEmail(localPart), - } + await db.deleteUser({userId: userToDelete.id}, userToDelete.id); + + assert.notExists(await db.getUser(userToDelete.id)); + assert.deepEqual(await getPersonalOrg(userToDelete), { errMessage: 'organization not found', status: 404 }); + + await db.connection.transaction(async (manager) => { + assert.isFalse(await userHasGroupUsers(userToDelete.id, manager)); + assert.isFalse(await userHasPrefs(userToDelete.id, manager)); + }); }); - const promise = db.deleteUser({userId: userToDelete.id}, userToDelete.id, userName); - await assert.isFulfilled(promise); - }); - }); - describe('initializeSpecialIds()', function () { - async function withDataBase(dbName: string, cb: (db: HomeDBManager) => Promise) { - // await prepareDatabase(tmpDir, dbName); - const database = path.join(tmpDir, dbName); - const localDb = new HomeDBManager(); - await localDb.createNewConnection({ name: 'special-id', database }); - await updateDb(localDb.connection); - try { - await cb(localDb); - } finally { - await localDb.connection.destroy(); - } - } + it("should remove the user when passed name corresponds to the user's name", async function () { + const localPart = ensureUnique('deleteuser-removes-user-when-name-matches'); + const userName = 'someone to delete'; + const userToDelete = await getOrCreateUser(localPart, { + profile: { + name: userName, + email: makeEmail(localPart), + } + }); - after(async function () { - // HACK: This is a weird case, we have established a different connection - // but the existing connection is also impacted. - // A found workaround consist in destroying and creating again the connection. - // - // TODO: Check whether using DataSource would help and avoid this hack. - await db.connection.destroy(); - await db.createNewConnection(); - }); + const promise = db.deleteUser({userId: userToDelete.id}, userToDelete.id, userName); - it('should initialize special ids', async function () { - return withDataBase('test-special-ids.db', async (localDb) => { - const specialAccounts = [ - {name: "Support", email: SUPPORT_EMAIL}, - {name: "Anonymous", email: ANONYMOUS_USER_EMAIL}, - {name: "Preview", email: PREVIEWER_EMAIL}, - {name: "Everyone", email: EVERYONE_EMAIL} - ]; - for (const {email} of specialAccounts) { - assert.notExists(await localDb.getExistingUserByLogin(email)); - } - await localDb.initializeSpecialIds(); - for (const {name, email} of specialAccounts) { - const res = await localDb.getExistingUserByLogin(email); - assertExists(res); - assert.equal(res.name, name); - } + await assert.isFulfilled(promise); }); }); - }); - describe('completeProfiles()', function () { - it('should return an empty array if no profiles are provided', async function () { - const res = await db.completeProfiles([]); - assert.deepEqual(res, []); - }); + describe('initializeSpecialIds()', function () { + it('should initialize special ids', async function () { + return withDataBase('test-special-ids', async (localDb) => { + const specialAccounts = [ + {name: "Support", email: SUPPORT_EMAIL}, + {name: "Anonymous", email: ANONYMOUS_USER_EMAIL}, + {name: "Preview", email: PREVIEWER_EMAIL}, + {name: "Everyone", email: EVERYONE_EMAIL} + ]; + for (const {email} of specialAccounts) { + assert.notExists(await localDb.getExistingUserByLogin(email)); + } - it("should complete a single user profile with looking by normalized address", async function () { - const localPart = ensureUnique('completeprofiles-with-single-profile'); - const email = makeEmail(localPart); - const profile = { - name: 'complete-profile-email-normalized-user', - email: makeEmail(localPart), - picture: 'https://mypic.com/me.png', - }; - const someLocale = 'fr-FR'; - const userCreated = await getOrCreateUser(localPart, { profile }); - await db.updateUserOptions(userCreated.id, {locale: someLocale}); - - const res = await db.completeProfiles([{name: 'whatever', email: email.toUpperCase()}]); - - assert.deepEqual(res, [{ - ...profile, - id: userCreated.id, - locale: someLocale, - anonymous: false - }]); + await localDb.initializeSpecialIds(); + + for (const {name, email} of specialAccounts) { + const res = await localDb.getExistingUserByLogin(email); + assertExists(res); + assert.equal(res.name, name); + } + }); + }); }); - it('should complete several user profiles', async function () { - const localPartPrefix = ensureUnique('completeprofiles-with-several-profiles'); - const seq = Array(10).fill(null).map((_, i) => i+1); - const localParts = seq.map(i => `${localPartPrefix}_${i}`); - const usersCreated = await Promise.all( - localParts.map(localPart => getOrCreateUser(localPart)) - ); - - const res = await db.completeProfiles( - localParts.map( - localPart => ({name: 'whatever', email: makeEmail(localPart)}) - ) - ); - assert.lengthOf(res, localParts.length); - for (const [index, localPart] of localParts.entries()) { - assert.deepInclude(res[index], { - id: usersCreated[index].id, + describe('completeProfiles()', function () { + it('should return an empty array if no profiles are provided', async function () { + const res = await db.completeProfiles([]); + assert.deepEqual(res, []); + }); + + it("should complete a single user profile with looking by normalized address", async function () { + const localPart = ensureUnique('completeprofiles-with-single-profile'); + const email = makeEmail(localPart); + const emailUpperCase = email.toUpperCase(); + const profile = { + name: 'completeprofiles-with-single-profile-username', email: makeEmail(localPart), - }); - } + picture: 'https://mypic.com/me.png', + }; + const someLocale = 'fr-FR'; + const userCreated = await getOrCreateUser(localPart, { profile }); + await db.updateUserOptions(userCreated.id, {locale: someLocale}); + + const res = await db.completeProfiles([{name: 'whatever', email: emailUpperCase}]); + + assert.deepEqual(res, [{ + ...profile, + id: userCreated.id, + locale: someLocale, + anonymous: false + }]); + }); + + it('should complete several user profiles', async function () { + const localPartPrefix = ensureUnique('completeprofiles-with-several-profiles'); + const seq = Array(10).fill(null).map((_, i) => i+1); + const localParts = seq.map(i => `${localPartPrefix}_${i}`); + const usersCreated = await Promise.all( + localParts.map(localPart => getOrCreateUser(localPart)) + ); + + const res = await db.completeProfiles( + localParts.map( + localPart => ({name: 'whatever', email: makeEmail(localPart)}) + ) + ); + assert.lengthOf(res, localParts.length); + for (const [index, localPart] of localParts.entries()) { + assert.deepInclude(res[index], { + id: usersCreated[index].id, + email: makeEmail(localPart), + }); + } + }); }); }); }); From d7af3af04b188917ab633db0e0c27a59c97b7dd3 Mon Sep 17 00:00:00 2001 From: Florent Date: Mon, 5 Aug 2024 21:39:52 +0200 Subject: [PATCH 12/22] Fix test failure for getUserByLoginWithRetry --- app/gen-server/lib/homedb/HomeDBManager.ts | 4 ++-- app/gen-server/lib/homedb/UsersManager.ts | 2 +- test/gen-server/lib/homedb/UsersManager.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/gen-server/lib/homedb/HomeDBManager.ts b/app/gen-server/lib/homedb/HomeDBManager.ts index bc75d39455..967affd6f2 100644 --- a/app/gen-server/lib/homedb/HomeDBManager.ts +++ b/app/gen-server/lib/homedb/HomeDBManager.ts @@ -476,14 +476,14 @@ export class HomeDBManager extends EventEmitter { * @see UsersManager.prototype.getUserByLoginWithRetry */ public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise { - return await this._usersManager.getUserByLoginWithRetry(email, options); + return this._usersManager.getUserByLoginWithRetry(email, options); } /** * @see UsersManager.prototype.getUserByLogin */ public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise { - return await this._usersManager.getUserByLogin(email, options); + return this._usersManager.getUserByLogin(email, options); } /** diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index f432e0605e..fcd44eb0ab 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -312,7 +312,7 @@ export class UsersManager { // unseen user fires off multiple api calls. public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise { try { - return this.getUserByLogin(email, options); + return await this.getUserByLogin(email, options); } catch (e) { if (e.name === 'QueryFailedError' && e.detail && e.detail.match(/Key \(email\)=[^ ]+ already exists/)) { diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 10f5947df2..1d8bc5646a 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -800,7 +800,7 @@ describe('UsersManager', function () { it('should reject immediately if the error is not a QueryFailedError', async function () { const errorMsg = 'my error'; sandbox.stub(UsersManager.prototype, 'getUserByLogin') - .rejects(new Error(errorMsg)) + .onFirstCall().rejects(new Error(errorMsg)) .callThrough(); const email = makeEmail(ensureUnique('getuserbyloginwithretry-rejects-immediately-when-not-queryfailederror')); From cd688669e06da96f3aa02550437f37cf5d66fdae Mon Sep 17 00:00:00 2001 From: Florent Date: Tue, 6 Aug 2024 19:17:40 +0200 Subject: [PATCH 13/22] Apply suggestions from code review Mostly styling issues --- app/gen-server/lib/homedb/UsersManager.ts | 8 ++-- test/gen-server/lib/homedb/UsersManager.ts | 47 +++++++++++----------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index fcd44eb0ab..041d14dac4 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -114,7 +114,7 @@ export class UsersManager { */ public getAnonymousUserId(): number { const id = this._specialUserIds[ANONYMOUS_USER_EMAIL]; - if (!id) { throw new Error("Anonymous user not available"); } + if (!id) { throw new Error("'Anonymous' user not available"); } return id; } @@ -123,7 +123,7 @@ export class UsersManager { */ public getPreviewerUserId(): number { const id = this._specialUserIds[PREVIEWER_EMAIL]; - if (!id) { throw new Error("Previewer user not available"); } + if (!id) { throw new Error("'Previewer' user not available"); } return id; } @@ -132,7 +132,7 @@ export class UsersManager { */ public getEveryoneUserId(): number { const id = this._specialUserIds[EVERYONE_EMAIL]; - if (!id) { throw new Error("'everyone' user not available"); } + if (!id) { throw new Error("'Everyone' user not available"); } return id; } @@ -141,7 +141,7 @@ export class UsersManager { */ public getSupportUserId(): number { const id = this._specialUserIds[SUPPORT_EMAIL]; - if (!id) { throw new Error("'support' user not available"); } + if (!id) { throw new Error("'Support' user not available"); } return id; } diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 1d8bc5646a..ce68ce18f3 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -156,7 +156,7 @@ describe('UsersManager', function () { const result = UsersManager.getUsersWithRole(groups, excludedUsersId); - assert.deepEqual( result, new Map([ ['editors', expectedUsers] ])); + assert.deepEqual(result, new Map([ ['editors', expectedUsers] ])); }); }); }); @@ -186,9 +186,9 @@ describe('UsersManager', function () { return localPart; } - function createUniqueUser(uniqueEmailLocalPart: string) { + function createUniqueUser(uniqueEmailLocalPart: string, options?: GetUserOptions) { ensureUnique(uniqueEmailLocalPart); - return getOrCreateUser(uniqueEmailLocalPart); + return getOrCreateUser(uniqueEmailLocalPart, options); } async function getOrCreateUser(localPart: string, options?: GetUserOptions) { @@ -269,10 +269,10 @@ describe('UsersManager', function () { describe('Without special id initialization', function () { it('should throw an error', async function () { await withDataBase('without-special-ids', async function (localDb) { - assert.throws(() => localDb.getAnonymousUserId(), "Anonymous user not available"); - assert.throws(() => localDb.getPreviewerUserId(), "Previewer user not available"); - assert.throws(() => localDb.getEveryoneUserId(), "'everyone' user not available"); - assert.throws(() => localDb.getSupportUserId(), "'support' user not available"); + assert.throws(() => localDb.getAnonymousUserId(), "'Anonymous' user not available"); + assert.throws(() => localDb.getPreviewerUserId(), "'Previewer' user not available"); + assert.throws(() => localDb.getEveryoneUserId(), "'Everyone' user not available"); + assert.throws(() => localDb.getSupportUserId(), "'Support' user not available"); }); }); }); @@ -299,7 +299,7 @@ describe('UsersManager', function () { assertExists(user, 'Should have returned a user'); assert.strictEqual(user.name, 'Support'); assert.strictEqual(user.loginEmail, SUPPORT_EMAIL); - assertExists(!user.prefs, "should not have retrieved user's prefs"); + assert.notExists(user.prefs, "should not have retrieved user's prefs"); }); it('should retrieve a user along with their prefs with `includePrefs` set to true', async function () { @@ -308,7 +308,7 @@ describe('UsersManager', function () { assertExists(user, "Should have retrieved the user"); assertExists(user.loginEmail); assert.strictEqual(user.loginEmail, expectedUser.loginEmail); - assertExists(Array.isArray(user.prefs), "should not have retrieved user's prefs"); + assert.isTrue(Array.isArray(user.prefs), "should not have retrieved user's prefs"); assert.deepEqual(user.prefs, [{ userId: expectedUser.id, orgId: expectedUser.personalOrg.id, @@ -330,7 +330,7 @@ describe('UsersManager', function () { name: 'Support' }; assert.deepInclude(user, expectedResult); - assert.ok(!user.anonymous, 'anonymous property should be falsy'); + assert.notOk(user.anonymous, 'anonymous property should be falsy'); }); it('should return the anonymous user', async function () { @@ -345,10 +345,10 @@ describe('UsersManager', function () { name: 'Anonymous', }; assert.deepInclude(user, expectedResult); - assert.ok(!user.isSupport, 'support property should be falsy'); + assert.notOk(user.isSupport, 'support property should be falsy'); }); - it('should throw when user is not found', async function () { + it('should reject when user is not found', async function () { await assert.isRejected(db.getFullUser(NON_EXISTING_USER_ID), "unable to find user"); }); }); @@ -418,7 +418,7 @@ describe('UsersManager', function () { const fullUser = db.makeFullUser(anon); assert.isTrue(fullUser.anonymous, "`anonymous` property should be set to true"); - assert.ok(!fullUser.isSupport, "`isSupport` should be falsy"); + assert.notOk(fullUser.isSupport, "`isSupport` should be falsy"); }); it('sets `isSupport` property to true for support account', async function () { @@ -427,7 +427,7 @@ describe('UsersManager', function () { const fullUser = db.makeFullUser(support!); assert.isTrue(fullUser.isSupport, "`isSupport` property should be set to true"); - assert.ok(!fullUser.anonymous, "`anonymouse` should be falsy"); + assert.notOk(fullUser.anonymous, "`anonymouse` should be falsy"); }); it('should throw when no displayEmail exist for this user', function () { @@ -621,7 +621,6 @@ describe('UsersManager', function () { await db.updateUserOptions(createdUser.id, options); const updatedUser = await getOrCreateUser(localPart); - assertExists(updatedUser); assertExists(updatedUser.options); assert.deepEqual(updatedUser.options, options); }); @@ -781,7 +780,7 @@ describe('UsersManager', function () { sandbox.stub(UsersManager.prototype, 'getUserByLogin') .onFirstCall().throws(makeQueryFailedError()) .callThrough(); - await ensureGetUserByLoginWithRetryWorks( 'getuserbyloginwithretry-makes-a-single-retry'); + await ensureGetUserByLoginWithRetryWorks('getuserbyloginwithretry-makes-a-single-retry'); }); it('should reject after 2 attempts', async function () { @@ -819,8 +818,7 @@ describe('UsersManager', function () { } it('should refuse to delete the account of someone else', async function () { - const localPart = ensureUnique('deleteuser-refuses-for-someone-else'); - const userToDelete = await getOrCreateUser(localPart); + const userToDelete = await createUniqueUser('deleteuser-refuses-for-someone-else'); const promise = db.deleteUser({userId: 2}, userToDelete.id); @@ -837,8 +835,9 @@ describe('UsersManager', function () { it('should refuse to delete the account if the passed name is not matching', async function () { disableLoggingLevel('debug'); - const localPart = ensureUnique('deleteuser-refuses-if-name-not-matching'); - const userToDelete = await getOrCreateUser(localPart, { + const localPart = 'deleteuser-refuses-if-name-not-matching'; + + const userToDelete = await createUniqueUser(localPart, { profile: { name: 'someone to delete', email: makeEmail(localPart), @@ -852,8 +851,8 @@ describe('UsersManager', function () { }); it('should remove the user and cleanup their info and personal organization', async function () { - const localPart = ensureUnique('deleteuser-removes-user-and-cleanups-info'); - const userToDelete = await getOrCreateUser(localPart, { + const localPart = 'deleteuser-removes-user-and-cleanups-info'; + const userToDelete = await createUniqueUser(localPart, { profile: { name: 'someone to delete', email: makeEmail(localPart), @@ -879,9 +878,9 @@ describe('UsersManager', function () { }); it("should remove the user when passed name corresponds to the user's name", async function () { - const localPart = ensureUnique('deleteuser-removes-user-when-name-matches'); const userName = 'someone to delete'; - const userToDelete = await getOrCreateUser(localPart, { + const localPart = 'deleteuser-removes-user-when-name-matches'; + const userToDelete = await createUniqueUser(localPart, { profile: { name: userName, email: makeEmail(localPart), From 705a34f6144d5442d8c5c850e76747669e96d81d Mon Sep 17 00:00:00 2001 From: fflorent Date: Tue, 6 Aug 2024 19:47:04 +0200 Subject: [PATCH 14/22] More remarks taken into account --- app/gen-server/lib/homedb/UsersManager.ts | 6 +- test/gen-server/lib/homedb/UsersManager.ts | 66 +++++++++++++++------- 2 files changed, 49 insertions(+), 23 deletions(-) diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index 041d14dac4..331439582f 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -97,8 +97,10 @@ export class UsersManager { public async testClearUserPrefs(emails: string[]) { return await this._connection.transaction(async manager => { for (const email of emails) { - const user = await this.getUserByLogin(email, {manager}); - await manager.delete(Pref, {userId: user.id}); + const user = await this.getExistingUserByLogin(email, manager); + if (user) { + await manager.delete(Pref, {userId: user.id}); + } } }); } diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index ce68ce18f3..ec1f5e17de 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -91,6 +91,24 @@ describe('UsersManager', function () { assert.deepEqual(result, usersList.flat()); }); + it('should deduplicate the results', function () { + const resources: Resource[] = [new Organization(), new Workspace()]; + const usersList = makeUsersList(2, 1); + const expectedResult = usersList.flat(); + + const duplicateUser = new User(); + duplicateUser.id = usersList[1][0].id; + usersList[0].unshift(duplicateUser); + + for (const [idx, resource] of resources.entries()) { + populateResourceWithMembers(resource, usersList[idx]); + } + + const result = UsersManager.getResourceUsers(resources); + + assert.deepEqual(result, expectedResult); + }); + it('should return users matching group names from all resources ACL', function () { const resources: Resource[] = [new Organization(), new Workspace(), new Document()]; const usersList = makeUsersList(3, 5); @@ -226,6 +244,21 @@ describe('UsersManager', function () { return sandbox.stub(log, method); } + /** + * Make a user profile. + * @param localPart A unique local part of the email (also used for the other fields). + */ + function makeProfile(localPart: string): UserProfile { + ensureUnique(localPart); + return { + email: makeEmail(localPart), + name: `NewUser ${localPart}`, + connectId: `ConnectId-${localPart}`, + picture: `https://mypic.com/${localPart}.png` + }; + } + + before(async function () { env = new EnvironmentSnapshot(); await prepareFilesystemDirectoryForTests(tmpDir); @@ -315,6 +348,10 @@ describe('UsersManager', function () { prefs: { showGristTour: true } as any }]); }); + + it('should return undefined when the id is not found', async function () { + assert.isUndefined(await db.getUser(NON_EXISTING_USER_ID)); + }); }); describe('getFullUser()', function () { @@ -450,20 +487,6 @@ describe('UsersManager', function () { userSaveSpy = sandbox.spy(User.prototype, 'save'); }); - /** - * Make a user profile. - * @param localPart A unique local part of the email (also used for the other fields). - */ - function makeProfile(localPart: string): UserProfile { - ensureUnique(localPart); - return { - email: makeEmail(localPart), - name: `NewUser ${localPart}`, - connectId: `ConnectId-${localPart}`, - picture: `https://mypic.com/${localPart}.png` - }; - } - async function checkUserInfo(profile: UserProfile) { const user = await db.getExistingUserByLogin(profile.email); assertExists(user, "the new user should be in database"); @@ -716,13 +739,6 @@ describe('UsersManager', function () { }); describe('when passing information to update (using `profile`)', function () { - const makeProfile = (newEmail: string): UserProfile => ({ - name: 'my user name', - email: newEmail, - picture: 'https://mypic.com/foo.png', - connectId: 'new-connect-id', - }); - it('should populate the firstTimeLogin and deduce the name from the email', async function () { sandbox.useFakeTimers(42_000); const localPart = ensureUnique('getuserbylogin-with-profile-populates-first_time_login-and-name'); @@ -737,6 +753,7 @@ describe('UsersManager', function () { it('should populate user with any passed information', async function () { const localPart = ensureUnique('getuserbylogin-with-profile-populates-user-with-passed-info_OLD'); await db.getUserByLogin(makeEmail(localPart)); + const originalNormalizedLoginEmail = makeEmail(localPart.toLowerCase()); const profile = makeProfile(makeEmail('getuserbylogin-with-profile-populates-user-with-passed-info_NEW')); const userOptions: UserOptions = {authSubject: 'my-auth-subject'}; @@ -749,6 +766,7 @@ describe('UsersManager', function () { }); assert.deepInclude(updatedUser.logins[0], { displayEmail: profile.email, + email: originalNormalizedLoginEmail, }); assert.deepInclude(updatedUser.options, { authSubject: userOptions.authSubject, @@ -817,12 +835,17 @@ describe('UsersManager', function () { return manager.exists('group_users', { where: {user_id: userId} }); } + async function assertUserStillExistsInDb(userId: number) { + assert.exists(await db.getUser(userId)); + } + it('should refuse to delete the account of someone else', async function () { const userToDelete = await createUniqueUser('deleteuser-refuses-for-someone-else'); const promise = db.deleteUser({userId: 2}, userToDelete.id); await assert.isRejected(promise, 'not permitted to delete this user'); + await assertUserStillExistsInDb(userToDelete.id); }); it('should refuse to delete a non existing account', async function () { @@ -848,6 +871,7 @@ describe('UsersManager', function () { await assert.isRejected(promise); await promise.catch(e => assert.match(e.message, /user name did not match/)); + await assertUserStillExistsInDb(userToDelete.id); }); it('should remove the user and cleanup their info and personal organization', async function () { From a5fd31444012a4c27c783ec4de4ce1531f5234d3 Mon Sep 17 00:00:00 2001 From: fflorent Date: Fri, 16 Aug 2024 15:18:26 +0200 Subject: [PATCH 15/22] @berhalak remarks and rework of the static methods tests --- app/gen-server/lib/homedb/UsersManager.ts | 4 +- app/server/companion.ts | 4 +- test/gen-server/lib/homedb/UsersManager.ts | 111 ++++++++++++--------- 3 files changed, 68 insertions(+), 51 deletions(-) diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index 331439582f..a219fa2384 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -321,7 +321,7 @@ export class UsersManager { // This is a postgres-specific error message. This problem cannot arise in sqlite, // because we have to serialize sqlite transactions in any case to get around a typeorm // limitation. - return this.getUserByLogin(email, options); + return await this.getUserByLogin(email, options); } throw e; } @@ -355,7 +355,7 @@ export class UsersManager { public async getUserByLogin(email: string, options: GetUserOptions = {}) { const {manager: transaction, profile, userOptions} = options; const normalizedEmail = normalizeEmail(email); - return this._runInTransaction(transaction, async manager => { + return await this._runInTransaction(transaction, async manager => { let needUpdate = false; const userQuery = manager.createQueryBuilder() .select('user') diff --git a/app/server/companion.ts b/app/server/companion.ts index cafc0ed545..ab454cb5a4 100644 --- a/app/server/companion.ts +++ b/app/server/companion.ts @@ -3,7 +3,7 @@ import { version } from 'app/common/version'; import { synchronizeProducts } from 'app/gen-server/entity/Product'; import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; import { applyPatch } from 'app/gen-server/lib/TypeORMPatches'; -import { createNewConnection, getMigrations, getTypeORMSettings, +import { getMigrations, getOrCreateConnection, getTypeORMSettings, undoLastMigration, updateDb } from 'app/server/lib/dbUtils'; import { getDatabaseUrl } from 'app/server/lib/serverUtils'; import { getTelemetryPrefs } from 'app/server/lib/Telemetry'; @@ -190,7 +190,7 @@ export function addDbCommand(program: commander.Command, if (!process.env.TYPEORM_LOGGING) { process.env.TYPEORM_LOGGING = 'true'; } - const connection = reuseConnection || await createNewConnection(); + const connection = reuseConnection || await getOrCreateConnection(); const exitCode = await op(connection); if (exitCode !== 0) { program.error('db command failed', {exitCode}); diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index ec1f5e17de..aff0b762fc 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -28,99 +28,122 @@ const username = process.env.USER || "nobody"; const tmpDir = path.join(tmpdir(), `grist_test_${username}_userendpoint`); describe('UsersManager', function () { - this.timeout(30000); + this.timeout('30s'); describe('static method', function () { + /** + * Create a simple iterator of integer starting from 0 which is incremented every time we call next() + */ function* makeIdxIterator() { for (let i = 0; i < 500; i++) { yield i; } } - function makeUsers(nbUsers: number, idxIt = makeIdxIterator()) { + /** + * Create a table of users. + * @param nbUsers The number of users to create + * @param [userIdIterator=makeIdxIterator()] An iterator used to create users' id, + * which keep track of the increment accross calls. + * Pass your own iterator if you want to call this methods several times and keep the id unique. + * If omitted, create its own iterator that starts from 0. + */ + function makeUsers(nbUsers: number, userIdIterator = makeIdxIterator()): User[] { return Array(nbUsers).fill(null).map(() => { const user = new User(); - const index = idxIt.next(); - if (index.done) { + const itItem = userIdIterator.next(); + if (itItem.done) { throw new Error('Excessive number of users created'); } - user.id = index.value; - user.name = `User ${index.value}`; + user.id = itItem.value; + user.name = `User ${itItem.value}`; return user; }); } - function makeUsersList(usersListLength: number, nbUsersByGroups: number) { - const idxIt = makeIdxIterator(); - return Array(usersListLength).fill(null).map(_ => makeUsers(nbUsersByGroups, idxIt)); - } - - describe('getResourceUsers()', function () { - function populateResourceWithMembers(res: Resource, members: User[], groupName?: string) { + /** + * Populate passed resources with members + * @param resources The Resources + * @param nbUsersByResource The number of users to create for each resources (each one is unique) + * @returns The Resources and their respective members + */ + function populateResourcesWithMembers( + resources: Resource[], nbUsersByResource: number, makeResourceGrpName?: (idx: number) => string + ): Map { + const membersByResource = new Map(); + const idxIterator = makeIdxIterator(); + for (const [idx, resource] of resources.entries()) { const aclRule = new AclRuleOrg(); const group = new Group(); - if (groupName) { - group.name = groupName; + if (makeResourceGrpName) { + group.name = makeResourceGrpName(idx); } + const members = makeUsers(nbUsersByResource, idxIterator); group.memberUsers = members; aclRule.group = group; - res.aclRules = [ + resource.aclRules = [ aclRule ]; + membersByResource.set(resource, members); } + return membersByResource; + } + + /** + * Populate a resource with members and return the members + */ + function populateSingleResourceWithMembers(resource: Resource, nbUsers: number) { + const membersByResource = populateResourcesWithMembers([resource], nbUsers); + return membersByResource.get(resource)!; + } + describe('getResourceUsers()', function () { it('should return all users from a single organization ACL', function () { const resource = new Organization(); - const users = makeUsers(5); - populateResourceWithMembers(resource, users); + const expectedUsers = populateSingleResourceWithMembers(resource, 5); const result = UsersManager.getResourceUsers(resource); - assert.deepEqual(result, users); + assert.deepEqual(result, expectedUsers); }); it('should return all users from all resources ACL', function () { const resources: Resource[] = [new Organization(), new Workspace(), new Document()]; - const usersList = makeUsersList(3, 5); - for (const [idx, resource] of resources.entries()) { - populateResourceWithMembers(resource, usersList[idx]); - } + const membersByResource = populateResourcesWithMembers(resources, 5); const result = UsersManager.getResourceUsers(resources); - assert.deepEqual(result, usersList.flat()); + assert.deepEqual(result, [...membersByResource.values()].flat()); }); it('should deduplicate the results', function () { const resources: Resource[] = [new Organization(), new Workspace()]; - const usersList = makeUsersList(2, 1); + const membersByResource = populateResourcesWithMembers(resources, 1); + const usersList = [...membersByResource.values()]; const expectedResult = usersList.flat(); const duplicateUser = new User(); duplicateUser.id = usersList[1][0].id; usersList[0].unshift(duplicateUser); - for (const [idx, resource] of resources.entries()) { - populateResourceWithMembers(resource, usersList[idx]); - } - const result = UsersManager.getResourceUsers(resources); assert.deepEqual(result, expectedResult); }); it('should return users matching group names from all resources ACL', function () { - const resources: Resource[] = [new Organization(), new Workspace(), new Document()]; - const usersList = makeUsersList(3, 5); - const allGroupNames = ['Grp1', 'Grp2', 'Grp3']; - const filteredGroupNames = [ 'Grp2', 'Grp3' ]; - for (const [idx, resource] of resources.entries()) { - populateResourceWithMembers(resource, usersList[idx], allGroupNames[idx]); - } + const someOrg = new Organization(); + const someWorkspace = new Workspace(); + const someDoc = new Document(); + const resources: Resource[] = [someOrg, someWorkspace, someDoc]; + const allGroupNames = ['OrgGrp', 'WorkspaceGrp', 'DocGrp']; + const membersByResource = populateResourcesWithMembers(resources, 5, i => allGroupNames[i]); + const filteredGroupNames = [ 'WorkspaceGrp', 'DocGrp' ]; const result = UsersManager.getResourceUsers(resources, filteredGroupNames); - assert.deepEqual(result, usersList.slice(1).flat(), 'should discard the users from the first resource'); + const expectedResult = [...membersByResource.get(someWorkspace)!, ...membersByResource.get(someDoc)!]; + assert.deepEqual(result, expectedResult, 'should discard the users from the first resource'); }); }); @@ -651,21 +674,15 @@ describe('UsersManager', function () { describe('getExistingUserByLogin()', function () { it('should return an existing user', async function () { - const localPart = 'getexistinguserbylogin-returns-existing-user'; - const createdUser = await createUniqueUser(localPart); - - const retrievedUser = await db.getExistingUserByLogin(createdUser.loginEmail!); + const retrievedUser = await db.getExistingUserByLogin(PREVIEWER_EMAIL); assertExists(retrievedUser); - assert.equal(retrievedUser.id, createdUser.id); - assert.equal(retrievedUser.name, createdUser.name); + assert.equal(retrievedUser.id, db.getPreviewerUserId()); + assert.equal(retrievedUser.name, 'Preview'); }); it('should normalize the passed user email', async function () { - const localPart = 'getexistinguserbylogin-normalizes-user-email'; - const newUser = await createUniqueUser(localPart); - - const retrievedUser = await db.getExistingUserByLogin(newUser.loginEmail!.toUpperCase()); + const retrievedUser = await db.getExistingUserByLogin(PREVIEWER_EMAIL.toUpperCase()); assertExists(retrievedUser); }); From 647667695b5388140090be9ee6c053b7eccf6053 Mon Sep 17 00:00:00 2001 From: fflorent Date: Fri, 16 Aug 2024 15:53:26 +0200 Subject: [PATCH 16/22] Rename generator helper function to be more accurate --- test/gen-server/lib/homedb/UsersManager.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index aff0b762fc..2d73bc89cb 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -34,7 +34,7 @@ describe('UsersManager', function () { /** * Create a simple iterator of integer starting from 0 which is incremented every time we call next() */ - function* makeIdxIterator() { + function* makeUserIdIterator() { for (let i = 0; i < 500; i++) { yield i; } @@ -48,7 +48,7 @@ describe('UsersManager', function () { * Pass your own iterator if you want to call this methods several times and keep the id unique. * If omitted, create its own iterator that starts from 0. */ - function makeUsers(nbUsers: number, userIdIterator = makeIdxIterator()): User[] { + function makeUsers(nbUsers: number, userIdIterator = makeUserIdIterator()): User[] { return Array(nbUsers).fill(null).map(() => { const user = new User(); const itItem = userIdIterator.next(); @@ -71,7 +71,7 @@ describe('UsersManager', function () { resources: Resource[], nbUsersByResource: number, makeResourceGrpName?: (idx: number) => string ): Map { const membersByResource = new Map(); - const idxIterator = makeIdxIterator(); + const idxIterator = makeUserIdIterator(); for (const [idx, resource] of resources.entries()) { const aclRule = new AclRuleOrg(); const group = new Group(); @@ -173,7 +173,7 @@ describe('UsersManager', function () { }); it('should retrieve users of passed groups', function () { - const idxIt = makeIdxIterator(); + const idxIt = makeUserIdIterator(); const groupsUsersMap = { 'editors': makeUsers(3, idxIt), 'owners': makeUsers(4, idxIt), From 8ebe47485bd9e9cccf7425c67eb251dbbe032fed Mon Sep 17 00:00:00 2001 From: fflorent Date: Fri, 16 Aug 2024 16:10:33 +0200 Subject: [PATCH 17/22] Make a temp dir with random suffix the tests To address @berhalak comment: > I'm just worry about running this tests twice (simultaneously) on the same machine, most of our tests (if not all of them) can be run simultaneously without a problem. --- test/gen-server/lib/homedb/UsersManager.ts | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 2d73bc89cb..1b7d0bcc11 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -5,9 +5,9 @@ import { Organization } from 'app/gen-server/entity/Organization'; import { SUPPORT_EMAIL, UsersManager } from 'app/gen-server/lib/homedb/UsersManager'; import { GetUserOptions, NonGuestGroup, Resource } from 'app/gen-server/lib/homedb/Interfaces'; import { tmpdir } from 'os'; +import fs from 'node:fs/promises'; import path from 'path'; import { prepareDatabase } from 'test/server/lib/helpers/PrepareDatabase'; -import { prepareFilesystemDirectoryForTests } from 'test/server/lib/helpers/PrepareFilesystemDirectoryForTests'; import { EnvironmentSnapshot } from 'test/server/testUtils'; import { getDatabase } from 'test/testUtils'; import { Workspace } from 'app/gen-server/entity/Workspace'; @@ -23,12 +23,26 @@ import { EntityManager } from 'typeorm'; import log from 'app/server/lib/log'; import winston from 'winston'; import { updateDb } from 'app/server/lib/dbUtils'; +import { isAffirmative } from 'app/common/gutil'; const username = process.env.USER || "nobody"; -const tmpDir = path.join(tmpdir(), `grist_test_${username}_userendpoint`); +const tmpDirPrefix = path.join(tmpdir(), `grist_test_${username}_userendpoint_`); +// it is sometimes useful in debugging to turn off automatic cleanup of sqlite databases. +const noCleanup = isAffirmative(process.env.NO_CLEANUP); describe('UsersManager', function () { this.timeout('30s'); + let tmpDir: string; + + before(async function () { + tmpDir = await fs.mkdtemp(tmpDirPrefix); + }); + + after(async function () { + if (!noCleanup) { + await fs.rm(tmpDir, {recursive: true}); + } + }); describe('static method', function () { /** @@ -284,7 +298,6 @@ describe('UsersManager', function () { before(async function () { env = new EnvironmentSnapshot(); - await prepareFilesystemDirectoryForTests(tmpDir); await prepareDatabase(tmpDir); db = await getDatabase(); }); From 894c55e770d1cb141098de6b54a3ff37f1b100a4 Mon Sep 17 00:00:00 2001 From: fflorent Date: Fri, 16 Aug 2024 16:21:48 +0200 Subject: [PATCH 18/22] Sort imports --- test/gen-server/lib/homedb/UsersManager.ts | 34 ++++++++++++---------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 1b7d0bcc11..d4651cbacb 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -1,29 +1,31 @@ -import { User } from 'app/gen-server/entity/User'; +import { isAffirmative } from 'app/common/gutil'; +import { FullUser, UserProfile } from 'app/common/LoginSessionAPI'; +import { ANONYMOUS_USER_EMAIL, EVERYONE_EMAIL, PREVIEWER_EMAIL, UserOptions } from 'app/common/UserAPI'; import { AclRuleOrg } from 'app/gen-server/entity/AclRule'; +import { Document } from 'app/gen-server/entity/Document'; import { Group } from 'app/gen-server/entity/Group'; +import { Login } from 'app/gen-server/entity/Login'; import { Organization } from 'app/gen-server/entity/Organization'; -import { SUPPORT_EMAIL, UsersManager } from 'app/gen-server/lib/homedb/UsersManager'; +import { Pref } from 'app/gen-server/entity/Pref'; +import { User } from 'app/gen-server/entity/User'; +import { Workspace } from 'app/gen-server/entity/Workspace'; +import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; import { GetUserOptions, NonGuestGroup, Resource } from 'app/gen-server/lib/homedb/Interfaces'; -import { tmpdir } from 'os'; -import fs from 'node:fs/promises'; -import path from 'path'; +import { SUPPORT_EMAIL, UsersManager } from 'app/gen-server/lib/homedb/UsersManager'; +import { updateDb } from 'app/server/lib/dbUtils'; import { prepareDatabase } from 'test/server/lib/helpers/PrepareDatabase'; import { EnvironmentSnapshot } from 'test/server/testUtils'; import { getDatabase } from 'test/testUtils'; -import { Workspace } from 'app/gen-server/entity/Workspace'; -import { Document } from 'app/gen-server/entity/Document'; -import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; -import Sinon, { SinonSandbox, SinonSpy } from 'sinon'; + +import log from 'app/server/lib/log'; import { assert } from 'chai'; -import { FullUser, UserProfile } from 'app/common/LoginSessionAPI'; -import { ANONYMOUS_USER_EMAIL, EVERYONE_EMAIL, PREVIEWER_EMAIL, UserOptions } from 'app/common/UserAPI'; -import { Login } from 'app/gen-server/entity/Login'; -import { Pref } from 'app/gen-server/entity/Pref'; +import Sinon, { SinonSandbox, SinonSpy } from 'sinon'; import { EntityManager } from 'typeorm'; -import log from 'app/server/lib/log'; import winston from 'winston'; -import { updateDb } from 'app/server/lib/dbUtils'; -import { isAffirmative } from 'app/common/gutil'; + +import fs from 'fs/promises'; +import { tmpdir } from 'os'; +import path from 'path'; const username = process.env.USER || "nobody"; const tmpDirPrefix = path.join(tmpdir(), `grist_test_${username}_userendpoint_`); From 55d10b3acff21be183eb2db99fc97ecfbbe6b4ec Mon Sep 17 00:00:00 2001 From: fflorent Date: Mon, 19 Aug 2024 10:32:41 +0200 Subject: [PATCH 19/22] Skip tests when using Postgresql --- test/gen-server/lib/homedb/UsersManager.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index d4651cbacb..c2c9625f58 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -299,13 +299,16 @@ describe('UsersManager', function () { before(async function () { + if (process.env.TYPEORM_TYPE === "postgres") { + this.skip(); + } env = new EnvironmentSnapshot(); await prepareDatabase(tmpDir); db = await getDatabase(); }); after(async function () { - env.restore(); + env?.restore(); }); beforeEach(function () { From e1f97eee0edc57bbeba0c58b78844d2b946897b1 Mon Sep 17 00:00:00 2001 From: fflorent Date: Mon, 19 Aug 2024 15:13:21 +0200 Subject: [PATCH 20/22] Introduce dereferenceConnection for TypeORM to be used in tests --- app/server/lib/dbUtils.ts | 2 +- test/gen-server/lib/homedb/UsersManager.ts | 2 ++ test/gen-server/seed.ts | 27 ++++++++++++++++------ 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/app/server/lib/dbUtils.ts b/app/server/lib/dbUtils.ts index 65a23b5e2a..754d774ecc 100644 --- a/app/server/lib/dbUtils.ts +++ b/app/server/lib/dbUtils.ts @@ -45,7 +45,7 @@ export async function updateDb(connection?: Connection) { await synchronizeProducts(connection, true); } -function getConnectionName() { +export function getConnectionName() { return process.env.TYPEORM_NAME || 'default'; } diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index c2c9625f58..736f171055 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -26,6 +26,7 @@ import winston from 'winston'; import fs from 'fs/promises'; import { tmpdir } from 'os'; import path from 'path'; +import { dereferenceConnection } from 'test/gen-server/seed'; const username = process.env.USER || "nobody"; const tmpDirPrefix = path.join(tmpdir(), `grist_test_${username}_userendpoint_`); @@ -276,6 +277,7 @@ describe('UsersManager', function () { // TODO: Check whether using DataSource would help and avoid this hack. await db.connection.destroy(); await db.createNewConnection(); + dereferenceConnection(dbName); } } diff --git a/test/gen-server/seed.ts b/test/gen-server/seed.ts index d935211ef9..4f9791ac3a 100644 --- a/test/gen-server/seed.ts +++ b/test/gen-server/seed.ts @@ -42,7 +42,9 @@ import {User} from "app/gen-server/entity/User"; import {Workspace} from "app/gen-server/entity/Workspace"; import {EXAMPLE_WORKSPACE_NAME} from 'app/gen-server/lib/homedb/HomeDBManager'; import {Permissions} from 'app/gen-server/lib/Permissions'; -import {getOrCreateConnection, runMigrations, undoLastMigration, updateDb} from 'app/server/lib/dbUtils'; +import { + getConnectionName, getOrCreateConnection, runMigrations, undoLastMigration, updateDb +} from 'app/server/lib/dbUtils'; import {FlexServer} from 'app/server/lib/FlexServer'; import * as fse from 'fs-extra'; @@ -526,17 +528,28 @@ class Seed { // When running mocha on several test files at once, we need to reset our database connection // if it exists. This is a little ugly since it is stored globally. -export async function removeConnection() { - if (getConnectionManager().connections.length > 0) { - if (getConnectionManager().connections.length > 1) { +export async function removeConnection(name?: string) { + const connections = getConnectionManager().connections; + if (connections.length > 0) { + if (connections.length > 1) { throw new Error("unexpected number of connections"); } - await getConnectionManager().connections[0].close(); - // There is still no official way to delete connections that I've found. - (getConnectionManager() as any).connectionMap = new Map(); + await connections[0].destroy(); + dereferenceConnection(getConnectionName()); } } +export function dereferenceConnection(name: string) { + // There seem to be no official way to delete connections. + // Also we should probably get rid of the use of connectionManager, which is deprecated + const connectionMgr = getConnectionManager(); + const connectionMap = (connectionMgr as any).connectionMap as Map; + if (!connectionMap.has(name)) { + throw new Error('connection with this name not found: ' + name); + } + connectionMap.delete(name); +} + export async function createInitialDb(connection?: Connection, migrateAndSeedData: boolean = true) { // In jenkins tests, we may want to reset the database to a clean // state. If so, TEST_CLEAN_DATABASE will have been set. How to From eaaff1bd884c5fabffb740ca2bc91572447ebb19 Mon Sep 17 00:00:00 2001 From: fflorent Date: Mon, 19 Aug 2024 15:49:35 +0200 Subject: [PATCH 21/22] Fix eslint errors after rebase --- test/gen-server/lib/HomeDBManager.ts | 56 ++++++++++++++-------------- test/gen-server/lib/emails.ts | 2 +- test/gen-server/lib/removedAt.ts | 2 +- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/test/gen-server/lib/HomeDBManager.ts b/test/gen-server/lib/HomeDBManager.ts index 77778fef5e..a22c55474e 100644 --- a/test/gen-server/lib/HomeDBManager.ts +++ b/test/gen-server/lib/HomeDBManager.ts @@ -33,14 +33,14 @@ describe('HomeDBManager', function() { it('can find existing user by email', async function() { const user = await home.getUserByLogin('chimpy@getgrist.com'); - assert.equal(user!.name, 'Chimpy'); + assert.equal(user.name, 'Chimpy'); }); it('can create new user by email, with personal org', async function() { const profile = {email: 'unseen@getgrist.com', name: 'Unseen'}; const user = await home.getUserByLogin('unseen@getgrist.com', {profile}); - assert.equal(user!.name, 'Unseen'); - const orgs = await home.getOrgs(user!.id, null); + assert.equal(user.name, 'Unseen'); + const orgs = await home.getOrgs(user.id, null); assert.isAtLeast(orgs.data!.length, 1); assert.equal(orgs.data![0].name, 'Personal'); assert.equal(orgs.data![0].owner.name, 'Unseen'); @@ -65,37 +65,37 @@ describe('HomeDBManager', function() { // log in without a name let user = await home.getUserByLogin('unseen2@getgrist.com'); // name is blank - assert.equal(user!.name, ''); + assert.equal(user.name, ''); // log in with a name const profile: UserProfile = {email: 'unseen2@getgrist.com', name: 'Unseen2'}; user = await home.getUserByLogin('unseen2@getgrist.com', {profile}); // name is now set - assert.equal(user!.name, 'Unseen2'); + assert.equal(user.name, 'Unseen2'); // log in without a name user = await home.getUserByLogin('unseen2@getgrist.com'); // name is still set - assert.equal(user!.name, 'Unseen2'); + assert.equal(user.name, 'Unseen2'); // no picture yet - assert.equal(user!.picture, null); + assert.equal(user.picture, null); // log in with picture link profile.picture = 'http://picture.pic'; user = await home.getUserByLogin('unseen2@getgrist.com', {profile}); // now should have a picture link - assert.equal(user!.picture, 'http://picture.pic'); + assert.equal(user.picture, 'http://picture.pic'); // log in without picture user = await home.getUserByLogin('unseen2@getgrist.com'); // should still have picture link - assert.equal(user!.picture, 'http://picture.pic'); + assert.equal(user.picture, 'http://picture.pic'); }); it('can add an org', async function() { const user = await home.getUserByLogin('chimpy@getgrist.com'); - const orgId = (await home.addOrg(user!, {name: 'NewOrg', domain: 'novel-org'}, teamOptions)).data!; - const org = await home.getOrg({userId: user!.id}, orgId); + const orgId = (await home.addOrg(user, {name: 'NewOrg', domain: 'novel-org'}, teamOptions)).data!; + const org = await home.getOrg({userId: user.id}, orgId); assert.equal(org.data!.name, 'NewOrg'); assert.equal(org.data!.domain, 'novel-org'); assert.equal(org.data!.billingAccount.product.name, TEAM_PLAN); - await home.deleteOrg({userId: user!.id}, orgId); + await home.deleteOrg({userId: user.id}, orgId); }); it('creates default plan if defined', async function() { @@ -104,28 +104,28 @@ describe('HomeDBManager', function() { try { // Set the default product to be the free plan. process.env.GRIST_DEFAULT_PRODUCT = FREE_PLAN; - let orgId = (await home.addOrg(user!, {name: 'NewOrg', domain: 'novel-org'}, { + let orgId = (await home.addOrg(user, {name: 'NewOrg', domain: 'novel-org'}, { setUserAsOwner: false, useNewPlan: true, // omit plan, to use a default one (teamInitial) // it will either be 'stub' or anything set in GRIST_DEFAULT_PRODUCT })).data!; - let org = await home.getOrg({userId: user!.id}, orgId); + let org = await home.getOrg({userId: user.id}, orgId); assert.equal(org.data!.name, 'NewOrg'); assert.equal(org.data!.domain, 'novel-org'); assert.equal(org.data!.billingAccount.product.name, FREE_PLAN); - await home.deleteOrg({userId: user!.id}, orgId); + await home.deleteOrg({userId: user.id}, orgId); // Now remove the default product, and check that the default plan is used. delete process.env.GRIST_DEFAULT_PRODUCT; - orgId = (await home.addOrg(user!, {name: 'NewOrg', domain: 'novel-org'}, { + orgId = (await home.addOrg(user, {name: 'NewOrg', domain: 'novel-org'}, { setUserAsOwner: false, useNewPlan: true, })).data!; - org = await home.getOrg({userId: user!.id}, orgId); + org = await home.getOrg({userId: user.id}, orgId); assert.equal(org.data!.billingAccount.product.name, STUB_PLAN); - await home.deleteOrg({userId: user!.id}, orgId); + await home.deleteOrg({userId: user.id}, orgId); } finally { oldEnv.restore(); } @@ -134,17 +134,17 @@ describe('HomeDBManager', function() { it('cannot duplicate a domain', async function() { const user = await home.getUserByLogin('chimpy@getgrist.com'); const domain = 'repeated-domain'; - const result = await home.addOrg(user!, {name: `${domain}!`, domain}, teamOptions); + const result = await home.addOrg(user, {name: `${domain}!`, domain}, teamOptions); const orgId = result.data!; assert.equal(result.status, 200); - await assert.isRejected(home.addOrg(user!, {name: `${domain}!`, domain}, teamOptions), + await assert.isRejected(home.addOrg(user, {name: `${domain}!`, domain}, teamOptions), /Domain already in use/); - await home.deleteOrg({userId: user!.id}, orgId); + await home.deleteOrg({userId: user.id}, orgId); }); it('cannot add an org with a (blacklisted) dodgy domain', async function() { const user = await home.getUserByLogin('chimpy@getgrist.com'); - const userId = user!.id; + const userId = user.id; const misses = [ 'thing!', ' thing', 'ww', 'docs-999', 'o-99', '_domainkey', 'www', 'api', 'thissubdomainiswaytoolongmyfriendyoushouldrethinkitoratleastsummarizeit', @@ -154,13 +154,13 @@ describe('HomeDBManager', function() { 'thing', 'jpl', 'xyz', 'appel', '123', '1google' ]; for (const domain of misses) { - const result = await home.addOrg(user!, {name: `${domain}!`, domain}, teamOptions); + const result = await home.addOrg(user, {name: `${domain}!`, domain}, teamOptions); assert.equal(result.status, 400); const org = await home.getOrg({userId}, domain); assert.equal(org.status, 404); } for (const domain of hits) { - const result = await home.addOrg(user!, {name: `${domain}!`, domain}, teamOptions); + const result = await home.addOrg(user, {name: `${domain}!`, domain}, teamOptions); assert.equal(result.status, 200); const org = await home.getOrg({userId}, domain); assert.equal(org.status, 200); @@ -189,7 +189,7 @@ describe('HomeDBManager', function() { // Fetch the doc and check that the updatedAt value is as expected. const kiwi = await home.getUserByLogin('kiwi@getgrist.com'); - const resp1 = await home.getOrgWorkspaces({userId: kiwi!.id}, primatelyOrgId); + const resp1 = await home.getOrgWorkspaces({userId: kiwi.id}, primatelyOrgId); assert.equal(resp1.status, 200); // Check that the apples metadata is as expected. updatedAt should have been set @@ -209,7 +209,7 @@ describe('HomeDBManager', function() { // Check that the shark metadata is as expected. updatedAt should have been set // to 2004. usage should be set. - const resp2 = await home.getOrgWorkspaces({userId: kiwi!.id}, fishOrgId); + const resp2 = await home.getOrgWorkspaces({userId: kiwi.id}, fishOrgId); assert.equal(resp2.status, 200); const shark = resp2.data![0].docs.find((doc: any) => doc.name === 'Shark'); assert.equal(shark!.updatedAt.toISOString(), setDateISO2); @@ -340,7 +340,7 @@ describe('HomeDBManager', function() { it('can fork docs', async function() { const user1 = await home.getUserByLogin('kiwi@getgrist.com'); - const user1Id = user1!.id; + const user1Id = user1.id; const orgId = await home.testGetId('Fish') as number; const doc1Id = await home.testGetId('Shark') as string; const scope = {userId: user1Id, urlId: doc1Id}; @@ -393,7 +393,7 @@ describe('HomeDBManager', function() { // Now fork "Shark" as Chimpy, and check that Kiwi's forks aren't listed. const user2 = await home.getUserByLogin('chimpy@getgrist.com'); - const user2Id = user2!.id; + const user2Id = user2.id; const resp4 = await home.getOrgWorkspaces({userId: user2Id}, orgId); const resp4Doc = resp4.data![0].docs.find((d: any) => d.name === 'Shark'); assert.deepEqual(resp4Doc!.forks, []); diff --git a/test/gen-server/lib/emails.ts b/test/gen-server/lib/emails.ts index aae3957cce..bff255aec5 100644 --- a/test/gen-server/lib/emails.ts +++ b/test/gen-server/lib/emails.ts @@ -19,7 +19,7 @@ describe('emails', function() { beforeEach(async function() { this.timeout(5000); server = new TestServer(this); - ref = (email: string) => server.dbManager.getUserByLogin(email).then((user) => user!.ref); + ref = (email: string) => server.dbManager.getUserByLogin(email).then((user) => user.ref); serverUrl = await server.start(); }); diff --git a/test/gen-server/lib/removedAt.ts b/test/gen-server/lib/removedAt.ts index 2e990707ba..219a9b3544 100644 --- a/test/gen-server/lib/removedAt.ts +++ b/test/gen-server/lib/removedAt.ts @@ -258,7 +258,7 @@ describe('removedAt', function() { 'test3@getgrist.com': 'editors', } }); - const userRef = (email: string) => home.dbManager.getUserByLogin(email).then((user) => user!.ref); + const userRef = (email: string) => home.dbManager.getUserByLogin(email).then((user) => user.ref); const idTest1 = (await home.dbManager.getUserByLogin("test1@getgrist.com"))!.id; const idTest2 = (await home.dbManager.getUserByLogin("test2@getgrist.com"))!.id; const idTest3 = (await home.dbManager.getUserByLogin("test3@getgrist.com"))!.id; From eb1c177ff621b2ba9ded460d519c1d2b6527c2e2 Mon Sep 17 00:00:00 2001 From: fflorent Date: Mon, 19 Aug 2024 18:11:46 +0200 Subject: [PATCH 22/22] TypeORM: Attempt to DataSources instead of Connections and other Also remove any deprecated functions/methods from TypeORM --- app/gen-server/ApiServer.ts | 6 +- app/gen-server/lib/Activations.ts | 2 +- app/gen-server/lib/Doom.ts | 2 +- app/gen-server/lib/Housekeeper.ts | 14 ++-- app/gen-server/lib/Usage.ts | 2 +- app/gen-server/lib/homedb/HomeDBManager.ts | 98 +++++++++++----------- app/gen-server/lib/homedb/UsersManager.ts | 2 +- app/server/companion.ts | 32 +++---- app/server/lib/DocApi.ts | 2 +- app/server/lib/FlexServer.ts | 8 +- app/server/lib/dbUtils.ts | 94 +++++++++++---------- app/server/lib/extractOrg.ts | 4 +- stubs/app/server/server.ts | 2 +- test/gen-server/ApiServer.ts | 10 +-- test/gen-server/ApiServerAccess.ts | 6 +- test/gen-server/ApiServerBenchmark.ts | 4 +- test/gen-server/AuthCaching.ts | 3 +- test/gen-server/apiUtils.ts | 13 ++- test/gen-server/lib/DocApiForwarder.ts | 7 +- test/gen-server/lib/DocWorkerMap.ts | 3 +- test/gen-server/lib/HealthCheck.ts | 6 +- test/gen-server/lib/HomeDBManager.ts | 4 +- test/gen-server/lib/Housekeeper.ts | 4 +- test/gen-server/lib/homedb/UsersManager.ts | 20 ++--- test/gen-server/lib/limits.ts | 4 +- test/gen-server/lib/mergedOrgs.ts | 3 +- test/gen-server/lib/previewer.ts | 2 +- test/gen-server/lib/scrubUserFromOrg.ts | 2 +- test/gen-server/lib/urlIds.ts | 2 +- test/gen-server/migrations.ts | 36 ++++---- test/gen-server/seed.ts | 83 +++++++----------- test/gen-server/testUtils.ts | 6 +- test/nbrowser/gristUtils.ts | 4 +- test/nbrowser/testServer.ts | 3 +- test/server/fixSiteProducts.ts | 4 +- test/server/lib/Authorizer.ts | 3 +- test/server/lib/DocApi.ts | 2 +- test/server/lib/GranularAccess.ts | 14 ++-- test/server/lib/HostedStorageManager.ts | 12 ++- test/testUtils.ts | 2 +- 40 files changed, 253 insertions(+), 277 deletions(-) diff --git a/app/gen-server/ApiServer.ts b/app/gen-server/ApiServer.ts index 1db45714d6..21b0f1816c 100644 --- a/app/gen-server/ApiServer.ts +++ b/app/gen-server/ApiServer.ts @@ -76,7 +76,7 @@ export function addOrg( billing?: BillingOptions, } ): Promise { - return dbManager.connection.transaction(async manager => { + return dbManager.dataSource.transaction(async manager => { const user = await manager.findOne(User, {where: {id: userId}}); if (!user) { return handleDeletedUser(); } const query = await dbManager.addOrg(user, props, { @@ -505,7 +505,7 @@ export class ApiServer { this._app.post('/api/profile/apikey', expressWrap(async (req, res) => { const userId = getAuthorizedUserId(req); const force = req.body ? req.body.force : false; - const manager = this._dbManager.connection.manager; + const manager = this._dbManager.dataSource.manager; let user = await manager.findOne(User, {where: {id: userId}}); if (!user) { return handleDeletedUser(); } if (!user.apiKey || force) { @@ -520,7 +520,7 @@ export class ApiServer { // Delete apiKey this._app.delete('/api/profile/apikey', expressWrap(async (req, res) => { const userId = getAuthorizedUserId(req); - await this._dbManager.connection.transaction(async manager => { + await this._dbManager.dataSource.transaction(async manager => { const user = await manager.findOne(User, {where: {id: userId}}); if (!user) { return handleDeletedUser(); } user.apiKey = null; diff --git a/app/gen-server/lib/Activations.ts b/app/gen-server/lib/Activations.ts index 2648c98b73..476485d2cb 100644 --- a/app/gen-server/lib/Activations.ts +++ b/app/gen-server/lib/Activations.ts @@ -15,7 +15,7 @@ export class Activations { // It will be created with an empty key column, which will get // filled in once an activation key is presented. public current(): Promise { - return this._db.connection.manager.transaction(async manager => { + return this._db.dataSource.manager.transaction(async manager => { let activation = await manager.findOne(Activation, {where: {}}); if (!activation) { activation = manager.create(Activation); diff --git a/app/gen-server/lib/Doom.ts b/app/gen-server/lib/Doom.ts index 1d6bc0d648..208c42c149 100644 --- a/app/gen-server/lib/Doom.ts +++ b/app/gen-server/lib/Doom.ts @@ -152,7 +152,7 @@ export class Doom { } } const candidate = sortBy(owners, ['email'])[0]; - await scrubUserFromOrg(orgId, userId, candidate.id, this._dbManager.connection.manager); + await scrubUserFromOrg(orgId, userId, candidate.id, this._dbManager.dataSource.manager); } // List the sites a user has access to. diff --git a/app/gen-server/lib/Housekeeper.ts b/app/gen-server/lib/Housekeeper.ts index c3012bec6b..570888fa54 100644 --- a/app/gen-server/lib/Housekeeper.ts +++ b/app/gen-server/lib/Housekeeper.ts @@ -185,7 +185,7 @@ export class Housekeeper { log.warn("logMetrics siteUsage starting"); // Avoid using a transaction since it may end up being held up for a while, and for no good // reason (atomicity matters for this reporting). - const manager = this._dbManager.connection.manager; + const manager = this._dbManager.dataSource.manager; const usageSummaries = await this._getOrgUsageSummaries(manager); // We sleep occasionally during this logging. We may log many MANY lines, which can hang up a @@ -212,7 +212,7 @@ export class Housekeeper { if (this._telemetry.shouldLogEvent('siteMembership')) { log.warn("logMetrics siteMembership starting"); - const manager = this._dbManager.connection.manager; + const manager = this._dbManager.dataSource.manager; const membershipSummaries = await this._getOrgMembershipSummaries(manager); await forEachWithBreaks("logMetrics siteMembership progress", membershipSummaries, summary => { this._telemetry.logEvent(null, 'siteMembership', { @@ -297,7 +297,7 @@ export class Housekeeper { } private async _getDocsToDelete() { - const docs = await this._dbManager.connection.createQueryBuilder() + const docs = await this._dbManager.dataSource.createQueryBuilder() .select('docs') .from(Document, 'docs') .leftJoinAndSelect('docs.workspace', 'workspaces') @@ -309,7 +309,7 @@ export class Housekeeper { } private async _getWorkspacesToDelete() { - const workspaces = await this._dbManager.connection.createQueryBuilder() + const workspaces = await this._dbManager.dataSource.createQueryBuilder() .select('workspaces') .from(Workspace, 'workspaces') .leftJoin('workspaces.docs', 'docs') @@ -323,7 +323,7 @@ export class Housekeeper { } private async _getForksToDelete() { - const forks = await this._dbManager.connection.createQueryBuilder() + const forks = await this._dbManager.dataSource.createQueryBuilder() .select('forks') .from(Document, 'forks') .where('forks.trunk_id IS NOT NULL') @@ -385,7 +385,7 @@ export class Housekeeper { * don't have to deal with its caprices. */ private _getThreshold() { - return fromNow(this._dbManager.connection.driver.options.type, AGE_THRESHOLD_OFFSET); + return fromNow(this._dbManager.dataSource.driver.options.type, AGE_THRESHOLD_OFFSET); } // Call a document endpoint with a permit, cleaning up after the call. @@ -475,7 +475,7 @@ export async function fixSiteProducts(options: { } // Find all billing accounts on teamFree product and change them to the Free. - return await db.connection.transaction(async (t) => { + return await db.dataSource.transaction(async (t) => { const freeProduct = await t.findOne(Product, {where: {name: 'Free'}}); const freeTeamProduct = await t.findOne(Product, {where: {name: 'teamFree'}}); diff --git a/app/gen-server/lib/Usage.ts b/app/gen-server/lib/Usage.ts index 9082bdcca9..29d3407113 100644 --- a/app/gen-server/lib/Usage.ts +++ b/app/gen-server/lib/Usage.ts @@ -43,7 +43,7 @@ export class Usage { private async _apply(): Promise { try { - const manager = this._dbManager.connection.manager; + const manager = this._dbManager.dataSource.manager; // raw count of users const userCount = await manager.count(User); // users who have logged in at least once diff --git a/app/gen-server/lib/homedb/HomeDBManager.ts b/app/gen-server/lib/homedb/HomeDBManager.ts index 967affd6f2..3eb4c99ab4 100644 --- a/app/gen-server/lib/homedb/HomeDBManager.ts +++ b/app/gen-server/lib/homedb/HomeDBManager.ts @@ -56,7 +56,7 @@ import { readJson } from 'app/gen-server/sqlUtils'; import {appSettings} from 'app/server/lib/AppSettings'; -import {createNewConnection, getOrCreateConnection} from 'app/server/lib/dbUtils'; +import {createNewDataSource} from 'app/server/lib/dbUtils'; import {makeId} from 'app/server/lib/idUtils'; import log from 'app/server/lib/log'; import {Permit} from 'app/server/lib/Permit'; @@ -248,7 +248,7 @@ export type BillingOptions = Partial { - this._connection = await getOrCreateConnection(); + public async initializeDataSource(overrideConf?: Partial): Promise { + this._dataSource = await createNewDataSource(overrideConf); } - public async createNewConnection(overrideConf?: Partial): Promise { - this._connection = await createNewConnection(overrideConf); + public async destroyDataSource(): Promise { + await this._dataSource.destroy(); } // make sure special users and workspaces are available @@ -386,12 +386,12 @@ export class HomeDBManager extends EventEmitter { } } - public get connection() { - return this._connection; + public get dataSource() { + return this._dataSource; } public async testQuery(sql: string, args: any[]): Promise { - return this._connection.query(sql, args); + return this._dataSource.query(sql, args); } /** @@ -873,7 +873,7 @@ export class HomeDBManager extends EventEmitter { shareKey} = parseUrlId(key.urlId); let doc: Document; if (shareKey) { - const res = await (transaction || this._connection).createQueryBuilder() + const res = await (transaction || this._dataSource).createQueryBuilder() .select('shares') .from(Share, 'shares') .leftJoinAndSelect('shares.doc', 'doc') @@ -1014,7 +1014,7 @@ export class HomeDBManager extends EventEmitter { // TODO: make a more efficient implementation if needed. public async flushSingleDocAuthCache(scope: DocScope, docId: string) { // Get all aliases of this document. - const aliases = await this._connection.manager.find(Alias, {where: {docId}}); + const aliases = await this._dataSource.manager.find(Alias, {where: {docId}}); // Construct a set of possible prefixes for cache keys. const names = new Set(aliases.map(a => stringifyUrlIdOrg(a.urlId, scope.org))); names.add(stringifyUrlIdOrg(docId, scope.org)); @@ -1047,7 +1047,7 @@ export class HomeDBManager extends EventEmitter { * deleting a document. */ public async getDocForks(docId: string): Promise { - return this._connection.createQueryBuilder() + return this._dataSource.createQueryBuilder() .select('forks') .from(Document, 'forks') .where('forks.trunk_id = :docId', {docId}) @@ -1377,7 +1377,7 @@ export class HomeDBManager extends EventEmitter { errMessage: 'Bad request: name required' }; } - return await this._connection.transaction(async manager => { + return await this._dataSource.transaction(async manager => { let orgQuery = this.org(scope, orgKey, { manager, markPermissions: Permissions.ADD, @@ -1424,7 +1424,7 @@ export class HomeDBManager extends EventEmitter { // query result with status 200 on success. public async updateWorkspace(scope: Scope, wsId: number, props: Partial): Promise> { - return await this._connection.transaction(async manager => { + return await this._dataSource.transaction(async manager => { const wsQuery = this._workspace(scope, wsId, { manager, markPermissions: Permissions.UPDATE @@ -1447,7 +1447,7 @@ export class HomeDBManager extends EventEmitter { // error. Otherwise deletes the given workspace. Returns an empty query result with // status 200 on success. public async deleteWorkspace(scope: Scope, wsId: number): Promise> { - return await this._connection.transaction(async manager => { + return await this._dataSource.transaction(async manager => { const wsQuery = this._workspace(scope, wsId, { manager, markPermissions: Permissions.REMOVE, @@ -1503,7 +1503,7 @@ export class HomeDBManager extends EventEmitter { errMessage: 'Bad request: name required' }; } - return await this._connection.transaction(async manager => { + return await this._dataSource.transaction(async manager => { let wsQuery = this._workspace(scope, wsId, { manager, markPermissions: Permissions.ADD @@ -1593,7 +1593,7 @@ export class HomeDBManager extends EventEmitter { } public addSecret(value: string, docId: string): Promise { - return this._connection.transaction(async manager => { + return this._dataSource.transaction(async manager => { const secret = new Secret(); secret.id = uuidv4(); secret.value = value; @@ -1605,7 +1605,7 @@ export class HomeDBManager extends EventEmitter { // Updates the secret matching id and docId, to the new value. public async updateSecret(id: string, docId: string, value: string, manager?: EntityManager): Promise { - const res = await (manager || this._connection).createQueryBuilder() + const res = await (manager || this._dataSource).createQueryBuilder() .update(Secret) .set({value}) .where("id = :id AND doc_id = :docId", {id, docId}) @@ -1616,7 +1616,7 @@ export class HomeDBManager extends EventEmitter { } public async getSecret(id: string, docId: string, manager?: EntityManager): Promise { - const secret = await (manager || this._connection).createQueryBuilder() + const secret = await (manager || this._dataSource).createQueryBuilder() .select('secrets') .from(Secret, 'secrets') .where('id = :id AND doc_id = :docId', {id, docId}) @@ -1663,7 +1663,7 @@ export class HomeDBManager extends EventEmitter { if (!unsubscribeKey && checkKey) { throw new ApiError('Bad request: unsubscribeKey required', 400); } - return await this._connection.transaction(async manager => { + return await this._dataSource.transaction(async manager => { if (checkKey) { const secret = await this.getSecret(id, docId, manager); if (!secret) { @@ -1752,7 +1752,7 @@ export class HomeDBManager extends EventEmitter { // error. Otherwise deletes the given document. Returns an empty query result with // status 200 on success. public async deleteDocument(scope: DocScope): Promise> { - return await this._connection.transaction(async manager => { + return await this._dataSource.transaction(async manager => { const {forkId} = parseUrlId(scope.urlId); if (forkId) { const forkQuery = this._fork(scope, { @@ -1815,7 +1815,7 @@ export class HomeDBManager extends EventEmitter { orgKey: string|number, callback: (billingAccount: BillingAccount, transaction: EntityManager) => void|Promise ): Promise> { - return await this._connection.transaction(async transaction => { + return await this._dataSource.transaction(async transaction => { const billingAccount = await this.getBillingAccount({userId}, orgKey, false, transaction); const billingAccountCopy = Object.assign({}, billingAccount); await callback(billingAccountCopy, transaction); @@ -1847,7 +1847,7 @@ export class HomeDBManager extends EventEmitter { permissionDelta.users![key] = delta.users[key] ? 'owners' : null; } - return await this._connection.transaction(async transaction => { + return await this._dataSource.transaction(async transaction => { const billingAccount = await this.getBillingAccount({userId}, orgKey, true, transaction); // At this point, we'll have thrown an error if userId is not a billing account manager. // Now check if the billing account has mutable managers (individual account does not). @@ -1901,7 +1901,7 @@ export class HomeDBManager extends EventEmitter { ): Promise> { const {userId} = scope; const notifications: Array<() => void> = []; - const result = await this._connection.transaction(async manager => { + const result = await this._dataSource.transaction(async manager => { const analysis = await this._usersManager.verifyAndLookupDeltaEmails(userId, delta, true, manager); const {userIdDelta} = analysis; let orgQuery = this.org(scope, orgKey, { @@ -1958,7 +1958,7 @@ export class HomeDBManager extends EventEmitter { ): Promise> { const {userId} = scope; const notifications: Array<() => void> = []; - const result = await this._connection.transaction(async manager => { + const result = await this._dataSource.transaction(async manager => { const analysis = await this._usersManager.verifyAndLookupDeltaEmails(userId, delta, false, manager); let {userIdDelta} = analysis; let wsQuery = this._workspace(scope, wsId, { @@ -2032,7 +2032,7 @@ export class HomeDBManager extends EventEmitter { delta: PermissionDelta ): Promise> { const notifications: Array<() => void> = []; - const result = await this._connection.transaction(async manager => { + const result = await this._dataSource.transaction(async manager => { const {userId} = scope; const analysis = await this._usersManager.verifyAndLookupDeltaEmails(userId, delta, false, manager); let {userIdDelta} = analysis; @@ -2111,7 +2111,7 @@ export class HomeDBManager extends EventEmitter { public async getWorkspaceAccess(scope: Scope, wsId: number): Promise> { // Run the query for the workspace and org in a transaction. This brings some isolation protection // against changes to the workspace or org while we are querying. - const { workspace, org, queryFailure } = await this._connection.transaction(async manager => { + const { workspace, org, queryFailure } = await this._dataSource.transaction(async manager => { const wsQueryResult = await this._getWorkspaceWithACLRules(scope, wsId, { manager }); if (wsQueryResult.status !== 200) { // If the query for the workspace failed, return the failure result. @@ -2261,7 +2261,7 @@ export class HomeDBManager extends EventEmitter { scope: DocScope, wsId: number ): Promise> { - return await this._connection.transaction(async manager => { + return await this._dataSource.transaction(async manager => { // Get the doc const docQuery = this._doc(scope, { manager, @@ -2373,7 +2373,7 @@ export class HomeDBManager extends EventEmitter { scope: DocScope, setPinned: boolean ): Promise> { - return await this._connection.transaction(async manager => { + return await this._dataSource.transaction(async manager => { // Find the doc to assert that it exists. Assert that the user has edit access to the // parent org. const permissions = Permissions.EDITOR; @@ -2411,7 +2411,7 @@ export class HomeDBManager extends EventEmitter { doc: Document, forkId: string, ): Promise> { - return await this._connection.transaction(async manager => { + return await this._dataSource.transaction(async manager => { const fork = new Document(); fork.id = forkId; fork.name = doc.name; @@ -2441,7 +2441,7 @@ export class HomeDBManager extends EventEmitter { }; } const docIds = Object.keys(docUpdateMap); - return this._connection.transaction(async manager => { + return this._dataSource.transaction(async manager => { const updateTasks = docIds.map(docId => { return manager.createQueryBuilder() .update(Document) @@ -2455,7 +2455,7 @@ export class HomeDBManager extends EventEmitter { } public async setDocGracePeriodStart(docId: string, gracePeriodStart: Date | null) { - return await this._connection.createQueryBuilder() + return await this._dataSource.createQueryBuilder() .update(Document) .set({gracePeriodStart}) .where({id: docId}) @@ -2463,7 +2463,7 @@ export class HomeDBManager extends EventEmitter { } public async getProduct(name: string): Promise { - return await this._connection.createQueryBuilder() + return await this._dataSource.createQueryBuilder() .select('product') .from(Product, 'product') .where('name = :name', {name}) @@ -2471,7 +2471,7 @@ export class HomeDBManager extends EventEmitter { } public async getDocFeatures(docId: string): Promise { - const billingAccount = await this._connection.createQueryBuilder() + const billingAccount = await this._dataSource.createQueryBuilder() .select('account') .from(BillingAccount, 'account') .leftJoinAndSelect('account.product', 'product') @@ -2489,7 +2489,7 @@ export class HomeDBManager extends EventEmitter { } public async getDocProduct(docId: string): Promise { - return await this._connection.createQueryBuilder() + return await this._dataSource.createQueryBuilder() .select('product') .from(Product, 'product') .leftJoinAndSelect('product.accounts', 'account') @@ -2610,7 +2610,7 @@ export class HomeDBManager extends EventEmitter { } public async getLimits(accountId: number): Promise { - const result = this._connection.transaction(async manager => { + const result = this._dataSource.transaction(async manager => { return await manager.createQueryBuilder() .select('limit') .from(Limit, 'limit') @@ -2630,7 +2630,7 @@ export class HomeDBManager extends EventEmitter { } public async removeLimit(scope: Scope, limitType: LimitType): Promise { - await this._connection.transaction(async manager => { + await this._dataSource.transaction(async manager => { const org = await this._org(scope, false, scope.org ?? null, {manager, needRealOrg: true}) .innerJoinAndSelect('orgs.billingAccount', 'billing_account') .innerJoinAndSelect('billing_account.product', 'product') @@ -2656,7 +2656,7 @@ export class HomeDBManager extends EventEmitter { delta: number, dryRun?: boolean, }): Promise { - const limitOrError: Limit|ApiError|null = await this._connection.transaction(async manager => { + const limitOrError: Limit|ApiError|null = await this._dataSource.transaction(async manager => { const org = await this._org(scope, false, scope.org ?? null, {manager, needRealOrg: true}) .innerJoinAndSelect('orgs.billingAccount', 'billing_account') .innerJoinAndSelect('billing_account.product', 'product') @@ -2720,7 +2720,7 @@ export class HomeDBManager extends EventEmitter { } public async syncShares(docId: string, shares: ShareInfo[]) { - return this._connection.transaction(async manager => { + return this._dataSource.transaction(async manager => { for (const share of shares) { const key = makeId(); await manager.createQueryBuilder() @@ -2755,7 +2755,7 @@ export class HomeDBManager extends EventEmitter { } public async getShareByKey(key: string) { - return this._connection.createQueryBuilder() + return this._dataSource.createQueryBuilder() .select('shares') .from(Share, 'shares') .where('shares.key = :key', {key}) @@ -2763,7 +2763,7 @@ export class HomeDBManager extends EventEmitter { } public async getShareByLinkId(docId: string, linkId: string) { - return this._connection.createQueryBuilder() + return this._dataSource.createQueryBuilder() .select('shares') .from(Share, 'shares') .where('shares.doc_id = :docId and shares.link_id = :linkId', {docId, linkId}) @@ -2793,7 +2793,7 @@ export class HomeDBManager extends EventEmitter { if (accountId === 0) { throw new Error(`getLimit: called for not existing account`); } - const result = this._connection.transaction(async manager => { + const result = this._dataSource.transaction(async manager => { let existing = await manager.createQueryBuilder() .select('limit') .from(Limit, 'limit') @@ -3265,7 +3265,7 @@ export class HomeDBManager extends EventEmitter { private _runInTransaction(transaction: EntityManager|undefined, op: (manager: EntityManager) => Promise): Promise { if (transaction) { return op(transaction); } - return this._connection.transaction(op); + return this._dataSource.transaction(op); } /** @@ -3283,7 +3283,7 @@ export class HomeDBManager extends EventEmitter { } private _docs(manager?: EntityManager) { - return (manager || this._connection).createQueryBuilder() + return (manager || this._dataSource).createQueryBuilder() .select('docs') .from(Document, 'docs'); } @@ -3377,7 +3377,7 @@ export class HomeDBManager extends EventEmitter { } private _workspaces(manager?: EntityManager) { - return (manager || this._connection).createQueryBuilder() + return (manager || this._dataSource).createQueryBuilder() .select('workspaces') .from(Workspace, 'workspaces'); } @@ -3430,7 +3430,7 @@ export class HomeDBManager extends EventEmitter { } private _orgs(manager?: EntityManager) { - return (manager || this._connection).createQueryBuilder() + return (manager || this._dataSource).createQueryBuilder() .select('orgs') .from(Organization, 'orgs'); } @@ -4273,7 +4273,7 @@ export class HomeDBManager extends EventEmitter { // Set Workspace.removedAt to null (undeletion) or to a datetime (soft deletion) private _setWorkspaceRemovedAt(scope: Scope, wsId: number, removedAt: Date|null) { - return this._connection.transaction(async manager => { + return this._dataSource.transaction(async manager => { const wsQuery = this._workspace({...scope, showAll: true}, wsId, { manager, markPermissions: Permissions.REMOVE @@ -4287,7 +4287,7 @@ export class HomeDBManager extends EventEmitter { // Set Document.removedAt to null (undeletion) or to a datetime (soft deletion) private _setDocumentRemovedAt(scope: DocScope, removedAt: Date|null) { - return this._connection.transaction(async manager => { + return this._dataSource.transaction(async manager => { let docQuery = this._doc({...scope, showAll: true}, { manager, markPermissions: Permissions.SCHEMA_EDIT | Permissions.REMOVE, diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index a219fa2384..2ac57c70e7 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -82,7 +82,7 @@ export class UsersManager { private _specialUserIds: {[name: string]: number} = {}; // id for anonymous user, previewer, etc private get _connection () { - return this._homeDb.connection; + return this._homeDb.dataSource; } public constructor( diff --git a/app/server/companion.ts b/app/server/companion.ts index ab454cb5a4..9bbbcdf9be 100644 --- a/app/server/companion.ts +++ b/app/server/companion.ts @@ -3,14 +3,14 @@ import { version } from 'app/common/version'; import { synchronizeProducts } from 'app/gen-server/entity/Product'; import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; import { applyPatch } from 'app/gen-server/lib/TypeORMPatches'; -import { getMigrations, getOrCreateConnection, getTypeORMSettings, +import { createNewDataSource, getMigrations, getTypeORMSettings, undoLastMigration, updateDb } from 'app/server/lib/dbUtils'; import { getDatabaseUrl } from 'app/server/lib/serverUtils'; import { getTelemetryPrefs } from 'app/server/lib/Telemetry'; import { Gristifier } from 'app/server/utils/gristify'; import { pruneActionHistory } from 'app/server/utils/pruneActionHistory'; import * as commander from 'commander'; -import { Connection } from 'typeorm'; +import { DataSource } from 'typeorm'; /** * Main entrypoint for a cli toolbox for configuring aspects of Grist @@ -184,14 +184,14 @@ export function addSiteCommand(program: commander.Command, // db url export function addDbCommand(program: commander.Command, options: CommandOptions, - reuseConnection?: Connection) { - function withConnection(op: (connection: Connection) => Promise) { + datasourceToReuse?: DataSource) { + function withDataSource(op: (dataSource: DataSource) => Promise) { return async () => { if (!process.env.TYPEORM_LOGGING) { process.env.TYPEORM_LOGGING = 'true'; } - const connection = reuseConnection || await getOrCreateConnection(); - const exitCode = await op(connection); + const dataSource = datasourceToReuse || await createNewDataSource(); + const exitCode = await op(dataSource); if (exitCode !== 0) { program.error('db command failed', {exitCode}); } @@ -205,25 +205,25 @@ export function addDbCommand(program: commander.Command, sub('migrate') .description('run all pending migrations on database') - .action(withConnection(async (connection) => { - await updateDb(connection); + .action(withDataSource(async (dataSource) => { + await updateDb(dataSource); return 0; })); sub('revert') .description('revert last migration on database') - .action(withConnection(async (connection) => { - await undoLastMigration(connection); + .action(withDataSource(async (dataSource) => { + await undoLastMigration(dataSource); return 0; })); sub('check') .description('check that there are no pending migrations on database') - .action(withConnection(dbCheck)); + .action(withDataSource(dbCheck)); sub('url') .description('construct a url for the database (for psql, catsql etc)') - .action(withConnection(async () => { + .action(withDataSource(async () => { console.log(getDatabaseUrl(getTypeORMSettings(), true)); return 0; })); @@ -254,9 +254,9 @@ export function addVersionCommand(program: commander.Command) { // Report the status of the database. Migrations appied, migrations pending, // product information applied, product changes pending. -export async function dbCheck(connection: Connection) { - const migrations = await getMigrations(connection); - const changingProducts = await synchronizeProducts(connection, false); +export async function dbCheck(dataSource: DataSource) { + const migrations = await getMigrations(dataSource); + const changingProducts = await synchronizeProducts(dataSource, false); // eslint-disable-next-line @typescript-eslint/no-shadow const log = process.env.TYPEORM_LOGGING === 'true' ? console.log : (...args: any[]) => null; const options = getTypeORMSettings(); @@ -286,7 +286,7 @@ export async function dbCheck(connection: Connection) { // Get an interface to the home db. export async function getHomeDBManager() { const dbManager = new HomeDBManager(); - await dbManager.connect(); + await dbManager.initializeDataSource(); await dbManager.initializeSpecialIds(); return dbManager; } diff --git a/app/server/lib/DocApi.ts b/app/server/lib/DocApi.ts index 1ceef80626..a5719e17d9 100644 --- a/app/server/lib/DocApi.ts +++ b/app/server/lib/DocApi.ts @@ -1123,7 +1123,7 @@ export class DocWorkerApi { if (req.body.resetTutorialMetadata) { const scope = getDocScope(req); const tutorialTrunkId = options.sourceDocId; - await this._dbManager.connection.transaction(async (manager) => { + await this._dbManager.dataSource.transaction(async (manager) => { // Fetch the tutorial trunk so we can replace the tutorial fork's name. const tutorialTrunk = await this._dbManager.getDoc({...scope, urlId: tutorialTrunkId}, manager); await this._dbManager.updateDocument( diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 37070d9871..7b1e3c04a8 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -493,7 +493,7 @@ export class FlexServer implements GristServer { checks.set('hooks', this._hasTestingHooks); } if (isParameterOn(req.query.db)) { - checks.set('db', asyncCheck(this._dbManager.connection.query('SELECT 1'))); + checks.set('db', asyncCheck(this._dbManager.dataSource.query('SELECT 1'))); } if (isParameterOn(req.query.redis)) { checks.set('redis', asyncCheck(this._docWorkerMap.getRedisClient()?.pingAsync())); @@ -760,10 +760,10 @@ export class FlexServer implements GristServer { if (this._check('homedb')) { return; } this._dbManager = new HomeDBManager(); this._dbManager.setPrefix(process.env.GRIST_ID_PREFIX || ""); - await this._dbManager.connect(); + await this._dbManager.initializeDataSource(); await this._dbManager.initializeSpecialIds(); // Report which database we are using, without sensitive credentials. - this.info.push(['database', getDatabaseUrl(this._dbManager.connection.options, false)]); + this.info.push(['database', getDatabaseUrl(this._dbManager.dataSource.options, false)]); // If the installation appears to be new, give it an id and a creation date. this._activations = new Activations(this._dbManager); await this._activations.current(); @@ -930,6 +930,8 @@ export class FlexServer implements GristServer { // Do this after _shutdown, since DocWorkerMap is used during shutdown. if (this._docWorkerMap) { await this._docWorkerMap.close(); } if (this._sessionStore) { await this._sessionStore.close(); } + // FIXME: can this be moved above instead? + await this._dbManager?.destroyDataSource(); } public addDocApiForwarder() { diff --git a/app/server/lib/dbUtils.ts b/app/server/lib/dbUtils.ts index 754d774ecc..5da5aa7641 100644 --- a/app/server/lib/dbUtils.ts +++ b/app/server/lib/dbUtils.ts @@ -1,7 +1,7 @@ import {synchronizeProducts} from 'app/gen-server/entity/Product'; import {codeRoot} from 'app/server/lib/places'; import {Mutex} from 'async-mutex'; -import {Connection, createConnection, DataSourceOptions, getConnection} from 'typeorm'; +import {DataSource, DataSourceOptions} from 'typeorm'; // Summary of migrations found in database and in code. interface MigrationSummary { @@ -11,10 +11,10 @@ interface MigrationSummary { } // Find the migrations in the database, the migrations in the codebase, and compare the two. -export async function getMigrations(connection: Connection): Promise { +export async function getMigrations(dataSource: DataSource): Promise { let migrationsInDb: string[]; try { - migrationsInDb = (await connection.query('select name from migrations')).map((rec: any) => rec.name); + migrationsInDb = (await dataSource.query('select name from migrations')).map((rec: any) => rec.name); } catch (e) { // If no migrations have run, there'll be no migrations table - which is fine, // it just means 0 migrations run yet. Sqlite+Postgres report this differently, @@ -27,7 +27,7 @@ export async function getMigrations(connection: Connection): Promise (m.constructor as any).name); + const migrationsInCode: string[] = dataSource.migrations.map(m => (m.constructor as any).name); const pendingMigrations = migrationsInCode.filter(m => !migrationsInDb.includes(m)); return { migrationsInDb, @@ -39,80 +39,82 @@ export async function getMigrations(connection: Connection): Promise { + if (!dataSource) { + return await withTmpDataSource(updateDb); + } + await runMigrations(dataSource); + await synchronizeProducts(dataSource, true); } -export function getConnectionName() { +function getDataSourceName() { return process.env.TYPEORM_NAME || 'default'; } /** - * Get a connection to db if one exists, or create one. Serialized to + * Get a datasource for db if one exists, or create one. Serialized to * avoid duplication. */ -const connectionMutex = new Mutex(); +const dataSourceMutex = new Mutex(); -async function buildConnection(overrideConf?: Partial) { +async function buildDataSource(overrideConf?: Partial) { const settings = getTypeORMSettings(overrideConf); - const connection = await createConnection(settings); + const dataSource = new DataSource(settings); + await dataSource.initialize(); // When using Sqlite, set a busy timeout of 3s to tolerate a little - // interference from connections made by tests. Logging doesn't show + // interference from datasources made by tests. Logging doesn't show // any particularly slow queries, but bad luck is possible. // This doesn't affect when Postgres is in use. It also doesn't have - // any impact when there is a single connection to the db, as is the + // any impact when there is a single datasource to the db, as is the // case when Grist is run as a single process. - if (connection.driver.options.type === 'sqlite') { - await connection.query('PRAGMA busy_timeout = 3000'); + if (dataSource.driver.options.type === 'sqlite') { + await dataSource.query('PRAGMA busy_timeout = 3000'); } - return connection; + return dataSource; } -export async function getOrCreateConnection(): Promise { - return connectionMutex.runExclusive(async () => { - try { - // If multiple servers are started within the same process, we - // share the database connection. This saves locking trouble - // with Sqlite. - return getConnection(getConnectionName()); - } catch (e) { - if (!String(e).match(/ConnectionNotFoundError/)) { - throw e; - } - return buildConnection(); - } +export async function createNewDataSource(overrideConf?: Partial): Promise { + return dataSourceMutex.runExclusive(async () => { + return buildDataSource(overrideConf); }); } -export async function createNewConnection(overrideConf?: Partial): Promise { - return connectionMutex.runExclusive(async () => { - return buildConnection(overrideConf); - }); +export async function withTmpDataSource( + cb: (dataSource: DataSource) => Promise, + overrideConf?: Partial +): Promise { + let dataSource: DataSource|null = null; + let res: T; + try { + dataSource = await createNewDataSource(overrideConf); + res = await cb(dataSource); + } finally { + await dataSource?.destroy(); + } + return res; } -export async function runMigrations(connection: Connection) { +export async function runMigrations(datasource: DataSource) { // on SQLite, migrations fail if we don't temporarily disable foreign key // constraint checking. This is because for sqlite typeorm copies each // table and rebuilds it from scratch for each schema change. // Also, we need to disable foreign key constraint checking outside of any // transaction, or it has no effect. - const sqlite = connection.driver.options.type === 'sqlite'; - if (sqlite) { await connection.query("PRAGMA foreign_keys = OFF;"); } - await connection.transaction(async tr => { + const sqlite = datasource.driver.options.type === 'sqlite'; + if (sqlite) { await datasource.query("PRAGMA foreign_keys = OFF;"); } + await datasource.transaction(async tr => { await tr.connection.runMigrations(); }); - if (sqlite) { await connection.query("PRAGMA foreign_keys = ON;"); } + if (sqlite) { await datasource.query("PRAGMA foreign_keys = ON;"); } } -export async function undoLastMigration(connection: Connection) { - const sqlite = connection.driver.options.type === 'sqlite'; - if (sqlite) { await connection.query("PRAGMA foreign_keys = OFF;"); } - await connection.transaction(async tr => { +export async function undoLastMigration(dataSource: DataSource) { + const sqlite = dataSource.driver.options.type === 'sqlite'; + if (sqlite) { await dataSource.query("PRAGMA foreign_keys = OFF;"); } + await dataSource.transaction(async tr => { await tr.connection.undoLastMigration(); }); - if (sqlite) { await connection.query("PRAGMA foreign_keys = ON;"); } + if (sqlite) { await dataSource.query("PRAGMA foreign_keys = ON;"); } } // Replace the old janky ormconfig.js file, which was always a source of @@ -135,7 +137,7 @@ export function getTypeORMSettings(overrideConf?: Partial): D } : undefined; return { - "name": getConnectionName(), + "name": getDataSourceName(), "type": (process.env.TYPEORM_TYPE as any) || "sqlite", // officially, TYPEORM_CONNECTION - // but if we use that, this file will never // be read, and we can't configure diff --git a/app/server/lib/extractOrg.ts b/app/server/lib/extractOrg.ts index 524aa97e38..c0987690bc 100644 --- a/app/server/lib/extractOrg.ts +++ b/app/server/lib/extractOrg.ts @@ -103,7 +103,7 @@ export class Hosts { } else { // Otherwise check for a custom host. const org = await mapGetOrSet(this._host2org, hostname, async () => { - const o = await this._dbManager.connection.manager.findOne(Organization, {where: {host: hostname}}); + const o = await this._dbManager.dataSource.manager.findOne(Organization, {where: {host: hostname}}); return o && o.domain || undefined; }); if (!org) { throw new ApiError(`Domain not recognized: ${hostname}`, 404); } @@ -154,7 +154,7 @@ export class Hosts { if (org && this._getHostType(req.headers.host!) === 'native' && !this._dbManager.isMergedOrg(org)) { // Check if the org has a preferred host. const orgHost = await mapGetOrSet(this._org2host, org, async () => { - const o = await this._dbManager.connection.manager.findOne(Organization, {where: {domain: org}}); + const o = await this._dbManager.dataSource.manager.findOne(Organization, {where: {domain: org}}); return o && o.host || undefined; }); if (orgHost && orgHost !== req.hostname) { diff --git a/stubs/app/server/server.ts b/stubs/app/server/server.ts index 95d3692113..c6576e3bdc 100644 --- a/stubs/app/server/server.ts +++ b/stubs/app/server/server.ts @@ -59,7 +59,7 @@ async function setupDb() { await updateDb(); } const db = new HomeDBManager(); - await db.connect(); + await db.initializeDataSource(); await db.initializeSpecialIds({skipWorkspaces: true}); // If a team/organization is specified, make sure it exists. diff --git a/test/gen-server/ApiServer.ts b/test/gen-server/ApiServer.ts index 07484e8c2b..f7fcaf7092 100644 --- a/test/gen-server/ApiServer.ts +++ b/test/gen-server/ApiServer.ts @@ -929,7 +929,7 @@ describe('ApiServer', function() { assert.isNotNull(org.updatedAt); // Upgrade this org to a fancier plan, and check that vanity domain starts working - const db = dbManager.connection.manager; + const db = dbManager.dataSource.manager; const dbOrg = await db.findOne(Organization, {where: {name: 'Magic'}, relations: ['billingAccount', 'billingAccount.product']}); @@ -2072,7 +2072,7 @@ describe('ApiServer', function() { const user = await dbManager.getUserByLogin('meep@getgrist.com', {profile}); const userId = user.id; // set up an api key - await dbManager.connection.query("update users set api_key = 'api_key_for_meep' where id = $1", [userId]); + await dbManager.dataSource.query("update users set api_key = 'api_key_for_meep' where id = $1", [userId]); // make sure we have at least one extra user, a login, an org, a group_user entry, // a billing account, a billing account manager. @@ -2110,7 +2110,7 @@ describe('ApiServer', function() { const userBlank = await dbManager.getUserByLogin('blank@getgrist.com', {profile: {email: 'blank@getgrist.com', name: ''}}); - await dbManager.connection.query("update users set api_key = 'api_key_for_blank' where id = $1", [userBlank.id]); + await dbManager.dataSource.query("update users set api_key = 'api_key_for_blank' where id = $1", [userBlank.id]); // check that user can delete themselves resp = await axios.delete(`${homeUrl}/api/users/${userBlank.id}`, @@ -2151,7 +2151,7 @@ describe('ApiServer', function() { const prevAccount = await dbManager.getBillingAccount( {userId: dbManager.getPreviewerUserId()}, 'best-friends-squad', false); - await dbManager.connection.query( + await dbManager.dataSource.query( 'update billing_accounts set product_id = (select id from products where name = $1) where id = $2', [TEAM_FREE_PLAN, prevAccount.id] ); @@ -2339,7 +2339,7 @@ describe('ApiServer', function() { // in postgres. async function getNextId(dbManager: HomeDBManager, table: 'orgs'|'workspaces') { // Check current top org id. - const row = await dbManager.connection.query(`select max(id) as id from ${table}`); + const row = await dbManager.dataSource.query(`select max(id) as id from ${table}`); const id = row[0]['id']; return id + 1; } diff --git a/test/gen-server/ApiServerAccess.ts b/test/gen-server/ApiServerAccess.ts index a95494f5f2..ac3566e2a4 100644 --- a/test/gen-server/ApiServerAccess.ts +++ b/test/gen-server/ApiServerAccess.ts @@ -1411,7 +1411,7 @@ describe('ApiServerAccess', function() { before(async function() { // Set NASA to be specifically on a team plan, with team plan restrictions. - const db = dbManager.connection.manager; + const db = dbManager.dataSource.manager; nasaOrg = (await db.findOne(Organization, {where: {domain: 'nasa'}, relations: ['billingAccount', 'billingAccount.product']}))!; @@ -1600,12 +1600,12 @@ describe('ApiServerAccess', function() { describe('force flag is not needed if apiKey is not set', function() { before(function() { // turn off api key access for chimpy - return dbManager.connection.query(`update users set api_key = null where name = 'Chimpy'`); + return dbManager.dataSource.query(`update users set api_key = null where name = 'Chimpy'`); }); after(function() { // bring back api key access for chimpy - return dbManager.connection.query(`update users set api_key = 'api_key_for_chimpy' where name = 'Chimpy'`); + return dbManager.dataSource.query(`update users set api_key = 'api_key_for_chimpy' where name = 'Chimpy'`); }); it('force flag is not needed', async function() { diff --git a/test/gen-server/ApiServerBenchmark.ts b/test/gen-server/ApiServerBenchmark.ts index 22591d6f4f..461d739e5e 100644 --- a/test/gen-server/ApiServerBenchmark.ts +++ b/test/gen-server/ApiServerBenchmark.ts @@ -6,7 +6,7 @@ import {assert} from 'chai'; import {FlexServer} from 'app/server/lib/FlexServer'; -import {createBenchmarkServer, removeConnection, setUpDB} from 'test/gen-server/seed'; +import {createBenchmarkServer, setUpDB} from 'test/gen-server/seed'; let home: FlexServer; let homeUrl: string; @@ -31,7 +31,7 @@ describe('ApiServerBenchmark', function() { after(async function() { if (home) { await home.stopListening(); - await removeConnection(); + await home.getHomeDBManager().destroyDataSource(); } }); diff --git a/test/gen-server/AuthCaching.ts b/test/gen-server/AuthCaching.ts index d28f738274..9418b0b972 100644 --- a/test/gen-server/AuthCaching.ts +++ b/test/gen-server/AuthCaching.ts @@ -10,7 +10,7 @@ import {tmpdir} from 'os'; import * as path from 'path'; import * as sinon from 'sinon'; import {TestSession} from 'test/gen-server/apiUtils'; -import {createInitialDb, removeConnection, setUpDB} from 'test/gen-server/seed'; +import {createInitialDb, setUpDB} from 'test/gen-server/seed'; import {configForUser, getGristConfig} from 'test/gen-server/testUtils'; import {openClient} from 'test/server/gristClient'; import * as testUtils from 'test/server/testUtils'; @@ -80,7 +80,6 @@ describe('AuthCaching', function() { await testUtils.captureLog('warn', async () => { await docsServer.close(); await homeServer.close(); - await removeConnection(); }); }); diff --git a/test/gen-server/apiUtils.ts b/test/gen-server/apiUtils.ts index 2dbaf73cdd..86ef0fe058 100644 --- a/test/gen-server/apiUtils.ts +++ b/test/gen-server/apiUtils.ts @@ -19,7 +19,7 @@ import axios from 'axios'; import FormData from 'form-data'; import fetch from 'node-fetch'; import * as path from 'path'; -import {createInitialDb, removeConnection, setUpDB} from 'test/gen-server/seed'; +import {createInitialDb, setUpDB} from 'test/gen-server/seed'; import {setPlan} from 'test/gen-server/testUtils'; import {fixturesRoot} from 'test/server/testUtils'; import {isAffirmative} from 'app/common/gutil'; @@ -58,7 +58,6 @@ export class TestServer { await delay(100); } } - await removeConnection(); } // Set up a profile for the given org, and return an axios configuration to @@ -74,7 +73,7 @@ export class TestServer { // add named user as billing manager to org (identified by domain) public async addBillingManager(userName: string, orgDomain: string) { - const ents = this.dbManager.connection.createEntityManager(); + const ents = this.dbManager.dataSource.createEntityManager(); const org = await ents.findOne(Organization, { relations: ['billingAccount'], where: {domain: orgDomain} @@ -122,7 +121,7 @@ export class TestServer { * inherits access from the workspace). */ public async listUserMemberships(email: string): Promise { - const rules = await this.dbManager.connection.createQueryBuilder() + const rules = await this.dbManager.dataSource.createQueryBuilder() .select('acl_rules') .from(AclRule, 'acl_rules') .leftJoinAndSelect('acl_rules.group', 'groups') @@ -168,7 +167,7 @@ export class TestServer { // Find instances of guests and members used in inheritance. public async getBadGroupLinks(): Promise { // guests and members should never be in other groups, or have groups. - return this.dbManager.connection.createQueryBuilder() + return this.dbManager.dataSource.createQueryBuilder() .select('groups') .from(Group, 'groups') .innerJoinAndSelect('groups.memberGroups', 'memberGroups') @@ -196,7 +195,7 @@ export class TestServer { * TODO: rework AclRule to automate this kind of step. */ private async _getResourceName(aclRule: AclRule): Promise { - const con = this.dbManager.connection.manager; + const con = this.dbManager.dataSource.manager; let res: Document|Workspace|Organization|null; if (aclRule instanceof AclRuleDoc) { res = await con.findOne(Document, {where: {id: aclRule.docId}}); @@ -216,7 +215,7 @@ export class TestServer { * Filters for groups of the specified name. */ private _listMembers(role: Role|null) { - let q = this.dbManager.connection.createQueryBuilder() + let q = this.dbManager.dataSource.createQueryBuilder() .select('users') .from(User, 'users') .leftJoin('users.groups', 'groups') diff --git a/test/gen-server/lib/DocApiForwarder.ts b/test/gen-server/lib/DocApiForwarder.ts index a9852a3d57..10a3a18f5e 100644 --- a/test/gen-server/lib/DocApiForwarder.ts +++ b/test/gen-server/lib/DocApiForwarder.ts @@ -11,7 +11,7 @@ import morganLogger from 'morgan'; import { AddressInfo } from 'net'; import sinon = require("sinon"); -import { createInitialDb, removeConnection, setUpDB } from "test/gen-server/seed"; +import { createInitialDb, setUpDB } from "test/gen-server/seed"; import { configForUser } from 'test/gen-server/testUtils'; import { DocApiForwarder } from "app/gen-server/lib/DocApiForwarder"; @@ -59,8 +59,8 @@ describe('DocApiForwarder', function() { before(async function() { setUpDB(this); dbManager = new HomeDBManager(); - await dbManager.connect(); - await createInitialDb(dbManager.connection); + await dbManager.initializeDataSource(); + await createInitialDb(dbManager.dataSource); await dbManager.initializeSpecialIds(); // create cheap doc worker @@ -95,7 +95,6 @@ describe('DocApiForwarder', function() { }); after(async function() { - await removeConnection(); homeServer.close(); docWorker.close(); dbManager.flushDocAuthCache(); // To avoid hanging up exit from tests. diff --git a/test/gen-server/lib/DocWorkerMap.ts b/test/gen-server/lib/DocWorkerMap.ts index 73b7d21ce2..25dfa8a0c7 100644 --- a/test/gen-server/lib/DocWorkerMap.ts +++ b/test/gen-server/lib/DocWorkerMap.ts @@ -8,7 +8,7 @@ import {assert, expect} from 'chai'; import {countBy, values} from 'lodash'; import {createClient, RedisClient} from 'redis'; import {TestSession} from 'test/gen-server/apiUtils'; -import {createInitialDb, removeConnection, setUpDB} from 'test/gen-server/seed'; +import {createInitialDb, setUpDB} from 'test/gen-server/seed'; import sinon from 'sinon'; import * as testUtils from 'test/server/testUtils'; @@ -413,7 +413,6 @@ describe('DocWorkerMap', function() { after(async function() { if (servers) { await Promise.all(Object.values(servers).map(server => server.close())); - await removeConnection(); delete process.env.REDIS_URL; delete process.env.GRIST_DOC_WORKER_ID; delete process.env.GRIST_WORKER_GROUP; diff --git a/test/gen-server/lib/HealthCheck.ts b/test/gen-server/lib/HealthCheck.ts index d2fa6441db..2ec8fd495a 100644 --- a/test/gen-server/lib/HealthCheck.ts +++ b/test/gen-server/lib/HealthCheck.ts @@ -61,7 +61,7 @@ describe('HealthCheck', function() { const resolvers: Array<() => void> = []; for (let i = 0; i < driver.master.options.max; i++) { const promise = new Promise((resolve) => { resolvers.push(resolve); }); - blockers.push(server.dbManager.connection.transaction((manager) => promise)); + blockers.push(server.dbManager.dataSource.transaction((manager) => promise)); } return { blockerPromise: Promise.all(blockers), @@ -70,14 +70,14 @@ describe('HealthCheck', function() { } it('reports error when database is unhealthy', async function() { - if (server.dbManager.connection.options.type !== 'postgres') { + if (server.dbManager.dataSource.options.type !== 'postgres') { // On postgres, we have a way to interfere with connections. Elsewhere (sqlite) it's not // so obvious how to make DB unhealthy, so don't bother testing that. this.skip(); } this.timeout(5000); - const {blockerPromise, resolve} = blockPostgres(server.dbManager.connection.driver as any); + const {blockerPromise, resolve} = blockPostgres(server.dbManager.dataSource.driver as any); try { const result = await fetch(server.server.getOwnUrl() + '/status?db=1&redis=1&timeout=500'); assert.match(await result.text(), /Grist server.*unhealthy.*db not ok, redis ok/); diff --git a/test/gen-server/lib/HomeDBManager.ts b/test/gen-server/lib/HomeDBManager.ts index a22c55474e..f54d458a1a 100644 --- a/test/gen-server/lib/HomeDBManager.ts +++ b/test/gen-server/lib/HomeDBManager.ts @@ -315,13 +315,13 @@ describe('HomeDBManager', function() { it('will fail on parameter collision', async function() { // Check collision in a simple query. // Note: it is query construction that fails, not query execution. - assert.throws(() => home.connection.createQueryBuilder().from('orgs', 'orgs') + assert.throws(() => home.dataSource.createQueryBuilder().from('orgs', 'orgs') .where('id = :id', {id: 1}).andWhere('id = :id', {id: 2}), /parameter collision/); // Check collision between subqueries. assert.throws( - () => home.connection.createQueryBuilder().from('orgs', 'orgs') + () => home.dataSource.createQueryBuilder().from('orgs', 'orgs') .select(q => q.subQuery().from('orgs', 'orgs').where('x IN :x', {x: ['five']})) .addSelect(q => q.subQuery().from('orgs', 'orgs').where('x IN :x', {x: ['six']})), /parameter collision/); diff --git a/test/gen-server/lib/Housekeeper.ts b/test/gen-server/lib/Housekeeper.ts index 929b2c8b35..b10c1b3fcb 100644 --- a/test/gen-server/lib/Housekeeper.ts +++ b/test/gen-server/lib/Housekeeper.ts @@ -35,12 +35,12 @@ describe('Housekeeper', function() { }); async function getDoc(docId: string) { - const manager = home.dbManager.connection.manager; + const manager = home.dbManager.dataSource.manager; return manager.findOneOrFail(Document, {where: {id: docId}}); } async function getWorkspace(wsId: number) { - const manager = home.dbManager.connection.manager; + const manager = home.dbManager.dataSource.manager; return manager.findOneOrFail(Workspace, {where: {id: wsId}}); } diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 736f171055..c32588eaea 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -26,7 +26,6 @@ import winston from 'winston'; import fs from 'fs/promises'; import { tmpdir } from 'os'; import path from 'path'; -import { dereferenceConnection } from 'test/gen-server/seed'; const username = process.env.USER || "nobody"; const tmpDirPrefix = path.join(tmpdir(), `grist_test_${username}_userendpoint_`); @@ -264,20 +263,17 @@ describe('UsersManager', function () { async function withDataBase(dbName: string, cb: (db: HomeDBManager) => Promise) { const database = path.join(tmpDir, dbName + '.db'); const localDb = new HomeDBManager(); - await localDb.createNewConnection({ name: dbName, database }); - await updateDb(localDb.connection); + await localDb.initializeDataSource({ name: dbName, database }); + await updateDb(localDb.dataSource); try { await cb(localDb); } finally { - await localDb.connection.destroy(); + await localDb.dataSource.destroy(); // HACK: This is a weird case, we have established a different connection // but the existing connection is also impacted. - // A found workaround consist in destroying and creating again the connection. - // - // TODO: Check whether using DataSource would help and avoid this hack. - await db.connection.destroy(); - await db.createNewConnection(); - dereferenceConnection(dbName); + // A found workaround consist in destroying and creating again the datasource. + await db.dataSource.destroy(); + await db.initializeDataSource(); } } @@ -922,7 +918,7 @@ describe('UsersManager', function () { assertExists(await getPersonalOrg(userToDelete)); - await db.connection.transaction(async (manager) => { + await db.dataSource.transaction(async (manager) => { assert.isTrue(await userHasGroupUsers(userToDelete.id, manager)); assert.isTrue(await userHasPrefs(userToDelete.id, manager)); }); @@ -932,7 +928,7 @@ describe('UsersManager', function () { assert.notExists(await db.getUser(userToDelete.id)); assert.deepEqual(await getPersonalOrg(userToDelete), { errMessage: 'organization not found', status: 404 }); - await db.connection.transaction(async (manager) => { + await db.dataSource.transaction(async (manager) => { assert.isFalse(await userHasGroupUsers(userToDelete.id, manager)); assert.isFalse(await userHasPrefs(userToDelete.id, manager)); }); diff --git a/test/gen-server/lib/limits.ts b/test/gen-server/lib/limits.ts index 669915f112..c6e7883853 100644 --- a/test/gen-server/lib/limits.ts +++ b/test/gen-server/lib/limits.ts @@ -41,7 +41,7 @@ describe('limits', function() { const samHome = await createUser(dbManager, 'sam'); // Overwrite default product const billingId = samHome.billingAccount.id; - await dbManager.connection.createQueryBuilder() + await dbManager.dataSource.createQueryBuilder() .update(BillingAccount) .set({product}) .where('id = :billingId', {billingId}) @@ -309,7 +309,7 @@ describe('limits', function() { // Try smuggling in a doc that breaks the rules // Tweak NASA's product to allow 4 shares per doc. - const db = dbManager.connection.manager; + const db = dbManager.dataSource.manager; const nasaOrg = await db.findOne(Organization, {where: {domain: 'nasa'}, relations: ['billingAccount', 'billingAccount.product']}); diff --git a/test/gen-server/lib/mergedOrgs.ts b/test/gen-server/lib/mergedOrgs.ts index b66860fc87..ca675b87b8 100644 --- a/test/gen-server/lib/mergedOrgs.ts +++ b/test/gen-server/lib/mergedOrgs.ts @@ -4,7 +4,7 @@ import {FlexServer} from 'app/server/lib/FlexServer'; import {main as mergedServerMain} from 'app/server/mergedServerMain'; import axios from 'axios'; import {assert} from 'chai'; -import {createInitialDb, removeConnection, setUpDB} from 'test/gen-server/seed'; +import {createInitialDb, setUpDB} from 'test/gen-server/seed'; import {configForUser, createUser, setPlan} from 'test/gen-server/testUtils'; import * as testUtils from 'test/server/testUtils'; @@ -28,7 +28,6 @@ describe('mergedOrgs', function() { after(async function() { await home.close(); - await removeConnection(); }); it('can list all shared workspaces from personal orgs', async function() { diff --git a/test/gen-server/lib/previewer.ts b/test/gen-server/lib/previewer.ts index 6671c384b2..f71d4fa394 100644 --- a/test/gen-server/lib/previewer.ts +++ b/test/gen-server/lib/previewer.ts @@ -34,7 +34,7 @@ describe('previewer', function() { dbManager = home.dbManager; homeUrl = home.serverUrl; // for these tests, give the previewer an api key. - await dbManager.connection.query(`update users set api_key = 'api_key_for_thumbnail' where name = 'Preview'`); + await dbManager.dataSource.query(`update users set api_key = 'api_key_for_thumbnail' where name = 'Preview'`); }); after(async function() { diff --git a/test/gen-server/lib/scrubUserFromOrg.ts b/test/gen-server/lib/scrubUserFromOrg.ts index c670a0f73f..2668d94da7 100644 --- a/test/gen-server/lib/scrubUserFromOrg.ts +++ b/test/gen-server/lib/scrubUserFromOrg.ts @@ -27,7 +27,7 @@ describe('scrubUserFromOrg', function() { // count how many rows there are in the group_users table, for sanity checks. async function countGroupUsers() { - return await server.dbManager.connection.manager.count('group_users'); + return await server.dbManager.dataSource.manager.count('group_users'); } // get the home api, making sure the user's api key is set. diff --git a/test/gen-server/lib/urlIds.ts b/test/gen-server/lib/urlIds.ts index d15fa46f29..0763e2bd72 100644 --- a/test/gen-server/lib/urlIds.ts +++ b/test/gen-server/lib/urlIds.ts @@ -80,7 +80,7 @@ describe('urlIds', function() { }); it(`cannot use an existing docId as a urlId in ${org}`, async function() { - const doc = await home.dbManager.connection.manager.findOneOrFail(Document, {where: {}}); + const doc = await home.dbManager.dataSource.manager.findOneOrFail(Document, {where: {}}); const prevDocId = doc.id; try { // Change doc id to ensure it has characters permitted for a urlId. diff --git a/test/gen-server/migrations.ts b/test/gen-server/migrations.ts index bc8dd1b1dc..2f4b19d9a7 100644 --- a/test/gen-server/migrations.ts +++ b/test/gen-server/migrations.ts @@ -4,7 +4,7 @@ import {Organization} from 'app/gen-server/entity/Organization'; import {HomeDBManager} from 'app/gen-server/lib/homedb/HomeDBManager'; import {Permissions} from 'app/gen-server/lib/Permissions'; import {assert} from 'chai'; -import {addSeedData, createInitialDb, removeConnection, setUpDB} from 'test/gen-server/seed'; +import {addSeedData, createInitialDb, setUpDB} from 'test/gen-server/seed'; import {EnvironmentSnapshot} from 'test/server/testUtils'; import {Initial1536634251710 as Initial} from 'app/gen-server/migration/1536634251710-Initial'; @@ -85,25 +85,25 @@ describe('migrations', function() { }); beforeEach(async function() { - await home.connect(); + await home.initializeDataSource(); // If testing against postgres, remove all tables. // If SQLite, we're using a fresh in-memory db each time. - const sqlite = home.connection.driver.options.type === 'sqlite'; + const sqlite = home.dataSource.driver.options.type === 'sqlite'; if (!sqlite) { - await home.connection.query('DROP SCHEMA public CASCADE'); - await home.connection.query('CREATE SCHEMA public'); + await home.dataSource.query('DROP SCHEMA public CASCADE'); + await home.dataSource.query('CREATE SCHEMA public'); } - await createInitialDb(home.connection, false); + await createInitialDb(home.dataSource, false); }); afterEach(async function() { - await removeConnection(); + await home.destroyDataSource(); }); // a test to exercise the rollback scripts a bit it('can migrate, do full rollback, and migrate again', async function() { this.timeout(60000); - const runner = home.connection.createQueryRunner(); + const runner = home.dataSource.createQueryRunner(); for (const migration of migrations) { await (new migration()).up(runner); } @@ -113,14 +113,14 @@ describe('migrations', function() { for (const migration of migrations) { await (new migration()).up(runner); } - await addSeedData(home.connection); + await addSeedData(home.dataSource); // if we made it this far without an exception, then the rollback scripts must // be doing something. }); it('can migrate UserUUID and UserUniqueRefUUID with user in table', async function() { this.timeout(60000); - const runner = home.connection.createQueryRunner(); + const runner = home.dataSource.createQueryRunner(); // Create 400 users to test the chunk (each chunk is 300 users) const nbUsersToCreate = 400; @@ -142,19 +142,19 @@ describe('migrations', function() { const setOfUserRefs = new Set(userList.map(u => u.ref)); assert.equal(nbUsersToCreate, userList.length); assert.equal(setOfUserRefs.size, userList.length); - await addSeedData(home.connection); + await addSeedData(home.dataSource); }); it('can correctly switch display_email column to non-null with data', async function() { this.timeout(60000); - const sqlite = home.connection.driver.options.type === 'sqlite'; + const sqlite = home.dataSource.driver.options.type === 'sqlite'; // sqlite migrations need foreign keys turned off temporarily - if (sqlite) { await home.connection.query("PRAGMA foreign_keys = OFF;"); } - const runner = home.connection.createQueryRunner(); + if (sqlite) { await home.dataSource.query("PRAGMA foreign_keys = OFF;"); } + const runner = home.dataSource.createQueryRunner(); for (const migration of migrations) { await (new migration()).up(runner); } - await addSeedData(home.connection); + await addSeedData(home.dataSource); // migrate back until just before display_email column added, so we have no // display_emails for (const migration of migrations.slice().reverse()) { @@ -164,18 +164,18 @@ describe('migrations', function() { // now check DisplayEmail and DisplayEmailNonNull succeed with data in the db. await (new DisplayEmail()).up(runner); await (new DisplayEmailNonNull()).up(runner); - if (sqlite) { await home.connection.query("PRAGMA foreign_keys = ON;"); } + if (sqlite) { await home.dataSource.query("PRAGMA foreign_keys = ON;"); } }); // a test to ensure the TeamMember migration works on databases with existing content it('can perform TeamMember migration with seed data set', async function() { this.timeout(30000); - const runner = home.connection.createQueryRunner(); + const runner = home.dataSource.createQueryRunner(); // Perform full up migration and add the seed data. for (const migration of migrations) { await (new migration()).up(runner); } - await addSeedData(home.connection); + await addSeedData(home.dataSource); const initAclCount = await getAclRowCount(runner); const initGroupCount = await getGroupRowCount(runner); diff --git a/test/gen-server/seed.ts b/test/gen-server/seed.ts index 4f9791ac3a..a44dfaeb23 100644 --- a/test/gen-server/seed.ts +++ b/test/gen-server/seed.ts @@ -25,7 +25,7 @@ import {addPath} from 'app-module-path'; import {Context} from 'mocha'; import * as path from 'path'; -import {Connection, getConnectionManager, Repository} from 'typeorm'; +import {DataSource, Repository} from 'typeorm'; if (require.main === module) { addPath(path.dirname(path.dirname(__dirname))); @@ -43,7 +43,7 @@ import {Workspace} from "app/gen-server/entity/Workspace"; import {EXAMPLE_WORKSPACE_NAME} from 'app/gen-server/lib/homedb/HomeDBManager'; import {Permissions} from 'app/gen-server/lib/Permissions'; import { - getConnectionName, getOrCreateConnection, runMigrations, undoLastMigration, updateDb + createNewDataSource, runMigrations, undoLastMigration, updateDb, withTmpDataSource } from 'app/server/lib/dbUtils'; import {FlexServer} from 'app/server/lib/FlexServer'; import * as fse from 'fs-extra'; @@ -284,9 +284,9 @@ class Seed { public groupRepository: Repository; public groups: {[key: string]: Groups}; - constructor(public connection: Connection) { - this.userRepository = connection.getRepository(User); - this.groupRepository = connection.getRepository(Group); + constructor(public dataSource: DataSource) { + this.userRepository = dataSource.getRepository(User); + this.groupRepository = dataSource.getRepository(Group); this.groups = {}; } @@ -422,7 +422,7 @@ class Seed { } public async addUserToGroup(user: User, group: Group) { - await this.connection.createQueryBuilder() + await this.dataSource.createQueryBuilder() .relation(Group, "memberUsers") .of(group) .add(user); @@ -490,7 +490,7 @@ class Seed { return; } - await this.connection.runMigrations(); + await this.dataSource.runMigrations(); const benchmarkOrgs = _generateData(100, 50, 20); // Create an access object giving Chimpy random access to the orgs. @@ -526,31 +526,7 @@ class Seed { } } -// When running mocha on several test files at once, we need to reset our database connection -// if it exists. This is a little ugly since it is stored globally. -export async function removeConnection(name?: string) { - const connections = getConnectionManager().connections; - if (connections.length > 0) { - if (connections.length > 1) { - throw new Error("unexpected number of connections"); - } - await connections[0].destroy(); - dereferenceConnection(getConnectionName()); - } -} - -export function dereferenceConnection(name: string) { - // There seem to be no official way to delete connections. - // Also we should probably get rid of the use of connectionManager, which is deprecated - const connectionMgr = getConnectionManager(); - const connectionMap = (connectionMgr as any).connectionMap as Map; - if (!connectionMap.has(name)) { - throw new Error('connection with this name not found: ' + name); - } - connectionMap.delete(name); -} - -export async function createInitialDb(connection?: Connection, migrateAndSeedData: boolean = true) { +export async function createInitialDb(dataSource?: DataSource, migrateAndSeedData: boolean = true) { // In jenkins tests, we may want to reset the database to a clean // state. If so, TEST_CLEAN_DATABASE will have been set. How to // clean the database depends on what kind of database it is. With @@ -559,10 +535,10 @@ export async function createInitialDb(connection?: Connection, migrateAndSeedDat // are only allowed to do this if there is no connection open to it // (so we fail if a connection has already been made). If the // sqlite db is in memory (":memory:") there's nothing to delete. - const uncommitted = !connection; // has user already created a connection? + const uncommitted = !dataSource; // has user already created a connection? // if so we won't be able to delete sqlite db - connection = connection || await getOrCreateConnection(); - const opt = connection.driver.options; + dataSource = dataSource || await createNewDataSource(); + const opt = dataSource.driver.options; if (process.env.TEST_CLEAN_DATABASE) { if (opt.type === 'sqlite') { const database = (opt as any).database; @@ -572,16 +548,16 @@ export async function createInitialDb(connection?: Connection, migrateAndSeedDat if (!uncommitted) { throw Error("too late to clean sqlite db"); } - await removeConnection(); if (await fse.pathExists(database)) { await fse.unlink(database); } - connection = await getOrCreateConnection(); + await dataSource.destroy(); + await dataSource.initialize(); } } else if (opt.type === 'postgres') { // recreate schema, destroying everything that was inside it - await connection.query("DROP SCHEMA public CASCADE;"); - await connection.query("CREATE SCHEMA public;"); + await dataSource.query("DROP SCHEMA public CASCADE;"); + await dataSource.query("CREATE SCHEMA public;"); } else { throw new Error(`do not know how to clean a ${opt.type} db`); } @@ -589,24 +565,27 @@ export async function createInitialDb(connection?: Connection, migrateAndSeedDat // Finally - actually initialize the database. if (migrateAndSeedData) { - await updateDb(connection); - await addSeedData(connection); + await updateDb(dataSource); + await addSeedData(dataSource); } } // add some test data to the database. -export async function addSeedData(connection: Connection) { - await synchronizeProducts(connection, true, testProducts); - await connection.transaction(async tr => { +export async function addSeedData(dataSource: DataSource) { + await synchronizeProducts(dataSource, true, testProducts); + await dataSource.transaction(async tr => { const seed = new Seed(tr.connection); await seed.run(); }); } -export async function createBenchmarkDb(connection?: Connection) { - connection = connection || await getOrCreateConnection(); - await updateDb(connection); - await connection.transaction(async tr => { +export async function createBenchmarkDb(dataSource?: DataSource): Promise { + if (!dataSource) { + return await withTmpDataSource(createBenchmarkDb); + } + + await updateDb(dataSource); + await dataSource.transaction(async tr => { const seed = new Seed(tr.connection); await seed.runBenchmark(); }); @@ -624,7 +603,7 @@ export async function createServer(port: number, initDb = createInitialDb): Prom flexServer.addApiMiddleware(); flexServer.addHomeApi(); flexServer.addApiErrorHandlers(); - await initDb(flexServer.getHomeDBManager().connection); + await initDb(flexServer.getHomeDBManager().dataSource); flexServer.summary(); return flexServer; } @@ -681,18 +660,18 @@ async function main() { await createInitialDb(); return; } else if (cmd === 'benchmark') { - const connection = await getOrCreateConnection(); + const connection = await createNewDataSource(); await createInitialDb(connection, false); await createBenchmarkDb(connection); return; } else if (cmd === 'migrate') { process.env.TYPEORM_LOGGING = 'true'; - const connection = await getOrCreateConnection(); + const connection = await createNewDataSource(); await runMigrations(connection); return; } else if (cmd === 'revert') { process.env.TYPEORM_LOGGING = 'true'; - const connection = await getOrCreateConnection(); + const connection = await createNewDataSource(); await undoLastMigration(connection); return; } else if (cmd === 'serve') { diff --git a/test/gen-server/testUtils.ts b/test/gen-server/testUtils.ts index a942773a57..2993c15c12 100644 --- a/test/gen-server/testUtils.ts +++ b/test/gen-server/testUtils.ts @@ -58,10 +58,10 @@ export async function createUser(dbManager: HomeDBManager, name: string): Promis */ export async function setPlan(dbManager: HomeDBManager, org: {billingAccount?: {id: number}}, productName: string) { - const product = await dbManager.connection.manager.findOne(Product, {where: {name: productName}}); + const product = await dbManager.dataSource.manager.findOne(Product, {where: {name: productName}}); if (!product) { throw new Error(`cannot find product ${productName}`); } if (!org.billingAccount) { throw new Error('must join billingAccount'); } - await dbManager.connection.createQueryBuilder() + await dbManager.dataSource.createQueryBuilder() .update(BillingAccount) .set({product}) .where('id = :bid', {bid: org.billingAccount.id}) @@ -92,7 +92,7 @@ export async function waitForAllNotifications(notifier: INotifier, maxWait: numb // count the number of rows in a table export async function getRowCount(dbManager: HomeDBManager, tableName: string): Promise { - const result = await dbManager.connection.query(`select count(*) as ct from ${tableName}`); + const result = await dbManager.dataSource.query(`select count(*) as ct from ${tableName}`); return parseInt(result[0].ct, 10); } diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 25052905a8..f1913dd375 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -768,7 +768,7 @@ export async function enterGridRows(cell: ICellSelect, rowsOfValues: string[][]) export async function setApiKey(username: string, apiKey?: string) { apiKey = apiKey || `api_key_for_${username.toLowerCase()}`; const dbManager = await server.getDatabase(); - await dbManager.connection.query(`update users set api_key = $1 where name = $2`, + await dbManager.dataSource.query(`update users set api_key = $1 where name = $2`, [apiKey, username]); if (!await dbManager.getUserByKey(apiKey)) { throw new Error(`setApiKey failed: user ${username} may not yet be in the database`); @@ -780,7 +780,7 @@ export async function setApiKey(username: string, apiKey?: string) { */ export async function updateOrgPlan(orgName: string, productName: string = 'team') { const dbManager = await server.getDatabase(); - const db = dbManager.connection.manager; + const db = dbManager.dataSource.manager; const dbOrg = await db.findOne(Organization, {where: {name: orgName}, relations: ['billingAccount', 'billingAccount.product']}); if (!dbOrg) { throw new Error(`cannot find org ${orgName}`); } diff --git a/test/nbrowser/testServer.ts b/test/nbrowser/testServer.ts index 11fded0178..0eb2f107ce 100644 --- a/test/nbrowser/testServer.ts +++ b/test/nbrowser/testServer.ts @@ -24,7 +24,6 @@ import {driver, IMochaServer, WebDriver} from 'mocha-webdriver'; import fetch from 'node-fetch'; import {tmpdir} from 'os'; import * as path from 'path'; -import {removeConnection} from 'test/gen-server/seed'; import {HomeUtil} from 'test/nbrowser/homeUtil'; import {getDatabase} from 'test/testUtils'; @@ -273,8 +272,8 @@ export class TestServerMerged extends EventEmitter implements IMochaServer { } public async closeDatabase() { + await this._dbManager?.destroyDataSource(); this._dbManager = undefined; - await removeConnection(); } public get driver() { diff --git a/test/server/fixSiteProducts.ts b/test/server/fixSiteProducts.ts index 353b4ac8e4..f23012378f 100644 --- a/test/server/fixSiteProducts.ts +++ b/test/server/fixSiteProducts.ts @@ -39,7 +39,7 @@ describe('fixSiteProducts', function() { it('fixes sites that where created with a wrong product', async function() { const db = server.dbManager; const user = await db.getUserByLogin(email, {profile}) as any; - const getOrg = (id: number) => db.connection.manager.findOne( + const getOrg = (id: number) => db.dataSource.manager.findOne( Organization, {where: {id}, relations: ['billingAccount', 'billingAccount.product']}); @@ -132,7 +132,7 @@ describe('fixSiteProducts', function() { product: 'teamFree', })); - const getOrg = (id: number) => db.connection.manager.findOne(Organization, + const getOrg = (id: number) => db.dataSource.manager.findOne(Organization, {where: {id}, relations: ['billingAccount', 'billingAccount.product']}); const productOrg = (id: number) => getOrg(id)?.then(org => org?.billingAccount?.product?.name); assert.equal(await productOrg(orgId), 'teamFree'); diff --git a/test/server/lib/Authorizer.ts b/test/server/lib/Authorizer.ts index c4e4fe9947..d8f1d17411 100644 --- a/test/server/lib/Authorizer.ts +++ b/test/server/lib/Authorizer.ts @@ -5,7 +5,7 @@ import {FlexServer} from 'app/server/lib/FlexServer'; import axios from 'axios'; import {assert} from 'chai'; import {toPairs} from 'lodash'; -import {createInitialDb, removeConnection, setUpDB} from 'test/gen-server/seed'; +import {createInitialDb, setUpDB} from 'test/gen-server/seed'; import {configForUser, getGristConfig} from 'test/gen-server/testUtils'; import {createDocTools} from 'test/server/docTools'; import {openClient} from 'test/server/gristClient'; @@ -90,7 +90,6 @@ describe('Authorizer', function() { after(async function() { const messages = await testUtils.captureLog('warn', async () => { await server.close(); - await removeConnection(); }); assert.lengthOf(messages, 0); oldEnv.restore(); diff --git a/test/server/lib/DocApi.ts b/test/server/lib/DocApi.ts index a12f17565a..3ca97addd0 100644 --- a/test/server/lib/DocApi.ts +++ b/test/server/lib/DocApi.ts @@ -2949,7 +2949,7 @@ function testDocApi() { assert.equal(resp.status, 200); const db = await getDatabase(); - const shares = await db.connection.query('select * from shares'); + const shares = await db.dataSource.query('select * from shares'); const {key} = shares[0]; resp = await axios.get(`${serverUrl}/api/docs/${docId}/tables/Table1/records`, chimpy); diff --git a/test/server/lib/GranularAccess.ts b/test/server/lib/GranularAccess.ts index c722de3072..03da6a689d 100644 --- a/test/server/lib/GranularAccess.ts +++ b/test/server/lib/GranularAccess.ts @@ -95,7 +95,7 @@ describe('GranularAccess', function() { } async function getShareKeyForUrl(linkId: string) { - const shares = await home.dbManager.connection.query( + const shares = await home.dbManager.dataSource.query( 'select * from shares where link_id = ?', [linkId]); const key = shares[0].key; if (!key) { @@ -3729,7 +3729,7 @@ describe('GranularAccess', function() { ]); // Check it reached the home db. - let shares = await home.dbManager.connection.query('select * from shares'); + let shares = await home.dbManager.dataSource.query('select * from shares'); assert.lengthOf(shares, 1); assert.equal(shares[0].link_id, 'x'); assert.deepEqual(JSON.parse(shares[0].options), @@ -3804,7 +3804,7 @@ describe('GranularAccess', function() { options: '{"publish": true, "test": true}' }], ]); - shares = await home.dbManager.connection.query('select * from shares'); + shares = await home.dbManager.dataSource.query('select * from shares'); assert.lengthOf(shares, 1); assert.deepEqual(JSON.parse(shares[0].options), {publish: true, test: true}); @@ -3822,7 +3822,7 @@ describe('GranularAccess', function() { await owner.applyUserActions(docId, [ ['RemoveRecord', '_grist_Shares', 1] ]); - shares = await home.dbManager.connection.query('select * from shares'); + shares = await home.dbManager.dataSource.query('select * from shares'); assert.lengthOf(shares, 0); }); @@ -4053,7 +4053,7 @@ describe('GranularAccess', function() { await hamShare.addRows('Customer', { Name: ['Foo'] }); await assert.isRejected(hamShare.addRows('Actor', { Name: ['Foo'] })); await removeShares(docId, owner); - const shares = await home.dbManager.connection.query('select * from shares'); + const shares = await home.dbManager.dataSource.query('select * from shares'); assert.lengthOf(shares, 0); }); @@ -4069,7 +4069,7 @@ describe('GranularAccess', function() { ]); // Check it reached the home db. - let shares = await home.dbManager.connection.query('select * from shares'); + let shares = await home.dbManager.dataSource.query('select * from shares'); assert.lengthOf(shares, 1); assert.equal(shares[0].link_id, 'x2'); @@ -4078,7 +4078,7 @@ describe('GranularAccess', function() { }); // Do anything with the new document. await owner.getDocAPI(copyDocId).getRows('Table1'); - shares = await home.dbManager.connection.query('select * from shares'); + shares = await home.dbManager.dataSource.query('select * from shares'); assert.lengthOf(shares, 2); assert.equal(shares[0].link_id, 'x2'); assert.equal(shares[1].link_id, 'x2'); diff --git a/test/server/lib/HostedStorageManager.ts b/test/server/lib/HostedStorageManager.ts index aa164892f8..3e49f9129c 100644 --- a/test/server/lib/HostedStorageManager.ts +++ b/test/server/lib/HostedStorageManager.ts @@ -5,6 +5,7 @@ import {DocWorkerMap} from 'app/gen-server/lib/DocWorkerMap'; import {HomeDBManager} from 'app/gen-server/lib/homedb/HomeDBManager'; import {ActiveDoc} from 'app/server/lib/ActiveDoc'; import {create} from 'app/server/lib/create'; +import { createNewDataSource } from 'app/server/lib/dbUtils'; import {DocManager} from 'app/server/lib/DocManager'; import {makeExceptionalDocSession} from 'app/server/lib/DocSession'; import {DELETED_TOKEN, ExternalStorage, wrapWithKeyMappedStorage} from 'app/server/lib/ExternalStorage'; @@ -23,10 +24,11 @@ import * as fse from 'fs-extra'; import * as path from 'path'; import {createClient, RedisClient} from 'redis'; import * as sinon from 'sinon'; -import {createInitialDb, removeConnection, setUpDB} from 'test/gen-server/seed'; +import {createInitialDb, setUpDB} from 'test/gen-server/seed'; import {createTmpDir, getGlobalPluginManager} from 'test/server/docTools'; import {EnvironmentSnapshot, setTmpLogLevel, useFixtureDoc} from 'test/server/testUtils'; import {waitForIt} from 'test/server/wait'; +import { DataSource } from 'typeorm'; import uuidv4 from "uuid/v4"; bluebird.promisifyAll(RedisClient.prototype); @@ -289,7 +291,7 @@ class TestStore { await this.end(); this._active = true; const dbManager = new HomeDBManager(); - await dbManager.connect(); + await dbManager.initializeDataSource(); await dbManager.initializeSpecialIds(); const options: HostedStorageOptions = { secondsBeforePush: 0.5, @@ -359,6 +361,7 @@ class TestStore { describe('HostedStorageManager', function() { + let dataSource: DataSource; setTmpLogLevel('info'); // allow info messages for this test since failures are hard to replicate this.timeout(60000); // s3 can be slow @@ -366,11 +369,12 @@ describe('HostedStorageManager', function() { before(async function() { setUpDB(this); - await createInitialDb(); + dataSource = await createNewDataSource(); + await createInitialDb(dataSource); }); after(async function() { - await removeConnection(); + await dataSource.destroy(); }); for (const storage of ['azure', 's3', 'minio', 'cached'] as const) { diff --git a/test/testUtils.ts b/test/testUtils.ts index e8affb5a49..10e814758f 100644 --- a/test/testUtils.ts +++ b/test/testUtils.ts @@ -6,7 +6,7 @@ export async function getDatabase(typeormDb?: string): Promise { process.env.TYPEORM_DATABASE = typeormDb; } const db = new HomeDBManager(); - await db.connect(); + await db.initializeDataSource(); await db.initializeSpecialIds(); if (origTypeormDB) { process.env.TYPEORM_DATABASE = origTypeormDB;