fix: improved error handling (#4)

integrations/elementor-plugin/form-actions/aam-deploy.php

@@ -100,7 +100,13 @@ public function run( $record, $ajax_handler ) {
 		);
 
         if ( is_wp_error( $res ) || $res['response']['code'] >= 400 ) {
-            $ajax_handler->add_error( '', 'Server error. Please contact system administrator.' );
+            // $res['body'] is a stringified JSON
+            // Checkout the interactive setup script for possible errors
+            if ( str_contains( $res['body'] , 'name already exists' ) ) {
+                $ajax_handler->add_error( 'name', 'Name already exists.' );
+            } else {
+                $ajax_handler->add_error_message( 'Server error. Please contact system administrator.' );
+            }
 			return False;
         }
 	}

package.json

@@ -31,7 +31,8 @@
     "@sentry/node": "^7.38.0",
     "reflect-metadata": "^0.1.13",
     "rimraf": "^3.0.2",
-    "rxjs": "^7.8.0"
+    "rxjs": "^7.8.0",
+    "tail": "^2.2.6"
   },
   "devDependencies": {
     "@nestjs/cli": "^9.2.0",
@@ -41,6 +42,7 @@
     "@types/jest": "^29.4.0",
     "@types/node": "^18.13.0",
     "@types/supertest": "^2.0.12",
+    "@types/tail": "^2.2.3",
     "@typescript-eslint/eslint-plugin": "^5.52.0",
     "@typescript-eslint/parser": "^5.52.0",
     "eslint": "^8.34.0",

src/app.controller.spec.ts

@@ -1,15 +1,39 @@
 import { AppController } from './app.controller';
 import { Test, TestingModule } from '@nestjs/testing';
-import { firstValueFrom, of, throwError } from 'rxjs';
+import {
+  firstValueFrom,
+  lastValueFrom,
+  of,
+  Subject,
+  Subscription,
+  throwError,
+} from 'rxjs';
 import { HttpService } from '@nestjs/axios';
 import { BadRequestException, UnauthorizedException } from '@nestjs/common';
 import { DeploymentInfo } from './deployment-info.dto';
 import { ConfigModule } from '@nestjs/config';
 import * as fs from 'fs';
 
+let onSubscription: Subscription;
+let lines: Subject<string>;
+jest.mock('tail', () => {
+  // Mock Tail constructor
+  return {
+    Tail: jest.fn().mockReturnValue({
+      on: (event, callback) => (onSubscription = lines.subscribe(callback)),
+      unwatch: () => {
+        console.log('subscription', onSubscription);
+        onSubscription.unsubscribe();
+      },
+    }),
+  };
+});
+
 describe('AppController', () => {
   let controller: AppController;
   let mockHttp: { post: jest.Mock };
+  let mockWs: { write: jest.Mock; close: jest.Mock };
+
   const deploymentData: DeploymentInfo = {
     name: 'test-name',
     locale: 'de',
@@ -23,6 +47,9 @@ describe('AppController', () => {
   };
 
   beforeEach(async () => {
+    lines = new Subject();
+    mockWs = { write: jest.fn(), close: jest.fn() };
+    jest.spyOn(fs, 'createWriteStream').mockReturnValue(mockWs as any);
     mockHttp = {
       post: jest.fn().mockReturnValue(of({ data: undefined })),
     };
@@ -51,45 +78,131 @@ describe('AppController', () => {
     });
   });
 
-  it('should throw bad request exception if data has wrong format', (done) => {
-    const invalidData = { ...deploymentData, name: 'with space' };
-    // TODO: add an extensive list of invalid formats including attempts someone could pass to try and inject code?
+  it('should throw bad request exception if name has wrong format', async () => {
+    passDeployment();
+    function testName(name: string) {
+      return lastValueFrom(controller.deployApp({ ...deploymentData, name }));
+    }
+    await expect(testName('with space')).rejects.toBeInstanceOf(
+      BadRequestException,
+    );
+    await expect(testName('withCapital')).resolves.toBeTruthy();
+    await expect(testName('withSymbol?')).rejects.toBeInstanceOf(
+      BadRequestException,
+    );
+    await expect(testName('withNumber123')).resolves.toBeTruthy();
+    await expect(testName('with-dash')).resolves.toBeTruthy();
+    await expect(testName('with_underscore')).rejects.toBeInstanceOf(
+      BadRequestException,
+    );
+  });
 
-    controller.deployApp(invalidData).subscribe({
-      error: (err) => {
-        expect(err).toBeInstanceOf(BadRequestException);
-        done();
-      },
+  function passDeployment() {
+    // automatically finish deployment
+    jest.spyOn(lines, 'subscribe').mockImplementation((fn) => {
+      setTimeout(() => fn('DONE'));
+      return { unsubscribe: () => undefined } as any;
     });
+  }
+
+  it('should throw bad request exception if username has wrong format', async () => {
+    passDeployment();
+    function testName(username: string) {
+      return lastValueFrom(
+        controller.deployApp({ ...deploymentData, username }),
+      );
+    }
+
+    await expect(testName('with space')).rejects.toBeInstanceOf(
+      BadRequestException,
+    );
+    await expect(testName('withCapital')).resolves.toBeTruthy();
+    await expect(testName('withSymbol?')).rejects.toBeInstanceOf(
+      BadRequestException,
+    );
+    await expect(testName('withNumber123')).resolves.toBeTruthy();
+    await expect(testName('with-dash')).resolves.toBeTruthy();
+    await expect(testName('with_underscore')).rejects.toBeInstanceOf(
+      BadRequestException,
+    );
   });
 
-  it('should write arguments to file', async () => {
-    const mockWs = { write: jest.fn(), close: jest.fn() };
-    jest.spyOn(fs, 'createWriteStream').mockReturnValue(mockWs as any);
+  it('should throw bad request exception if email has wrong format', async () => {
+    passDeployment();
+    function testMail(email: string) {
+      return lastValueFrom(controller.deployApp({ ...deploymentData, email }));
+    }
 
-    await firstValueFrom(controller.deployApp(deploymentData));
+    await expect(testMail('testmail')).rejects.toBeInstanceOf(
+      BadRequestException,
+    );
+    await expect(testMail('test@mail')).rejects.toBeInstanceOf(
+      BadRequestException,
+    );
+    await expect(testMail('Test@[email protected]')).rejects.toBeInstanceOf(
+      BadRequestException,
+    );
+    await expect(testMail('[email protected]')).resolves.toBeTruthy();
+    await expect(testMail('[email protected]')).resolves.toBeTruthy();
+    await expect(testMail('[email protected]')).resolves.toBeTruthy();
+    await expect(testMail('[email protected]')).resolves.toBeTruthy();
+    await expect(testMail('[email protected]')).resolves.toBeTruthy();
+  });
+
+  it('should throw error if ERROR is written to log', async () => {
+    const res = firstValueFrom(controller.deployApp(deploymentData));
+    lines.next('some logs');
+    lines.next('ERROR my custom error');
+
+    try {
+      await res;
+    } catch (err) {
+      expect(err).toBeInstanceOf(BadRequestException);
+      expect(err.message).toBe('my custom error');
+      // Ensure tail is properly "unwatched"
+      expect(onSubscription.closed).toBeTruthy();
+      return;
+    }
+
+    throw new Error('No error thrown');
+  });
+
+  it('should write arguments to file', () => {
+    const res = firstValueFrom(controller.deployApp(deploymentData));
+
+    lines.next('DONE');
 
+    expect(res).resolves.toBeTruthy();
     expect(mockWs.write).toHaveBeenCalledWith(
       'test-name de [email protected] test-username test-base y n',
     );
     expect(mockWs.close).toHaveBeenCalled();
+    // Ensure tail is properly "unwatched"
+    expect(onSubscription.closed).toBeTruthy();
   });
 
-  it('should use the default locale if empty or omitted', async () => {
-    const mockWs = { write: jest.fn(), close: jest.fn() };
-    jest.spyOn(fs, 'createWriteStream').mockReturnValue(mockWs as any);
+  it('should use the default locale if empty', (done) => {
+    const emptyLocale = { ...deploymentData, locale: '' };
+    controller.deployApp(emptyLocale).subscribe(() => {
+      expect(mockWs.write).toHaveBeenCalledWith(
+        'test-name en [email protected] test-username test-base y n',
+      );
+      done();
+    });
 
-    const withoutLocale = { ...deploymentData, locale: '' };
-    await firstValueFrom(controller.deployApp(withoutLocale));
-    expect(mockWs.write).toHaveBeenCalledWith(
-      'test-name en [email protected] test-username test-base y n',
-    );
+    lines.next('DONE');
+  });
 
-    mockWs.write.mockReset();
-    delete withoutLocale.locale;
-    await firstValueFrom(controller.deployApp(withoutLocale));
-    expect(mockWs.write).toHaveBeenCalledWith(
-      'test-name en [email protected] test-username test-base y n',
-    );
+  it('should use the default locale if omitted', (done) => {
+    const noLocale = { ...deploymentData };
+    delete noLocale.locale;
+    controller.deployApp(noLocale).subscribe(() => {
+      expect(mockWs.write).toHaveBeenCalledWith(
+        'test-name en [email protected] test-username test-base y n',
+      );
+      done();
+    });
+
+    lines.next('DONE');
   });
 });

src/app.controller.ts

@@ -10,7 +10,8 @@ import { DeploymentInfo } from './deployment-info.dto';
 import * as fs from 'fs';
 import { HttpService } from '@nestjs/axios';
 import { ConfigService } from '@nestjs/config';
-import { catchError, map } from 'rxjs';
+import { catchError, mergeMap, Observable, Subject } from 'rxjs';
+import { Tail } from 'tail';
 
 @Controller()
 export class AppController {
@@ -37,11 +38,11 @@ export class AppController {
           }
           throw err;
         }),
-        map(() => this.writeCommandsToPipe(deploymentInfo)),
+        mergeMap(() => this.writeCommandsToPipe(deploymentInfo)),
       );
   }
 
-  private writeCommandsToPipe(deploymentInfo: DeploymentInfo) {
+  private writeCommandsToPipe(deploymentInfo: DeploymentInfo): Observable<any> {
     console.log('info', deploymentInfo);
 
     if (
@@ -50,6 +51,24 @@ export class AppController {
       throw new BadRequestException('No spaces allowed in arguments');
     }
 
+    if (!deploymentInfo.name.match(/^[a-zA-Z0-9\-]*$/)) {
+      throw new BadRequestException(
+        'Only letters, numbers and dashes are allowed in name',
+      );
+    }
+
+    if (!deploymentInfo.username.match(/^[a-zA-Z0-9\-]*$/)) {
+      throw new BadRequestException(
+        'Only letters, numbers and dashes are allowed in username',
+      );
+    }
+
+    // See https://regex101.com/r/lHs2R3/1
+    const emailRegex = /^[\w\-.]+@([\w-]+\.)+[\w-]{2,}$/;
+    if (!deploymentInfo.email.match(emailRegex)) {
+      throw new BadRequestException('Not a valid email');
+    }
+
     const args = `${deploymentInfo.name} ${deploymentInfo.locale || 'en'} ${
       deploymentInfo.email
     } ${deploymentInfo.username} ${deploymentInfo.base} ${
@@ -59,6 +78,26 @@ export class AppController {
     const ws = fs.createWriteStream('dist/assets/arg-pipe');
     ws.write(args);
     ws.close();
-    return { ok: true };
+    return this.getResult();
+  }
+
+  private getResult() {
+    const result = new Subject();
+    const tail = new Tail('dist/assets/log.txt');
+    tail.on('line', (line: string) => {
+      if (line.startsWith('ERROR')) {
+        // Error found, text after error is returned
+        const message = line.replace('ERROR ', '');
+        tail.unwatch();
+        result.error(new BadRequestException(message));
+        result.complete();
+      } else if (line.startsWith('DONE')) {
+        // Success, app is deployed
+        tail.unwatch();
+        result.next({ ok: true });
+        result.complete();
+      }
+    });
+    return result.asObservable();
   }
 }