Add openstack_groups to assertion

Currently, a keystone IdP does not provide the
groups to which user belong when generating SAML
assertions.This patch adds an additional attribute
called "openstack_groups" in the assertion.

Change-Id: I205e8bbf9a4579b16177f57e29e363f4205a2b48
Closes-Bug: #1641625

11 files changed, 254 insertions, 37 deletions

diff --git a/devstack/files/federation/attribute-map.xml b/devstack/files/federation/attribute-map.xml
index 4094caad0..6e1b3c7da 100644
--- a/devstack/files/federation/attribute-map.xml
+++ b/devstack/files/federation/attribute-map.xml
@@ -12,6 +12,7 @@
     <Attribute id="openstack_roles" name="openstack_roles"/>
     <Attribute id="openstack_user" name="openstack_user"/>
     <Attribute id="openstack_user_domain" name="openstack_user_domain"/>
+    <Attribute id="openstack_groups" name="openstack_groups"/>
 
     <!-- First some useful eduPerson attributes that many sites might use. -->
     <Attribute name="urn:mace:dir:attribute-def:eduPersonPrincipalName" id="eppn">
diff --git a/doc/source/admin/federation/mapping_combinations.rst b/doc/source/admin/federation/mapping_combinations.rst
index 60869d7e7..7af5a2e7b 100644
--- a/doc/source/admin/federation/mapping_combinations.rst
+++ b/doc/source/admin/federation/mapping_combinations.rst
@@ -892,7 +892,7 @@ look as follows:
 
 .. code-block:: xml
 
-    <!-- example from a K2k Shibboleth implementation -->
+    <!-- example 1 from a K2k Shibboleth implementation -->
     <Attribute name="openstack_user" id="openstack_user"/>
     <Attribute name="openstack_user_domain" id="openstack_user_domain"/>
 
@@ -933,6 +933,57 @@ names we have in the Identity Provider. It will map any user with the name
        ]
    }
 
+A keystone user's groups can also be mapped to groups in the service provider.
+For example, with the following attributes declared in Shibboleth's attributes file:
+
+.. code-block:: xml
+
+    <!-- example 2 from a K2k Shibboleth implementation -->
+    <Attribute name="openstack_user" id="openstack_user"/>
+    <Attribute name="openstack_groups" id="openstack_groups"/>
+
+Then the following mapping can be used to map the user's group membership from the keystone
+IdP to groups in the keystone SP:
+
+.. code-block:: json
+
+    {
+        "rules": [
+            {
+                "local":
+                [
+                    {
+                        "user":
+                            {
+                                "name": "{0}"
+                            }
+                    },
+                    {
+                        "groups": "{1}"
+                    }
+                ],
+                "remote":
+                [
+                    {
+                        "type": "openstack_user"
+                    },
+                    {
+                        "type": "openstack_groups"
+                    }
+                ]
+            }
+        ]
+    }
+
+
+``openstack_user``, and ``openstack_groups`` will be matched by service
+provider to the attribute names we have in the Identity Provider. It will
+take the ``openstack_user`` attribute and finds in the assertion then inserts
+it directly in the mapping.  The identity provider will set the value of
+``openstack_groups`` by group name and domain name to which the user belongs
+in the Idp. Suppose the user belongs to 'group1' in domain 'Default' in the IdP
+then it will map to a group with the same name and same domain's name in the SP.
+
 The possible attributes that can be used in a mapping are `openstack_user`,
-`openstack_user_domain`, `openstack_roles`, `openstack_project`, and
-`openstack_project_domain`.
+`openstack_user_domain`, `openstack_roles`, `openstack_project`,
+`openstack_project_domain` and `openstack_groups`.
diff --git a/doc/source/admin/federation/shibboleth.inc b/doc/source/admin/federation/shibboleth.inc
index 9415659ad..95710c834 100644
--- a/doc/source/admin/federation/shibboleth.inc
+++ b/doc/source/admin/federation/shibboleth.inc
@@ -236,11 +236,12 @@ attributes will be needed in ``/etc/shibboleth/attribute-map.xml``:
 
 .. code-block:: xml
 
