Merge lp:~canonical-isd-hackers/canonical-identity-provider/readonly-improvements into lp:canonical-identity-provider/release

[Anthony Lenton]

This branch includes fixes for bug #544370, bug #524611 and bug #538014:
- Allow readonly code to connect to https servers, and using virtual hosts.
- The readonly_secret setting has been removed and replaced with the standard secret_key setting.
- app_servers is filtered out for duplicates. And did the same for db_connections, for free.

This is untidy, as it's fixing three bugs at once, I know. I thought it would be better to have them all together as two of them have practically no QA involved, but I should have made three branches still, and eventually deploy them all together to ec2.

Let me know if I should break this up for better review.

[Ricardo Kirkner (ricardokirkner)]

nice work.

review: Approve

=== modified file '.bzrignore'
--- .bzrignore	2010-05-03 14:35:12 +0000
+++ .bzrignore	2010-05-05 16:31:24 +0000
@@ -1,2 +1,5 @@
 .config
 local.cfg
+coverage
+.coverage
+.coverage.*

=== modified file 'config.example/settings.py'
--- config.example/settings.py	2010-04-23 15:37:57 +0000
+++ config.example/settings.py	2010-05-05 16:31:24 +0000
@@ -41,7 +41,10 @@
 # api settings
 API_ENABLED = False
 API_HOST = 'openid.localdomain'
-APP_SERVERS = ['localhost']
+APP_SERVERS = [{'SERVER_ID': 'localhost',
+                'HOST': 'localhost',
+                'PORT': 8000,
+               }]
 LAZR_RESTFUL_CODE_REVISION = '1'
 LAZR_RESTFUL_HOSTNAME = 'sso'
 LAZR_RESTFUL_PATH_OVERRIDE = None

=== modified file 'doctests/stories/api-workflows.txt'
--- doctests/stories/api-workflows.txt	2010-04-21 15:29:24 +0000
+++ doctests/stories/api-workflows.txt	2010-05-05 16:31:24 +0000
@@ -148,7 +148,7 @@
 in return you'll get dictionary:
 
     >>> api.accounts.me()
-    {u'username': ..., u'preferred_email': None, u'displayname': u'', u'unverified_emails': [u'[e-mail]'], u'verified_emails': [], u'openid_identifier': ...}
+    {u'username': ..., u'preferred_email': None, u'displayname': ..., u'unverified_emails': [u'[e-mail]'], u'verified_emails': [], u'openid_identifier': ...}
 
 Also you can ask weather given user is a member of list of teams:
 

=== modified file 'identityprovider/management/commands/readonly.py'
--- identityprovider/management/commands/readonly.py	2010-04-21 15:29:24 +0000
+++ identityprovider/management/commands/readonly.py	2010-05-05 16:31:24 +0000
@@ -52,12 +52,14 @@
 
         # determine servers to act upon
         if all_servers:
-            servers = settings.APP_SERVERS
+            servers = [app['SERVER_ID'] for app in settings.APP_SERVERS]
             servers.sort()
         else:
             if not args and not list_servers:
-                msg = _('Enter at least one server, '
-                        'or specify the --all option.')
+                msg = _('Enter at least one server, or specify the'
+                        ' --all option.') + '\n  ' + _('Use --list to get'
+                        ' a list of configured servers.')
+                        
                 raise CommandError(msg)
             servers = args
 

=== modified file 'identityprovider/templates/admin/readonly.txt'
--- identityprovider/templates/admin/readonly.txt	2010-04-21 15:29:24 +0000
+++ identityprovider/templates/admin/readonly.txt	2010-05-05 16:31:24 +0000
@@ -1,7 +1,8 @@
+{% comment %}
 Copyright 2010 Canonical Ltd.  This software is licensed under the
 GNU Affero General Public License version 3 (see the file LICENSE).
 
-Readonly status per application server
+{% endcomment %}Readonly status per application server
 -------------------------------------------------------------------------------
 {% for server in appservers %}
  {{server.name}} -- {% if server.reachable %}{% if server.readonly %}In readonly mode{% else %}Operating normally{% endif %}{% else %}Server is unreachable or out of sync{% endif %}{% if server.reachable %}{% for conn in server.connections %}

