Merge lp:~roadmr/canonical-identity-provider/metadata-with-custom-cert into lp:canonical-identity-provider/release

[Daniel Manrique]

Add SP-specific metadata view.

This allows URLs such as /+saml/metadata/4. If the SP with id 4 has a
custom certificate, it will be used in the metadata. If not, valid metadata
with the default global cert is shown. If no SP with the given primary key exists,
a 404 is raised.

This avoids having to tell SPs "use this metadata URL but this certificate
because the one in the metadata is bad".

The intended flow would be:

1- create the SPConfig, even if with partial config.
2- Add a custom cert
3- We can now give the SP's support people a metadata link with nice certificate.

Add SP-specific metadata view.

This allows URLs such as /+saml/metadata/4. If the SP with id 4 has a
custom certificate, it will be used in the metadata. If not, valid metadata
with the default global cert is shown. If no SP with the given primary key exists,
a 404 is raised.

This avoids having to tell SPs "use this metadata URL but this certificate
because the one in the metadata is bad".

The intended flow would be:

1- create the SPConfig, even if with partial config.
2- Add a custom cert
3- We can now give the SP's support people a metadata link with nice certificate.

[Ricardo Kirkner (ricardokirkner)]

Good idea and code lgtm, but what about using some other field (eg, acs_url) to identify the SP instead of exposing our internal PK?

[Daniel Manrique (roadmr)]

I wanted to specifically avoid changing the DB schema, and the PK seemed like the most innocuous, simple field to use. But I agree it's not nice to use it like that.

We could use the acs_url but since it's a url (and they can be quite ugly) I worry about tacking one url as a component to another url.

I'd be happy to implement any of these options:

1- Add a "slug" field so we can have URLs like /metadata/a-nice-provider (restrict to a-z0-9-). It's one more field to worry about, but it'd be stable for each remote.
2- Calculate a slug with those constraints using the existing "display name" field. Avoids adding a new field, but auto-slugs tend to be crappy and if the display name changes, the metadata location will also change.
3- Have a "sp identifier" field or something, which we auto-populate on record creation with a random value. Similar to 1, saves the operator from having to think of a slug value, but could yield ugly URLs (/metadata/a5Ewkhr4)
5- Use the acs_url. We'd probably need to come up with some cleaning/prettyfying.
6- Open to any other ideas

My preference would be option 1, it feels like the cleanest and is not hard to implement.

[Ricardo Kirkner (ricardokirkner)]

Ok, I think it's not that bad after all (after you explained it it didn't sound that aweful).
Maybe just changing the name of the field would be enough, eg instead of using pk for the variable, let's use sp_id (which is exactly the same really, but isn't forced to be the "primary" key; also, /metadata/pk would suggest it's the primary key for a metadata object).

[Daniel Manrique (roadmr)]

Done, I changed it in the URL and the view which are the most exposed places, though I needed to keep calling it pk in the places where I hit the database.

Let me know if that works!

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

=== modified file 'src/ubuntu_sso_saml/tests/test_views.py'
--- src/ubuntu_sso_saml/tests/test_views.py	2017-05-11 15:22:36 +0000
+++ src/ubuntu_sso_saml/tests/test_views.py	2018-01-05 23:23:10 +0000
@@ -4,7 +4,11 @@
 from saml2idp import saml2idp_metadata
 
 from identityprovider.tests.utils import SSOBaseTestCase
-from ubuntu_sso_saml.tests.helpers import find_cert_path
+from ubuntu_sso_saml.models import SAMLConfig
+from ubuntu_sso_saml.tests.helpers import cert_string_from_file, find_cert_path
+
+
+CUSTOM_TEST_CERTIFICATE = "custom-test-certificate.pem"
 
 
 class SAMLMetadataViewTestCase(SSOBaseTestCase):
@@ -25,8 +29,47 @@
         }
         saml2idp_metadata.SAML2IDP_CONFIG.update(test_config)
 