-   <Attribute name="openstack_user" id="openstack_user"/>
-   <Attribute name="openstack_roles" id="openstack_roles"/>
-   <Attribute name="openstack_project" id="openstack_project"/>
-   <Attribute name="openstack_user_domain" id="openstack_user_domain"/>
-   <Attribute name="openstack_project_domain" id="openstack_project_domain"/>
+    <Attribute name="openstack_user" id="openstack_user"/>
+    <Attribute name="openstack_roles" id="openstack_roles"/>
+    <Attribute name="openstack_project" id="openstack_project"/>
+    <Attribute name="openstack_user_domain" id="openstack_user_domain"/>
+    <Attribute name="openstack_project_domain" id="openstack_project_domain"/>
+    <Attribute name="openstack_groups" id="openstack_groups"/>
 
 And update the ``REMOTE_USER`` variable in ``/etc/shibboleth/shibboleth2.xml``
 if desired:
diff --git a/keystone/api/_shared/saml.py b/keystone/api/_shared/saml.py
index 2e338957c..956acb8c9 100644
--- a/keystone/api/_shared/saml.py
+++ b/keystone/api/_shared/saml.py
@@ -10,6 +10,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from oslo_serialization import jsonutils
+
 from keystone.common import provider_api
 import keystone.conf
 from keystone import exception
@@ -47,8 +49,28 @@ def create_base_saml_assertion(auth):
     project_domain_name = token.project_domain['name']
     subject_domain_name = token.user_domain['name']
 
+    def group_membership():
+        """Return a list of dictionaries serialized as strings.
+
+        The expected return structure is::
+
+        ['JSON:{"name":"group1","domain":{"name":"Default"}}',
+        'JSON:{"name":"group2","domain":{"name":"Default"}}']
+        """
+        user_groups = []
+        groups = PROVIDERS.identity_api.list_groups_for_user(
+            token.user_id)
+        for group in groups:
+            user_group = {}
+            group_domain_name = PROVIDERS.resource_api.get_domain(
+                group['domain_id'])['name']
+            user_group["name"] = group['name']
+            user_group["domain"] = {'name': group_domain_name}
+            user_groups.append('JSON:' + jsonutils.dumps(user_group))
+        return user_groups
+    groups = group_membership()
     generator = keystone_idp.SAMLGenerator()
     response = generator.samlize_token(
         issuer, sp_url, subject, subject_domain_name,
-        role_names, project, project_domain_name)
+        role_names, project, project_domain_name, groups)
     return response, service_provider
diff --git a/keystone/federation/idp.py b/keystone/federation/idp.py
index e0c983e7b..fd464f5c2 100644
--- a/keystone/federation/idp.py
+++ b/keystone/federation/idp.py
@@ -48,7 +48,8 @@ class SAMLGenerator(object):
         self.assertion_id = uuid.uuid4().hex
 
     def samlize_token(self, issuer, recipient, user, user_domain_name, roles,
-                      project, project_domain_name, expires_in=None):
+                      project, project_domain_name, groups,
+                      expires_in=None):
         """Convert Keystone attributes to a SAML assertion.
 
         :param issuer: URL of the issuing party
@@ -65,6 +66,9 @@ class SAMLGenerator(object):
         :type project: string
         :param project_domain_name: Project Domain name
         :type project_domain_name: string
+        :param groups: List of strings of user groups and domain name, where
+                       strings are serialized dictionaries.
+        :type groups: list
         :param expires_in: Sets how long the assertion is valid for, in seconds
         :type expires_in: int
 
@@ -76,7 +80,8 @@ class SAMLGenerator(object):
         saml_issuer = self._create_issuer(issuer)
         subject = self._create_subject(user, expiration_time, recipient)
         attribute_statement = self._create_attribute_statement(
-            user, user_domain_name, roles, project, project_domain_name)
+            user, user_domain_name, roles, project, project_domain_name,
+            groups)
         authn_statement = self._create_authn_statement(issuer, expiration_time)
         signature = self._create_signature()
 
@@ -162,7 +167,8 @@ class SAMLGenerator(object):
         return subject
 
     def _create_attribute_statement(self, user, user_domain_name, roles,
-                                    project, project_domain_name):
+                                    project, project_domain_name,
+                                    groups):
         """Create an object that represents a SAML AttributeStatement.
 
         <ns0:AttributeStatement>
