feat(server): extend key creation API to allow specifying a port (#1505)

* feat(server): extend key creation API to allow specifying a port

* Use `validateNumberParam` in `setPortForNewAccessKey`.

* Only check if the port is valid if it was specified.

* Undo the removal of use of `Promise.all(...)`.

* Fix tests.

* Only run validation if parameters are provided.

* Correct types for API validation methods where `undefined` is a possible return type.

src/shadowbox/model/access_key.ts

@@ -59,6 +59,8 @@ export interface AccessKeyCreateParams {
   readonly password?: string;
   // The data transfer limit to apply to the access key.
   readonly dataLimit?: DataLimit;
+  // The port number to use for the access key.
+  readonly portNumber?: number;
 }
 
 export interface AccessKeyRepository {

src/shadowbox/model/errors.ts

@@ -24,15 +24,14 @@ class OutlineError extends Error {
 }
 
 export class InvalidPortNumber extends OutlineError {
-  // Since this is the error when a non-numeric value is passed to `port`, it takes type `string`.
-  constructor(public port: string) {
-    super(port);
+  constructor(public port: number) {
+    super(`Port ${port} is invalid: must be an integer in range [0, 65353]`);
   }
 }
 
 export class PortUnavailable extends OutlineError {
   constructor(public port: number) {
-    super(port.toString());
+    super(`Port ${port} is unavailable`);
   }
 }
 

src/shadowbox/server/api.yml

@@ -154,13 +154,13 @@ paths:
                   type: string
                 port:
                   type: integer
-                dataLimit:
+                limit:
                   $ref: "#/components/schemas/DataLimit"
             examples:
               'No params specified':
                 value: '{"method":"aes-192-gcm"}'
               'Provide params':
-                value: '{"method":"aes-192-gcm","name":"First","password":"8iu8V8EeoFVpwQvQeS9wiD","limit":{"bytes":10000}}'
+                value: '{"method":"aes-192-gcm","name":"First","password":"8iu8V8EeoFVpwQvQeS9wiD","port": 12345,"limit":{"bytes":10000}}'
       responses:
         '201':
           description: The newly created access key
@@ -196,11 +196,11 @@ paths:
                   type: string
                 port:
                   type: integer
-                dataLimit:
+                limit:
                   $ref: "#/components/schemas/DataLimit"
             examples:
               '0':
-                value: '{"id":"123","method":"aes-192-gcm","name":"First","password":"8iu8V8EeoFVpwQvQeS9wiD","limit":{"bytes":10000}}'
+                value: '{"id":"123","method":"aes-192-gcm","name":"First","password":"8iu8V8EeoFVpwQvQeS9wiD","port": 12345,"limit":{"bytes":10000}}'
       responses:
         '201':
           description: The newly created access key

src/shadowbox/server/manager_service.spec.ts

@@ -531,6 +531,59 @@ describe('ShadowsocksManagerService', () => {
             done();
           });
         });
+
+        it('uses the default port for new keys when no port is provided', async (done) => {
+          const res = {
+            send: (httpCode, data) => {
+              expect(httpCode).toEqual(201);
+              expect(data.port).toBeDefined();
+              responseProcessed = true; // required for afterEach to pass.
+            },
+          };
+          await serviceMethod({params: {id: accessKeyId}}, res, done);
+        });
+
+        it('uses the provided port when one is provided', async (done) => {
+          const res = {
+            send: (httpCode, data) => {
+              expect(httpCode).toEqual(201);
+              expect(data.port).toEqual(NEW_PORT);
+              responseProcessed = true; // required for afterEach to pass.
+            },
+          };
+          await serviceMethod({params: {id: accessKeyId, port: NEW_PORT}}, res, done);
+        });
+
+        it('rejects ports that are not numbers', async (done) => {
+          const res = {send: SEND_NOTHING};
+          await serviceMethod({params: {id: accessKeyId, port: '1234'}}, res, (error) => {
+            expect(error.statusCode).toEqual(400);
+            responseProcessed = true; // required for afterEach to pass.
+            done();
+          });
+        });
+
+        it('rejects invalid port numbers', async (done) => {
+          const res = {send: SEND_NOTHING};
+          await serviceMethod({params: {id: accessKeyId, port: 1.4}}, res, (error) => {
+            expect(error.statusCode).toEqual(400);
+            responseProcessed = true; // required for afterEach to pass.
+            done();
+          });
+        });
+
+        it('rejects port numbers already in use', async (done) => {
+          const server = new net.Server();
+          server.listen(NEW_PORT, async () => {
+            const res = {send: SEND_NOTHING};
+            await serviceMethod({params: {id: accessKeyId, port: NEW_PORT}}, res, (error) => {
+              expect(error.statusCode).toEqual(409);
+              responseProcessed = true; // required for afterEach to pass.
+              server.close();
+              done();
+            });
+          });
+        });
       });
     }
   });
