Merge lp:~frankban/charms/precise/juju-gui/support-default-charm-icon into lp:~juju-gui/charms/precise/juju-gui/trunk

[Francesco Banconi]

Make the GUI server redirect to the default icon.

Create a specialized proxy handler for handling
Juju HTTP API requests. In this subclass, handle
the case a request is for a local charm icon
that cannot be found on the Juju server.

Tests: `make unittest`.

QA:
- `juju bootstrap`;
- from the branch root, run `make deploy`;
- wait for the GUI service to be ready;
- switch to the trunk branch:
  `juju set juju-gui juju-gui-source=develop`
- wait for the GUI to be ready;
- deploy local charms including an icon:
  you should see the icons are correctly displayed in the
  service blocks and inspector header;
- deploy a local charm not including an icon:
  you should see the fallback icon displayed both in
  the service block and the inspector;
- destroy the environment, done.

https://codereview.appspot.com/86100043/

[Francesco Banconi (frankban)]

Reviewers: mp+214985_code.launchpad.net,

Message:
Please take a look.

Description:
Make the GUI server redirect to the default icon.

Create a specialized proxy handler for handling
Juju HTTP API requests. In this subclass, handle
the case a request is for a local charm icon
that cannot be found on the Juju server.

Tests: `make unittest`.

QA:
- `juju bootstrap`;
- from the branch root, run `make deploy`;
- wait for the GUI service to be ready;
- switch to the trunk branch:
   `juju set juju-gui juju-gui-source=develop`
- wait for the GUI to be ready;
- deploy local charms including an icon:
   you should see the icons are correctly displayed in the
   service blocks and inspector header;
- deploy a local charm not including an icon:
   you should see the fallback icon displayed both in
   the service block and the inspector;
- destroy the environment, done.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/support-default-charm-icon/+merge/214985

(do not edit description out of merge proposal)

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

Affected files (+148, -21 lines):
   A [revision details]
   M revision
   M server/guiserver/__init__.py
   M server/guiserver/apps.py
   M server/guiserver/handlers.py
   M server/guiserver/tests/test_handlers.py

[Brad Crittenden (bac)]

Code LGTM

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

https://codereview.appspot.com/86100043/diff/1/server/guiserver/handlers.py#newcode255
server/guiserver/handlers.py:255: # Handle POST requests the same way
GET ones are handled.
s/ones/requests/ -- just reads better.

https://codereview.appspot.com/86100043/diff/1/server/guiserver/handlers.py#newcode320
server/guiserver/handlers.py:320: Override to handle the case a when a
charm icon is not found.
s/a when/when/ or /for when/?

https://codereview.appspot.com/86100043/

[lp:~frankban/charms/precise/juju-gui/support-default-charm-icon]

180. By Francesco Banconi on 2014-04-09

Changes as per review.

[Francesco Banconi (frankban)]

Please take a look.

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

https://codereview.appspot.com/86100043/diff/1/server/guiserver/handlers.py#newcode255
server/guiserver/handlers.py:255: # Handle POST requests the same way
GET ones are handled.
On 2014/04/09 16:32:42, bac wrote:
> s/ones/requests/ -- just reads better.

Done.

https://codereview.appspot.com/86100043/diff/1/server/guiserver/handlers.py#newcode320
server/guiserver/handlers.py:320: Override to handle the case a when a
charm icon is not found.
On 2014/04/09 16:32:42, bac wrote:
> s/a when/when/ or /for when/?

Done.

https://codereview.appspot.com/86100043/

[Brad Crittenden (bac)]

QA OK too

https://codereview.appspot.com/86100043/

[Francesco Banconi (frankban)]

*** Submitted:

Make the GUI server redirect to the default icon.

Create a specialized proxy handler for handling
Juju HTTP API requests. In this subclass, handle
the case a request is for a local charm icon
that cannot be found on the Juju server.

Tests: `make unittest`.

QA:
- `juju bootstrap`;
- from the branch root, run `make deploy`;
- wait for the GUI service to be ready;
- switch to the trunk branch:
   `juju set juju-gui juju-gui-source=develop`
- wait for the GUI to be ready;
- deploy local charms including an icon:
   you should see the icons are correctly displayed in the
   service blocks and inspector header;
- deploy a local charm not including an icon:
   you should see the fallback icon displayed both in
   the service block and the inspector;
- destroy the environment, done.

R=bac
CC=
https://codereview.appspot.com/86100043

https://codereview.appspot.com/86100043/

[Francesco Banconi (frankban)]

Thank you Brad.

https://codereview.appspot.com/86100043/

=== modified file 'revision'
--- revision	2014-03-17 12:45:59 +0000
+++ revision	2014-04-09 16:48:27 +0000
@@ -1,1 +1,1 @@
-109
+110