@@ -188,6 +194,15 @@ class SAMLGenerator(object):
                 <ns0:AttributeValue
                   xsi:type="xs:string">Default</ns0:AttributeValue>
             </ns0:Attribute>
+            <ns0:Attribute Name="openstack_groups">
+                <ns0:AttributeValue
+                   xsi:type="xs:string">JSON:{"name":"group1","domain":{"name":"Default"}}
+                </ns0:AttributeValue>
+                <ns0:AttributeValue
+                   xsi:type="xs:string">JSON:{"name":"group2","domain":{"name":"Default"}}
+                </ns0:AttributeValue>
+            </ns0:Attribute>
+
         </ns0:AttributeStatement>
 
         :returns: XML <AttributeStatement> object
@@ -218,6 +233,11 @@ class SAMLGenerator(object):
         attribute_statement.attribute.append(project_attribute)
         attribute_statement.attribute.append(project_domain_attribute)
         attribute_statement.attribute.append(user_domain_attribute)
+
+        if groups:
+            groups_attribute = _build_attribute(
+                'openstack_groups', groups)
+            attribute_statement.attribute.append(groups_attribute)
         return attribute_statement
 
     def _create_authn_statement(self, issuer, expiration_time):
diff --git a/keystone/federation/utils.py b/keystone/federation/utils.py
index 7dea74f08..aab9b3524 100644
--- a/keystone/federation/utils.py
+++ b/keystone/federation/utils.py
@@ -19,6 +19,7 @@ import flask
 import jsonschema
 from oslo_config import cfg
 from oslo_log import log
+from oslo_serialization import jsonutils
 from oslo_utils import timeutils
 import six
 
@@ -555,6 +556,48 @@ class RuleProcessor(object):
         LOG.debug('mapped_properties: %s', mapped_properties)
         return mapped_properties
 
