Merge lp:~ricardokirkner/canonical-identity-provider/saml-no-clobber-issuer into lp:canonical-identity-provider/release

[Ricardo Kirkner]

keep AuthnRequest Issuer in REQUEST_ISSUER to avoid clobbering system-wide ISSUER value

saml2idp keeps the system-wide issuer (which is used when rendering the saml response) in the ISSUER key; storing the AuthnRequest issuer in that key was breaking saml responses (as they'd report as being issued by the SP instead of the IdP).

[Daniel Manrique (roadmr)]

Oops... looks OK to put this in a different dict key.

Thanks!

review: Approve

=== modified file 'src/ubuntu_sso_saml/processors.py'
--- src/ubuntu_sso_saml/processors.py	2018-04-06 19:41:49 +0000
+++ src/ubuntu_sso_saml/processors.py	2018-04-09 19:02:02 +0000
@@ -328,7 +328,7 @@
         if issuer is not None:
             issuer = issuer.text
         params['ACS_URL'] = acs_url
-        params['ISSUER'] = issuer
+        params['REQUEST_ISSUER'] = issuer
         params['REQUEST_ID'] = request['id']
         params['DESTINATION'] = request.get('destination', '')
         params['PROVIDER_NAME'] = request.get('providername', '')
@@ -371,7 +371,7 @@
 
     def _validate_sp_config(self):
         acs_url = self._request_params.get('ACS_URL')
-        issuer = self._request_params.get('ISSUER')
+        issuer = self._request_params.get('REQUEST_ISSUER')
         sp_config = get_config_from_processor(self)
         if sp_config is not None:
             return sp_config

=== modified file 'src/ubuntu_sso_saml/tests/test_processors.py'
--- src/ubuntu_sso_saml/tests/test_processors.py	2018-04-06 19:41:49 +0000
+++ src/ubuntu_sso_saml/tests/test_processors.py	2018-04-09 19:02:02 +0000
@@ -134,7 +134,7 @@
 
     def setup_saml_sp(self, **kwargs):
         acs_url = kwargs.pop('acs_url', self.ACS_URL)
-        issuer = kwargs.pop('issuer', getattr(self, 'ISSUER', None))
+        issuer = kwargs.pop('issuer', getattr(self, 'REQUEST_ISSUER', None))
         prefer_canonical_email = kwargs.get('prefer_canonical_email', False)
         twofactor_required = kwargs.pop('twofactor_required', False)
         allowed_teams = kwargs.pop('allowed_teams', '')
@@ -1167,7 +1167,7 @@
 
 """)  # noqa
     ACS_URL = 'https://saml.example.com/saml_acs_url'
-    ISSUER = 'https://saml.example.com/metadata'
+    REQUEST_ISSUER = 'https://saml.example.com/metadata'
     AUDIENCE = 'https://login.ubuntu.com/+saml'
     RELAY_STATE = 'https://example.com/'
     # Note REQUEST_DATA is incomplete and needs a SAMLRequest added by
@@ -1202,7 +1202,7 @@
         data = dict(**self.REQUEST_DATA)
         payload = self.SAML_PAYLOAD.format(
             audience=self.AUDIENCE, acs_url=self.ACS_URL,
-            issuer=self.ISSUER, relay_state=self.RELAY_STATE)
+            issuer=self.REQUEST_ISSUER, relay_state=self.RELAY_STATE)
         if compressed:
             data['SAMLRequest'] = codex.deflate_and_base64_encode(payload)
         else:
@@ -1285,7 +1285,7 @@
 
         data = self.get_request_data()
         payload = SAML_PAYLOAD.format(
-            audience=self.AUDIENCE, issuer=self.ISSUER)
+            audience=self.AUDIENCE, issuer=self.REQUEST_ISSUER)
         # override data with ACS_URL-less payload
         data['SAMLRequest'] = base64.b64encode(payload)
 
@@ -1322,7 +1322,7 @@
 
         data = self.get_request_data()
         payload = SAML_PAYLOAD.format(
-            audience=self.AUDIENCE, issuer=self.ISSUER)
+            audience=self.AUDIENCE, issuer=self.REQUEST_ISSUER)
         # override data with ACS_URL-less payload
         data['SAMLRequest'] = base64.b64encode(payload)
 
@@ -1359,7 +1359,7 @@
 
         data = self.get_request_data()
         payload = SAML_PAYLOAD.format(
-            audience=self.AUDIENCE, issuer=self.ISSUER)
+            audience=self.AUDIENCE, issuer=self.REQUEST_ISSUER)
         # override data with ACS_URL-less payload
         data['SAMLRequest'] = base64.b64encode(payload)
 
@@ -1375,6 +1375,47 @@
         assert len(forms) == 1
         self.assertEqual(forms[0].get('action'), self.ACS_URL)
 
+    def test_response_with_system_issuer_not_authnrequest_issuer(self):
+        self.setup_saml_sp()
+
+        SAML_PAYLOAD = ("""
+<?xml version="1.0" encoding="UTF-8"?>
+<samlp:AuthnRequest xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
+                    ID="THE_ID"
+                    Version="2.0"
+                    IssueInstant="2016-09-08T21:40:15.988Z"
+                    ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"
+                    Destination="{audience}"
+                    >
+    <saml:Issuer xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">{issuer}</saml:Issuer>
+    <samlp:NameIDPolicy xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
+                        Format="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress"
+                        AllowCreate="true"
+                        />
+    <samlp:RequestedAuthnContext xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
+                                 Comparison="exact"
+                                 >
+        <saml:AuthnContextClassRef xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml:AuthnContextClassRef>
+    </samlp:RequestedAuthnContext>
+</samlp:AuthnRequest>
+
+
+""")  # noqa
+
+        data = self.get_request_data()
+        payload = SAML_PAYLOAD.format(
+            audience=self.AUDIENCE, issuer=self.REQUEST_ISSUER)
+        # override data with ACS_URL-less payload
+        data['SAMLRequest'] = base64.b64encode(payload)
+
+        samlresponse = self.do_saml_request(data=data)
+        self.assertNotIn(
+            'https://saml.example.com/metadata</saml:Issuer>', samlresponse)
+        self.assertIn(
+            '{}</saml:Issuer>'.format(
+                saml2idp_metadata.SAML2IDP_CONFIG['issuer']),
+            samlresponse)
+
     def test_enabled_db_spconfig_overrides_settings(self):
         # make sure there is an "old/custom" processor handling this request
         self.ACS_URL = google_apps.GOOGLE_APPS_ACS

=== modified file 'src/ubuntu_sso_saml/utils.py'
--- src/ubuntu_sso_saml/utils.py	2018-04-06 19:41:49 +0000
+++ src/ubuntu_sso_saml/utils.py	2018-04-09 19:02:02 +0000
@@ -29,7 +29,7 @@
 
 def get_config_from_processor(proc):
     acs_url = proc._request_params.get('ACS_URL')
-    issuer = proc._request_params.get('ISSUER')
+    issuer = proc._request_params.get('REQUEST_ISSUER')
     return get_config_for_acs(acs_url, issuer=issuer)