More validation relative to discovery

2 files changed, 79 insertions, 2 deletions

diff --git a/synapse/config/oidc2.py b/synapse/config/oidc2.py
index 4772e23183..eb00a622fc 100644
--- a/synapse/config/oidc2.py
+++ b/synapse/config/oidc2.py
@@ -136,14 +136,50 @@ class OIDCProviderModel(BaseModel):
 
     # the OIDC userinfo endpoint. Required if discovery is disabled and the
     # "openid" scope is not requested.
-    # TODO: required if discovery is disabled and the openid scope isn't requested
     userinfo_endpoint: Optional[StrictStr]
 
+    @validator("userinfo_endpoint", always=True)
+    def userinfo_endpoint_required_without_discovery_and_without_openid_scope(
+        cls, userinfo_endpoint: Optional[str], values: Mapping[str, object]
+    ) -> Optional[str]:
+        discovery_disabled = "discover" in values and not values["discover"]
+        openid_scope_not_requested = (
+            "scopes" in values and "openid" not in values["scopes"]
+        )
+        if (
+            discovery_disabled
+            and openid_scope_not_requested
+            and userinfo_endpoint is None
+        ):
+            raise ValueError(
+                "userinfo_requirement is required if discovery is disabled and"
+                "the 'openid' scope is not requested"
+            )
+        return userinfo_endpoint
+
     # URI where to fetch the JWKS. Required if discovery is disabled and the
     # "openid" scope is used.
-    # TODO: required if discovery is disabled and the openid scope IS requested
     jwks_uri: Optional[StrictStr]
 
+    @validator("jwks_uri", always=True)
+    def jwks_uri_required_without_discovery_but_with_openid_scope(
+        cls, jwks_uri: Optional[str], values: Mapping[str, object]
+    ) -> Optional[str]:
+        discovery_disabled = "discover" in values and not values["discover"]
+        openid_scope_requested = (
+            "scopes" in values and "openid" in values["scopes"]
+        )
+        if (
+            discovery_disabled
+            and openid_scope_requested
+            and jwks_uri is None
+        ):
+            raise ValueError(
+                "jwks_uri is required if discovery is disabled and"
+                "the 'openid' scope is not requested"
+            )
+        return jwks_uri
+
     # Whether to skip metadata verification
     skip_verification: StrictBool = False
 
diff --git a/tests/config/test_oidc2.py b/tests/config/test_oidc2.py
index 04b98909ee..53819580d0 100644
--- a/tests/config/test_oidc2.py
+++ b/tests/config/test_oidc2.py
@@ -292,3 +292,44 @@ class PydanticOIDCTestCase(TestCase):
         # If not specified, discovery is also on by default.
         del self.config["discover"]
         check_all_cases_pass()
+
+    def test_userinfo_endpoint(self) -> None:
+        """Example of a more fiddly validator"""
+        # This field is required if discovery is disabled and the openid scope
+        # not requested.
+        self.assertNotIn("userinfo_endpoint", self.config)
+        with self.assertRaises(ValidationError):
+            self.config["discover"] = False
+            self.config["scopes"] = ()
+            OIDCProviderModel.parse_obj(self.config)
+
+        # Still an error even if other scopes are provided
+        with self.assertRaises(ValidationError):
+            self.config["discover"] = False
+            self.config["scopes"] = ("potato", "tomato")
+            OIDCProviderModel.parse_obj(self.config)
+
+        # Passing an explicit None for userinfo_endpoint should also be an error.
+        with self.assertRaises(ValidationError):
+            self.config["discover"] = False
+            self.config["scopes"] = ()
+            self.config["userinfo_endpoint"] = None
+            OIDCProviderModel.parse_obj(self.config)
+
+        # No error if we enable discovery.
+        self.config["discover"] = True
+        self.config["scopes"] = ()
+        self.config["userinfo_endpoint"] = None
+        OIDCProviderModel.parse_obj(self.config)
+
+        # No error if we enable the openid scope.
+        self.config["discover"] = False
+        self.config["scopes"] = ("openid",)
+        self.config["userinfo_endpoint"] = None
+        OIDCProviderModel.parse_obj(self.config)
+
+        # No error if we don't specify scopes. (They default to `("openid", )`)
+        self.config["discover"] = False
+        del self.config["scopes"]
+        self.config["userinfo_endpoint"] = None
+        OIDCProviderModel.parse_obj(self.config)