fix(cli-repl): do not enable telemetry unless user id is persisted MONGOSH-1320

.evergreen.yml

@@ -10224,6 +10224,25 @@ tasks:
         vars:
           node_js_version: "16.17.0"
           dockerfile: ubuntu22.04-deb
+  - name: pkg_test_docker_deb_x64_ubuntu22_04_nohome_deb
+    tags: ["smoke-test"]
+    depends_on:
+      - name: package_and_upload_artifact_deb_x64
+        variant: linux_package
+    commands:
+      - func: checkout
+      - func: get_artifact_url
+        vars:
+          source_package_variant: deb-x64
+      - func: write_preload_script
+      - func: install
+        vars:
+          node_js_version: "16.17.0"
+          npm_deps_mode: cli_build
+      - func: test_artifact_docker
+        vars:
+          node_js_version: "16.17.0"
+          dockerfile: ubuntu22.04-nohome-deb
   - name: pkg_test_docker_deb_x64_debian9_deb
     tags: ["smoke-test"]
     depends_on:
@@ -11826,6 +11845,7 @@ buildvariants:
       - name: pkg_test_docker_deb_x64_ubuntu18_04_deb
       - name: pkg_test_docker_deb_x64_ubuntu20_04_deb
       - name: pkg_test_docker_deb_x64_ubuntu22_04_deb
+      - name: pkg_test_docker_deb_x64_ubuntu22_04_nohome_deb
       - name: pkg_test_docker_deb_x64_debian9_deb
       - name: pkg_test_docker_deb_x64_debian10_deb
       - name: pkg_test_docker_deb_x64_debian11_deb

config/release-package-matrix.js

@@ -20,7 +20,7 @@ exports.RELEASE_PACKAGE_MATRIX = [
     compileBuildVariant: 'linux_x64_build',
     packages: [
       { name: 'linux-x64', description: 'Linux Tarball 64-bit', packageOn: 'linux_package', smokeTestKind: 'docker', smokeTestDockerfiles: ['ubuntu20.04-tgz'] },
-      { name: 'deb-x64', description: 'Debian / Ubuntu 64-bit', packageOn: 'linux_package', smokeTestKind: 'docker', smokeTestDockerfiles: ['ubuntu18.04-deb', 'ubuntu20.04-deb', 'ubuntu22.04-deb', 'debian9-deb', 'debian10-deb', 'debian11-deb'] },
+      { name: 'deb-x64', description: 'Debian / Ubuntu 64-bit', packageOn: 'linux_package', smokeTestKind: 'docker', smokeTestDockerfiles: ['ubuntu18.04-deb', 'ubuntu20.04-deb', 'ubuntu22.04-deb', 'ubuntu22.04-nohome-deb', 'debian9-deb', 'debian10-deb', 'debian11-deb'] },
       { name: 'rpm-x64', description: 'RHEL / CentOS / Fedora / Suse 64-bit', packageOn: 'linux_package', smokeTestKind: 'docker', smokeTestDockerfiles: ['centos7-rpm', 'amazonlinux2-rpm', 'amazonlinux2022-rpm', 'rocky8-rpm', 'rocky9-rpm', 'fedora34-rpm', 'suse12-rpm', 'suse15-rpm', 'amazonlinux1-rpm'] }
     ]
   },

packages/cli-repl/src/cli-repl.ts

