feat(base-driver): add https server support by devhazem · Pull Request #18562 · appium/appium

devhazem · 2023-04-23T18:03:59Z

Proposed changes

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

I have implemented this feature as I need appium to run on HTTPS to make communication from an application running on HTTPS posssible.

Types of changes

What types of changes does your code introduce to Appium?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I have read the CONTRIBUTING doc
I have signed the CLA
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
Any dependent changes have been merged and published in downstream modules

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

linux-foundation-easycla · 2023-04-23T18:04:03Z

❌ - login: @devhazem . The commit (1fd2802) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

mykola-mokhnach · 2023-04-23T18:16:58Z

@devhazem Could you please describe the purpose of the PR and sign the CLA?

devhazem · 2023-04-23T18:27:47Z

@devhazem Could you please describe the purpose of the PR and sign the CLA?
@mykola-mokhnach I have described the purpose of this PR. I will shortly sign the CLA too.

jlipps

thanks @devhazem! this is a good idea. i made some comments.

jlipps · 2023-04-25T19:57:07Z

README.md

 single server process. Refer the corresponding driver documentations regarding which mode is optimal
 for the particular driver or whether it supports parallel sessions.

+Appium runs by default on an HTTP server. If an HTTPS server should be used instead, simply set


this should be documented in the docs as well, please!

imo these should be server options; configurable in a config file or via CLI args. env vars probably ok too, but otherwise it deviates from how people normally configure appium.

jlipps · 2023-04-25T19:58:50Z

packages/base-driver/lib/express/ssl-handler.js

@@ -0,0 +1,66 @@
+import {readFileSync} from 'fs';


I think we should have typedoc docstrings for all the member fields and methods in this file.

jlipps · 2023-04-25T19:59:44Z

packages/base-driver/lib/express/ssl-handler.js