=== modified file 'server/guiserver/__init__.py'
--- server/guiserver/__init__.py	2014-01-27 15:12:32 +0000
+++ server/guiserver/__init__.py	2014-04-09 16:48:27 +0000
@@ -37,7 +37,7 @@
 which originally made the request.
 """
 
-VERSION = (0, 3, 0)
+VERSION = (0, 4, 0)
 
 
 def get_version():

=== modified file 'server/guiserver/apps.py'
--- server/guiserver/apps.py	2014-02-04 17:23:57 +0000
+++ server/guiserver/apps.py	2014-04-09 16:48:27 +0000
@@ -56,13 +56,17 @@
             # The tokens collection for authentication token requests.
             'tokens': tokens,
         }
+        juju_proxy_handler_options = {
+            'target_url': utils.ws_to_http(options.apiurl),
+            'charmworld_url': options.charmworldurl,
+        }
         server_handlers.extend([
             # Handle WebSocket connections.
             (r'^/ws$', handlers.WebSocketHandler, websocket_handler_options),
             # Handle connections to the juju-core HTTPS server.
             # The juju-core HTTPS and WebSocket servers share the same URL.
-            (r'^/juju-core/(.*)', handlers.ProxyHandler,
-             {'target_url': utils.ws_to_http(options.apiurl)}),
+            (r'^/juju-core/(.*)', handlers.JujuProxyHandler,
+             juju_proxy_handler_options),
         ])
     if options.testsroot:
         params = {'path': options.testsroot, 'default_filename': 'index.html'}

=== modified file 'server/guiserver/handlers.py'
--- server/guiserver/handlers.py	2014-01-29 10:21:03 +0000
+++ server/guiserver/handlers.py	2014-04-09 16:48:27 +0000
@@ -20,6 +20,7 @@
 import logging
 import os
 import time
+import urlparse
 
 from tornado import (
     escape,
@@ -47,6 +48,10 @@
 )
 
 
+# Define the path to the fallback charm icon hosted by charmworld.
+DEFAULT_CHARM_ICON_PATH = '/static/img/charm_160.svg'
+
+
 class WebSocketHandler(websocket.WebSocketHandler):
     """WebSocket handler supporting secure WebSockets.
 
@@ -225,12 +230,14 @@
 class ProxyHandler(web.RequestHandler):
     """An HTTP(S) proxy from the server to the given target URL."""
 
-    def initialize(self, target_url):
+    def initialize(self, target_url, validate_cert=True):
         """Initialize the proxy.
 
-        Receive the target URL where to redirect to.
+        Receive the target URL where to redirect to, and a flag indicating
+        whether to validate remote server certificates.
         """
         self.target_url = target_url
+        self.validate_cert = validate_cert
 
     @gen.coroutine
     def get(self, path):