@@ -98,6 +98,7 @@ class CliRepl implements MongoshIOProvider {
   onExit: (code?: number) => Promise<never>;
   closing = false;
   isContainerizedEnvironment = false;
+  hasOnDiskTelemetryId = false;
 
   /**
    * Instantiate the new CLI Repl.
@@ -126,10 +127,12 @@ class CliRepl implements MongoshIOProvider {
         this.bus.emit('mongosh:error', err, 'config');
       })
       .on('new-config', (config: CliUserConfigOnDisk) => {
+        this.hasOnDiskTelemetryId = !!(config.userId || config.telemetryAnonymousId);
         this.setTelemetryEnabled(config.enableTelemetry);
         this.bus.emit('mongosh:new-user', { userId: config.userId, anonymousId: config.telemetryAnonymousId });
       })
       .on('update-config', (config: CliUserConfigOnDisk) => {
+        this.hasOnDiskTelemetryId = !!(config.userId || config.telemetryAnonymousId);
         this.setTelemetryEnabled(config.enableTelemetry);
         this.bus.emit('mongosh:update-user', { userId: config.userId, anonymousId: config.telemetryAnonymousId });
       });
@@ -368,7 +371,8 @@ class CliRepl implements MongoshIOProvider {
       // case.
       return;
     }
-    if (enabled && !this.forceDisableTelemetry) {
+
+    if (enabled && this.hasOnDiskTelemetryId && !this.forceDisableTelemetry) {
       this.toggleableAnalytics.enable();
     } else {
       this.toggleableAnalytics.disable();

packages/cli-repl/src/smoke-tests.ts

@@ -24,11 +24,24 @@ export async function runSmokeTests(smokeTestServer: string | undefined, executa
   const expectFipsSupport = !!process.env.MONGOSH_SMOKE_TEST_OS_HAS_FIPS_SUPPORT && buildInfo().sharedOpenssl;
   console.log('FIPS support required to pass?', { expectFipsSupport });
 
-  for (const { input, output, testArgs, includeStderr } of [{
+  for (const { input, output, testArgs, includeStderr, exitCode } of [{
     input: 'print("He" + "llo" + " Wor" + "ld!")',
     output: /Hello World!/,
     includeStderr: false,
     testArgs: ['--nodb'],
+    exitCode: 0
+  }, {
+    input: '',
+    output: /ReferenceError/,
+    includeStderr: true,
+    testArgs: ['--nodb', '--eval', 'foo.bar()'],
+    exitCode: 1
+  }, {
+    input: '',
+    output: /Hello World!/,
+    includeStderr: false,
+    testArgs: ['--nodb', '--eval', 'print("He" + "llo" + " Wor" + "ld!")'],
+    exitCode: 0,
   }, {
     input: 'crypto.createHash("md5").update("hello").digest("hex")',
     output: expectFipsSupport ?
@@ -54,14 +67,23 @@ export async function runSmokeTests(smokeTestServer: string | undefined, executa
       db.dropDatabase();`,
     output: /Test succeeded/,
     includeStderr: false,
+    exitCode: 0,
     testArgs: [smokeTestServer as string]
   }, {
     input: fleSmokeTestScript,
     output: /Test succeeded|Test skipped/,
     includeStderr: false,
+    exitCode: 0,
     testArgs: [smokeTestServer as string]
   }] : [])) {
-    await runSmokeTest(executable, [...args, ...testArgs], input, output, includeStderr);
+    await runSmokeTest({
+      executable,
+      args: [...args, ...testArgs],
+      input,
+      output,
+      includeStderr,
+      exitCode
+    });
   }
   console.log('all tests passed');
 }
@@ -74,7 +96,9 @@ export async function runSmokeTests(smokeTestServer: string | undefined, executa
  * @param input stdin contents of the executable
  * @param output Expected contents of stdout
  */
-async function runSmokeTest(executable: string, args: string[], input: string, output: RegExp, includeStderr?: boolean): Promise<void> {
+async function runSmokeTest({ executable, args, input, output, exitCode, includeStderr } : {
+  executable: string, args: string[], input: string, output: RegExp, exitCode?: number, includeStderr?: boolean
+}): Promise<void> {
   const proc = spawn(executable, [...args], {
     stdio: ['pipe', 'pipe', includeStderr ? 'pipe' : 'inherit']
   });
@@ -83,12 +107,20 @@ async function runSmokeTest(executable: string, args: string[], input: string, o
   proc.stdout!.setEncoding('utf8').on('data', (chunk) => { stdout += chunk; });
   proc.stderr?.setEncoding('utf8').on('data', (chunk) => { stderr += chunk; });
   proc.stdin!.end(input);
-  await once(proc.stdout!, 'end');
+  const [ [actualExitCode] ] = await Promise.all([
+    once(proc, 'exit'),
+    once(proc.stdout!, 'end'),
+    proc.stderr && once(proc.stderr, 'end')
+  ]);
+  const metadata = { input, output, stdout, stderr, executable, actualExitCode, args: args.map(arg => redactURICredentials(arg)) };
   try {
     assert.match(includeStderr ? `${stdout}\n${stderr}` : stdout, output);
-    console.error({ status: 'success', input, output, stdout, executable, args: args.map(arg => redactURICredentials(arg)) });
+    if (exitCode !== undefined) {
+      assert.strictEqual(actualExitCode, exitCode);
+    }
+    console.error({ status: 'success', ...metadata });
   } catch (err: any) {
-    console.error({ status: 'failure', input, output, stdout, executable, args: args.map(arg => redactURICredentials(arg)) });
+    console.error({ status: 'failure', ...metadata });
     throw err;
   }
 }

packages/cli-repl/test/e2e.spec.ts

@@ -1058,6 +1058,10 @@ describe('e2e', function() {
           expect(shell.output).to.include('Warning: Could not access file:');
         });
         expect(await shell.executeLine('print(123 + 456)')).to.include('579');
+        await shell.executeLine('sleep(100)');
+        expect(shell.output).to.not.include('anonymousId');
+        expect(shell.output).to.not.include('AssertionError');
+        expect(shell.assertNoErrors());
       });
 
       it('keeps working when the log files cannot be created', async() => {
@@ -1069,6 +1073,10 @@ describe('e2e', function() {
         });
         expect(await shell.executeLine('print(123 + 456)')).to.include('579');
         expect(await shell.executeLine('enableTelemetry()')).to.include('Telemetry is now enabled');
+        await shell.executeLine('sleep(100)');
+        expect(shell.output).to.not.include('anonymousId');
+        expect(shell.output).to.not.include('AssertionError');
+        expect(shell.assertNoErrors());
       });
 
       it('keeps working when the config file is present but not writable', async function() {
@@ -1083,6 +1091,10 @@ describe('e2e', function() {
           expect(shell.output).to.include('Warning: Could not access file:');
         });
         expect(await shell.executeLine('print(123 + 456)')).to.include('579');
+        await shell.executeLine('sleep(100)');
+        expect(shell.output).to.not.include('anonymousId');
+        expect(shell.output).to.not.include('AssertionError');
+        expect(shell.assertNoErrors());
       });
     });
   });