@@ -629,6 +682,7 @@ describe('ShadowsocksManagerService', () => {
       const server = new net.Server();
       server.listen(NEW_PORT, async () => {
         await service.setPortForNewAccessKeys({params: {port: NEW_PORT}}, res, next);
+        server.close();
       });
     });
 

src/shadowbox/server/manager_service.ts

@@ -189,7 +189,7 @@ function validateAccessKeyId(accessKeyId: unknown): string {
   return accessKeyId;
 }
 
-function validateDataLimit(limit: unknown): DataLimit {
+function validateDataLimit(limit: unknown): DataLimit | undefined {
   if (typeof limit === 'undefined') {
     return undefined;
   }
@@ -204,7 +204,7 @@ function validateDataLimit(limit: unknown): DataLimit {
   return limit as DataLimit;
 }
 
-function validateStringParam(param: unknown, paramName: string): string {
+function validateStringParam(param: unknown, paramName: string): string | undefined {
   if (typeof param === 'undefined') {
     return undefined;
   }
@@ -218,6 +218,20 @@ function validateStringParam(param: unknown, paramName: string): string {
   return param;
 }
 
+function validateNumberParam(param: unknown, paramName: string): number | undefined {
+  if (typeof param === 'undefined') {
+    return undefined;
+  }
+
+  if (typeof param !== 'number') {
+    throw new restifyErrors.InvalidArgumentError(
+      {statusCode: 400},
+      `Expected a number for ${paramName}, instead got ${param} of type ${typeof param}`
+    );
+  }
+  return param;
+}
+
 // The ShadowsocksManagerService manages the access keys that can use the server
 // as a proxy using Shadowsocks. It runs an instance of the Shadowsocks server
 // for each existing access key, with the port and password assigned for that access key.
@@ -342,6 +356,7 @@ export class ShadowsocksManagerService {
       const name = validateStringParam(req.params.name || '', 'name');
       const dataLimit = validateDataLimit(req.params.limit);
       const password = validateStringParam(req.params.password, 'password');
+      const portNumber = validateNumberParam(req.params.port, 'port');
 
       const accessKeyJson = accessKeyToApiJson(
         await this.accessKeys.createNewAccessKey({
@@ -350,13 +365,16 @@ export class ShadowsocksManagerService {
           name,
           dataLimit,
           password,
+          portNumber,
         })
       );
       return accessKeyJson;
     } catch (error) {
       logging.error(error);
-      if (error instanceof errors.InvalidCipher) {
+      if (error instanceof errors.InvalidCipher || error instanceof errors.InvalidPortNumber) {
         throw new restifyErrors.InvalidArgumentError({statusCode: 400}, error.message);
+      } else if (error instanceof errors.PortUnavailable) {
+        throw new restifyErrors.ConflictError(error.message);
       }
       throw error;
     }
@@ -381,10 +399,7 @@ export class ShadowsocksManagerService {
       return next();
     } catch (error) {
       logging.error(error);
-      if (
-        error instanceof restifyErrors.InvalidArgumentError ||
-        error instanceof restifyErrors.MissingParameterError
-      ) {
+      if (error instanceof restifyErrors.HttpError) {
         return next(error);
       }
       return next(new restifyErrors.InternalServerError());
@@ -409,10 +424,7 @@ export class ShadowsocksManagerService {
       if (error instanceof errors.AccessKeyConflict) {
         return next(new restifyErrors.ConflictError(error.message));
       }
-      if (
-        error instanceof restifyErrors.InvalidArgumentError ||
-        error instanceof restifyErrors.MissingParameterError
-      ) {
+      if (error instanceof restifyErrors.HttpError) {
         return next(error);
       }
       return next(new restifyErrors.InternalServerError());
@@ -427,18 +439,11 @@ export class ShadowsocksManagerService {
   ): Promise<void> {
     try {
       logging.debug(`setPortForNewAccessKeys request ${JSON.stringify(req.params)}`);
-      const port = req.params.port;
-      if (!port) {
+      const port = validateNumberParam(req.params.port, 'port');
+      if (port === undefined) {
         return next(
           new restifyErrors.MissingParameterError({statusCode: 400}, 'Parameter `port` is missing')
         );
-      } else if (typeof port !== 'number') {
-        return next(
-          new restifyErrors.InvalidArgumentError(
-            {statusCode: 400},
-            `Expected a numeric port, instead got ${port} of type ${typeof port}`
-          )
-        );
       }
       await this.accessKeys.setPortForNewAccessKeys(port);
       this.serverConfig.data().portForNewAccessKeys = port;
@@ -451,6 +456,8 @@ export class ShadowsocksManagerService {
         return next(new restifyErrors.InvalidArgumentError({statusCode: 400}, error.message));
       } else if (error instanceof errors.PortUnavailable) {
         return next(new restifyErrors.ConflictError(error.message));
+      } else if (error instanceof restifyErrors.HttpError) {
+        return next(error);
       }
       return next(new restifyErrors.InternalServerError(error));
     }
@@ -556,10 +563,7 @@ export class ShadowsocksManagerService {
       return next();
     } catch (error) {
       logging.error(error);
-      if (
-        error instanceof restifyErrors.InvalidArgumentError ||
-        error instanceof restifyErrors.MissingParameterError
-      ) {
+      if (error instanceof restifyErrors.HttpError) {
         return next(error);
       }
       return next(new restifyErrors.InternalServerError());

src/shadowbox/server/server_access_key.spec.ts

@@ -99,6 +99,77 @@ describe('ServerAccessKeyRepository', () => {
     });
   });
 
+  it('createNewAccessKey throws on creating keys with invalid port', async (done) => {
+    const repo = new RepoBuilder().build();
+    await expectAsyncThrow(
+      repo.createNewAccessKey.bind(repo, {portNumber: -123}),
+      errors.InvalidPortNumber
+    );
+    done();
+  });
+
+  it('createNewAccessKey rejects invalid port numbers', async (done) => {
+    const repo = new RepoBuilder().build();
+    await expectAsyncThrow(
+      repo.createNewAccessKey.bind(repo, {portNumber: 0}),
+      errors.InvalidPortNumber
+    );
+    await expectAsyncThrow(
+      repo.createNewAccessKey.bind(repo, {portNumber: -1}),
+      errors.InvalidPortNumber
+    );
+    await expectAsyncThrow(
+      repo.createNewAccessKey.bind(repo, {portNumber: 100.1}),
+      errors.InvalidPortNumber
+    );
+    await expectAsyncThrow(
+      repo.createNewAccessKey.bind(repo, {portNumber: 65536}),
+      errors.InvalidPortNumber
+    );
+    done();
+  });
+
+  it('createNewAccessKey rejects specified ports in use', async (done) => {
+    const portProvider = new PortProvider();
+    const port = await portProvider.reserveNewPort();
+    const repo = new RepoBuilder().build();
+    const server = new net.Server();
+    server.listen(port, async () => {
+      try {
+        await repo.createNewAccessKey({portNumber: port});
+        fail(`createNewAccessKey should reject already used port ${port}.`);
+      } catch (error) {
+        expect(error instanceof errors.PortUnavailable);
+      }
+      server.close();
+      done();
+    });
+  });
+
+  it('createNewAccessKey creates keys with the correct default port', async (done) => {
+    const portProvider = new PortProvider();
+    const defaultPort = await portProvider.reserveNewPort();
+    const repo = new RepoBuilder().port(defaultPort).build();
+    repo.createNewAccessKey().then((accessKey) => {
+      expect(accessKey).toBeDefined();
+      expect(accessKey.proxyParams.portNumber).toEqual(defaultPort);
+      done();
+    });
+  });
+
+  it('createNewAccessKey creates keys with the port correctly', async (done) => {
+    const portProvider = new PortProvider();
+    const defaultPort = await portProvider.reserveNewPort();
+    const newPort = await portProvider.reserveNewPort();
+    const repo = new RepoBuilder().port(defaultPort).build();
+    repo.createNewAccessKey({portNumber: newPort}).then((accessKey) => {
+      expect(accessKey).toBeDefined();
+      expect(accessKey.proxyParams.portNumber).not.toEqual(defaultPort);
+      expect(accessKey.proxyParams.portNumber).toEqual(newPort);
+      done();
+    });
+  });
+
   it('Creates access keys under limit', async (done) => {
     const repo = new RepoBuilder().build();
     const accessKey = await repo.createNewAccessKey();

src/shadowbox/server/server_access_key.ts

@@ -106,6 +106,10 @@ function isValidCipher(cipher: string): boolean {
   return true;
 }
 
+function isValidPort(port: number): boolean {
+  return Number.isInteger(port) && port > 0 && port <= 65535;
+}
+
 // AccessKeyRepository that keeps its state in a config file and uses ShadowsocksServer
 // to start and stop per-access-key Shadowsocks instances.  Requires external validation
 // that portForNewAccessKeys is valid.
@@ -161,15 +165,19 @@ export class ServerAccessKeyRepository implements AccessKeyRepository {
     });
   }
 
+  private async isPortAvailable(port: number): Promise<boolean> {
+    return this.isExistingAccessKeyPort(port) || !(await isPortUsed(port));
+  }
+
   setHostname(hostname: string): void {
     this.proxyHostname = hostname;
   }
 
   async setPortForNewAccessKeys(port: number): Promise<void> {
-    if (!Number.isInteger(port) || port < 1 || port > 65535) {
-      throw new errors.InvalidPortNumber(port.toString());
+    if (!isValidPort(port)) {
+      throw new errors.InvalidPortNumber(port);
     }
-    if (!this.isExistingAccessKeyPort(port) && (await isPortUsed(port))) {
+    if (!(await this.isPortAvailable(port))) {
       throw new errors.PortUnavailable(port);
     }
     this.portForNewAccessKeys = port;
@@ -198,15 +206,27 @@ export class ServerAccessKeyRepository implements AccessKeyRepository {
     }
     const metricsId = uuidv4();
     const password = params?.password ?? generatePassword();
-    const encryptionMethod = params?.encryptionMethod || this.NEW_USER_ENCRYPTION_METHOD;
 
-    // Validate encryption method.
-    if (!isValidCipher(encryptionMethod)) {
-      throw new errors.InvalidCipher(encryptionMethod);
+    const encryptionMethod = params?.encryptionMethod || this.NEW_USER_ENCRYPTION_METHOD;
+    if (encryptionMethod !== this.NEW_USER_ENCRYPTION_METHOD) {
+      if (!isValidCipher(encryptionMethod)) {
+        throw new errors.InvalidCipher(encryptionMethod);
+      }
     }
+
+    const portNumber = params?.portNumber ?? this.portForNewAccessKeys;
+    if (portNumber !== this.portForNewAccessKeys) {
+      if (!isValidPort(portNumber)) {
+        throw new errors.InvalidPortNumber(portNumber);
+      }
+      if (!(await this.isPortAvailable(portNumber))) {
+        throw new errors.PortUnavailable(portNumber);
+      }
+    }
+
     const proxyParams = {
       hostname: this.proxyHostname,
-      portNumber: this.portForNewAccessKeys,
+      portNumber,
       encryptionMethod,
       password,
     };