Enhance federation group mapping validation

A group must be reffered either with an ID, or the name _and_ the
domain. Change the JSON validation schema to check this.

Closes-Bug: #1657978

Change-Id: I213876e30fc0521195848479278080bdac8387de
(cherry picked from commit a9d79e098732445efcd58a6b03148fe6c62e044a)

3 files changed, 124 insertions, 30 deletions

diff --git a/keystone/federation/utils.py b/keystone/federation/utils.py
index 1d215a686..1e3d536f8 100644
--- a/keystone/federation/utils.py
+++ b/keystone/federation/utils.py
@@ -61,12 +61,7 @@ MAPPING_SCHEMA = {
                                         "name": {"type": "string"},
                                         "email": {"type": "string"},
                                         "domain": {
-                                            "type": "object",
-                                            "properties": {
-                                                "id": {"type": "string"},
-                                                "name": {"type": "string"}
-                                            },
-                                            "additionalProperties": False,
+                                            "$ref": "#/definitions/domain"
                                         },
                                         "type": {
                                             "type": "string",
@@ -78,19 +73,10 @@ MAPPING_SCHEMA = {
                                 },
                                 "group": {
                                     "type": "object",
-                                    "properties": {
-                                        "id": {"type": "string"},
-                                        "name": {"type": "string"},
-                                        "domain": {
-                                            "type": "object",
-                                            "properties": {
-                                                "id": {"type": "string"},
-                                                "name": {"type": "string"}
-                                            },
-                                            "additionalProperties": False,
-                                        },
-                                    },
-                                    "additionalProperties": False,
+                                    "oneOf": [
+                                        {"$ref": "#/definitions/group_by_id"},
+                                        {"$ref": "#/definitions/group_by_name"}
+                                    ]
                                 },
                                 "groups": {
                                     "type": "string"
@@ -98,14 +84,7 @@ MAPPING_SCHEMA = {
                                 "group_ids": {
                                     "type": "string"
                                 },
-                                "domain": {
-                                    "type": "object",
-                                    "properties": {
-                                        "id": {"type": "string"},
-                                        "name": {"type": "string"}
-                                    },
-                                    "additionalProperties": False
-                                }
+                                "domain": {"$ref": "#/definitions/domain"},
                             }
                         }
                     },
@@ -195,6 +174,31 @@ MAPPING_SCHEMA = {
                     "type": "array"
                 }
             }
+        },
+        "domain": {
+            "type": "object",
+            "properties": {
+                "id": {"type": "string"},
+                "name": {"type": "string"}
+            },
+            "additionalProperties": False
+        },
+        "group_by_id": {
+            "type": "object",
+            "properties": {
+                "id": {"type": "string"}
+            },
+            "additionalProperties": False,
+            "required": ["id"]
+        },
+        "group_by_name": {
+            "type": "object",
+            "properties": {
+                "name": {"type": "string"},
+                "domain": {"$ref": "#/definitions/domain"}
+            },
+            "additionalProperties": False,
+            "required": ["name", "domain"]
         }
     }
 }
diff --git a/keystone/tests/unit/contrib/federation/test_utils.py b/keystone/tests/unit/contrib/federation/test_utils.py
index 4feb3905f..eb9b01cc9 100644
--- a/keystone/tests/unit/contrib/federation/test_utils.py
+++ b/keystone/tests/unit/contrib/federation/test_utils.py
@@ -21,7 +21,6 @@ from keystone import exception
 from keystone.federation import utils as mapping_utils
 from keystone.tests import unit
 from keystone.tests.unit import mapping_fixtures
-from keystone.tests.unit import utils as test_utils
 
 
 FAKE_MAPPING_ID = uuid.uuid4().hex
@@ -630,14 +629,30 @@ class MappingRuleEngineTests(unit.BaseTestCase):
         mapping = mapping_fixtures.MAPPING_GROUP_NAMES
         mapping_utils.validate_mapping_structure(mapping)
 
-    @test_utils.wip('waiting for fix the validator '
-                    'to choke on group name without domain')
+    def test_mapping_validation_bad_domain(self):
+        mapping = mapping_fixtures.MAPPING_BAD_DOMAIN
+        self.assertRaises(exception.ValidationError,
+                          mapping_utils.validate_mapping_structure,
+                          mapping)
+
+    def test_mapping_validation_bad_group(self):
+        mapping = mapping_fixtures.MAPPING_BAD_GROUP
+        self.assertRaises(exception.ValidationError,
+                          mapping_utils.validate_mapping_structure,
+                          mapping)
+
     def test_mapping_validation_with_group_name_without_domain(self):
         mapping = mapping_fixtures.MAPPING_GROUP_NAME_WITHOUT_DOMAIN
         self.assertRaises(exception.ValidationError,
                           mapping_utils.validate_mapping_structure,
                           mapping)
 
+    def test_mapping_validation_with_group_id_and_domain(self):
+        mapping = mapping_fixtures.MAPPING_GROUP_ID_WITH_DOMAIN
+        self.assertRaises(exception.ValidationError,
+                          mapping_utils.validate_mapping_structure,
+                          mapping)
+
     def test_mapping_validation_no_local(self):
         mapping = mapping_fixtures.MAPPING_MISSING_LOCAL
         self.assertRaises(exception.ValidationError,
diff --git a/keystone/tests/unit/mapping_fixtures.py b/keystone/tests/unit/mapping_fixtures.py
index 1e3d06d28..e4a836fb7 100644
--- a/keystone/tests/unit/mapping_fixtures.py
+++ b/keystone/tests/unit/mapping_fixtures.py
@@ -607,6 +607,81 @@ MAPPING_GROUP_NAME_WITHOUT_DOMAIN = {
     ]
 }
 
+MAPPING_GROUP_ID_WITH_DOMAIN = {
+
+    "rules": [
+        {
+            "local": [
+                {
+                    "group": {
+                        "id": EMPLOYEE_GROUP_ID,
+                        "domain": {
+                            "id": DEVELOPER_GROUP_DOMAIN_ID
+                        }
+                    }
+                }
+            ],
+            "remote": [
+                {
+                    "type": "orgPersonType",
+                    "any_one_of": [
+                        "Employee"
+                    ],
+                }
+            ]
+        },
+    ]
+}
+
+MAPPING_BAD_GROUP = {
+
+    "rules": [
+        {
+            "local": [
+                {
+                    "group": {
+                    }
+                }
+            ],
+            "remote": [
+                {
+                    "type": "orgPersonType",
+                    "any_one_of": [
+                        "Employee"
+                    ],
+                }
+            ]
+        },
+    ]
+}
+
+MAPPING_BAD_DOMAIN = {
+
+    "rules": [
+        {
+            "local": [
+                {
+                    "group": {
+                        "id": EMPLOYEE_GROUP_ID,
+                        "domain": {
+                            "id": DEVELOPER_GROUP_DOMAIN_ID,
+                            "badkey": "badvalue"
+                        }
+                    }
+                }
+            ],
+            "remote": [
+                {
+                    "type": "orgPersonType",
+                    "any_one_of": [
+                        "Employee"
+                    ],
+                }
+            ]
+        },
+    ]
+}
+
 MAPPING_EPHEMERAL_USER = {
     "rules": [
         {