Merge lp:~frankban/charms/precise/juju-gui/server-insecure-mode into lp:~juju-gui/charms/precise/juju-gui/trunk

[Francesco Banconi]

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://codereview.appspot.com/12088046/

[Francesco Banconi (frankban)]

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.
+ """
+...

[Gary Poster (gary)]

LGTM! I did not qa--ran out of time.

Thank you.

https://codereview.appspot.com/12088046/

[Nicola Larosa (teknico)]

LGTM with possibly some changes, see below. "make unittest" passes.

https://codereview.appspot.com/12088046/diff/1/server/guiserver/manage.py
File server/guiserver/manage.py (right):

https://codereview.appspot.com/12088046/diff/1/server/guiserver/manage.py#newcode112
server/guiserver/manage.py:112: 'understand and accept the risks.')
Simpler help: "Set to True to serve the GUI over an insecure connection
(HTTP). Please understand and accept the inherent risks before doing
so."

https://codereview.appspot.com/12088046/diff/1/server/guiserver/manage.py#newcode124
server/guiserver/manage.py:124: server().listen(80)
Would it be useful to add a redirector from 443 to 80 here?

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py
File server/guiserver/tests/test_manage.py (right):

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py#newcode157
server/guiserver/tests/test_manage.py:157: managers = nested(
As discussed, the "nested" function is deprecated and does not add value
here, so it would be better not using it. If the linter gives you a hard
time with the multiple manager form of the "with" statement, the linter
is broken and should be fixed. :-)

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py#newcode176
server/guiserver/tests/test_manage.py:176:
redirector_listen.assert_called_once_with(80)
Better swap these two asserts, to keep the same ordering as above.

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py#newcode182
server/guiserver/tests/test_manage.py:182: self.assertEqual(0,
redirector_listen.call_count)
Better swap these two asserts too, for the same reason.

https://codereview.appspot.com/12088046/

[lp:~frankban/charms/precise/juju-gui/server-insecure-mode]

85. By Francesco Banconi on 2013-07-31

Changes as per review.

[Francesco Banconi (frankban)]

*** Submitted:

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.

R=gary.poster, teknico
CC=
https://codereview.appspot.com/12088046

https://codereview.appspot.com/12088046/diff/1/server/guiserver/manage.py
File server/guiserver/manage.py (right):

https://codereview.appspot.com/12088046/diff/1/server/guiserver/manage.py#newcode112
server/guiserver/manage.py:112: 'understand and accept the risks.')
On 2013/07/31 09:45:11, teknico wrote:
> Simpler help: "Set to True to serve the GUI over an insecure
connection (HTTP).
> Please understand and accept the inherent risks before doing so."

Yes, that's simpler, but I like the more explicit "Don't do that
unless..." in the original help.

https://codereview.appspot.com/12088046/diff/1/server/guiserver/manage.py#newcode124
server/guiserver/manage.py:124: server().listen(80)
On 2013/07/31 09:45:11, teknico wrote:
> Would it be useful to add a redirector from 443 to 80 here?

AFAIK, this is only used in the Saucelabs CI tests for ie. In that
story, I don't think what you suggest is useful. I am not sure this is a
good idea also in general, from the user perspective: a 404 is better
than an automatic (implicit) redirection from a secure connection to an
insecure one IMHO.

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py
File server/guiserver/tests/test_manage.py (right):

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py#newcode157
server/guiserver/tests/test_manage.py:157: managers = nested(
On 2013/07/31 09:45:11, teknico wrote:
> As discussed, the "nested" function is deprecated and does not add
value here,
> so it would be better not using it. If the linter gives you a hard
time with the
> multiple manager form of the "with" statement, the linter is broken
and should
> be fixed. :-)

Done.

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py#newcode176
server/guiserver/tests/test_manage.py:176:
redirector_listen.assert_called_once_with(80)
On 2013/07/31 09:45:11, teknico wrote:
> Better swap these two asserts, to keep the same ordering as above.

Done.

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py#newcode182
server/guiserver/tests/test_manage.py:182: self.assertEqual(0,
redirector_listen.call_count)
On 2013/07/31 09:45:11, teknico wrote:
> Better swap these two asserts too, for the same reason.

Done.

https://codereview.appspot.com/12088046/

[Francesco Banconi (frankban)]

Thank you both for the reviews.

https://codereview.appspot.com/12088046/

=== modified file 'server/guiserver/manage.py'
--- server/guiserver/manage.py	2013-07-25 16:41:45 +0000
+++ server/guiserver/manage.py	2013-07-31 10:35:51 +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()

=== 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-31 10:35:51 +0000
@@ -136,3 +136,47 @@
         }
         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)
+        with \
+                mock.patch('guiserver.manage.IOLoop') as ioloop, \
+                mock.patch('guiserver.manage.options', mock.Mock(**options)), \
+                mock.patch('guiserver.manage.redirector') as redirector, \
+                mock.patch('guiserver.manage.server') as 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',
+        }
+        redirector_listen.assert_called_once_with(80)
+        server_listen.assert_called_once_with(
+            443, ssl_options=expected_ssl_options)
+
+    def test_http_mode(self):
+        # The application is correctly run in HTTP mode.
+        _, redirector_listen, server_listen = self.mock_and_run(http=True)
+        self.assertEqual(0, redirector_listen.call_count)
+        server_listen.assert_called_once_with(80)
+
+    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()