From 2dd2e90e2b0b78f82d0e3372bacba9a84240302b Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 15 Dec 2020 12:39:56 +0000
Subject: [PATCH] Test `get_extra_attributes` fallback

despite the warnings saying "don't implement get_extra_attributes", we had
implemented it, so the tests weren't doing what we thought they were.
---
 tests/handlers/test_oidc.py   | 32 +++++++++++++++++++++-----------
 tests/rest/client/v1/utils.py |  2 +-
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index 464e569ac8..1794a169e0 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -19,7 +19,7 @@ from mock import ANY, Mock, patch
 
 import pymacaroons
 
-from synapse.handlers.oidc_handler import OidcError, OidcMappingProvider
+from synapse.handlers.oidc_handler import OidcError
 from synapse.handlers.sso import MappingException
 from synapse.types import UserID
 
@@ -55,11 +55,14 @@ COOKIE_NAME = b"oidc_session"
 COOKIE_PATH = "/_synapse/oidc"
 
 
-class TestMappingProvider(OidcMappingProvider):
+class TestMappingProvider:
     @staticmethod
     def parse_config(config):
         return
 
+    def __init__(self, config):
+        pass
+
     def get_remote_user_id(self, userinfo):
         return userinfo["sub"]
 
@@ -360,6 +363,13 @@ class OidcHandlerTestCase(HomeserverTestCase):
          - when the userinfo fetching fails
          - when the code exchange fails
         """
+
+        # ensure that we are correctly testing the fallback when "get_extra_attributes"
+        # is not implemented.
+        mapping_provider = self.handler._user_mapping_provider
+        with self.assertRaises(AttributeError):
+            _ = mapping_provider.get_extra_attributes
+
         token = {
             "type": "bearer",
             "id_token": "id_token",
@@ -396,7 +406,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
         self.get_success(self.handler.handle_oidc_callback(request))
 
         auth_handler.complete_sso_login.assert_called_once_with(
-            expected_user_id, request, client_redirect_url, {},
+            expected_user_id, request, client_redirect_url, None,
         )
         self.handler._exchange_code.assert_called_once_with(code)
         self.handler._parse_id_token.assert_called_once_with(token, nonce=nonce)
@@ -427,7 +437,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
         self.get_success(self.handler.handle_oidc_callback(request))
 
         auth_handler.complete_sso_login.assert_called_once_with(
-            expected_user_id, request, client_redirect_url, {},
+            expected_user_id, request, client_redirect_url, None,
         )
         self.handler._exchange_code.assert_called_once_with(code)
         self.handler._parse_id_token.assert_not_called()
@@ -616,7 +626,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
         }
         self._make_callback_with_userinfo(userinfo)
         auth_handler.complete_sso_login.assert_called_once_with(
-            "@test_user:test", ANY, ANY, {}
+            "@test_user:test", ANY, ANY, None,
         )
         auth_handler.complete_sso_login.reset_mock()
 
@@ -627,7 +637,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
         }
         self._make_callback_with_userinfo(userinfo)
         auth_handler.complete_sso_login.assert_called_once_with(
-            "@test_user_2:test", ANY, ANY, {}
+            "@test_user_2:test", ANY, ANY, None,
         )
         auth_handler.complete_sso_login.reset_mock()
 
@@ -664,14 +674,14 @@ class OidcHandlerTestCase(HomeserverTestCase):
         }
         self._make_callback_with_userinfo(userinfo)
         auth_handler.complete_sso_login.assert_called_once_with(
-            user.to_string(), ANY, ANY, {},
+            user.to_string(), ANY, ANY, None,
         )
         auth_handler.complete_sso_login.reset_mock()
 
         # Subsequent calls should map to the same mxid.
         self._make_callback_with_userinfo(userinfo)
         auth_handler.complete_sso_login.assert_called_once_with(
-            user.to_string(), ANY, ANY, {},
+            user.to_string(), ANY, ANY, None,
         )
         auth_handler.complete_sso_login.reset_mock()
 
@@ -686,7 +696,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
         }
         self._make_callback_with_userinfo(userinfo)
         auth_handler.complete_sso_login.assert_called_once_with(
-            user.to_string(), ANY, ANY, {},
+            user.to_string(), ANY, ANY, None,
         )
         auth_handler.complete_sso_login.reset_mock()
 
@@ -722,7 +732,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
 
         self._make_callback_with_userinfo(userinfo)
         auth_handler.complete_sso_login.assert_called_once_with(
-            "@TEST_USER_2:test", ANY, ANY, {},
+            "@TEST_USER_2:test", ANY, ANY, None,
         )
 
     def test_map_userinfo_to_invalid_localpart(self):
@@ -756,7 +766,7 @@ class OidcHandlerTestCase(HomeserverTestCase):
 
         # test_user is already taken, so test_user1 gets registered instead.
         auth_handler.complete_sso_login.assert_called_once_with(
-            "@test_user1:test", ANY, ANY, {},
+            "@test_user1:test", ANY, ANY, None,
         )
         auth_handler.complete_sso_login.reset_mock()
 
diff --git a/tests/rest/client/v1/utils.py b/tests/rest/client/v1/utils.py
index 5a18af8d34..1b31669489 100644
--- a/tests/rest/client/v1/utils.py
+++ b/tests/rest/client/v1/utils.py
@@ -445,7 +445,7 @@ class RestHelper:
         return channel.json_body
 
 
-# an 'oidc_config' suitable for login_with_oidc.
+# an 'oidc_config' suitable for login_via_oidc.
 TEST_OIDC_CONFIG = {
     "enabled": True,
     "discover": False,