Add tests for UsersManager

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);
     }));
 

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';
@@ -70,6 +70,7 @@ import {
   Brackets,
   Connection,
   DatabaseType,
+  DataSourceOptions,
   EntityManager,
   ObjectLiteral,
   SelectQueryBuilder,
@@ -248,7 +249,6 @@ export type BillingOptions = Partial<Pick<BillingAccount,
 export class HomeDBManager extends EventEmitter {
   private _usersManager = new UsersManager(this, this._runInTransaction.bind(this));
   private _connection: Connection;
-  private _dbType: DatabaseType;
   private _exampleWorkspaceId: number;
   private _exampleOrgId: number;
   private _idPrefix: string = "";  // Place this before ids in subdomains, used in routing to
@@ -258,6 +258,10 @@ export class HomeDBManager extends EventEmitter {
   // In restricted mode, documents should be read-only.
   private _restrictedMode: boolean = false;
 
+  private get _dbType(): DatabaseType {
+    return this._connection.driver.options.type;
+  }
+
   /**
    * Five aclRules, each with one group (with the names 'owners', 'editors', 'viewers',
    * 'guests', and 'members') are created by default on every new entity (Organization,
@@ -348,7 +352,10 @@ export class HomeDBManager extends EventEmitter {
 
   public async connect(): Promise<void> {
     this._connection = await getOrCreateConnection();
-    this._dbType = this._connection.driver.options.type;
+  }
+
+  public async createNewConnection(overrideConf?: Partial<DataSourceOptions>): Promise<void> {
+    this._connection = await createNewConnection(overrideConf);
   }
 
   // make sure special users and workspaces are available
@@ -461,25 +468,21 @@ 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<UserOptions>) {
     return this._usersManager.updateUserOptions(userId, props);
   }
 
   /**
    * @see UsersManager.prototype.getUserByLoginWithRetry
    */
-  public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise<User|undefined> {
+  public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise<User> {
     return this._usersManager.getUserByLoginWithRetry(email, options);
   }
 
   /**
    * @see UsersManager.prototype.getUserByLogin
    */
-  public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise<User|undefined> {
+  public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise<User> {
     return this._usersManager.getUserByLogin(email, options);
   }
 
@@ -4362,7 +4365,6 @@ export class HomeDBManager extends EventEmitter {
     });
     return verifyEntity(orgQuery);
   }
-
 }
 
 // Return a QueryResult reflecting the output of a query builder.

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 = <T>(
   transaction: EntityManager|undefined,
-  op: ((manager: EntityManager) => Promise<any>)
-) => Promise<any>;
+  op: ((manager: EntityManager) => Promise<T>)
+) => Promise<T>;

app/gen-server/lib/homedb/UsersManager.ts

@@ -97,7 +97,7 @@ 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});
+        const user = await this.getExistingUserByLogin(email, manager);
         if (user) {
           await manager.delete(Pref, {userId: user.id});
         }
@@ -116,7 +116,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;
   }
 
@@ -125,7 +125,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;
   }
 
@@ -134,7 +134,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;
   }
 
@@ -143,7 +143,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;
   }
 
@@ -221,9 +221,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();
@@ -286,13 +283,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<UserOptions>) {
     const user = await User.findOne({where: {id: userId}});
     if (!user) { throw new ApiError("unable to find user", 400); }
@@ -321,7 +312,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<User|undefined> {
+  public async getUserByLoginWithRetry(email: string, options: GetUserOptions = {}): Promise<User> {
     try {
       return await this.getUserByLogin(email, options);
     } catch (e) {
@@ -361,10 +352,10 @@ export class UsersManager {
    * unset/outdated fields of an existing record.
    *
    */
-  public async getUserByLogin(email: string, options: GetUserOptions = {}): Promise<User|undefined> {
+  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 await this._runInTransaction(transaction, async manager => {
       let needUpdate = false;
       const userQuery = manager.createQueryBuilder()
         .select('user')
@@ -473,9 +464,8 @@ export class UsersManager {
         // In principle this could be optimized, but this is simpler to maintain.
         user = await userQuery.getOne();
       }
-      return user;
+      return user!;
     });
-    return userByLogin;
   }
 
   /**
@@ -520,6 +510,63 @@ export class UsersManager {
     };
   }
 
+  public async initializeSpecialIds(): Promise<void> {
+    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<FullUser[]> {
+    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.
@@ -589,25 +636,6 @@ export class UsersManager {
     };
   }
 
-  public async initializeSpecialIds(): Promise<void> {
-    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
@@ -684,34 +712,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<FullUser[]> {
-    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.
@@ -735,7 +735,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;

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,

app/server/lib/TestLogin.ts

@@ -25,10 +25,8 @@ export async function getTestLoginSystem(): Promise<GristLoginSystem> {
           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";
         },