From d8031605e2fe76d7ed8145088973625831266e38 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 2 Feb 2021 23:39:03 +0000
Subject: [PATCH] Fix URI escaping bug, and update test

---
 .../res/templates/sso_login_idp_picker.html   | 25 +++++++++----------
 tests/rest/client/v1/test_login.py            | 16 +++++++++---
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/synapse/res/templates/sso_login_idp_picker.html b/synapse/res/templates/sso_login_idp_picker.html
index 27f064506e48..53b82db84eff 100644
--- a/synapse/res/templates/sso_login_idp_picker.html
+++ b/synapse/res/templates/sso_login_idp_picker.html
@@ -43,19 +43,18 @@ <h1>Log in to {{ server_name }} </h1>
             <p>Choose an identity provider to log in</p>
         </header>
         <main>
-                <ul class="providers">
-                    {% for p in providers %}
-                    <li>
-                        <a href="pick_idp?idp={{ p.idp_id }}&redirectUrl={{ redirect_url }}">
-                            {% if p.idp_icon %}
-                            <img src="{{ p.idp_icon | mxc_to_http(32, 32) }}"/>
-                            {% endif %}
-                            <span>{{ p.idp_name }}</span>
-                        </a>
-                    </li>
-                    {% endfor %}
-                </ul>
-            </form>
+            <ul class="providers">
+                {% for p in providers %}
+                <li>
+                    <a href="pick_idp?idp={{ p.idp_id }}&redirectUrl={{ redirect_url | urlencode }}">
+                        {% if p.idp_icon %}
+                        <img src="{{ p.idp_icon | mxc_to_http(32, 32) }}"/>
+                        {% endif %}
+                        <span>{{ p.idp_name }}</span>
+                    </a>
+                </li>
+                {% endfor %}
+            </ul>
         </main>
         {% include "sso_footer.html" without context %}
     </body>
diff --git a/tests/rest/client/v1/test_login.py b/tests/rest/client/v1/test_login.py
index 66dfdaffbc9a..ceb4ad236690 100644
--- a/tests/rest/client/v1/test_login.py
+++ b/tests/rest/client/v1/test_login.py
@@ -15,7 +15,7 @@
 
 import time
 import urllib.parse
-from typing import Any, Dict, Union
+from typing import Any, Dict, List, Union
 from urllib.parse import urlencode
 
 from mock import Mock
@@ -493,13 +493,21 @@ def test_multi_sso_redirect(self):
         self.assertEqual(channel.code, 200, channel.result)
 
         # parse the form to check it has fields assumed elsewhere in this class
+        html = channel.result["body"].decode("utf-8")
         p = TestHtmlParser()
-        p.feed(channel.result["body"].decode("utf-8"))
+        p.feed(html)
         p.close()
 
-        self.assertCountEqual(p.radios["idp"], ["cas", "oidc", "oidc-idp1", "saml"])
+        # there should be a link for each href
+        returned_idps = []  # type: List[str]
+        for link in p.links:
+            path, query = link.split("?", 1)
+            self.assertEqual(path, "pick_idp")
+            params = urllib.parse.parse_qs(query)
+            self.assertEqual(params["redirectUrl"], [TEST_CLIENT_REDIRECT_URL])
+            returned_idps.append(params["idp"][0])
 
-        self.assertEqual(p.hiddens["redirectUrl"], TEST_CLIENT_REDIRECT_URL)
+        self.assertCountEqual(returned_idps, ["cas", "oidc", "oidc-idp1", "saml"])
 
     def test_multi_sso_redirect_to_cas(self):
         """If CAS is chosen, should redirect to the CAS server"""