Allow multiple values for OIDC attribute requirements

changelog.d/12488.feature

@@ -0,0 +1 @@
+Add the ability of filtering multiple SSO attribute values.

docs/sample_config.yaml

@@ -1996,17 +1996,22 @@ saml2_config:
 #
 #   It is possible to configure Synapse to only allow logins if certain attributes
 #   match particular values in the OIDC userinfo. The requirements can be listed under
-#   `attribute_requirements` as shown below. All of the listed attributes must
-#   match for the login to be permitted. Additional attributes can be added to
-#   userinfo by expanding the `scopes` section of the OIDC config to retrieve
-#   additional information from the OIDC provider.
+#   `attribute_requirements` as shown below. All of the listed in `value` attributes and
+#   any of the listed in `values` attributes must match for the login to be permitted.
+#   Additional attributes can be added to userinfo by expanding the `scopes` section
+#   of the OIDC config to retrieve additional information from the OIDC provider.
+#
+#   If you specify both `value` and `values` entries, then only `value` entry will be checked
+#   and `values` will be ignored.
 #
 #   If the OIDC claim is a list, then the attribute must match any value in the list.
 #   Otherwise, it must exactly match the value of the claim. Using the example
-#   below, the `family_name` claim MUST be "Stephensson", but the `groups`
-#   claim MUST contain "admin".
+#   below, the `given_name` claim MUST be either "Andries" or "Michael", the `family_name`
+#   claim MUST be "Stephensson", but the `groups` claim MUST contain "admin".
 #
 #   attribute_requirements:
+#     - attribute: given_name
+#       values: ["Andries", "Michael"]
 #     - attribute: family_name
 #       value: "Stephensson"
 #     - attribute: groups

synapse/config/oidc.py

@@ -208,17 +208,22 @@ def generate_config_section(self, **kwargs: Any) -> str:
         #
         #   It is possible to configure Synapse to only allow logins if certain attributes
         #   match particular values in the OIDC userinfo. The requirements can be listed under
-        #   `attribute_requirements` as shown below. All of the listed attributes must
-        #   match for the login to be permitted. Additional attributes can be added to
-        #   userinfo by expanding the `scopes` section of the OIDC config to retrieve
-        #   additional information from the OIDC provider.
+        #   `attribute_requirements` as shown below. All of the listed in `value` attributes and
+        #   any of the listed in `values` attributes must match for the login to be permitted.
+        #   Additional attributes can be added to userinfo by expanding the `scopes` section
+        #   of the OIDC config to retrieve additional information from the OIDC provider.
+        #
+        #   If you specify both `value` and `values` entries, then only `value` entry will be checked
+        #   and `values` will be ignored.
         #
         #   If the OIDC claim is a list, then the attribute must match any value in the list.
         #   Otherwise, it must exactly match the value of the claim. Using the example
-        #   below, the `family_name` claim MUST be "Stephensson", but the `groups`
-        #   claim MUST contain "admin".
+        #   below, the `given_name` claim MUST be either "Andries" or "Michael", the `family_name`
+        #   claim MUST be "Stephensson", but the `groups` claim MUST contain "admin".
         #
         #   attribute_requirements:
+        #     - attribute: given_name
+        #       values: ["Andries", "Michael"]
         #     - attribute: family_name
         #       value: "Stephensson"
         #     - attribute: groups

synapse/config/sso.py

@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import Any, Dict, Optional
+from typing import Any, Dict, List, Optional
 
 import attr
 
@@ -36,13 +36,27 @@ class SsoAttributeRequirement:
     """Object describing a single requirement for SSO attributes."""
 
     attribute: str
-    # If a value is not given, than the attribute must simply exist.
-    value: Optional[str]
+    # If a value or values are not given, than the attribute must simply exist.
+    value: Optional[str] = None
+    values: Optional[List[str]] = []
 
     JSON_SCHEMA = {
         "type": "object",
-        "properties": {"attribute": {"type": "string"}, "value": {"type": "string"}},
-        "required": ["attribute", "value"],
+        "properties": {
+            "attribute": {"type": "string"},
+            "value": {"type": "string"},
+            "values": {
+                "type": "array",
+                "items": {
+                    "type": "string",
+                },
+            },
+        },
+        "required": ["attribute"],
+        "oneOf": [
+            {"required": ["value"]},
+            {"required": ["values"]},
+        ],
     }
 
 

synapse/handlers/sso.py