=== modified file 'identityprovider/tests/test_command_readonly.py'
--- identityprovider/tests/test_command_readonly.py	2010-04-21 15:29:24 +0000
+++ identityprovider/tests/test_command_readonly.py	2010-05-05 16:31:24 +0000
@@ -2,7 +2,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import StringIO
-import copy
+from copy import copy, deepcopy
 import shutil
 import sys
 import tempfile
@@ -24,22 +24,30 @@
         sys.stdout = StringIO.StringIO()
         sys.stderr = StringIO.StringIO()
 
-        self.servers = ['localhost:8000', 'localhost:8001']
+        self.servers = [{'SERVER_ID': 'localhost',
+                         'HOST': 'localhost',
+                         'PORT': '8000',
+                        },
+                        {'SERVER_ID': 'otherhost',
+                         'HOST': 'localhost',
+                         'PORT': '8001',
+                        }
+                       ]
         self._APP_SERVERS = settings.APP_SERVERS
         settings.APP_SERVERS = self.servers
         self._DBFAILOVER_FLAG_DIR = getattr(settings, 'DBFAILOVER_FLAG_DIR',
                                             None)
         settings.DBFAILOVER_FLAG_DIR = tempfile.mkdtemp()
-        self._DB_CONNECTIONS = settings.DB_CONNECTIONS
-        backup_db = copy.copy(settings.DB_CONNECTIONS[0])
+        self._DB_CONNECTIONS = deepcopy(settings.DB_CONNECTIONS)
+        backup_db = copy(settings.DB_CONNECTIONS[0])
         backup_db['DATABASE_ID'] = 'backup'
         settings.DB_CONNECTIONS.append(backup_db)
 
         # setup wsgi intercept mechanism to simulate wsgi server
         urllib2_intercept.install_opener()
         for server in self.servers:
-            host, port = server.split(':')
-            add_wsgi_intercept(host, int(port), WSGIHandler)
+            add_wsgi_intercept(server['HOST'], int(server['PORT']),
+                               WSGIHandler)
 
     def tearDown(self):
         # make sure we're clear everything up before leaving
@@ -48,14 +56,14 @@
 
         # remove wsgi intercept mechanism
         for server in self.servers:
-            host, port = server.split(':')
-            remove_wsgi_intercept(host, int(port))
+            remove_wsgi_intercept(server['HOST'], int(server['PORT']))
         urllib2_intercept.uninstall_opener()
 
         settings.APP_SERVERS = self._APP_SERVERS
         shutil.rmtree(settings.DBFAILOVER_FLAG_DIR, True)
         settings.DBFAILOVER_FLAG_DIR = self._DBFAILOVER_FLAG_DIR
         settings.DB_CONNECTIONS = self._DB_CONNECTIONS
+        readonly_manager.set_db(settings.DB_CONNECTIONS[0])
 
         sys.stdout = self._stdout
         sys.stderr = self._stderr
@@ -71,44 +79,46 @@
 
     def test_readonly_list_all(self):
         output = self.get_status()
-        self.assertTrue(self.servers[0] in output)
+        self.assertTrue(self.servers[0]['SERVER_ID'] in output)
 
     def test_readonly_set(self):
-        call_command('readonly', self.servers[0], action='set')
+        call_command('readonly', self.servers[0]['SERVER_ID'], action='set')
         readonly_manager.check_readonly()
         self.assertTrue(readonly_manager.is_readonly())
 
     def test_readonly_clear(self):
-        call_command('readonly', self.servers[0], action='clear')
+        call_command('readonly', self.servers[0]['SERVER_ID'], action='clear')
         readonly_manager.check_readonly()
         self.assertFalse(readonly_manager.is_readonly())
 
     def test_readonly_enable(self):
         # prepare test
