Code review comment for lp://staging/~frankban/charms/precise/juju-gui/server-insecure-mode

Reviewers: mp+177649_code.launchpad.net,

Message:
Please take a look.

Description:
Support serving the GUI over a non secure conn.

The GUI server can be configured to run the
GUI static files server and the WebSocket server over
an HTTP connection.
This way we support the secure=false charm option.

Tests:
   Run `make unittest` from the branch root.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/server-insecure-mode/+merge/177649

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/12088046/

Affected files:
   A [revision details]
   M server/guiserver/manage.py
   M server/guiserver/tests/test_manage.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision:
<email address hidden>

Index: server/guiserver/manage.py
=== modified file 'server/guiserver/manage.py'
--- server/guiserver/manage.py 2013-07-25 16:41:45 +0000
+++ server/guiserver/manage.py 2013-07-30 16:51:58 +0000
@@ -105,6 +105,11 @@
      define(
          'sslpath', type=str, default=DEFAULT_SSL_PATH,
          help='The path where the SSL certificates are stored.')
+ define(
+ 'http', type=bool, default=False,
+ help='In order to run the GUI over a non secure connection (HTTP)
set '
+ 'this flag to True. Do not set this property unless you '
+ 'understand and accept the risks.')
      # In Tornado, parsing the options also sets up the default logger.
      parse_command_line()
      _validate_required('guiroot', 'apiurl')
@@ -114,8 +119,13 @@

  def run():
      """Run the server"""
- server().listen(443, ssl_options=_get_ssl_options())
- redirector().listen(80)
+ if options.http:
+ # Run the server over HTTP.
+ server().listen(80)
+ else:
+ # Default configuration: run the server over a secure connection.
+ server().listen(443, ssl_options=_get_ssl_options())
+ redirector().listen(80)
      version = guiserver.get_version()
      logging.info('starting Juju GUI server v{}'.format(version))
      IOLoop.instance().start()

Index: server/guiserver/tests/test_manage.py
=== modified file 'server/guiserver/tests/test_manage.py'
--- server/guiserver/tests/test_manage.py 2013-07-26 09:22:03 +0000
+++ server/guiserver/tests/test_manage.py 2013-07-30 16:41:48 +0000
@@ -16,7 +16,10 @@

  """Tests for the Juju GUI server management helpers."""

-from contextlib import contextmanager
+from contextlib import (
+ contextmanager,
+ nested,
+)
  import logging
  import unittest

@@ -136,3 +139,49 @@
          }
          with mock.patch('guiserver.manage.options', mock_options):
              self.assertEqual(expected, manage._get_ssl_options())
+
+
+class TestRun(unittest.TestCase):
+
+ def mock_and_run(self, **kwargs):
+ """Run the application after mocking the IO loop and the
options/apps.
+
+ Additional options can be specified using kwargs.
+ """
+ options = {
+ 'apiversion': 'go',
+ 'guiroot': '/my/guiroot',
+ 'sslpath': '/my/sslpath',
+ }
+ options.update(kwargs)
+ managers = nested(
+ mock.patch('guiserver.manage.IOLoop'),
+ mock.patch('guiserver.manage.options', mock.Mock(**options)),
+ mock.patch('guiserver.manage.redirector'),
+ mock.patch('guiserver.manage.server'),
+ )
+ with managers as (ioloop, _, redirector, server):
+ manage.run()
+ return ioloop.instance().start, redirector().listen,
server().listen
+
+ def test_secure_mode(self):
+ # The application is correctly run in secure mode.
+ _, redirector_listen, server_listen = self.mock_and_run(http=False)
+ expected_ssl_options = {
+ 'certfile': '/my/sslpath/juju.crt',
+ 'keyfile': '/my/sslpath/juju.key',
+ }
+ server_listen.assert_called_once_with(
+ 443, ssl_options=expected_ssl_options)
+ redirector_listen.assert_called_once_with(80)
+
+ def test_http_mode(self):
+ # The application is correctly run in HTTP mode.
+ _, redirector_listen, server_listen = self.mock_and_run(http=True)
+ server_listen.assert_called_once_with(80)
+ self.assertEqual(0, redirector_listen.call_count)
+
+ def test_ioloop_started(self):
+ # The IO loop instance is started when the application is run.
+ ioloop_start, _, _ = self.mock_and_run()
+ ioloop_start.assert_called_once_with()