@@ -241,12 +248,23 @@
         The response will then be sent back to the client.
         """
         url = join_url(self.target_url, path, self.request.query)
-        # Server certificates are not validated: we use this function to
-        # connect to juju-core, and we would need to obtain ca-certificates
-        # from it. Unfortunately we don't have that information, and for this
-        # reason we skip validation for both WebSocket and HTTPS connections.
-        # This is not ideal but currently is our best option.
-        request = clone_request(self.request, url, validate_cert=False)
+        response = yield self.send_request(url)
+        if response is not None:
+            self.send_response(response)
+
+    # Handle POST requests the same way GET requests are handled.
+    post = get
+
+    @gen.coroutine
+    def send_request(self, url):
+        """Send an asynchronous request to the given URL.
+
+        Return the server response.
+        If an error occurs in the communication, return None and call
+        self._send_error with the given error.
+        """
+        request = clone_request(
+            self.request, url, validate_cert=self.validate_cert)
         client = httpclient.AsyncHTTPClient()
         try:
             response = yield client.fetch(request)
@@ -254,13 +272,9 @@
             response = getattr(err, 'response', None)
             if not response:
                 self._send_error(url, err)
-                return
-        self._send_response(response)
-
-    # Handle POST requests the same way GET ones are handled.
-    post = get
-
-    def _send_response(self, response):
+        raise gen.Return(response)
+
+    def send_response(self, response):
         """Prepare and send the response to the client."""
         self.set_status(response.code)
         set_header = self.set_header
@@ -279,6 +293,55 @@
         self.write('Internal server error:\n{}'.format(msg))
 
 
+class JujuProxyHandler(ProxyHandler):
+    """A specialized proxy handler used for the juju-core HTTP API."""
+
+    def initialize(self, target_url, charmworld_url):
+        """Initialize the proxy.
+
+        Receive the target URL where to redirect to, and the charmworld URL
+        used to retrieve the default charm icon.
+        """
+        # Server certificates are not validated: we use this handler to connect
+        # to juju-core, and we would need to obtain ca-certificates from it.
+        # Unfortunately we don't have that information, and for this reason we
+        # skip validation for both WebSocket and HTTPS connections. This is not
+        # ideal but currently is our best option.
+        super(JujuProxyHandler, self).initialize(
+            target_url, validate_cert=False)
+        self.default_charm_icon_url = urlparse.urljoin(
+            charmworld_url, DEFAULT_CHARM_ICON_PATH)
+
+    @gen.coroutine
+    def get(self, path):
+        """Handle GET requests.
+        See the ProxyHandler.get method.
+
+        Override to handle the case when a charm icon is not found.
+        """
+        url = join_url(self.target_url, path, self.request.query)
+        response = yield self.send_request(url)
+        if response is not None:
+            if response.code == 404 and self._charm_icon_requested(path):
+                # This is a request for a charm icon file, and the icon is not
+                # found: redirect to the fallback icon hosted on charmworld.
+                self.redirect(self.default_charm_icon_url)
+            else:
+                # Return the response to the client as usual.
+                self.send_response(response)
+
+    def _charm_icon_requested(self, path):
+        """Return True if the current request is for a charm icon."""
+        return (
+            # The request is for a local charm.
+            path == 'charms' and
+            # The charm URL is specified.
+            self.get_argument('url', None) and
+            # The icon file is requested.
+            self.get_argument('file', None) == 'icon.svg'
+        )
+
+
 class InfoHandler(web.RequestHandler):
     """Return information about the GUI server."""
 

=== modified file 'server/guiserver/tests/test_handlers.py'
--- server/guiserver/tests/test_handlers.py	2014-01-28 15:48:59 +0000
+++ server/guiserver/tests/test_handlers.py	2014-04-09 16:48:27 +0000
@@ -517,6 +517,7 @@
         'Server': 'Apache/2.4.1 (Unix)',
         'WWW-Authenticate': 'Basic',
     }
+    expected_validate_cert = True
 
     def get_app(self):
         # Set up an application exposing the proxy handler.
@@ -570,8 +571,6 @@
         self.assertEqual(self.target_url + '/remote-path/', remote_request.url)
         self.assert_include_headers(
             self.request_headers, remote_request.headers)
-        # Certificates are automatically accepted.
-        self.assertFalse(remote_request.validate_cert)
 
     def test_post_request(self):
         # POST requests are properly sent to the target URL.
@@ -595,6 +594,19 @@
         # Also the body is propagated.
         self.assertEqual('original body', remote_request.body)
 
+    def test_validate_certificates(self):
+        # Server certificates are properly handled.
+        remote_response = helpers.make_response(200)
+        with self.patch_http_client(remote_response) as mock_client:
+            self.fetch('/base/remote-path/', headers=self.request_headers)
+        mock_fetch = mock_client().fetch
+        # The client's fetch method has been used to fetch the remote resource.
+        mock_fetch = mock_client().fetch
+        self.assertEqual(1, mock_fetch.call_count)
+        remote_request = mock_fetch.call_args[0][0]
+        self.assertEqual(
+            self.expected_validate_cert, remote_request.validate_cert)
+
     def test_remote_path(self):
         # The corresponding path on the remote server is properly generated.
         remote_response = helpers.make_response(200)
@@ -617,6 +629,15 @@
         self.assertEqual('try harder', response.body)
         self.assertEqual('Bad Request', response.reason)
 
+    def test_not_found_response(self):
+        # 404 responses are returned to the original client.
+        remote_response = helpers.make_response(404, body='try later')
+        with self.patch_http_client(remote_response):
+            response = self.fetch('/base/remote-path/')
+        self.assertEqual(404, response.code)
+        self.assertEqual('try later', response.body)
+        self.assertEqual('Not Found', response.reason)
+
     def test_internal_server_error(self):
         # A 500 error is returned if an HTTP error occurs during the remote
         # request/response process.
@@ -632,6 +653,43 @@
         self.assertEqual('Internal Server Error', response.reason)
 
 
+class TestJujuProxyHandler(TestProxyHandler):
+
+    charmworld_url = 'https://charmworld.example.com'
+    expected_validate_cert = False
+
+    def get_app(self):
+        # Set up an application exposing the proxy handler.
+        options = {
+            'target_url': self.target_url,
+            'charmworld_url': self.charmworld_url,
+        }
+        return web.Application([
+            (r'^/base/(.*)', handlers.JujuProxyHandler, options)])
+
+    def test_default_charm_icon(self):
+        # If a charm icon is not found, a GET request redirects to the fallback
+        # icon available on charmworld.
+        remote_response = helpers.make_response(404)
+        path = '/base/charms?url=local:trusty/django=42&file=icon.svg'
+        with self.patch_http_client(remote_response):
+            response = self.fetch(path, follow_redirects=False)
+        self.assertEqual(302, response.code)
+        self.assertEqual(
+            self.charmworld_url + handlers.DEFAULT_CHARM_ICON_PATH,
+            response.headers['location'])
+
+    def test_charm_file_not_found(self):
+        # If a charm file is not found and it is not the icon, a 404 is
+        # correctly returned to the original client.
+        remote_response = helpers.make_response(404)
+        path = '/base/charms?url=local:trusty/django=42&file=readme.rst'
+        with self.patch_http_client(remote_response):
+            response = self.fetch(path)
+        self.assertEqual(404, response.code)
+        self.assertEqual('Not Found', response.reason)
+
+
 class TestInfoHandler(LogTrapTestCase, AsyncHTTPTestCase):
 
     def get_app(self):