@@ -1042,12 +1042,17 @@ def _check_attribute_requirement(
         logger.info("SSO attribute missing: %s", req.attribute)
         return False
 
-    # If the requirement is None, the attribute existing is enough.
-    if req.value is None:
+    req_values = []
+    if req.value is not None:
+        req_values.append(req.value)
+    elif req.values:
+        req_values.extend(req.values)
+    else:
+        # If the requirement is None or empty, the attribute existing is enough.
         return True
 
     values = attributes[req.attribute]
-    if req.value in values:
+    if any(req_value in values for req_value in req_values):
         return True
 
     logger.info(

tests/handlers/test_oidc.py

@@ -1114,6 +1114,46 @@ def test_attribute_requirements(self) -> None:
             auth_provider_session_id=None,
         )
 
+    @override_config(
+        {
+            "oidc_config": {
+                **DEFAULT_CONFIG,
+                "attribute_requirements": [{"attribute": "test", "values": []}],
+            }
+        }
+    )
+    def test_attribute_requirements_exists(self) -> None:
+        """The required attributes must exists."""
+        auth_handler = self.hs.get_auth_handler()
+        auth_handler.complete_sso_login = simple_async_mock()
+
+        # userinfo lacking "test": "foobar" attribute should fail.
+        userinfo = {
+            "sub": "tester",
+            "username": "tester",
+        }
+        self.get_success(_make_callback_with_userinfo(self.hs, userinfo))
+        auth_handler.complete_sso_login.assert_not_called()
+
+        # userinfo with "test": "foobar" attribute should succeed.
+        userinfo = {
+            "sub": "tester",
+            "username": "tester",
+            "test": "foobar",
+        }
+        self.get_success(_make_callback_with_userinfo(self.hs, userinfo))
+
+        # check that the auth handler got called as expected
+        auth_handler.complete_sso_login.assert_called_once_with(
+            "@tester:test",
+            "oidc",
+            ANY,
+            ANY,
+            None,
+            new_user=True,
+            auth_provider_session_id=None,
+        )
+
     @override_config(
         {
             "oidc_config": {
@@ -1145,6 +1185,39 @@ def test_attribute_requirements_contains(self) -> None:
             auth_provider_session_id=None,
         )
 
+    @override_config(
+        {
+            "oidc_config": {
+                **DEFAULT_CONFIG,
+                "attribute_requirements": [
+                    {"attribute": "test", "values": ["not_foobar", "bar"]}
+                ],
+            }
+        }
+    )
+    def test_attribute_requirements_contains_any_values(self) -> None:
+        """Test that auth succeeds if userinfo attribute CONTAINS ANY required values"""
+        auth_handler = self.hs.get_auth_handler()
+        auth_handler.complete_sso_login = simple_async_mock()
+        # userinfo with "test": ["foobar", "foo", "bar"] attribute should succeed.
+        userinfo = {
+            "sub": "tester",
+            "username": "tester",
+            "test": ["foobar", "foo", "bar"],
+        }
+        self.get_success(_make_callback_with_userinfo(self.hs, userinfo))
+
+        # check that the auth handler got called as expected
+        auth_handler.complete_sso_login.assert_called_once_with(
+            "@tester:test",
+            "oidc",
+            ANY,
+            ANY,
+            None,
+            new_user=True,
+            auth_provider_session_id=None,
+        )
+
     @override_config(
         {
             "oidc_config": {
@@ -1237,6 +1310,81 @@ def _generate_oidc_session_token(
             ),
         )
 
+    @override_config(
+        {
+            "oidc_config": {
+                **DEFAULT_CONFIG,
+                "attribute_requirements": [
+                    {"attribute": "test", "values": ["foobar", "barfoo"]}
+                ],
+            }
+        }
+    )
+    def test_attribute_requirements_mismatch_any_values(self) -> None:
+        """
+        Test that auth fails if attributes exist but don't match ANY VALUE,
+        or are non-string values.
+        """
+        auth_handler = self.hs.get_auth_handler()
+        auth_handler.complete_sso_login = simple_async_mock()
+        # userinfo with "test": "not_foobar" attribute should fail
+        userinfo: dict = {
+            "sub": "tester",
+            "username": "tester",
+            "test": "not_foobar",
+        }
+        self.get_success(_make_callback_with_userinfo(self.hs, userinfo))
+        auth_handler.complete_sso_login.assert_not_called()
+
+        # userinfo with "test": ["foo", "bar"] attribute should fail
+        userinfo = {
+            "sub": "tester",
+            "username": "tester",
+            "test": ["foo", "bar"],
+        }
+        self.get_success(_make_callback_with_userinfo(self.hs, userinfo))
+        auth_handler.complete_sso_login.assert_not_called()
+
+        # userinfo with "test": False attribute should fail
+        # this is largely just to ensure we don't crash here
+        userinfo = {
+            "sub": "tester",
+            "username": "tester",
+            "test": False,
+        }
+        self.get_success(_make_callback_with_userinfo(self.hs, userinfo))
+        auth_handler.complete_sso_login.assert_not_called()
+
+        # userinfo with "test": None attribute should fail
+        # a value of None breaks the OIDC spec, but it's important to not crash here
+        userinfo = {
+            "sub": "tester",
+            "username": "tester",
+            "test": None,
+        }
+        self.get_success(_make_callback_with_userinfo(self.hs, userinfo))
+        auth_handler.complete_sso_login.assert_not_called()
+
+        # userinfo with "test": 1 attribute should fail
+        # this is largely just to ensure we don't crash here
+        userinfo = {
+            "sub": "tester",
+            "username": "tester",
+            "test": 1,
+        }
+        self.get_success(_make_callback_with_userinfo(self.hs, userinfo))
+        auth_handler.complete_sso_login.assert_not_called()
+
+        # userinfo with "test": 3.14 attribute should fail
+        # this is largely just to ensure we don't crash here
+        userinfo = {
+            "sub": "tester",
+            "username": "tester",
+            "test": 3.14,
+        }
+        self.get_success(_make_callback_with_userinfo(self.hs, userinfo))
+        auth_handler.complete_sso_login.assert_not_called()
+
 
 async def _make_callback_with_userinfo(
     hs: HomeServer, userinfo: dict, client_redirect_url: str = "http://client/redirect"