Incorporate review

synapse/handlers/account_validity.py

@@ -430,7 +430,9 @@ class AccountValidityHandler:
             # Show the user in the directory again by setting them to active
             # We get the profile handler here so that we don't cause a cyclic dependency
             # at startup.
-            # FIXME: this doesn't work with the new account validity module stuff
+            # FIXME: this doesn't work with the new account validity module stuff, in
+            #  that this code is only run when no account validity module implements the
+            #  on_legacy_renew callback - which synapse-email-account-validity does.
             await self.hs.get_profile_handler().set_active(
                 [UserID.from_string(user_id)], True, True
             )

synapse/handlers/identity.py

@@ -202,17 +202,17 @@ class IdentityHandler(BaseHandler):
         if id_access_token is None:
             use_v2 = False
 
-        # if we have a rewrite rule set for the identity server,
-        # apply it now, but only for sending the request (not
-        # storing in the database).
-        id_server_url = self.rewrite_id_server_url(id_server, add_https=True)
-
-        if not valid_id_server_location(id_server_url):
+        if not valid_id_server_location(id_server):
             raise SynapseError(
                 400,
                 "id_server must be a valid hostname with optional port and path components",
             )
 
+        # if we have a rewrite rule set for the identity server,
+        # apply it now, but only for sending the request (not
+        # storing in the database).
+        id_server_url = self.rewrite_id_server_url(id_server, add_https=True)
+
         # Decide which API endpoint URLs to use
         headers = {}
         bind_data = {"sid": sid, "client_secret": client_secret, "mxid": mxid}

synapse/util/stringutils.py

@@ -147,12 +147,6 @@ def valid_id_server_location(id_server: str) -> bool:
     Returns:
         True if valid, False otherwise.
     """
-
-    # A previous call to rewrite_id_server_url might have added a protocol scheme to the
-    # server's address - we don't care about that so remove it just for these checks.
-    if id_server.startswith("https://"):
-        id_server = id_server[8:]
-
     components = id_server.split("/", 1)
 
     host = components[0]