Merge lp:~roadmr/canonical-identity-provider/dont-clobber-saml-attribute-email into lp:canonical-identity-provider/release

[Daniel Manrique]

ensure persistent id-honoring SAML peers don't mess with {{email}} attrib substitution

to QA:

- setup a saml config with "honor persistent id" and "don't use email as persistent id"
- add an attribute using the {{email}} substitution
- hit it form a suitably-configured RP
- ensure the attribute contains the email and not the sha256 persistent id

[Jonathan Hartley (tartley)]

Looks good to me, but with one nitwit idea attached. My apologies.

review: Approve

[Daniel Manrique (roadmr)]

Thanks!

[lp:~roadmr/canonical-identity-provider/dont-clobber-saml-attribute-email]

1733. By Daniel Manrique on 2020-04-08

Review comment fixes

1734. By Daniel Manrique on 2020-04-08

tweak comments

[Jonathan Hartley (tartley)]

Approved! Splendid.

review: Approve

=== modified file 'src/ubuntu_sso_saml/processors.py'
--- src/ubuntu_sso_saml/processors.py	2020-03-26 21:49:05 +0000
+++ src/ubuntu_sso_saml/processors.py	2020-04-08 15:21:38 +0000
@@ -378,7 +378,8 @@
 
     def _determine_subject(self):
         account = self._django_request.user
-        preferred = account.preferredemail.email
+        # By default the subject (user identifier) is the email.
+        email = subject = account.preferredemail.email
 
         # check if SP prefers @canonical emails
         sp_config = get_config_from_processor(self)
@@ -388,19 +389,23 @@
                     account,
                     honor_preferred=(
                         sp_config.honor_preferred_email))
+                # If the peer prefers canonical emails then use that for
+                # both subject and email.
                 if canonical_email is not None:
-                    preferred = canonical_email
-            # Maybe override subject if we need to use a true persistent id
+                    email = subject = canonical_email
             if (sp_config.honor_authnrequest_nameidpolicy_format and
                     not sp_config.send_email_as_persistent and
                     self._subject_format.split(":")[-1] == "persistent"):
+                # If the SP config requires it, replace the subject by a
+                # persistent identifier (but leave email alone).
                 # openid identifier is unicode (or str in py3 at some point)
                 # sha256 eats strings (or bytes in py3)
                 # so we must ENcode
                 identifier = account.openid_identifier.encode('utf-8')
-                preferred = sha256(identifier).hexdigest()
+                subject = sha256(identifier).hexdigest()
 
-        self._subject = preferred
+        self._email = email
+        self._subject = subject
 
     def _format_assertion(self):
         sp_config = get_config_from_processor(self)
@@ -501,7 +506,7 @@
                 if displayname:
                     attributes[key] = displayname
             elif value == '{{email}}':
-                email = self._subject
+                email = self._email
                 if email:
                     attributes[key] = email
         # Convert keys and values to utf8-encoded str and return that

=== modified file 'src/ubuntu_sso_saml/tests/test_processors.py'
--- src/ubuntu_sso_saml/tests/test_processors.py	2020-03-26 21:49:05 +0000
+++ src/ubuntu_sso_saml/tests/test_processors.py	2020-04-08 15:21:38 +0000
@@ -1978,6 +1978,26 @@
                         email))
         self.assertIn(expected, samlresponse)
 
+    def test_email_as_attribute_persistent_id(self):
+        # Ensure the email is still substituted properly in attributes
+        # even when using persistent id
+        self.setup_saml_sp(
+            attributes=json.dumps({
+                'email-from-attrib': '{{email}}',
+            }),
+            honor_authnrequest_nameidpolicy_format=True,
+            send_email_as_persistent=False
+        )
+        email = self.setup_saml_emails()
+
+        data = self.get_request_data(nameid_format="persistent")
+        samlresponse = self.do_saml_request(data=data)
+        expected = ('<saml:Attribute Name="email-from-attrib">'
+                    '<saml:AttributeValue>'
+                    '{}</saml:AttributeValue></saml:Attribute>'.format(
+                        email))
+        self.assertIn(expected, samlresponse)
+
     def test_displayname_as_attribute(self):
         self.setup_saml_sp(attributes=json.dumps({
             'fullname': '{{displayname}}',