+    def _normalize_groups(self, identity_value):
+        # In this case, identity_value['groups'] is a string
+        # representation of a list, and we want a real list.  This is
+        # due to the way we do direct mapping substitutions today (see
+        # function _update_local_mapping() )
+        if 'name' in identity_value['groups']:
+            try:
+                group_names_list = ast.literal_eval(
+                    identity_value['groups'])
+            except (ValueError, SyntaxError):
+                group_names_list = [identity_value['groups']]
+
+            def convert_json(group):
+                if group.startswith('JSON:'):
+                    return jsonutils.loads(group.lstrip('JSON:'))
+                return group
+
+            group_dicts = [convert_json(g) for g in group_names_list]
+            for g in group_dicts:
+                if 'domain' not in g:
+                    msg = _("Invalid rule: %(identity_value)s. Both "
+                            "'groups' and 'domain' keywords must be "
+                            "specified.")
+                    msg = msg % {'identity_value': identity_value}
+                    raise exception.ValidationError(msg)
+        else:
+            if 'domain' not in identity_value:
+                msg = _("Invalid rule: %(identity_value)s. Both "
+                        "'groups' and 'domain' keywords must be "
+                        "specified.")
+                msg = msg % {'identity_value': identity_value}
+                raise exception.ValidationError(msg)
+            try:
+                group_names_list = ast.literal_eval(
+                    identity_value['groups'])
+            except (ValueError, SyntaxError):
+                group_names_list = [identity_value['groups']]
+            domain = identity_value['domain']
+            group_dicts = [{'name': name, 'domain': domain} for name in
+                           group_names_list]
+        return group_dicts
+
     def _transform(self, identity_values):
         """Transform local mappings, to an easier to understand format.
 
@@ -642,24 +685,7 @@ class RuleProcessor(object):
                     groups_by_domain.setdefault(domain, list()).append(group)
                 group_names.extend(extract_groups(groups_by_domain))
             if 'groups' in identity_value:
-                if 'domain' not in identity_value:
-                    msg = _("Invalid rule: %(identity_value)s. Both 'groups' "
-                            "and 'domain' keywords must be specified.")
-                    msg = msg % {'identity_value': identity_value}
-                    raise exception.ValidationError(msg)
-                # In this case, identity_value['groups'] is a string
-                # representation of a list, and we want a real list.  This is
-                # due to the way we do direct mapping substitutions today (see
-                # function _update_local_mapping() )
-                try:
-                    group_names_list = ast.literal_eval(
-                        identity_value['groups'])
-                except (ValueError, SyntaxError):
-                    group_names_list = [identity_value['groups']]
-                domain = identity_value['domain']
-                group_dicts = [{'name': name, 'domain': domain} for name in
-                               group_names_list]
-
+                group_dicts = self._normalize_groups(identity_value)
                 group_names.extend(group_dicts)
             if 'group_ids' in identity_value:
                 # If identity_values['group_ids'] is a string representation
diff --git a/keystone/tests/unit/contrib/federation/test_utils.py b/keystone/tests/unit/contrib/federation/test_utils.py
index b182cb10f..29b7f11a0 100644
--- a/keystone/tests/unit/contrib/federation/test_utils.py
+++ b/keystone/tests/unit/contrib/federation/test_utils.py
@@ -801,6 +801,31 @@ class MappingRuleEngineTests(unit.BaseTestCase):
         ]
         self.assertEqual(expected_projects, values['projects'])
 
+    def test_rule_engine_for_groups_and_domain(self):
+        """Should return user's groups and group domain.
+
+        The GROUP_DOMAIN_ASSERTION should successfully have a match in
+        MAPPING_GROUPS_DOMAIN_OF_USER. This will test the case where a groups
+        with its domain will exist`, and return user's groups and group domain.
+
+        """
+        mapping = mapping_fixtures.MAPPING_GROUPS_DOMAIN_OF_USER
+        assertion = mapping_fixtures.GROUPS_DOMAIN_ASSERTION
+        rp = mapping_utils.RuleProcessor(FAKE_MAPPING_ID, mapping['rules'])
+        values = rp.process(assertion)
+
+        self.assertValidMappedUserObject(values)
+        user_name = assertion.get('openstack_user')
+        user_groups = ['group1', 'group2']  # since we know the input assertion
+        groups = values.get('group_names', {})
+        group_list = [g.get('name') for g in groups]
+        group_ids = values.get('group_ids')
+        name = values.get('user', {}).get('name')
+
+        self.assertEqual(user_name, name)
+        self.assertEqual(user_groups, group_list)
+        self.assertEqual([], group_ids, )
+
 
 class TestUnicodeAssertionData(unit.BaseTestCase):
     """Ensure that unicode data in the assertion headers works.
diff --git a/keystone/tests/unit/mapping_fixtures.py b/keystone/tests/unit/mapping_fixtures.py
index 0a9d194e2..341ba897a 100644
--- a/keystone/tests/unit/mapping_fixtures.py
+++ b/keystone/tests/unit/mapping_fixtures.py
@@ -1494,6 +1494,35 @@ MAPPING_GROUPS_WITH_EMAIL = {
     ]
 }
 
+
+MAPPING_GROUPS_DOMAIN_OF_USER = {
+    "rules": [
+        {
+            "local":
+            [
+                {
+                    "user":
+                        {
+                            "name": "{0}"
+                        }
+                },
+                {
+                    "groups": "{1}"
+                }
+            ],
+            "remote":
+            [
+                {
+                    "type": "openstack_user"
+                },
+                {
+                    "type": "openstack_groups"
+                }
+            ]
+        }
+    ]
+}
+
 EMPLOYEE_ASSERTION = {
     'Email': 'tim@example.com',
     'UserName': 'tbo',
@@ -1652,6 +1681,14 @@ GROUPS_ASSERTION_ONLY_ONE_GROUP = {
     'groups': 'ALL USERS'
 }
 
+GROUPS_DOMAIN_ASSERTION = {
+    'openstack_user': 'bwilliams',
+    'openstack_user_domain': 'default',
+    'openstack_roles': 'Admin',
+    'openstack_groups': 'JSON:{"name":"group1","domain":{"name":"xxx"}};'
+                        'JSON:{"name":"group2","domain":{"name":"yyy"}}'
+}
+
 MAPPING_UNICODE = {
     "rules": [
         {
diff --git a/keystone/tests/unit/saml2/signed_saml2_assertion.xml b/keystone/tests/unit/saml2/signed_saml2_assertion.xml
index 414ff9cf7..1e7bd0db2 100644
--- a/keystone/tests/unit/saml2/signed_saml2_assertion.xml
+++ b/keystone/tests/unit/saml2/signed_saml2_assertion.xml
@@ -65,5 +65,9 @@ UHeBXxQq/GmfBv3l+V5ObQ+EHKnyDodLHCk=</ns1:X509Certificate>
     <ns0:Attribute Name="openstack_project_domain" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
       <ns0:AttributeValue xsi:type="xs:string">project_domain</ns0:AttributeValue>
     </ns0:Attribute>
+    <ns0:Attribute Name="openstack_groups" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
+      <ns0:AttributeValue xsi:type="xs:string">JSON:{"name":"group1","domain":{"name":"Default"}}</ns0:AttributeValue>
+      <ns0:AttributeValue xsi:type="xs:string">JSON:{"name":"group2","domain":{"name":"Default"}}</ns0:AttributeValue>
+    </ns0:Attribute>
   </ns0:AttributeStatement>
 </ns0:Assertion>
diff --git a/keystone/tests/unit/test_v3_federation.py b/keystone/tests/unit/test_v3_federation.py
index a24ddfc46..61175a17e 100644
--- a/keystone/tests/unit/test_v3_federation.py
+++ b/keystone/tests/unit/test_v3_federation.py
@@ -3818,6 +3818,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
     ROLES = ['admin', 'member']
     PROJECT = 'development'
     PROJECT_DOMAIN = 'project_domain'
+    GROUPS = ['JSON:{"name":"group1","domain":{"name":"Default"}}',
+              'JSON:{"name":"group2","domain":{"name":"Default"}}']
     SAML_GENERATION_ROUTE = '/auth/OS-FEDERATION/saml2'
     ECP_GENERATION_ROUTE = '/auth/OS-FEDERATION/saml2/ecp'
     ASSERTION_VERSION = "2.0"
@@ -3849,7 +3851,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
                                                self.SUBJECT,
                                                self.SUBJECT_DOMAIN,
                                                self.ROLES, self.PROJECT,
-                                               self.PROJECT_DOMAIN)
+                                               self.PROJECT_DOMAIN,
+                                               self.GROUPS)
 
         assertion = response.assertion
         self.assertIsNotNone(assertion)
@@ -3879,6 +3882,10 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
         self.assertEqual(self.PROJECT_DOMAIN,
                          project_domain_attribute.attribute_value[0].text)
 
+        group_attribute = assertion.attribute_statement[0].attribute[5]
+        for attribute_value in group_attribute.attribute_value:
+            self.assertIn(attribute_value.text, self.GROUPS)
+
     def test_comma_in_certfile_path(self):
         self.config_fixture.config(
             group='saml',
@@ -3893,7 +3900,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
             self.SUBJECT_DOMAIN,
             self.ROLES,
             self.PROJECT,
-            self.PROJECT_DOMAIN)
+            self.PROJECT_DOMAIN,
+            self.GROUPS)
 
     def test_comma_in_keyfile_path(self):
         self.config_fixture.config(
@@ -3909,7 +3917,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
             self.SUBJECT_DOMAIN,
             self.ROLES,
             self.PROJECT,
-            self.PROJECT_DOMAIN)
+            self.PROJECT_DOMAIN,
+            self.GROUPS)
 
     def test_verify_assertion_object(self):
         """Test that the Assertion object is built properly.
@@ -3925,7 +3934,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
                                                self.SUBJECT,
                                                self.SUBJECT_DOMAIN,
                                                self.ROLES, self.PROJECT,
-                                               self.PROJECT_DOMAIN)
+                                               self.PROJECT_DOMAIN,
+                                               self.GROUPS)
         assertion = response.assertion
         self.assertEqual(self.ASSERTION_VERSION, assertion.version)
 
@@ -3944,7 +3954,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
                                                self.SUBJECT,
                                                self.SUBJECT_DOMAIN,
                                                self.ROLES, self.PROJECT,
-                                               self.PROJECT_DOMAIN)
+                                               self.PROJECT_DOMAIN,
+                                               self.GROUPS)
 
         saml_str = response.to_string()
         response = etree.fromstring(saml_str)
@@ -3970,6 +3981,10 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
         project_domain_attribute = assertion[4][4]
         self.assertEqual(self.PROJECT_DOMAIN, project_domain_attribute[0].text)
 
+        group_attribute = assertion[4][5]
+        for attribute_value in group_attribute:
+            self.assertIn(attribute_value.text, self.GROUPS)
+
     def test_assertion_using_explicit_namespace_prefixes(self):
         def mocked_subprocess_check_output(*popenargs, **kwargs):
             # the last option is the assertion file to be signed
@@ -3988,7 +4003,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
                                                self.SUBJECT,
                                                self.SUBJECT_DOMAIN,
                                                self.ROLES, self.PROJECT,
-                                               self.PROJECT_DOMAIN)
+                                               self.PROJECT_DOMAIN,
+                                               self.GROUPS)
             assertion_xml = response.assertion.to_string()
             # The expected values in the assertions bellow need to be 'str' in
             # Python 2 and 'bytes' in Python 3
@@ -4017,7 +4033,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
         response = generator.samlize_token(self.ISSUER, self.RECIPIENT,
                                            self.SUBJECT, self.SUBJECT_DOMAIN,
                                            self.ROLES, self.PROJECT,
-                                           self.PROJECT_DOMAIN)
+                                           self.PROJECT_DOMAIN,
+                                           self.GROUPS)
 
         signature = response.assertion.signature
         self.assertIsNotNone(signature)
@@ -4132,6 +4149,9 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
         project_domain_attribute = assertion[4][4]
         self.assertIsInstance(project_domain_attribute[0].text, str)
 
+        group_attribute = assertion[4][5]
+        self.assertIsInstance(group_attribute[0].text, str)
+
     def test_invalid_scope_body(self):
         """Test that missing the scope in request body raises an exception.
 
@@ -4247,6 +4267,9 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
         project_domain_attribute = assertion[4][4]
         self.assertIsInstance(project_domain_attribute[0].text, str)
 
+        group_attribute = assertion[4][5]
+        self.assertIsInstance(group_attribute[0].text, str)
+
     @mock.patch('saml2.create_class_from_xml_string')
     @mock.patch('oslo_utils.fileutils.write_to_tempfile')
     @mock.patch.object(subprocess, 'check_output')
diff --git a/releasenotes/notes/bug-1641625-fe463874dc5edb10.yaml b/releasenotes/notes/bug-1641625-fe463874dc5edb10.yaml
new file mode 100644
index 000000000..035338ee0
--- /dev/null
+++ b/releasenotes/notes/bug-1641625-fe463874dc5edb10.yaml
@@ -0,0 +1,7 @@
+---
+features:
+  - |
+    [`bug 1641625 <https://bugs.launchpad.net/keystone/+bug/1641625>`_]
+    The keystone configured as an identity provider now includes an additional
+    attribute called `openstack_groups` in the assertion when generating SAML
+    assertions.