packages/logging/src/analytics-helpers.spec.ts

@@ -41,4 +41,15 @@ describe('ToggleableAnalytics', () => {
       [ 'track', { userId: 'me', event: 'something2', properties: { mongosh_version: '1.2.3' } } ]
     ]);
   });
+
+  it('emits an error for invalid messages if telemetry is enabled', () => {
+    const toggleable = new ToggleableAnalytics(target);
+
+    toggleable.identify({} as any);
+    expect(() => toggleable.enable()).to.throw('Telemetry setup is missing userId or anonymousId');
+
+    toggleable.disable();
+    expect(() => toggleable.enable()).to.not.throw();
+    expect(() => toggleable.track({} as any)).to.throw('Telemetry setup is missing userId or anonymousId');
+  });
 });

packages/logging/src/analytics-helpers.ts

@@ -40,12 +40,14 @@ export class ToggleableAnalytics implements MongoshAnalytics {
   _queue: Array<['identify', Parameters<MongoshAnalytics['identify']>] | ['track', Parameters<MongoshAnalytics['track']>]> = [];
   _state: 'enabled' | 'disabled' | 'paused' = 'paused';
   _target: MongoshAnalytics;
+  _pendingError?: Error;
 
   constructor(target: MongoshAnalytics = new NoopAnalytics()) {
     this._target = target;
   }
 
   identify(...args: Parameters<MongoshAnalytics['identify']>): void {
+    this._validateArgs(args);
     switch (this._state) {
       case 'enabled':
         this._target.identify(...args);
@@ -59,6 +61,7 @@ export class ToggleableAnalytics implements MongoshAnalytics {
   }
 
   track(...args: Parameters<MongoshAnalytics['track']>): void {
+    this._validateArgs(args);
     switch (this._state) {
       case 'enabled':
         this._target.track(...args);
@@ -72,6 +75,9 @@ export class ToggleableAnalytics implements MongoshAnalytics {
   }
 
   enable() {
+    if (this._pendingError) {
+      throw this._pendingError;
+    }
     this._state = 'enabled';
     const queue = this._queue;
     this._queue = [];
@@ -83,10 +89,34 @@ export class ToggleableAnalytics implements MongoshAnalytics {
 
   disable() {
     this._state = 'disabled';
+    this._pendingError = undefined;
     this._queue = [];
   }
 
   pause() {
     this._state = 'paused';
   }
+
+  _validateArgs([firstArg]: [MongoshAnalyticsIdentity]): void {
+    // Validate that the first argument of a track() or identify() call has
+    // a .userId or .anonymousId property set.
+    // This validation is also performed by the segment package, but is done
+    // here for two reasons: One, if telemetry is disabled, then we lose the
+    // stack trace information for where the buggy call came from, and two,
+    // this way the validation affects all tests in CI, not just the ones that
+    // are explicitly written to enable telemetry to a fake endpoint.
+    if (!('userId' in firstArg && firstArg.userId) &&
+        !('anonymousId' in firstArg && firstArg.anonymousId)) {
+      const err = new Error('Telemetry setup is missing userId or anonymousId');
+      switch (this._state) {
+        case 'enabled':
+          throw err;
+        case 'paused':
+          this._pendingError ??= err;
+          break;
+        default:
+          break;
+      }
+    }
+  }
 }

scripts/docker/ubuntu22.04-nohome-deb.Dockerfile

@@ -0,0 +1,18 @@
+FROM ubuntu:22.04
+
+ARG artifact_url=""
+ADD ${artifact_url} /tmp
+RUN apt-get update
+RUN apt-get install -y /tmp/*mongosh*.deb
+RUN /usr/bin/mongosh --build-info
+
+# alternate config for regression-testing https://jira.mongodb.org/browse/MONGOSH-1320
+RUN useradd --user-group --system --create-home --no-log-init mongodb
+RUN mkdir /home/mongodb/.mongodb
+RUN chmod -w /home/mongodb/.mongodb
+RUN touch /home/mongodb/.mongoshrc.js
+
+USER mongodb
+WORKDIR /home/mongodb
+
+ENTRYPOINT [ "mongosh" ]