-        call_command('readonly', self.servers[0], action='disable',
-                     database='master')
+        call_command('readonly', self.servers[0]['SERVER_ID'],
+                     action='disable', database='master')
         # perform test
-        call_command('readonly', self.servers[0], action='enable',
-                     database='master')
+        call_command('readonly', self.servers[0]['SERVER_ID'],
+                     action='enable', database='master')
         self.assertFalse(readonly_manager.is_failed('master'))
 
     def test_readonly_disable(self):
-        call_command('readonly', self.servers[0], action='disable',
-                     database='master')
+        call_command('readonly', self.servers[0]['SERVER_ID'],
+                     action='disable', database='master')
         self.assertTrue(readonly_manager.is_failed('master'))
 
     def test_readonly_set_all(self):
         call_command('readonly', action='set', all_servers=True)
         output = self.get_status()
         for server in settings.APP_SERVERS:
-            self.assertTrue("%s -- In readonly mode" % server in output)
+            expected = "%s -- In readonly mode" % server['SERVER_ID']
+            self.assertTrue(expected in output)
 
     def test_readonly_clear_all(self):
         call_command('readonly', self.servers[0], action='clear',
                      all_servers=True)
         output = self.get_status()
         for server in settings.APP_SERVERS:
-            self.assertTrue("%s -- Operating normally" % server in output)
+            expected = "%s -- Operating normally" % server['SERVER_ID']
+            self.assertTrue(expected in output)
 
     def test_readonly_enable_all(self):
         call_command('readonly', action='enable', database='master',

=== modified file 'identityprovider/tests/test_readonly.py'
--- identityprovider/tests/test_readonly.py	2010-04-21 15:29:24 +0000
+++ identityprovider/tests/test_readonly.py	2010-05-05 16:31:24 +0000
@@ -4,8 +4,12 @@
 import socket
 import urllib2
 from StringIO import StringIO
+
+from django.conf import settings
+from django.utils import simplejson
+
+from identityprovider.readonly import readonly_manager
 from identityprovider.tests.utils import SQLCachedTestCase
-from django.conf import settings
 from identityprovider import models
 from identityprovider.backend.base import DatabaseError
 from identityprovider.views.readonly import _remote_req
@@ -40,7 +44,12 @@
         self.assertRaises(DatabaseError, p.save)
 
 
-class ReadOnlyAdminTest(SQLCachedTestCase):
+class RemoteRequestTest(SQLCachedTestCase):
+    msg = 'hello'
+    host = 'myhost'
+    scheme = 'https'
+    vhost = 'http://foobar.baz'
+
     def setup_mock_urlopen(self, func):
         self.orig_urlopen = urllib2.urlopen
         urllib2.urlopen = func
@@ -48,12 +57,115 @@
     def restore_orig_urlopen(self):
         urllib2.urlopen = self.orig_urlopen
 
-    def test_remote_req(self):
-        msg = 'hello'
-
-        def mock_urlopen(url, data=None):
-            self.assertEquals(5, socket.getdefaulttimeout())
-            return StringIO(msg)
-        self.setup_mock_urlopen(mock_urlopen)
-        self.assertEquals(msg, _remote_req('localhost'))
-        self.restore_orig_urlopen()
+    def mock_urlopen(self, req, data=None):
+        self.req = req
+        self.assertEquals(5, socket.getdefaulttimeout())
+        return StringIO(self.msg)
+
+    def test_plain_remote_req(self):
+        self.setup_mock_urlopen(self.mock_urlopen)
+        server = {'host': self.host}
+        self.assertEquals(self.msg, _remote_req(**server))
+        self.assertEquals(self.host, self.req.get_host())
+        self.restore_orig_urlopen()
+
+    def test_https_remote_req(self):
+        self.setup_mock_urlopen(self.mock_urlopen)
+        server = {'host': self.host,
+                  'scheme': self.scheme,
+                 }
+        self.assertEquals(self.msg, _remote_req(**server))
+        self.assertEquals(self.scheme, self.req.get_type())
+        self.assertEquals(self.host, self.req.get_host())
+        self.restore_orig_urlopen()
+
+    def test_vhost_remote_req(self):
+        self.setup_mock_urlopen(self.mock_urlopen)
+        server = {'host': self.host,
+                  'scheme': self.scheme,
+                  'virtual_host': self.vhost,
+                 }
+        self.assertEquals(self.msg, _remote_req(**server))
+        headers = {'Host': self.vhost}
+        self.assertEquals(headers, self.req.headers)
+        self.assertEquals(self.scheme, self.req.get_type())
+        self.assertEquals(self.host, self.req.get_host())
+        self.restore_orig_urlopen()
+
+class ReadOnlyDataTest(SQLCachedTestCase):
+    def setUp(self):
+        self.orig_readonly_secret = settings.READONLY_SECRET
+        settings.READONLY_SECRET = 'test secret'
+        self.orig_db_connections = settings.DB_CONNECTIONS
+        settings.DB_CONNECTIONS = [{'DATABASE_ID': 'master',
+                                    'DATABASE_HOST': 'dbhost1',
+                                    'DATABASE_NAME': 'masterdb',
+                                   },
+                                   {'DATABASE_ID': 'slave',
+                                    'DATABASE_HOST': 'dbhost2',
+                                    'DATABASE_NAME': 'slavedb',
+                                   }]
+
+    def tearDown(self):
+        settings.READONLY_SECRET = self.orig_readonly_secret
+        settings.DB_CONNECTIONS = self.orig_db_connections
+        readonly_manager.clear_readonly()
+        readonly_manager.clear_failed('master')
+        readonly_manager.set_db(settings.DB_CONNECTIONS[0])
+
+    def test_readonly_returns_404_for_get(self):
+        response = self.client.get('/readonlydata')
+        self.assertEquals(404, response.status_code)
+
+    def test_readonly_returns_404_without_readonly_secret(self):
+        response = self.client.post('/readonlydata')
+        self.assertEquals(404, response.status_code)
+
+    def test_readonly_returns_404_with_bad_secret(self):
+        data = {'secret': 'something different'}
+        response = self.client.post('/readonlydata', data)
+        self.assertEquals(404, response.status_code)
+
+    def test_readonly_returns_200_with_right_secret(self):
+        data = {'secret': settings.READONLY_SECRET}
+        response = self.client.post('/readonlydata', data)
+        self.assertEquals(200, response.status_code)
+
+    def test_readonly_data(self):
+        post = {'secret': settings.READONLY_SECRET}
+        response = self.client.post('/readonlydata', post)
+        data = simplejson.loads(response.content)
+        self.assertTrue('readonly' in data)
+        self.assertTrue('connections' in data)
+
+    def test_readonly_set_readonly(self):
+        post = {'secret': settings.READONLY_SECRET, 'action': 'set'}
+        response = self.client.post('/readonlydata', post)
+        data = simplejson.loads(response.content)
+        self.assertTrue(data['readonly'])
+
+    def test_readonly_clear_readonly(self):
+        post = {'secret': settings.READONLY_SECRET, 'action': 'clear'}
+        response = self.client.post('/readonlydata', post)
+        data = simplejson.loads(response.content)
+        self.assertFalse(data['readonly'])
+
+    def test_readonly_disable_master(self):
+        post = {'secret': settings.READONLY_SECRET,
+                'action': 'disable',
+                'conn': 'master'}
+        response = self.client.post('/readonlydata', post)
+        data = simplejson.loads(response.content)
+        self.assertTrue(data['connections'][0]['failed'])
+        self.assertTrue(data['readonly'])
+
+    def test_readonly_reenable_master(self):
+        post = {'secret': settings.READONLY_SECRET,
+                'action': 'disable',
+                'conn': 'master'}
+        response = self.client.post('/readonlydata', post)
+        post['action'] = 'enable'
+        response = self.client.post('/readonlydata', post)
+        data = simplejson.loads(response.content)
+        self.assertFalse(data['connections'][0]['failed'])
+        self.assertTrue(data['readonly'])

=== modified file 'identityprovider/views/readonly.py'
--- identityprovider/views/readonly.py	2010-04-21 15:29:24 +0000
+++ identityprovider/views/readonly.py	2010-05-05 16:31:24 +0000
@@ -12,18 +12,44 @@
 import urllib
 import urllib2
 
+def lowercase_keys(dicts):
+    """ Convert all keys in a list of dicts to lowercase. """
+    return [dict((k.lower(), v) for (k, v) in d.items()) for d in dicts]
 
-def _remote_req(server, post=None):
+def _remote_req(host, port=None, scheme=None, server_id=None,
+                virtual_host=None, post=None):
     """Makes a request to a specific appserver.
 
-    if post is provided, it should be a sequence of 2-tuples that will be
+    The first five arguments are the same as the keys for each dictionary in
+    the APP_SERVERS setting:
+
+    * host: The host at which we can reach the app server (can be an IP)
+    * port: The port that should be used (optional)
+    * scheme: The scheme to use to contact this app server (http/https)
+              (defaults to http)
+    * server_id: Some canonical name for this app server (optional)
+    * virtual_host: The virtual host that should be used, for app servers that
+                    serve multiple sites (optional).
+
+    If post is provided, it should be a sequence of 2-tuples that will be
     encoded in to POST data.
     """
     if post is None:
         post = []
     post.append(('secret', settings.READONLY_SECRET))
     post = urllib.urlencode(post)
-    req = urllib2.Request('http://%s/readonlydata' % server, data=post)
+
+    if scheme is None:
+        scheme = 'http'
+    if port is None:
+        portstr = ''
+    else:
+        portstr = ':' + port
+    url = '%s://%s%s/readonlydata' % (scheme, host, portstr)
+    headers = {}
+    if virtual_host is not None:
+        headers['Host'] = virtual_host
+    req = urllib2.Request(url, headers=headers, data=post)
     try:
         oldtimeout = socket.getdefaulttimeout()
         socket.setdefaulttimeout(5)
@@ -34,20 +60,18 @@
         data = None
     return data
 
-
 @staff_member_required
 def readonly_admin(request):
     atts = get_server_atts(settings.APP_SERVERS)
     return render_to_response('admin/readonly.html', atts)
 
-
 def get_server_atts(servers):
     """Provides a report about readonly status of all app servers."""
     appservers = []
     set_all_readonly = False
     clear_all_readonly = False
-    for server in servers:
-        datastr = _remote_req(server)
+    for server in lowercase_keys(servers):
+        datastr = _remote_req(**server)
         if datastr is None:
             data = {'reachable': False}
         else:
@@ -63,7 +87,7 @@
                 clear_all_readonly = True
             else:
                 set_all_readonly = True
-        data['name'] = server
+        data['name'] = server['server_id']
         appservers.append(data)
     atts = {'appservers': appservers,
             'admin_media_prefix': settings.ADMIN_MEDIA_PREFIX,
@@ -72,7 +96,6 @@
             }
     return atts
 
-
 @staff_member_required
 def readonly_confirm(request, action, appserver=None, conn=None):
     if request.method == 'POST':
@@ -84,23 +107,23 @@
            }
     return render_to_response('admin/readonly_confirm.html', atts)
 
-
 def update_server(action, appserver=None, conn=None):
     if appserver is None:
         appservers = settings.APP_SERVERS
     else:
-        appservers = [appserver]
+        appservers = [server for server in settings.APP_SERVERS
+                             if server['SERVER_ID'] == appserver]
+    appservers = lowercase_keys(appservers)
     for server in appservers:
         post = [('action', action),
                 ('conn', conn),
                ]
-        datastr = _remote_req(server, post)
+        datastr = _remote_req(post=post, **server)
     # If we're leaving readonly mode across all appservers,
     # try to sync up data back from memcached to the DB.
     if appserver is None and action == 'clear':
         sync_memcached_to_db()
 
-
 def readonly_data(request):
     """Provides data about the readonly status of this app server."""
     if request.method != 'POST':