+
+class SslHandler {
+  constructor() {
+    this.secure = false;


Our pattern for classes is to define member fields up top. This can also avoid the need for a constructor. something like this:

class SslHandler { /** @type {boolean} */ secure = false;

and so on

jlipps · 2023-04-25T20:00:39Z

packages/base-driver/lib/express/ssl-handler.js

+        );
+      }
+      log.info(`ENV 'APPIUM_SSL_KEY_PATH' is set to '${keyPath}'`);
+      this.httpsOptions = {cert: readFileSync(certPath), key: readFileSync(keyPath)};


these should be async calls. that means this function, and the getInstance function should also be async

jlipps · 2023-04-25T20:01:02Z

packages/base-driver/lib/express/ssl-handler.js

+   */
+  handleHttpsOptions() {
+    const secure = process.env.APPIUM_SECURE;
+    if (secure === 'true') {


i think we should allow both true and 1 as values

jlipps · 2023-04-25T20:02:57Z

packages/base-driver/test/e2e/express/server.e2e.spec.js

+let givens = [{protocol: 'http'}, {protocol: 'https'}];
+
+async function startHttpsServer(port, routeConfiguringFunction, serverUpdaters = []) {
+  await exec('openssl genrsa 2048 > private.pem');


where will this file be generated? should we not use an absolute path to be sure we're not littering test data around the filesystem or overwriting files?

use a JS wrapper for generating a keypair. this will fail when we add windows to the matrix.

jlipps · 2023-04-25T20:03:17Z

packages/base-driver/test/e2e/express/server.e2e.spec.js

+
+let givens = [{protocol: 'http'}, {protocol: 'https'}];
+
+async function startHttpsServer(port, routeConfiguringFunction, serverUpdaters = []) {


will this test only work on macos or *nix?

jlipps · 2023-04-25T20:05:57Z

packages/base-driver/test/e2e/express/server.e2e.spec.js

  afterEach(async function () {
    try {
-      await hwServer.close();
+      //await httpHwServer.close();


should this be commented out?

jlipps · 2023-04-25T20:06:48Z

packages/base-driver/test/e2e/express/server.e2e.spec.js

+  given(givens).it(
+    'should allow one or more plugins to update the server',
+    async function ({protocol}) {
+      let correctPort;


throughout this file i think we can use port instead of correctPort

jlipps · 2023-04-25T20:08:08Z

packages/base-driver/lib/express/ssl-handler.js

+
+const log = logger.getLogger('Appium');
+
+class SslHandler {


it sort of feels like this could all simply be a single async function rather than a class. after we construct the httpsoptions we're not doing very much with this class. it doesn't have behaviours on it really.

boneskull · 2023-05-24T20:11:08Z

packages/base-driver/test/e2e/express/server.e2e.spec.js

 import B from 'bluebird';
 import _ from 'lodash';
 import {TEST_HOST, getTestPort} from '@appium/driver-test-support';
+import {given} from 'mocha-testdata';


mocha-testdata is deprecated; please remove

boneskull · 2023-05-24T20:12:36Z

packages/base-driver/test/e2e/express/server.e2e.spec.js

 describe('server', function () {
-  let hwServer;
-  let port;
+  let httpHwServer;


all of these should be typed

boneskull · 2023-05-24T20:14:02Z

packages/base-driver/test/e2e/express/server.e2e.spec.js

+      let {data} = await axios.get(`${protocol}://${TEST_HOST}:${correctPort}/plugin1`);
+      data.should.eql('res from plugin1 route');


use expressions like this instead (here and elsewhere):

Suggested change

let {data} = await axios.get(`${protocol}://${TEST_HOST}:${correctPort}/plugin1`);

data.should.eql('res from plugin1 route');

await axios.get(`${protocol}://${TEST_HOST}:${correctPort}/plugin1`).should.eventually.eql({data: 'res from plugin1 route'});

boneskull · 2023-05-24T20:14:24Z

packages/base-driver/test/e2e/express/server.e2e.spec.js

+            throw new Error('ugh');
+          },
+        ],
+      }).should.eventually.be.rejectedWith(/ugh/);


Suggested change

}).should.eventually.be.rejectedWith(/ugh/);

}).should.be.rejectedWith(/ugh/);

github-actions bot added @appium/base-driver appium core chore refactoring, overhead, tests, etc. Dependencies issues with dependencies Documentation related to writing, reading, or generating documentation Enhancement feature labels Apr 23, 2023

mykola-mokhnach requested a review from jlipps April 23, 2023 18:16

devhazem force-pushed the master branch 3 times, most recently from 5653da7 to 6e8cebf Compare April 24, 2023 05:23

feat: add https support to appium server

1fd2802

devhazem force-pushed the master branch from 430ef8a to 1fd2802 Compare April 25, 2023 19:47

jlipps requested changes Apr 25, 2023

View reviewed changes

boneskull reviewed May 24, 2023

View reviewed changes

devhazem closed this Jun 6, 2023

mykola-mokhnach mentioned this pull request Jun 13, 2023

fix: ensure logServerAddress gets an actual URL #18754

Merged

mykola-mokhnach mentioned this pull request Sep 2, 2023

feat(base-driver): Add server support of TLS and SPDY protocols #19105

Merged

4 tasks


		let givens = [{protocol: 'http'}, {protocol: 'https'}];

		async function startHttpsServer(port, routeConfiguringFunction, serverUpdaters = []) {

		let {data} = await axios.get(`${protocol}://${TEST_HOST}:${correctPort}/plugin1`);
		data.should.eql('res from plugin1 route');

	let {data} = await axios.get(`${protocol}://${TEST_HOST}:${correctPort}/plugin1`);
	data.should.eql('res from plugin1 route');
	await axios.get(`${protocol}://${TEST_HOST}:${correctPort}/plugin1`).should.eventually.eql({data: 'res from plugin1 route'});

	}).should.eventually.be.rejectedWith(/ugh/);
	}).should.be.rejectedWith(/ugh/);

Uh oh!

Conversation

devhazem commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Checklist

Further comments

Uh oh!

linux-foundation-easycla bot commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mykola-mokhnach commented Apr 23, 2023

Uh oh!

devhazem commented Apr 23, 2023

Uh oh!

jlipps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

devhazem commented Apr 23, 2023 •

edited

Loading

linux-foundation-easycla bot commented Apr 23, 2023 •

edited

Loading