Merge lp:~roadmr/canonical-identity-provider/honor-authnrequest-nameid-policy-format-field into lp:canonical-identity-provider

Proposed by Daniel Manrique on 2019-01-18
Status: Merged
Approved by: Daniel Manrique on 2019-01-21
Approved revision: 1681
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: lp:~roadmr/canonical-identity-provider/honor-authnrequest-nameid-policy-format-field
Merge into: lp:canonical-identity-provider
Diff against target: 64 lines (+41/-0)
3 files modified
src/ubuntu_sso_saml/migrations/0008_samlconfig_honor_authnrequest_nameidpolicy_format.py (+20/-0)
src/ubuntu_sso_saml/models.py (+15/-0)
src/ubuntu_sso_saml/tests/test_models.py (+6/-0)
To merge this branch: bzr merge lp:~roadmr/canonical-identity-provider/honor-authnrequest-nameid-policy-format-field
Reviewer Review Type Date Requested Status
Natalia Bidart 2019-01-18 Approve on 2019-01-18
Review via email: mp+361981@code.launchpad.net

Commit message

Add honor_authnrequest_nameidpolicy_format boolean field to SAMLConfig.

  The intended use is to enable honoring the nameid policy format requested
  by the SP's AuthnRequest, on a config-by-config basis.

  This is done because a change to fully support honoring this with
  appropriate semantics is large and likely to break existing remotes, so this
  allows a smaller change that works for specific remotes.

Description of the change

Add honor_authnrequest_nameidpolicy_format boolean field to SAMLConfig.

  The intended use is to enable honoring the nameid policy format requested
  by the SP's AuthnRequest, on a config-by-config basis.

  This is done because a change to fully support honoring this with
  appropriate semantics is large and likely to break existing remotes, so this
  allows a smaller change that works for specific remotes.

To post a comment you must log in.
Natalia Bidart (nataliabidart) wrote :

Any chance you could add a test to ensure 1- the new attr "exists" and 2- is nullable?

1680. By Daniel Manrique on 2019-01-18

Test the honor_authnrequest_nameidpolicy_format attrib is there for newly created records and is created as NULL by default

1681. By Daniel Manrique on 2019-01-18

Update migration per the previous commit

Daniel Manrique (roadmr) wrote :

Sure! Thanks!

I added a single check that verifies both because separating them left a test that would ERROR if the attribute is not there on a newly-created record, and I remember we always prefer tests that FAIL if expectations are not met in an expected way (i.e. not relying on an ERROR to stand for a FAIL).

Natalia Bidart (nataliabidart) wrote :

lgtm

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'src/ubuntu_sso_saml/migrations/0008_samlconfig_honor_authnrequest_nameidpolicy_format.py'
2--- src/ubuntu_sso_saml/migrations/0008_samlconfig_honor_authnrequest_nameidpolicy_format.py 1970-01-01 00:00:00 +0000
3+++ src/ubuntu_sso_saml/migrations/0008_samlconfig_honor_authnrequest_nameidpolicy_format.py 2019-01-18 20:41:31 +0000
4@@ -0,0 +1,20 @@
5+# -*- coding: utf-8 -*-
6+# Generated by Django 1.11.16 on 2019-01-18 20:40
7+from __future__ import unicode_literals
8+
9+from django.db import migrations, models
10+
11+
12+class Migration(migrations.Migration):
13+
14+ dependencies = [
15+ ('ubuntu_sso_saml', '0007_samlconfig_issuer'),
16+ ]
17+
18+ operations = [
19+ migrations.AddField(
20+ model_name='samlconfig',
21+ name='honor_authnrequest_nameidpolicy_format',
22+ field=models.NullBooleanField(help_text=b'Whether to honor the NameID policy format in the SAML AuthnRequest. Since we only support "email" or "persistent" policies and we\'re not fully compliant with SAML spec for "persistent" identifiers, normally we prefer to ignore the AuthnRequest and reply with "email" NameID format identifiers. Set to "Yes" only if you know what you\'re doing and know the SP requires a "persistent" NameID but won\'t break if the actual identifier is not really persistent'),
23+ ),
24+ ]
25
26=== modified file 'src/ubuntu_sso_saml/models.py'
27--- src/ubuntu_sso_saml/models.py 2018-04-05 16:26:07 +0000
28+++ src/ubuntu_sso_saml/models.py 2019-01-18 20:41:31 +0000
29@@ -75,6 +75,21 @@
30 blank=True,
31 validators=[validate_links],
32 help_text='This field should contain a valid json document.')
33+ # Semantically the below does not need to be nullable (We need True or
34+ # False, with False being a reasonable default), but for operational
35+ # reasons, all field additions must be null=True or the equivalent.
36+ # Null is falsy so our requirement is still met in a boolean context.
37+ honor_authnrequest_nameidpolicy_format = models.NullBooleanField(
38+ help_text=('Whether to honor the NameID policy format in the SAML '
39+ 'AuthnRequest. Since we only support "email" or '
40+ '"persistent" policies and we\'re not fully compliant '
41+ 'with SAML spec for "persistent" identifiers, normally '
42+ 'we prefer to ignore the AuthnRequest and reply with '
43+ '"email" NameID format identifiers. Set to "Yes" only '
44+ 'if you know what you\'re doing and know the SP '
45+ 'requires a "persistent" NameID but won\'t break if '
46+ 'the actual identifier is not really persistent')
47+ )
48
49 def __getitem__(self, key):
50 return self.__dict__[key]
51
52=== modified file 'src/ubuntu_sso_saml/tests/test_models.py'
53--- src/ubuntu_sso_saml/tests/test_models.py 2018-04-05 16:26:07 +0000
54+++ src/ubuntu_sso_saml/tests/test_models.py 2019-01-18 20:41:31 +0000
55@@ -170,3 +170,9 @@
56 allowed_teams=', two ,')
57 self.assertEqual(conf.get_allowed_teams_list(),
58 ['two'])
59+
60+ def test_honor_authnrequest_nameidpolicy_format_exists_and_default(self):
61+ conf = SAMLConfig.objects.create(acs_url='http://localhost')
62+ self.assertTrue(
63+ hasattr(conf, 'honor_authnrequest_nameidpolicy_format'))
64+ self.assertIsNone(conf.honor_authnrequest_nameidpolicy_format)