+        custom_cert_filename = find_cert_path(CUSTOM_TEST_CERTIFICATE)
+        with open(custom_cert_filename) as custom_cert_file:
+            custom_cert_string = custom_cert_file.read()
+        self.custom_sp_config = SAMLConfig.objects.create(
+            acs_url="https://custom.example.com",
+            certificate=custom_cert_string)
+        self.default_sp_config = SAMLConfig.objects.create(
+            acs_url="https://default.example.com")
+        self.custom_cert_string = cert_string_from_file(custom_cert_filename)
+
     def test_metadata_view(self):
         response = self.client.get(reverse('saml-metadata'))
         self.assertEqual(response['content-type'], 'application/xml')
         self.assertContains(response, reverse('logout'))
         self.assertContains(response, reverse('login_begin'))
+
+    def test_sp_specific_metadata_view_nonexistent_sp(self):
+        # Can never have a record with sp_id of 0
+        # (relying on the fact that the sp_id is actually the database ID for
+        # the record).
+        response = self.client.get(
+            reverse('saml-metadata', args=(0,)))
+        self.assertEqual(response.status_code, 404)
+
+    def test_sp_specific_metadata_view_default_cert(self):
+        cert_filename = saml2idp_metadata.SAML2IDP_CONFIG['certificate_file']
+
+        response = self.client.get(
+            reverse('saml-metadata', args=(self.default_sp_config.pk,)))
+        self.assertEqual(response['content-type'], 'application/xml')
+        self.assertContains(response, reverse('logout'))
+        self.assertContains(response, reverse('login_begin'))
+
+        self.assertContains(
+            response, cert_string_from_file(cert_filename))
+
+    def test_sp_specific_metadata_view_custom_cert(self):
+        response = self.client.get(
+            reverse('saml-metadata', args=(self.custom_sp_config.pk,)))
+        self.assertEqual(response['content-type'], 'application/xml')
+        self.assertContains(response, reverse('logout'))
+        self.assertContains(response, reverse('login_begin'))
+
+        self.assertContains(response, self.custom_cert_string)

=== modified file 'src/ubuntu_sso_saml/urls.py'
--- src/ubuntu_sso_saml/urls.py	2017-05-12 15:39:10 +0000
+++ src/ubuntu_sso_saml/urls.py	2018-01-05 23:23:10 +0000
@@ -9,6 +9,7 @@
     url(r'^$', 'saml_begin', name='login_begin'),
     url(r'^/process$', 'saml_process', name='login_process'),
     url(r'^/metadata$', 'saml_metadata', name='saml-metadata'),
+    url(r'^/metadata/(?P<sp_id>\d+)$', 'saml_metadata', name='saml-metadata'),
 )
 
 # Automagically detect deep link URLs from settings:

=== modified file 'src/ubuntu_sso_saml/views.py'
--- src/ubuntu_sso_saml/views.py	2017-05-12 15:39:10 +0000
+++ src/ubuntu_sso_saml/views.py	2018-01-05 23:23:10 +0000
@@ -4,7 +4,7 @@
 from urlparse import urlparse, urlunparse
 
 from django.core.urlresolvers import reverse
-from django.shortcuts import render, render_to_response
+from django.shortcuts import get_object_or_404, render, render_to_response
 from django.template import RequestContext
 from django.utils.datastructures import MultiValueDictKeyError
 from django.views.decorators.csrf import csrf_exempt
@@ -20,6 +20,7 @@
 # XXX: we should not import webui here
 from webui.decorators import sso_login_required
 
+from ubuntu_sso_saml.models import SAMLConfig
 from ubuntu_sso_saml.processors import InfoProcessor
 from ubuntu_sso_saml.utils import (
     get_config_and_link_for_resource,
@@ -99,7 +100,7 @@
 
 
 # backported descriptor view (from saml2idp.views) as it needed fixing
-def saml_metadata(request):
+def saml_metadata(request, sp_id=None):
     """
     Replies with the XML Metadata IDSSODescriptor.
     """
@@ -108,6 +109,12 @@
     slo_url = request.build_absolute_uri(reverse('logout'))
     sso_url = request.build_absolute_uri(reverse('login_begin'))
     pubkey = xml_signing.load_cert_data(config['certificate_file'])
+    if sp_id:
+        sp = get_object_or_404(SAMLConfig, pk=sp_id)
+        if sp.certificate:
+            pubkey = xml_signing.load_cert_data_from_string(
+                str(sp.certificate))
+
     tv = {
         'entity_id': entity_id,
         'cert_public_key': pubkey,