From 6c67c5ae6b1b4e51583eb6e982746bcc9335c63a Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 23 Sep 2022 13:50:37 +0400 Subject: [PATCH 01/75] add support for handling avatar with SSO login --- synapse/handlers/oidc.py | 10 +++++++++ synapse/handlers/sso.py | 45 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index d7a82269006a..f13afb1d3fc8 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1116,6 +1116,7 @@ class UserAttributeDict(TypedDict): localpart: Optional[str] confirm_localpart: bool display_name: Optional[str] + picture: Optional[str] emails: List[str] @@ -1204,6 +1205,7 @@ def jinja_finalize(thing: Any) -> Any: @attr.s(slots=True, frozen=True, auto_attribs=True) class JinjaOidcMappingConfig: subject_claim: str + picture_claim: str localpart_template: Optional[Template] display_name_template: Optional[Template] email_template: Optional[Template] @@ -1223,6 +1225,7 @@ def __init__(self, config: JinjaOidcMappingConfig): @staticmethod def parse_config(config: dict) -> JinjaOidcMappingConfig: subject_claim = config.get("subject_claim", "sub") + picture_claim = config.get("picture_claim", "picture") def parse_template_config(option_name: str) -> Optional[Template]: if option_name not in config: @@ -1256,6 +1259,7 @@ def parse_template_config(option_name: str) -> Optional[Template]: return JinjaOidcMappingConfig( subject_claim=subject_claim, + picture_claim=picture_claim, localpart_template=localpart_template, display_name_template=display_name_template, email_template=email_template, @@ -1295,10 +1299,16 @@ def render_template_field(template: Optional[Template]) -> Optional[str]: if email: emails.append(email) + if "picture" in userinfo: + picture = userinfo["picture"] + else: + picture = "" + return UserAttributeDict( localpart=localpart, display_name=display_name, emails=emails, + picture=picture, confirm_localpart=self._config.confirm_localpart, ) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 6bc1cbd78790..5fcf93fff1af 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import abc +import io import logging from typing import ( TYPE_CHECKING, @@ -29,6 +30,7 @@ from urllib.parse import urlencode import attr +import requests from typing_extensions import NoReturn, Protocol from twisted.web.iweb import IRequest @@ -137,6 +139,7 @@ class UserAttributes: localpart: Optional[str] confirm_localpart: bool = False display_name: Optional[str] = None + picture: str = "" emails: Collection[str] = attr.Factory(list) @@ -191,6 +194,7 @@ def __init__(self, hs: "HomeServer"): self._error_template = hs.config.sso.sso_error_template self._bad_user_template = hs.config.sso.sso_auth_bad_user_template self._profile_handler = hs.get_profile_handler() + self.media_repo = hs.get_media_repository() # The following template is shown after a successful user interactive # authentication session. It tells the user they can close the window. @@ -693,8 +697,49 @@ async def _register_mapped_user( await self._store.record_user_external_id( auth_provider_id, remote_user_id, registered_user_id ) + + # Set avatar, if available + if attributes.picture: + await self.set_avatar(registered_user_id, attributes.picture) + return registered_user_id + async def set_avatar(self, user_id: str, picture_https_url: str) -> None: + try: + uid = UserID.from_string(user_id) + + # download picture + http_response = requests.get(picture_https_url) + if http_response.status_code != 200: + http_response.raise_for_status() + + content_type = http_response.headers["Content-Type"] + content_length = int(http_response.headers["Content-Length"]) + + # convert image into BytesIO + b = io.BytesIO(http_response.content) + + # store it in media repository + avatar_mxc_url = await self.media_repo.create_content( + content_type, + None, + b, + content_length, + uid, + ) + + # save it as user avatar + await self._profile_handler.set_avatar_url( + uid, + create_requester(uid), + str(avatar_mxc_url), + ) + + logger.info("successfully saved the user avatar via SSO: %s", user_id) + except Exception as e: + logger.info("failed to save the user avatar via SSO: %s", user_id) + logger.exception(e) + async def complete_sso_ui_auth_request( self, auth_provider_id: str, From 0fd47d12099519d6d1bf005d4a99123ad3d115fc Mon Sep 17 00:00:00 2001 From: Ashfame Date: Tue, 27 Sep 2022 12:11:15 +0400 Subject: [PATCH 02/75] add changelog file --- changelog.d/13917.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13917.feature diff --git a/changelog.d/13917.feature b/changelog.d/13917.feature new file mode 100644 index 000000000000..a6da8e2503f7 --- /dev/null +++ b/changelog.d/13917.feature @@ -0,0 +1 @@ +Adds support for handling avatar in SSO login. Contributed by @ashfame From 546ea47c86d38c92f889b667a40b988635a6e30a Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Wed, 28 Sep 2022 14:56:32 +0400 Subject: [PATCH 03/75] change log message Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/handlers/sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 5fcf93fff1af..78e00e2a7a89 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -735,7 +735,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: str(avatar_mxc_url), ) - logger.info("successfully saved the user avatar via SSO: %s", user_id) + logger.info("successfully saved the user avatar") except Exception as e: logger.info("failed to save the user avatar via SSO: %s", user_id) logger.exception(e) From 4d7494351a76a74dd3032f849151e164ff7e9963 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Wed, 28 Sep 2022 14:57:44 +0400 Subject: [PATCH 04/75] remove separate log call, just pass message while logging exception Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/handlers/sso.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 78e00e2a7a89..3c59edb51986 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -736,9 +736,8 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: ) logger.info("successfully saved the user avatar") - except Exception as e: - logger.info("failed to save the user avatar via SSO: %s", user_id) - logger.exception(e) + except Exception: + logger.exception("failed to save the user avatar") async def complete_sso_ui_auth_request( self, From d7285f723fb121d8cd711f2736708e9dc948d275 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Tue, 27 Sep 2022 15:18:40 +0400 Subject: [PATCH 05/75] ensure avatar image size is within the max_avatar_size allowed as per config --- synapse/handlers/sso.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 3c59edb51986..dd0483f5e276 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -708,14 +708,23 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: try: uid = UserID.from_string(user_id) - # download picture - http_response = requests.get(picture_https_url) + # ensure picture size respects max_avatar_size defined in config + http_response = requests.options(picture_https_url) if http_response.status_code != 200: http_response.raise_for_status() content_type = http_response.headers["Content-Type"] content_length = int(http_response.headers["Content-Length"]) + if self._profile_handler.max_avatar_size is not None: + if content_length > self._profile_handler.max_avatar_size: + raise Exception("sso avatar too big in size") + + # download picture + http_response = requests.get(picture_https_url) + if http_response.status_code != 200: + http_response.raise_for_status() + # convert image into BytesIO b = io.BytesIO(http_response.content) From f1767d2e3cdf894276eb5018edff1cf58b67aa8e Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 28 Sep 2022 11:46:23 +0400 Subject: [PATCH 06/75] added missing period in changelog file --- changelog.d/13917.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/13917.feature b/changelog.d/13917.feature index a6da8e2503f7..4eb942ab3850 100644 --- a/changelog.d/13917.feature +++ b/changelog.d/13917.feature @@ -1 +1 @@ -Adds support for handling avatar in SSO login. Contributed by @ashfame +Adds support for handling avatar in SSO login. Contributed by @ashfame. From 670237ccb082330d3767cf1e99068b0413edeef0 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 28 Sep 2022 11:53:38 +0400 Subject: [PATCH 07/75] use synapse's instantiated http client rather than requests library for http requests --- synapse/handlers/sso.py | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index dd0483f5e276..6fb44923d3fa 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -30,9 +30,9 @@ from urllib.parse import urlencode import attr -import requests from typing_extensions import NoReturn, Protocol +from twisted.web.client import readBody from twisted.web.iweb import IRequest from twisted.web.server import Request @@ -44,6 +44,7 @@ from synapse.http import get_request_user_agent from synapse.http.server import respond_with_html, respond_with_redirect from synapse.http.site import SynapseRequest +from synapse.logging.context import make_deferred_yieldable from synapse.types import ( JsonDict, UserID, @@ -195,6 +196,7 @@ def __init__(self, hs: "HomeServer"): self._bad_user_template = hs.config.sso.sso_auth_bad_user_template self._profile_handler = hs.get_profile_handler() self.media_repo = hs.get_media_repository() + self._http_client = hs.get_proxied_http_client() # The following template is shown after a successful user interactive # authentication session. It tells the user they can close the window. @@ -709,30 +711,34 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: uid = UserID.from_string(user_id) # ensure picture size respects max_avatar_size defined in config - http_response = requests.options(picture_https_url) - if http_response.status_code != 200: - http_response.raise_for_status() + response = await self._http_client.request("OPTIONS", picture_https_url) + if response.code != 200: + raise Exception("error sending OPTIONS request to get image size") - content_type = http_response.headers["Content-Type"] - content_length = int(http_response.headers["Content-Length"]) + content_length = response.length + headers = response.headers.getAllRawHeaders() + for header in headers: + if header[0].decode("utf-8") == "Content-Type": + content_type = header[1][0].decode("utf-8") + break if self._profile_handler.max_avatar_size is not None: if content_length > self._profile_handler.max_avatar_size: raise Exception("sso avatar too big in size") # download picture - http_response = requests.get(picture_https_url) - if http_response.status_code != 200: - http_response.raise_for_status() - - # convert image into BytesIO - b = io.BytesIO(http_response.content) + response = await self._http_client.request("GET", picture_https_url) + if response.code != 200: + raise Exception( + "error sending GET request to provided sso avatar image" + ) + image = await make_deferred_yieldable(readBody(response)) # store it in media repository avatar_mxc_url = await self.media_repo.create_content( content_type, None, - b, + io.BytesIO(image), # convert image into BytesIO content_length, uid, ) From 879f8ad7007dcac6f921b38f4855077d34b3450f Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 28 Sep 2022 12:17:39 +0400 Subject: [PATCH 08/75] ensure content_type is checked against allowed mime types --- synapse/handlers/sso.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 6fb44923d3fa..f1738e30333a 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -710,22 +710,31 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: try: uid = UserID.from_string(user_id) - # ensure picture size respects max_avatar_size defined in config + # OPTIONS request to find image size & mime type before download response = await self._http_client.request("OPTIONS", picture_https_url) if response.code != 200: raise Exception("error sending OPTIONS request to get image size") - content_length = response.length + content_type = None headers = response.headers.getAllRawHeaders() for header in headers: if header[0].decode("utf-8") == "Content-Type": content_type = header[1][0].decode("utf-8") break + content_length = response.length + # ensure picture size respects max_avatar_size defined in config if self._profile_handler.max_avatar_size is not None: if content_length > self._profile_handler.max_avatar_size: raise Exception("sso avatar too big in size") + # ensure picture is of allowed mime type + if content_type is None and ( + self._profile_handler.allowed_avatar_mimetypes + and content_type not in self._profile_handler.allowed_avatar_mimetypes + ): + raise Exception("mime type not allowed for sso avatar") + # download picture response = await self._http_client.request("GET", picture_https_url) if response.code != 200: @@ -736,7 +745,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: # store it in media repository avatar_mxc_url = await self.media_repo.create_content( - content_type, + str(content_type), None, io.BytesIO(image), # convert image into BytesIO content_length, From c6de28d05ed0b7b6d5d003a0e1ae26a10cf3729c Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 28 Sep 2022 12:53:33 +0400 Subject: [PATCH 09/75] oops: HEAD request is to be used and not OPTIONS request for getting headers --- synapse/handlers/sso.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index f1738e30333a..cddcccfae219 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -710,10 +710,9 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: try: uid = UserID.from_string(user_id) - # OPTIONS request to find image size & mime type before download - response = await self._http_client.request("OPTIONS", picture_https_url) + # HEAD request to find image size & mime type before download + response = await self._http_client.request("HEAD", picture_https_url) if response.code != 200: - raise Exception("error sending OPTIONS request to get image size") content_type = None headers = response.headers.getAllRawHeaders() @@ -721,6 +720,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: if header[0].decode("utf-8") == "Content-Type": content_type = header[1][0].decode("utf-8") break + raise Exception("error sending HEAD request to get image size") content_length = response.length # ensure picture size respects max_avatar_size defined in config From 93662ee2de053004e05ce0bc7d5bfb4cb669836b Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 28 Sep 2022 12:58:42 +0400 Subject: [PATCH 10/75] better way to obtain headers from response --- synapse/handlers/sso.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index cddcccfae219..ca449da81a47 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -713,15 +713,14 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: # HEAD request to find image size & mime type before download response = await self._http_client.request("HEAD", picture_https_url) if response.code != 200: - - content_type = None - headers = response.headers.getAllRawHeaders() - for header in headers: - if header[0].decode("utf-8") == "Content-Type": - content_type = header[1][0].decode("utf-8") - break raise Exception("error sending HEAD request to get image size") - content_length = response.length + + content_length = int( + response.headers.getRawHeaders(b"Content-Length")[0].decode("utf-8") + ) + content_type = response.headers.getRawHeaders(b"Content-Type")[0].decode( + "utf-8" + ) # ensure picture size respects max_avatar_size defined in config if self._profile_handler.max_avatar_size is not None: @@ -729,7 +728,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: raise Exception("sso avatar too big in size") # ensure picture is of allowed mime type - if content_type is None and ( + if ( self._profile_handler.allowed_avatar_mimetypes and content_type not in self._profile_handler.allowed_avatar_mimetypes ): From 586170db9626f9c8483841e1a9dad8eead4c8e07 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 28 Sep 2022 15:22:30 +0400 Subject: [PATCH 11/75] define picture attribute as Optional[str] with default value as None to be consistent with other attributes --- synapse/handlers/sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index ca449da81a47..1370b9c5c78a 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -140,7 +140,7 @@ class UserAttributes: localpart: Optional[str] confirm_localpart: bool = False display_name: Optional[str] = None - picture: str = "" + picture: Optional[str] = None emails: Collection[str] = attr.Factory(list) From 25d25949c48fbc29d6c5c11abb83c56c6f3fad76 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Mon, 31 Oct 2022 20:16:12 +0400 Subject: [PATCH 12/75] follow standard pattern for getting value out of userinfo with a default provided Co-authored-by: David Robertson --- synapse/handlers/oidc.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 19c4f1490649..b9373e97f044 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1306,10 +1306,7 @@ def render_template_field(template: Optional[Template]) -> Optional[str]: if email: emails.append(email) - if "picture" in userinfo: - picture = userinfo["picture"] - else: - picture = "" + picture = userinfo.get("picture", "") return UserAttributeDict( localpart=localpart, From b343a43db01896f1eb3ed242926ddb1ecaa9e99c Mon Sep 17 00:00:00 2001 From: Ashfame Date: Mon, 31 Oct 2022 20:55:54 +0400 Subject: [PATCH 13/75] add picture_claim explanation to config docs --- docs/usage/configuration/config_documentation.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index d81eda52c156..a8e103706a2a 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -2990,6 +2990,10 @@ Options for each entry include: for the user. Defaults to 'sub', which OpenID Connect compliant providers should provide. + * picture_claim: name of the claim containing an url for user's profile picture. + Defaults to 'picture', which OpenID Connect compliant providers should provide + and has to refer to a direct image file such as PNG, JPEG, or GIF image file. + * `localpart_template`: Jinja2 template for the localpart of the MXID. If this is not set, the user will be prompted to choose their own username (see the documentation for the `sso_auth_account_details.html` From 07cbe39b07068161d4807b6005d243e2714242db Mon Sep 17 00:00:00 2001 From: Ashfame Date: Tue, 1 Nov 2022 13:06:10 +0400 Subject: [PATCH 14/75] prefix media_repo attribute with underscore to indicate private usage within the class only --- synapse/handlers/sso.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index f1db9b7b210d..664715663c28 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -198,7 +198,7 @@ def __init__(self, hs: "HomeServer"): self._error_template = hs.config.sso.sso_error_template self._bad_user_template = hs.config.sso.sso_auth_bad_user_template self._profile_handler = hs.get_profile_handler() - self.media_repo = hs.get_media_repository() + self._media_repo = hs.get_media_repository() self._http_client = hs.get_proxied_http_client() # The following template is shown after a successful user interactive @@ -751,7 +751,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: image = await make_deferred_yieldable(readBody(response)) # store it in media repository - avatar_mxc_url = await self.media_repo.create_content( + avatar_mxc_url = await self._media_repo.create_content( str(content_type), None, io.BytesIO(image), # convert image into BytesIO From f4c1d39228a92f65a30620ce1aa25adb6718b075 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Tue, 1 Nov 2022 15:44:19 +0400 Subject: [PATCH 15/75] declare docstring for set_avatar() --- synapse/handlers/sso.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 664715663c28..3609ec73ad72 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -715,6 +715,16 @@ async def _register_mapped_user( return registered_user_id async def set_avatar(self, user_id: str, picture_https_url: str) -> None: + """Set avatar of the user. + + This downloads the image file from the url provided, store that in + the media repository and then sets the avatar on user's profile. + + Args: + user_id: matrix user ID in the form @localpart:domain as a string. + + picture_https_url: HTTPS url for the picture image file. + """ try: uid = UserID.from_string(user_id) From 1594a3f5b0d3811cd11c0ee959d0875648904257 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Tue, 1 Nov 2022 17:35:58 +0400 Subject: [PATCH 16/75] add additional details to exceptions --- synapse/handlers/sso.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 3609ec73ad72..c0153e4a6646 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -731,7 +731,11 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: # HEAD request to find image size & mime type before download response = await self._http_client.request("HEAD", picture_https_url) if response.code != 200: - raise Exception("error sending HEAD request to get image size") + raise Exception( + "error sending HEAD request to get image size, status code = {}".format( + response.code + ) + ) content_length = int( response.headers.getRawHeaders(b"Content-Length")[0].decode("utf-8") @@ -743,20 +747,28 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: # ensure picture size respects max_avatar_size defined in config if self._profile_handler.max_avatar_size is not None: if content_length > self._profile_handler.max_avatar_size: - raise Exception("sso avatar too big in size") + raise Exception( + "sso avatar too big in size, size = {} but max size allowed = {}".format( + content_length, self._profile_handler.max_avatar_size + ) + ) # ensure picture is of allowed mime type if ( self._profile_handler.allowed_avatar_mimetypes and content_type not in self._profile_handler.allowed_avatar_mimetypes ): - raise Exception("mime type not allowed for sso avatar") + raise Exception( + "mime type {} not allowed for sso avatar".format(content_type) + ) # download picture response = await self._http_client.request("GET", picture_https_url) if response.code != 200: raise Exception( - "error sending GET request to provided sso avatar image" + "error sending GET request to provided sso avatar image, status code = {}".format( + response.code + ) ) image = await make_deferred_yieldable(readBody(response)) From 3a18057992c5cb07f15cfafafde3887e123a4561 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Tue, 1 Nov 2022 17:37:32 +0400 Subject: [PATCH 17/75] log as warning with any raised Exception inside set_avatar() and not as exception --- synapse/handlers/sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index c0153e4a6646..096d186a7f50 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -790,7 +790,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: logger.info("successfully saved the user avatar") except Exception: - logger.exception("failed to save the user avatar") + logger.warning("failed to save the user avatar") async def complete_sso_ui_auth_request( self, From 06bbd4456416f87a71fb72838f8a54f85d232a8f Mon Sep 17 00:00:00 2001 From: Ashfame Date: Tue, 1 Nov 2022 18:14:48 +0400 Subject: [PATCH 18/75] handle reading http headers better --- synapse/handlers/sso.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 096d186a7f50..ada0c74df61f 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -737,12 +737,15 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: ) ) - content_length = int( - response.headers.getRawHeaders(b"Content-Length")[0].decode("utf-8") - ) - content_type = response.headers.getRawHeaders(b"Content-Type")[0].decode( - "utf-8" - ) + # ensure http headers are present + if ( + response.headers.getRawHeaders("Content-Length") is None + or response.headers.getRawHeaders("Content-Type") is None + ): + raise Exception("can not parse headers of sso avatar image file") + + content_length = int(response.headers.getRawHeaders("Content-Length")[0]) + content_type = response.headers.getRawHeaders("Content-Type")[0] # ensure picture size respects max_avatar_size defined in config if self._profile_handler.max_avatar_size is not None: From 93ffe6a82d7f2bbe1f1bd2e9752d7d7f8f42d1a2 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Tue, 1 Nov 2022 18:25:54 +0400 Subject: [PATCH 19/75] improve exception messages --- synapse/handlers/sso.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index ada0c74df61f..c7fcc4d16814 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -732,7 +732,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: response = await self._http_client.request("HEAD", picture_https_url) if response.code != 200: raise Exception( - "error sending HEAD request to get image size, status code = {}".format( + "HEAD request to check sso avatar image headers returned {}".format( response.code ) ) @@ -742,7 +742,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: response.headers.getRawHeaders("Content-Length") is None or response.headers.getRawHeaders("Content-Type") is None ): - raise Exception("can not parse headers of sso avatar image file") + raise Exception("HTTP headers missing for sso avatar image") content_length = int(response.headers.getRawHeaders("Content-Length")[0]) content_type = response.headers.getRawHeaders("Content-Type")[0] @@ -751,7 +751,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: if self._profile_handler.max_avatar_size is not None: if content_length > self._profile_handler.max_avatar_size: raise Exception( - "sso avatar too big in size, size = {} but max size allowed = {}".format( + "SSO avatar image {} should be less than {}".format( content_length, self._profile_handler.max_avatar_size ) ) @@ -762,14 +762,16 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: and content_type not in self._profile_handler.allowed_avatar_mimetypes ): raise Exception( - "mime type {} not allowed for sso avatar".format(content_type) + "SSO avatar image does not allow mime type of {}".format( + content_type + ) ) # download picture response = await self._http_client.request("GET", picture_https_url) if response.code != 200: raise Exception( - "error sending GET request to provided sso avatar image, status code = {}".format( + "GET request to download sso avatar image returned {}".format( response.code ) ) From e432b537a54b8925d24a7a1ecd6cb4a652192b8b Mon Sep 17 00:00:00 2001 From: Ashfame Date: Tue, 8 Nov 2022 22:08:20 +0400 Subject: [PATCH 20/75] enforce max size limit while downloading the image file --- synapse/handlers/sso.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index c7fcc4d16814..855ecb7b2103 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -768,20 +768,22 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: ) # download picture - response = await self._http_client.request("GET", picture_https_url) - if response.code != 200: + picture = io.BytesIO() + response = await self._http_client.get_file( + picture_https_url, picture, self._profile_handler.max_avatar_size + ) + if response[3] != 200: raise Exception( "GET request to download sso avatar image returned {}".format( response.code ) ) - image = await make_deferred_yieldable(readBody(response)) # store it in media repository avatar_mxc_url = await self._media_repo.create_content( str(content_type), None, - io.BytesIO(image), # convert image into BytesIO + picture, content_length, uid, ) From 669c7b04493f6f76cd054461ac31153b73814418 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Tue, 8 Nov 2022 22:09:14 +0400 Subject: [PATCH 21/75] update avatar with every subsequent login too --- synapse/handlers/sso.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 855ecb7b2103..5633b8ea99d6 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -499,6 +499,8 @@ async def complete_sso_login_request( await self._profile_handler.set_displayname( user_id_obj, requester, attributes.display_name, True ) + if attributes.picture: + await self.set_avatar(user_id, attributes.picture) await self._auth_handler.complete_sso_login( user_id, From f5379edcfab042fcb70dd8d7d5b6d9ce285fc992 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 9 Nov 2022 13:20:01 +0400 Subject: [PATCH 22/75] add unit tests --- tests/handlers/test_sso.py | 127 +++++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 tests/handlers/test_sso.py diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py new file mode 100644 index 000000000000..dda8ec4fca98 --- /dev/null +++ b/tests/handlers/test_sso.py @@ -0,0 +1,127 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from tests import unittest +from unittest.mock import Mock +from twisted.test.proto_helpers import MemoryReactor +from synapse.util import Clock +from synapse.server import HomeServer +from tests.test_utils import SMALL_PNG +from tests.test_utils import FakeResponse, simple_async_mock +from twisted.web.http_headers import Headers +from typing import BinaryIO, Optional, Callable, Union, Mapping, Tuple, Dict, List +from synapse.logging.context import make_deferred_yieldable +from twisted.web.client import readBody + +RawHeaders = Union[Mapping[str, "RawHeaderValue"], Mapping[bytes, "RawHeaderValue"]] + + +class TestSSOHandler(unittest.HomeserverTestCase): + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: + self.http_client = Mock(spec=["request", "get_file"]) + self.http_client.request.side_effect = mock_request + self.http_client.get_file.side_effect = mock_get_file + self.http_client.user_agent = b"Synapse Test" + hs = self.setup_test_homeserver(proxied_http_client=self.http_client) + return hs + + async def test_set_avatar(self): + """Tests successfully setting the avatar of a newly created user""" + handler = self.hs.get_sso_handler() + + # TODO: Create a new user to set avatar for + reg_handler = self.hs.get_registration_handler() + user_id = await reg_handler.register_user(approved=True) + # user_id = "@sso-user:test" + + with self.assertLogs() as cm: + await handler.set_avatar(user_id, "http://my.server/me.png") + self.assertEqual( + cm.output, ["INFO:synapse.handlers.sso:successfully saved the user avatar"] + ) + + # TODO: ensure avatar returned via user's profile is SMALL_PNG + profile_handler = self.hs.get_profile_handler() + profile = await profile_handler.get_profile(user_id) + # profile["avatar_url"] + + @unittest.override_config({"max_avatar_size": 99}) + async def test_set_avatar_too_big_image(self): + """Tests saving of avatar failed when image size is too big""" + handler = self.hs.get_sso_handler() + + # any random user works since image check is supposed to fail + user_id = "@sso-user:test" + + with self.assertLogs() as cm: + await handler.set_avatar(user_id, "http://my.server/big.png") + self.assertEqual( + cm.output, ["WARNING:synapse.handlers.sso:failed to save the user avatar"] + ) + + @unittest.override_config({"allowed_avatar_mimetypes": ["image/jpeg"]}) + async def test_set_avatar_incorrect_mime_type(self): + """Tests saving of avatar failed when not allowed mimetype of image was used""" + handler = self.hs.get_sso_handler() + + # any random user works since image check is supposed to fail + user_id = "@sso-user:test" + + with self.assertLogs() as cm: + await handler.set_avatar(user_id, "http://my.server/me.png") + self.assertEqual( + cm.output, ["WARNING:synapse.handlers.sso:failed to save the user avatar"] + ) + + +async def mock_request(method: str, url: str): + # for the purpose of test returning GET request body for HEAD request is fine + if url == "http://my.server/me.png": + if method == "HEAD": + return FakeResponse( + code=200, + headers=Headers( + {"Content-Type": ["image/png"], "Content-Length": ["67"]} + ), + ) + elif method == "GET": + return FakeResponse( + code=200, + headers=Headers( + {"Content-Type": ["image/png"], "Content-Length": ["67"]} + ), + body=SMALL_PNG, + ) + else: + return simple_async_mock(return_value=FakeResponse(code=400)) + elif url == "http://my.server/big.png": + if method == "HEAD": + return FakeResponse( + code=200, + headers=Headers( + {"Content-Type": ["image/png"], "Content-Length": ["999"]} + ), + ) + + return simple_async_mock(return_value=FakeResponse(code=404)) + + +async def mock_get_file( + url: str, + output_stream: BinaryIO, + max_size: Optional[int] = None, + headers: Optional[RawHeaders] = None, + is_allowed_content_type: Optional[Callable[[str], bool]] = None, +) -> Tuple[int, Dict[bytes, List[bytes]], str, int]: + response = await mock_request("GET", url) + output_stream = await make_deferred_yieldable(readBody(response)) + + return (67, [], "", 200) From b51b4f834a828839d797e4c7ae834449d2bea698 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 9 Nov 2022 13:37:17 +0400 Subject: [PATCH 23/75] declare new variable instead of reusing as type is different --- synapse/handlers/sso.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 2cd7bdcbbd36..29dda4791f80 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -772,13 +772,13 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: # download picture picture = io.BytesIO() - response = await self._http_client.get_file( + download_response = await self._http_client.get_file( picture_https_url, picture, self._profile_handler.max_avatar_size ) - if response[3] != 200: + if download_response[3] != 200: raise Exception( "GET request to download sso avatar image returned {}".format( - response.code + download_response[3] ) ) From adf0640600fecbf7d44a7520649a5f641ff232f5 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 9 Nov 2022 13:43:26 +0400 Subject: [PATCH 24/75] remove unused imports --- synapse/handlers/sso.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 29dda4791f80..59c663ffaa42 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -32,7 +32,6 @@ import attr from typing_extensions import NoReturn, Protocol -from twisted.web.client import readBody from twisted.web.iweb import IRequest from twisted.web.server import Request @@ -44,7 +43,6 @@ from synapse.http import get_request_user_agent from synapse.http.server import respond_with_html, respond_with_redirect from synapse.http.site import SynapseRequest -from synapse.logging.context import make_deferred_yieldable from synapse.types import ( JsonDict, UserID, From 225be627acd5c09b3ffc6ba0921ced5b7b6bab33 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 9 Nov 2022 20:19:46 +0400 Subject: [PATCH 25/75] fix type issues reported by linter --- tests/handlers/test_sso.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index dda8ec4fca98..f135ea591f6e 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -9,6 +9,9 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import io + +from synapse.http.client import RawHeaders from tests import unittest from unittest.mock import Mock from twisted.test.proto_helpers import MemoryReactor @@ -17,12 +20,10 @@ from tests.test_utils import SMALL_PNG from tests.test_utils import FakeResponse, simple_async_mock from twisted.web.http_headers import Headers -from typing import BinaryIO, Optional, Callable, Union, Mapping, Tuple, Dict, List +from typing import BinaryIO, Optional, Callable, Tuple, Dict, List from synapse.logging.context import make_deferred_yieldable from twisted.web.client import readBody -RawHeaders = Union[Mapping[str, "RawHeaderValue"], Mapping[bytes, "RawHeaderValue"]] - class TestSSOHandler(unittest.HomeserverTestCase): def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: @@ -122,6 +123,10 @@ async def mock_get_file( is_allowed_content_type: Optional[Callable[[str], bool]] = None, ) -> Tuple[int, Dict[bytes, List[bytes]], str, int]: response = await mock_request("GET", url) - output_stream = await make_deferred_yieldable(readBody(response)) + body = await make_deferred_yieldable(readBody(response)) + output_stream = io.BytesIO(body) + + resp_headers = dict() + resp_headers[b"Content-Type"] = [b"image/png"] - return (67, [], "", 200) + return (67, resp_headers, "", 200) From dfd26f07d410adbcd3d9942299bb329a91fbe12c Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 10 Nov 2022 00:40:25 +0400 Subject: [PATCH 26/75] reorder imports --- tests/handlers/test_sso.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index f135ea591f6e..c4b6f0ea922b 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -10,19 +10,20 @@ # See the License for the specific language governing permissions and # limitations under the License. import io - -from synapse.http.client import RawHeaders -from tests import unittest +from typing import BinaryIO, Callable, Dict, List, Optional, Tuple from unittest.mock import Mock + from twisted.test.proto_helpers import MemoryReactor -from synapse.util import Clock -from synapse.server import HomeServer -from tests.test_utils import SMALL_PNG -from tests.test_utils import FakeResponse, simple_async_mock +from twisted.web.client import readBody from twisted.web.http_headers import Headers -from typing import BinaryIO, Optional, Callable, Tuple, Dict, List + +from synapse.http.client import RawHeaders from synapse.logging.context import make_deferred_yieldable -from twisted.web.client import readBody +from synapse.server import HomeServer +from synapse.util import Clock + +from tests import unittest +from tests.test_utils import SMALL_PNG, FakeResponse, simple_async_mock class TestSSOHandler(unittest.HomeserverTestCase): From 11ff1b4ab9b31178a971da94c92e657db6785518 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 10 Nov 2022 11:38:43 +0400 Subject: [PATCH 27/75] fix semantic checks - rewrite unnecessary dict call as a literal - comment out unused variable (need to run unit tests on CI, timing out locally) - --- tests/handlers/test_sso.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index c4b6f0ea922b..a550361da73f 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -51,8 +51,8 @@ async def test_set_avatar(self): ) # TODO: ensure avatar returned via user's profile is SMALL_PNG - profile_handler = self.hs.get_profile_handler() - profile = await profile_handler.get_profile(user_id) + # profile_handler = self.hs.get_profile_handler() + # profile = await profile_handler.get_profile(user_id) # profile["avatar_url"] @unittest.override_config({"max_avatar_size": 99}) @@ -127,7 +127,4 @@ async def mock_get_file( body = await make_deferred_yieldable(readBody(response)) output_stream = io.BytesIO(body) - resp_headers = dict() - resp_headers[b"Content-Type"] = [b"image/png"] - - return (67, resp_headers, "", 200) + return 67, {b"Content-Type": [b"image/png"]}, "", 200 From 1592d432d1985d8e2d93a58e8c61c709e58b100e Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 10 Nov 2022 12:17:16 +0400 Subject: [PATCH 28/75] stream output correctly in test --- tests/handlers/test_sso.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index a550361da73f..e8a04221ff8c 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -9,16 +9,13 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -import io from typing import BinaryIO, Callable, Dict, List, Optional, Tuple from unittest.mock import Mock from twisted.test.proto_helpers import MemoryReactor -from twisted.web.client import readBody from twisted.web.http_headers import Headers -from synapse.http.client import RawHeaders -from synapse.logging.context import make_deferred_yieldable +from synapse.http.client import RawHeaders, read_body_with_max_size from synapse.server import HomeServer from synapse.util import Clock @@ -124,7 +121,6 @@ async def mock_get_file( is_allowed_content_type: Optional[Callable[[str], bool]] = None, ) -> Tuple[int, Dict[bytes, List[bytes]], str, int]: response = await mock_request("GET", url) - body = await make_deferred_yieldable(readBody(response)) - output_stream = io.BytesIO(body) + read_body_with_max_size(response, output_stream, max_size) return 67, {b"Content-Type": [b"image/png"]}, "", 200 From c15e87367d7ad54fafc88485823c5f0436e2aa26 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 10 Nov 2022 14:07:22 +0400 Subject: [PATCH 29/75] define return types on functions and define disallow_untyped_defs as True on test file --- mypy.ini | 2 ++ tests/handlers/test_sso.py | 9 +++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/mypy.ini b/mypy.ini index 8f1141a23905..2e8cc134a243 100644 --- a/mypy.ini +++ b/mypy.ini @@ -132,6 +132,8 @@ disallow_untyped_defs = True [mypy-tests.utils] disallow_untyped_defs = True +[mypy-tests.handlers.test_sso] +disallow_untyped_defs = True ;; Dependencies without annotations ;; Before ignoring a module, check to see if type stubs are available. diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index e8a04221ff8c..0d48b3bbc62f 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -14,6 +14,7 @@ from twisted.test.proto_helpers import MemoryReactor from twisted.web.http_headers import Headers +from twisted.web.iweb import IResponse from synapse.http.client import RawHeaders, read_body_with_max_size from synapse.server import HomeServer @@ -32,7 +33,7 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: hs = self.setup_test_homeserver(proxied_http_client=self.http_client) return hs - async def test_set_avatar(self): + async def test_set_avatar(self) -> None: """Tests successfully setting the avatar of a newly created user""" handler = self.hs.get_sso_handler() @@ -53,7 +54,7 @@ async def test_set_avatar(self): # profile["avatar_url"] @unittest.override_config({"max_avatar_size": 99}) - async def test_set_avatar_too_big_image(self): + async def test_set_avatar_too_big_image(self) -> None: """Tests saving of avatar failed when image size is too big""" handler = self.hs.get_sso_handler() @@ -67,7 +68,7 @@ async def test_set_avatar_too_big_image(self): ) @unittest.override_config({"allowed_avatar_mimetypes": ["image/jpeg"]}) - async def test_set_avatar_incorrect_mime_type(self): + async def test_set_avatar_incorrect_mime_type(self) -> None: """Tests saving of avatar failed when not allowed mimetype of image was used""" handler = self.hs.get_sso_handler() @@ -81,7 +82,7 @@ async def test_set_avatar_incorrect_mime_type(self): ) -async def mock_request(method: str, url: str): +async def mock_request(method: str, url: str) -> IResponse: # for the purpose of test returning GET request body for HEAD request is fine if url == "http://my.server/me.png": if method == "HEAD": From c0861f8828ecd6e78ae7299a97fb7dcef9172368 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 10 Nov 2022 14:17:16 +0400 Subject: [PATCH 30/75] improved description of function --- synapse/handlers/sso.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 59c663ffaa42..1b459ecb484a 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -721,6 +721,14 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: This downloads the image file from the url provided, store that in the media repository and then sets the avatar on user's profile. + Currently, it only supports server configurations which runs media repository + within the same process. + + It silently fails and log a warning by raising exception & catching it internally + if it is unable to fetch the image itself (non 200 status code) or + if the image supplied is bigger than max allowed size or + if the image type is not one of the allowed image types. + Args: user_id: matrix user ID in the form @localpart:domain as a string. @@ -768,7 +776,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: ) ) - # download picture + # download picture, enforcing size limit picture = io.BytesIO() download_response = await self._http_client.get_file( picture_https_url, picture, self._profile_handler.max_avatar_size From 7ff332015e4d4459fcc1da9e1559cc111a087366 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 10 Nov 2022 21:48:58 +0400 Subject: [PATCH 31/75] Update tests/handlers/test_sso.py Co-authored-by: David Robertson --- tests/handlers/test_sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 0d48b3bbc62f..a57597e5d29e 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -39,7 +39,7 @@ async def test_set_avatar(self) -> None: # TODO: Create a new user to set avatar for reg_handler = self.hs.get_registration_handler() - user_id = await reg_handler.register_user(approved=True) + user_id = self.get_success(reg_handler.register_user(approved=True)) # user_id = "@sso-user:test" with self.assertLogs() as cm: From 70d3cb54ced2acabd1bf345c431b0e6a2d827577 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 10 Nov 2022 21:49:07 +0400 Subject: [PATCH 32/75] Update tests/handlers/test_sso.py Co-authored-by: David Robertson --- tests/handlers/test_sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index a57597e5d29e..b781e3c5cbde 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -43,7 +43,7 @@ async def test_set_avatar(self) -> None: # user_id = "@sso-user:test" with self.assertLogs() as cm: - await handler.set_avatar(user_id, "http://my.server/me.png") + self.get_success(set_avatar(user_id, "http://my.server/me.png")) self.assertEqual( cm.output, ["INFO:synapse.handlers.sso:successfully saved the user avatar"] ) From 5271480eabb2e4f16fc1cf4f84f5e2ddfcb26ca0 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 13:17:41 +0400 Subject: [PATCH 33/75] do not await in unit tests, use self.get_success() --- tests/handlers/test_sso.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index b781e3c5cbde..cb1eba6977af 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -43,7 +43,7 @@ async def test_set_avatar(self) -> None: # user_id = "@sso-user:test" with self.assertLogs() as cm: - self.get_success(set_avatar(user_id, "http://my.server/me.png")) + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) self.assertEqual( cm.output, ["INFO:synapse.handlers.sso:successfully saved the user avatar"] ) @@ -62,7 +62,7 @@ async def test_set_avatar_too_big_image(self) -> None: user_id = "@sso-user:test" with self.assertLogs() as cm: - await handler.set_avatar(user_id, "http://my.server/big.png") + self.get_success(handler.set_avatar(user_id, "http://my.server/big.png")) self.assertEqual( cm.output, ["WARNING:synapse.handlers.sso:failed to save the user avatar"] ) @@ -76,7 +76,7 @@ async def test_set_avatar_incorrect_mime_type(self) -> None: user_id = "@sso-user:test" with self.assertLogs() as cm: - await handler.set_avatar(user_id, "http://my.server/me.png") + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) self.assertEqual( cm.output, ["WARNING:synapse.handlers.sso:failed to save the user avatar"] ) From e691a91f489675cbf673af6bf2c35b1f89ff368e Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 13:24:31 +0400 Subject: [PATCH 34/75] user registeration is sorted, remove TODO --- tests/handlers/test_sso.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index cb1eba6977af..a8f105c5ee43 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -37,10 +37,9 @@ async def test_set_avatar(self) -> None: """Tests successfully setting the avatar of a newly created user""" handler = self.hs.get_sso_handler() - # TODO: Create a new user to set avatar for + # Create a new user to set avatar for reg_handler = self.hs.get_registration_handler() user_id = self.get_success(reg_handler.register_user(approved=True)) - # user_id = "@sso-user:test" with self.assertLogs() as cm: self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) From 7101fc0fb00aaa750a858f2e16c5e2825e367a60 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 13:51:40 +0400 Subject: [PATCH 35/75] consistently return FakeResponse and change return type with explanation in comment --- tests/handlers/test_sso.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index a8f105c5ee43..ff672a5b6099 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -21,7 +21,7 @@ from synapse.util import Clock from tests import unittest -from tests.test_utils import SMALL_PNG, FakeResponse, simple_async_mock +from tests.test_utils import SMALL_PNG, FakeResponse class TestSSOHandler(unittest.HomeserverTestCase): @@ -81,6 +81,8 @@ async def test_set_avatar_incorrect_mime_type(self) -> None: ) +# FakeResponse intends to implement IResponse interface but still doesn't completely +# implement it, hence why the return type is such async def mock_request(method: str, url: str) -> IResponse: # for the purpose of test returning GET request body for HEAD request is fine if url == "http://my.server/me.png": @@ -100,7 +102,7 @@ async def mock_request(method: str, url: str) -> IResponse: body=SMALL_PNG, ) else: - return simple_async_mock(return_value=FakeResponse(code=400)) + return FakeResponse(code=400) elif url == "http://my.server/big.png": if method == "HEAD": return FakeResponse( @@ -110,7 +112,7 @@ async def mock_request(method: str, url: str) -> IResponse: ), ) - return simple_async_mock(return_value=FakeResponse(code=404)) + return FakeResponse(code=404) async def mock_get_file( From 2d46136739a98f19825f94252a4a787af3bed880 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 13:52:56 +0400 Subject: [PATCH 36/75] add await to ready_body_with_max_size() --- tests/handlers/test_sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index ff672a5b6099..6ab8a1850f28 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -123,6 +123,6 @@ async def mock_get_file( is_allowed_content_type: Optional[Callable[[str], bool]] = None, ) -> Tuple[int, Dict[bytes, List[bytes]], str, int]: response = await mock_request("GET", url) - read_body_with_max_size(response, output_stream, max_size) + await read_body_with_max_size(response, output_stream, max_size) return 67, {b"Content-Type": [b"image/png"]}, "", 200 From b034ee3a249de759ce8a7889795faef3d34bc1fb Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 14:00:03 +0400 Subject: [PATCH 37/75] move to alphabetical order --- mypy.ini | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mypy.ini b/mypy.ini index 2e8cc134a243..7cce5cfb3a09 100644 --- a/mypy.ini +++ b/mypy.ini @@ -120,6 +120,9 @@ disallow_untyped_defs = True [mypy-tests.storage.test_profile] disallow_untyped_defs = True +[mypy-tests.handlers.test_sso] +disallow_untyped_defs = True + [mypy-tests.storage.test_user_directory] disallow_untyped_defs = True @@ -132,9 +135,6 @@ disallow_untyped_defs = True [mypy-tests.utils] disallow_untyped_defs = True -[mypy-tests.handlers.test_sso] -disallow_untyped_defs = True - ;; Dependencies without annotations ;; Before ignoring a module, check to see if type stubs are available. ;; The `typeshed` project maintains stubs here: From 6d370b74566cec6c751748234cc954fdc627c97c Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 14:11:23 +0400 Subject: [PATCH 38/75] remove unnecessary cast --- synapse/handlers/sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 1b459ecb484a..52d6d7722f6c 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -790,7 +790,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: # store it in media repository avatar_mxc_url = await self._media_repo.create_content( - str(content_type), + content_type, None, picture, content_length, From e2cc4bf2fba3bc853df2607cf56c6fc34c9209e4 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 15:22:19 +0400 Subject: [PATCH 39/75] only use get_file() on http client to enforce size & mime type restrictions as opposed to HEAD check --- synapse/handlers/sso.py | 67 +++++++++++++---------------------------- 1 file changed, 21 insertions(+), 46 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 52d6d7722f6c..1ae94e424ff4 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -737,50 +737,25 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: try: uid = UserID.from_string(user_id) - # HEAD request to find image size & mime type before download - response = await self._http_client.request("HEAD", picture_https_url) - if response.code != 200: - raise Exception( - "HEAD request to check sso avatar image headers returned {}".format( - response.code - ) - ) - - # ensure http headers are present - if ( - response.headers.getRawHeaders("Content-Length") is None - or response.headers.getRawHeaders("Content-Type") is None - ): - raise Exception("HTTP headers missing for sso avatar image") - - content_length = int(response.headers.getRawHeaders("Content-Length")[0]) - content_type = response.headers.getRawHeaders("Content-Type")[0] - - # ensure picture size respects max_avatar_size defined in config - if self._profile_handler.max_avatar_size is not None: - if content_length > self._profile_handler.max_avatar_size: - raise Exception( - "SSO avatar image {} should be less than {}".format( - content_length, self._profile_handler.max_avatar_size - ) - ) - - # ensure picture is of allowed mime type - if ( - self._profile_handler.allowed_avatar_mimetypes - and content_type not in self._profile_handler.allowed_avatar_mimetypes - ): - raise Exception( - "SSO avatar image does not allow mime type of {}".format( - content_type - ) - ) - - # download picture, enforcing size limit + def allowed_mime_type(content_type: str) -> bool: + if ( + self._profile_handler.allowed_avatar_mimetypes + and content_type + not in self._profile_handler.allowed_avatar_mimetypes + ): + return False + return True + + # download picture, enforcing size limit & mime type check picture = io.BytesIO() + download_response = await self._http_client.get_file( - picture_https_url, picture, self._profile_handler.max_avatar_size + url=picture_https_url, + output_stream=picture, + max_size=self._profile_handler.max_avatar_size, + is_allowed_content_type=allowed_mime_type, ) + if download_response[3] != 200: raise Exception( "GET request to download sso avatar image returned {}".format( @@ -790,11 +765,11 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: # store it in media repository avatar_mxc_url = await self._media_repo.create_content( - content_type, - None, - picture, - content_length, - uid, + media_type=download_response[1][b"Content-Type"][0].decode("utf-8"), + upload_name=None, + content=picture, + content_length=download_response[0], + auth_user=uid, ) # save it as user avatar From 966edfde620ce93f88eeb9edccc43b79816a7775 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 17:42:31 +0400 Subject: [PATCH 40/75] simplify tests as per changed approach --- tests/handlers/test_sso.py | 77 +++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 43 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 6ab8a1850f28..415d3c03457e 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -9,14 +9,15 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from http import HTTPStatus from typing import BinaryIO, Callable, Dict, List, Optional, Tuple from unittest.mock import Mock from twisted.test.proto_helpers import MemoryReactor from twisted.web.http_headers import Headers -from twisted.web.iweb import IResponse -from synapse.http.client import RawHeaders, read_body_with_max_size +from synapse.api.errors import Codes, SynapseError +from synapse.http.client import RawHeaders from synapse.server import HomeServer from synapse.util import Clock @@ -26,8 +27,7 @@ class TestSSOHandler(unittest.HomeserverTestCase): def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: - self.http_client = Mock(spec=["request", "get_file"]) - self.http_client.request.side_effect = mock_request + self.http_client = Mock(spec=["get_file"]) self.http_client.get_file.side_effect = mock_get_file self.http_client.user_agent = b"Synapse Test" hs = self.setup_test_homeserver(proxied_http_client=self.http_client) @@ -44,7 +44,8 @@ async def test_set_avatar(self) -> None: with self.assertLogs() as cm: self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) self.assertEqual( - cm.output, ["INFO:synapse.handlers.sso:successfully saved the user avatar"] + cm.output[-1], + "INFO:synapse.handlers.sso:successfully saved the user avatar", ) # TODO: ensure avatar returned via user's profile is SMALL_PNG @@ -52,7 +53,7 @@ async def test_set_avatar(self) -> None: # profile = await profile_handler.get_profile(user_id) # profile["avatar_url"] - @unittest.override_config({"max_avatar_size": 99}) + @unittest.override_config({"max_avatar_size": 65}) async def test_set_avatar_too_big_image(self) -> None: """Tests saving of avatar failed when image size is too big""" handler = self.hs.get_sso_handler() @@ -61,7 +62,7 @@ async def test_set_avatar_too_big_image(self) -> None: user_id = "@sso-user:test" with self.assertLogs() as cm: - self.get_success(handler.set_avatar(user_id, "http://my.server/big.png")) + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) self.assertEqual( cm.output, ["WARNING:synapse.handlers.sso:failed to save the user avatar"] ) @@ -81,40 +82,6 @@ async def test_set_avatar_incorrect_mime_type(self) -> None: ) -# FakeResponse intends to implement IResponse interface but still doesn't completely -# implement it, hence why the return type is such -async def mock_request(method: str, url: str) -> IResponse: - # for the purpose of test returning GET request body for HEAD request is fine - if url == "http://my.server/me.png": - if method == "HEAD": - return FakeResponse( - code=200, - headers=Headers( - {"Content-Type": ["image/png"], "Content-Length": ["67"]} - ), - ) - elif method == "GET": - return FakeResponse( - code=200, - headers=Headers( - {"Content-Type": ["image/png"], "Content-Length": ["67"]} - ), - body=SMALL_PNG, - ) - else: - return FakeResponse(code=400) - elif url == "http://my.server/big.png": - if method == "HEAD": - return FakeResponse( - code=200, - headers=Headers( - {"Content-Type": ["image/png"], "Content-Length": ["999"]} - ), - ) - - return FakeResponse(code=404) - - async def mock_get_file( url: str, output_stream: BinaryIO, @@ -122,7 +89,31 @@ async def mock_get_file( headers: Optional[RawHeaders] = None, is_allowed_content_type: Optional[Callable[[str], bool]] = None, ) -> Tuple[int, Dict[bytes, List[bytes]], str, int]: - response = await mock_request("GET", url) - await read_body_with_max_size(response, output_stream, max_size) + + fake_response = FakeResponse(code=404) + if url == "http://my.server/me.png": + fake_response = FakeResponse( + code=200, + headers=Headers({"Content-Type": ["image/png"], "Content-Length": ["67"]}), + body=SMALL_PNG, + ) + + if max_size is not None and max_size < 67: + raise SynapseError( + HTTPStatus.BAD_GATEWAY, + "Requested file is too large > %r bytes" % (max_size,), + Codes.TOO_LARGE, + ) + + if is_allowed_content_type and not is_allowed_content_type("image/png"): + raise SynapseError( + HTTPStatus.BAD_GATEWAY, + ( + "Requested file's content type not allowed for this operation: %s" + % "image/png" + ), + ) + + output_stream.write(fake_response.body) return 67, {b"Content-Type": [b"image/png"]}, "", 200 From 19a417d8cc917159be2f5c6ca2c1826a61c92856 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 19:04:03 +0400 Subject: [PATCH 41/75] ensure avatar is set by querying from the profile handler --- tests/handlers/test_sso.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 415d3c03457e..675a1ac80ef0 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -48,10 +48,11 @@ async def test_set_avatar(self) -> None: "INFO:synapse.handlers.sso:successfully saved the user avatar", ) - # TODO: ensure avatar returned via user's profile is SMALL_PNG - # profile_handler = self.hs.get_profile_handler() - # profile = await profile_handler.get_profile(user_id) - # profile["avatar_url"] + # Ensure avatar is set on this newly created user, + # so no need to compare for the exact image + profile_handler = self.hs.get_profile_handler() + profile = self.get_success(profile_handler.get_profile(user_id)) + self.assertIsNot(profile["avatar_url"], None) @unittest.override_config({"max_avatar_size": 65}) async def test_set_avatar_too_big_image(self) -> None: From ef9af8251b592f2203574861500bf0329755ef93 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 19:05:57 +0400 Subject: [PATCH 42/75] save hash as the upload name for avatar --- synapse/handlers/sso.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 1ae94e424ff4..cf7344b8548a 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import abc +import hashlib import io import logging from typing import ( @@ -763,10 +764,13 @@ def allowed_mime_type(content_type: str) -> bool: ) ) + # upload name includes hash of the image file's content so that we can + # easily check if it requires an update or not, the next time user logs in + upload_name = "sso_avatar_" + hashlib.md5(picture.read()).hexdigest() # store it in media repository avatar_mxc_url = await self._media_repo.create_content( media_type=download_response[1][b"Content-Type"][0].decode("utf-8"), - upload_name=None, + upload_name=upload_name, content=picture, content_length=download_response[0], auth_user=uid, From 0714752f01a5c166d875b778fa319ddf1e3eab94 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 11 Nov 2022 19:23:54 +0400 Subject: [PATCH 43/75] bail early if the user already has the same avatar saved --- synapse/handlers/sso.py | 13 +++++++++++++ tests/handlers/test_sso.py | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index cf7344b8548a..510287b99f83 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -722,6 +722,9 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: This downloads the image file from the url provided, store that in the media repository and then sets the avatar on user's profile. + It can detect if the same image is being saved again and bails early by storing + hash of the file in upload_name of the avatar image. + Currently, it only supports server configurations which runs media repository within the same process. @@ -767,6 +770,16 @@ def allowed_mime_type(content_type: str) -> bool: # upload name includes hash of the image file's content so that we can # easily check if it requires an update or not, the next time user logs in upload_name = "sso_avatar_" + hashlib.md5(picture.read()).hexdigest() + + # bail if user already has the same avatar + profile = await self._profile_handler.get_profile(user_id) + if profile["avatar_url"] is not None: + media_id = profile["avatar_url"].split("/")[-1] + media = await self._media_repo.store.get_local_media(media_id) + if media is not None and upload_name == media["upload_name"]: + logger.info("skipping saving the user avatar") + return + # store it in media repository avatar_mxc_url = await self._media_repo.create_content( media_type=download_response[1][b"Content-Type"][0].decode("utf-8"), diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 675a1ac80ef0..22aa39f676b6 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -82,6 +82,28 @@ async def test_set_avatar_incorrect_mime_type(self) -> None: cm.output, ["WARNING:synapse.handlers.sso:failed to save the user avatar"] ) + async def test_skip_saving_avatar_when_not_changed(self) -> None: + """Tests whether saving of avatar correctly skips if the avatar hasn't changed""" + handler = self.hs.get_sso_handler() + + # Create a new user to set avatar for + reg_handler = self.hs.get_registration_handler() + user_id = self.get_success(reg_handler.register_user(approved=True)) + + with self.assertLogs() as cm: + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + self.assertEqual( + cm.output[-1], + "INFO:synapse.handlers.sso:successfully saved the user avatar", + ) + + with self.assertLogs() as cm: + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + self.assertEqual( + cm.output[-1], + "INFO:synapse.handlers.sso:skipping saving the user avatar", + ) + async def mock_get_file( url: str, From ecdfe1ad19e8989ed740f46f2ca26ab1280e6f04 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Mon, 14 Nov 2022 12:05:49 +0400 Subject: [PATCH 44/75] switch to get_proxied_blacklisted_http_client() for blacklisting local sensitive urls --- synapse/handlers/sso.py | 2 +- tests/handlers/test_sso.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 510287b99f83..c171aad9f9b1 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -199,7 +199,7 @@ def __init__(self, hs: "HomeServer"): self._bad_user_template = hs.config.sso.sso_auth_bad_user_template self._profile_handler = hs.get_profile_handler() self._media_repo = hs.get_media_repository() - self._http_client = hs.get_proxied_http_client() + self._http_client = hs.get_proxied_blacklisted_http_client() # The following template is shown after a successful user interactive # authentication session. It tells the user they can close the window. diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 22aa39f676b6..6ceb430ae92c 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -30,7 +30,7 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: self.http_client = Mock(spec=["get_file"]) self.http_client.get_file.side_effect = mock_get_file self.http_client.user_agent = b"Synapse Test" - hs = self.setup_test_homeserver(proxied_http_client=self.http_client) + hs = self.setup_test_homeserver(proxied_blacklisted_http_client=self.http_client) return hs async def test_set_avatar(self) -> None: From 465bade52c8dc46bc9c2fe3ba6500cac6a4ad55b Mon Sep 17 00:00:00 2001 From: Ashfame Date: Mon, 14 Nov 2022 12:10:20 +0400 Subject: [PATCH 45/75] let default for picture be None instead of empty string --- synapse/handlers/oidc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 363de72f429b..5d6d92233200 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1615,7 +1615,7 @@ def render_template_field(template: Optional[Template]) -> Optional[str]: if email: emails.append(email) - picture = userinfo.get("picture", "") + picture = userinfo.get("picture") return UserAttributeDict( localpart=localpart, From 2f1819cb442385410ce86831c435366758eca9c4 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Mon, 14 Nov 2022 13:04:15 +0400 Subject: [PATCH 46/75] bail early if synapse is not running media repo in-process --- synapse/handlers/sso.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index c171aad9f9b1..7364a3372307 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -198,6 +198,7 @@ def __init__(self, hs: "HomeServer"): self._error_template = hs.config.sso.sso_error_template self._bad_user_template = hs.config.sso.sso_auth_bad_user_template self._profile_handler = hs.get_profile_handler() + self._can_load_media_repo = hs.config.media.can_load_media_repo self._media_repo = hs.get_media_repository() self._http_client = hs.get_proxied_blacklisted_http_client() @@ -738,6 +739,9 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: picture_https_url: HTTPS url for the picture image file. """ + if not self._can_load_media_repo: + return + try: uid = UserID.from_string(user_id) From 006800f306f7c53a92958295b23405159730f043 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 16 Nov 2022 19:21:22 +0000 Subject: [PATCH 47/75] Run linter --- tests/handlers/test_sso.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 6ceb430ae92c..a774274cfeba 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -30,7 +30,9 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: self.http_client = Mock(spec=["get_file"]) self.http_client.get_file.side_effect = mock_get_file self.http_client.user_agent = b"Synapse Test" - hs = self.setup_test_homeserver(proxied_blacklisted_http_client=self.http_client) + hs = self.setup_test_homeserver( + proxied_blacklisted_http_client=self.http_client + ) return hs async def test_set_avatar(self) -> None: From 4a7805214acb489a7815452bc62b309b02b02b0d Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 15:59:59 +0400 Subject: [PATCH 48/75] grammar fixes in docstring Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- synapse/handlers/sso.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 7364a3372307..67a6c8e0265a 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -720,19 +720,20 @@ async def _register_mapped_user( async def set_avatar(self, user_id: str, picture_https_url: str) -> None: """Set avatar of the user. - This downloads the image file from the url provided, store that in - the media repository and then sets the avatar on user's profile. + This downloads the image file from the URL provided, stores that in + the media repository and then sets the avatar on the user's profile. It can detect if the same image is being saved again and bails early by storing - hash of the file in upload_name of the avatar image. + the hash of the file in the `upload_name` of the avatar image. - Currently, it only supports server configurations which runs media repository + Currently, it only supports server configurations which run the media repository within the same process. - It silently fails and log a warning by raising exception & catching it internally - if it is unable to fetch the image itself (non 200 status code) or - if the image supplied is bigger than max allowed size or - if the image type is not one of the allowed image types. + It silently fails and logs a warning by raising an exception and catching it + internally if: + * it is unable to fetch the image itself (non 200 status code) or + * the image supplied is bigger than max allowed size or + * the image type is not one of the allowed image types. Args: user_id: matrix user ID in the form @localpart:domain as a string. From b1b0a91da5ea2d49050f777cb5604d9fcc85c2ec Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 16:01:25 +0400 Subject: [PATCH 49/75] sha256 hash the image instead of md5 hash Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- synapse/handlers/sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 67a6c8e0265a..5dadeac9d46b 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -774,7 +774,7 @@ def allowed_mime_type(content_type: str) -> bool: # upload name includes hash of the image file's content so that we can # easily check if it requires an update or not, the next time user logs in - upload_name = "sso_avatar_" + hashlib.md5(picture.read()).hexdigest() + upload_name = "sso_avatar_" + hashlib.sha256(picture.read()).hexdigest() # bail if user already has the same avatar profile = await self._profile_handler.get_profile(user_id) From b7100235f35c5c2b87161d2bfb468b357edba053 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 16:13:40 +0400 Subject: [PATCH 50/75] cleaner to destructure get_file() results than use indexing to access --- synapse/handlers/sso.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 5dadeac9d46b..c14fdb8c97b1 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -758,18 +758,16 @@ def allowed_mime_type(content_type: str) -> bool: # download picture, enforcing size limit & mime type check picture = io.BytesIO() - download_response = await self._http_client.get_file( + content_length, headers, uri, code = await self._http_client.get_file( url=picture_https_url, output_stream=picture, max_size=self._profile_handler.max_avatar_size, is_allowed_content_type=allowed_mime_type, ) - if download_response[3] != 200: + if code != 200: raise Exception( - "GET request to download sso avatar image returned {}".format( - download_response[3] - ) + "GET request to download sso avatar image returned {}".format(code) ) # upload name includes hash of the image file's content so that we can @@ -787,10 +785,10 @@ def allowed_mime_type(content_type: str) -> bool: # store it in media repository avatar_mxc_url = await self._media_repo.create_content( - media_type=download_response[1][b"Content-Type"][0].decode("utf-8"), + media_type=headers[b"Content-Type"][0].decode("utf-8"), upload_name=upload_name, content=picture, - content_length=download_response[0], + content_length=content_length, auth_user=uid, ) From f118e8b35d75bbf8bfcb22121ae968d8a504cfa0 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 16:16:29 +0400 Subject: [PATCH 51/75] rename function to include a verb --- synapse/handlers/sso.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index c14fdb8c97b1..17eab2dd9bc5 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -746,7 +746,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: try: uid = UserID.from_string(user_id) - def allowed_mime_type(content_type: str) -> bool: + def is_allowed_mime_type(content_type: str) -> bool: if ( self._profile_handler.allowed_avatar_mimetypes and content_type @@ -762,7 +762,7 @@ def allowed_mime_type(content_type: str) -> bool: url=picture_https_url, output_stream=picture, max_size=self._profile_handler.max_avatar_size, - is_allowed_content_type=allowed_mime_type, + is_allowed_content_type=is_allowed_mime_type, ) if code != 200: From 751855c7986f35f5bda0dba608209378fc1d73ac Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 16:30:33 +0400 Subject: [PATCH 52/75] make set_avatar() return bool so that its easier to test --- synapse/handlers/sso.py | 8 +++++--- tests/handlers/test_sso.py | 39 +++++++++----------------------------- 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 17eab2dd9bc5..3eb36dc23d0f 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -717,7 +717,7 @@ async def _register_mapped_user( return registered_user_id - async def set_avatar(self, user_id: str, picture_https_url: str) -> None: + async def set_avatar(self, user_id: str, picture_https_url: str) -> bool: """Set avatar of the user. This downloads the image file from the URL provided, stores that in @@ -741,7 +741,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> None: picture_https_url: HTTPS url for the picture image file. """ if not self._can_load_media_repo: - return + return False try: uid = UserID.from_string(user_id) @@ -781,7 +781,7 @@ def is_allowed_mime_type(content_type: str) -> bool: media = await self._media_repo.store.get_local_media(media_id) if media is not None and upload_name == media["upload_name"]: logger.info("skipping saving the user avatar") - return + return False # store it in media repository avatar_mxc_url = await self._media_repo.create_content( @@ -800,8 +800,10 @@ def is_allowed_mime_type(content_type: str) -> bool: ) logger.info("successfully saved the user avatar") + return True except Exception: logger.warning("failed to save the user avatar") + return False async def complete_sso_ui_auth_request( self, diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index a774274cfeba..0571f21ac93a 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -43,12 +43,7 @@ async def test_set_avatar(self) -> None: reg_handler = self.hs.get_registration_handler() user_id = self.get_success(reg_handler.register_user(approved=True)) - with self.assertLogs() as cm: - self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) - self.assertEqual( - cm.output[-1], - "INFO:synapse.handlers.sso:successfully saved the user avatar", - ) + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) # Ensure avatar is set on this newly created user, # so no need to compare for the exact image @@ -64,11 +59,7 @@ async def test_set_avatar_too_big_image(self) -> None: # any random user works since image check is supposed to fail user_id = "@sso-user:test" - with self.assertLogs() as cm: - self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) - self.assertEqual( - cm.output, ["WARNING:synapse.handlers.sso:failed to save the user avatar"] - ) + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) @unittest.override_config({"allowed_avatar_mimetypes": ["image/jpeg"]}) async def test_set_avatar_incorrect_mime_type(self) -> None: @@ -78,33 +69,21 @@ async def test_set_avatar_incorrect_mime_type(self) -> None: # any random user works since image check is supposed to fail user_id = "@sso-user:test" - with self.assertLogs() as cm: - self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) - self.assertEqual( - cm.output, ["WARNING:synapse.handlers.sso:failed to save the user avatar"] - ) + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"), False) async def test_skip_saving_avatar_when_not_changed(self) -> None: - """Tests whether saving of avatar correctly skips if the avatar hasn't changed""" + """Tests whether saving of avatar correctly skips if the avatar hasn't + changed""" handler = self.hs.get_sso_handler() # Create a new user to set avatar for reg_handler = self.hs.get_registration_handler() user_id = self.get_success(reg_handler.register_user(approved=True)) - with self.assertLogs() as cm: - self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) - self.assertEqual( - cm.output[-1], - "INFO:synapse.handlers.sso:successfully saved the user avatar", - ) - - with self.assertLogs() as cm: - self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) - self.assertEqual( - cm.output[-1], - "INFO:synapse.handlers.sso:skipping saving the user avatar", - ) + # set avatar for the first time, should be a success + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + # set same avatar for the second time, should be a failure + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"), False) async def mock_get_file( From 1ef48ddc9ef18421d47e6373a3e097d07ce35bc1 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 16:34:17 +0400 Subject: [PATCH 53/75] don't hardcode size, use len() in tests --- tests/handlers/test_sso.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 0571f21ac93a..3759e351c6f2 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -51,7 +51,7 @@ async def test_set_avatar(self) -> None: profile = self.get_success(profile_handler.get_profile(user_id)) self.assertIsNot(profile["avatar_url"], None) - @unittest.override_config({"max_avatar_size": 65}) + @unittest.override_config({"max_avatar_size": len(SMALL_PNG) - 1}) async def test_set_avatar_too_big_image(self) -> None: """Tests saving of avatar failed when image size is too big""" handler = self.hs.get_sso_handler() @@ -98,11 +98,13 @@ async def mock_get_file( if url == "http://my.server/me.png": fake_response = FakeResponse( code=200, - headers=Headers({"Content-Type": ["image/png"], "Content-Length": ["67"]}), + headers=Headers( + {"Content-Type": ["image/png"], "Content-Length": [str(len(SMALL_PNG))]} + ), body=SMALL_PNG, ) - if max_size is not None and max_size < 67: + if max_size is not None and max_size < len(SMALL_PNG): raise SynapseError( HTTPStatus.BAD_GATEWAY, "Requested file is too large > %r bytes" % (max_size,), @@ -120,4 +122,4 @@ async def mock_get_file( output_stream.write(fake_response.body) - return 67, {b"Content-Type": [b"image/png"]}, "", 200 + return len(SMALL_PNG), {b"Content-Type": [b"image/png"]}, "", 200 From f815f3b5f78881da4177f1cfcab36909b375effb Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 16:38:31 +0400 Subject: [PATCH 54/75] ensure test doesn't break even if SMALL_PNG is changed in future --- tests/handlers/test_sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 3759e351c6f2..0ed2bd64cd09 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -51,7 +51,7 @@ async def test_set_avatar(self) -> None: profile = self.get_success(profile_handler.get_profile(user_id)) self.assertIsNot(profile["avatar_url"], None) - @unittest.override_config({"max_avatar_size": len(SMALL_PNG) - 1}) + @unittest.override_config({"max_avatar_size": 1}) async def test_set_avatar_too_big_image(self) -> None: """Tests saving of avatar failed when image size is too big""" handler = self.hs.get_sso_handler() From fe8e1bac1da598765871de3bfb913b5b6041a0d9 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 16:44:31 +0400 Subject: [PATCH 55/75] fix grammar Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- tests/handlers/test_sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 0ed2bd64cd09..121ee65e442f 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -63,7 +63,7 @@ async def test_set_avatar_too_big_image(self) -> None: @unittest.override_config({"allowed_avatar_mimetypes": ["image/jpeg"]}) async def test_set_avatar_incorrect_mime_type(self) -> None: - """Tests saving of avatar failed when not allowed mimetype of image was used""" + """Tests that saving an avatar fails when its mime type is not allowed""" handler = self.hs.get_sso_handler() # any random user works since image check is supposed to fail From a973e75523dd8939e1ec8eb9d662bcca44006fa1 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 16:46:27 +0400 Subject: [PATCH 56/75] log info message since out-of-process media repos are not supported --- synapse/handlers/sso.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 3eb36dc23d0f..3d1c9832af64 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -741,6 +741,10 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> bool: picture_https_url: HTTPS url for the picture image file. """ if not self._can_load_media_repo: + logger.info( + "failed to set user avatar because out-of-process media repositories " + "are not supported yet " + ) return False try: From 7e165f450f2f9c5ce1fd113d039be0fdeb3330c3 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 16:49:00 +0400 Subject: [PATCH 57/75] add comment in respect to type annotations Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- synapse/handlers/oidc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index 5d6d92233200..1d17156dcdec 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -1435,7 +1435,7 @@ class UserAttributeDict(TypedDict): localpart: Optional[str] confirm_localpart: bool display_name: Optional[str] - picture: Optional[str] + picture: Optional[str] # may be omitted by older `OidcMappingProviders` emails: List[str] From 2da03c223b79adc50b2aa3e98d2399b2a38d1b69 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 16:53:31 +0400 Subject: [PATCH 58/75] ensure skipping of avatar updation is tested properly --- tests/handlers/test_sso.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 121ee65e442f..4e3dfc03ae4f 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -82,9 +82,19 @@ async def test_skip_saving_avatar_when_not_changed(self) -> None: # set avatar for the first time, should be a success self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + + # get avatar picture for comparison after another attempt + profile_handler = self.hs.get_profile_handler() + profile = self.get_success(profile_handler.get_profile(user_id)) + url_to_match = profile["avatar_url"] + # set same avatar for the second time, should be a failure self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"), False) + # compare avatar picture's url from previous step + profile = self.get_success(profile_handler.get_profile(user_id)) + self.assertEqual(profile["avatar_url"], url_to_match) + async def mock_get_file( url: str, From 7a29b06f49490ad7d48032bf2fe20729cc3ab097 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 17:02:41 +0400 Subject: [PATCH 59/75] fix grammar Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- docs/usage/configuration/config_documentation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 63c849d4ff52..5e24bb70bc54 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -2997,7 +2997,7 @@ Options for each entry include: for the user. Defaults to 'sub', which OpenID Connect compliant providers should provide. - * picture_claim: name of the claim containing an url for user's profile picture. + * picture_claim: name of the claim containing an url for the user's profile picture. Defaults to 'picture', which OpenID Connect compliant providers should provide and has to refer to a direct image file such as PNG, JPEG, or GIF image file. From 4ebb1dbceaaff409c00c7313134437e3b42e5915 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 17:03:05 +0400 Subject: [PATCH 60/75] fix grammar Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- tests/handlers/test_sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 4e3dfc03ae4f..811ad3facdf1 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -53,7 +53,7 @@ async def test_set_avatar(self) -> None: @unittest.override_config({"max_avatar_size": 1}) async def test_set_avatar_too_big_image(self) -> None: - """Tests saving of avatar failed when image size is too big""" + """Tests that saving an avatar fails when it is too big""" handler = self.hs.get_sso_handler() # any random user works since image check is supposed to fail From 104eef8a786c07d705458740a824453cf4704c8a Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 18:14:53 +0400 Subject: [PATCH 61/75] add missing backticks --- docs/usage/configuration/config_documentation.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 5e24bb70bc54..f30ceb8bf39a 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -2993,11 +2993,11 @@ Options for each entry include: For the default provider, the following settings are available: - * subject_claim: name of the claim containing a unique identifier + * `subject_claim`: name of the claim containing a unique identifier for the user. Defaults to 'sub', which OpenID Connect compliant providers should provide. - * picture_claim: name of the claim containing an url for the user's profile picture. + * `picture_claim`: name of the claim containing an url for the user's profile picture. Defaults to 'picture', which OpenID Connect compliant providers should provide and has to refer to a direct image file such as PNG, JPEG, or GIF image file. From 1e87ac48f859c32fc9fc00637fc7ff150b2e21a3 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 18:21:49 +0400 Subject: [PATCH 62/75] ensure before skipping avatar updation we check for server_name to match as well, since avatar can be a remote file with a conflicting media id as well --- synapse/handlers/sso.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 3d1c9832af64..3e8453ff0ebf 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -781,11 +781,14 @@ def is_allowed_mime_type(content_type: str) -> bool: # bail if user already has the same avatar profile = await self._profile_handler.get_profile(user_id) if profile["avatar_url"] is not None: + server_name = profile["avatar_url"].split("/")[-2] media_id = profile["avatar_url"].split("/")[-1] - media = await self._media_repo.store.get_local_media(media_id) - if media is not None and upload_name == media["upload_name"]: - logger.info("skipping saving the user avatar") - return False + media = None + if server_name == self._server_name: + media = await self._media_repo.store.get_local_media(media_id) + if media is not None and upload_name == media["upload_name"]: + logger.info("skipping saving the user avatar") + return False # store it in media repository avatar_mxc_url = await self._media_repo.create_content( From 0526ec285b6d42ef321694ced80c9d537435304a Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 18:42:30 +0400 Subject: [PATCH 63/75] add comment for limited support for picture_claim --- docs/usage/configuration/config_documentation.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index f30ceb8bf39a..3b41f4f12a50 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3000,6 +3000,9 @@ Options for each entry include: * `picture_claim`: name of the claim containing an url for the user's profile picture. Defaults to 'picture', which OpenID Connect compliant providers should provide and has to refer to a direct image file such as PNG, JPEG, or GIF image file. + + Currently only supported in server configurations where media repository runs within + the Synapse process. * `localpart_template`: Jinja2 template for the localpart of the MXID. If this is not set, the user will be prompted to choose their From 68eaef79eb04f8761d2fb87310be0cab27eadfab Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 19:01:21 +0400 Subject: [PATCH 64/75] document return value Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- synapse/handlers/sso.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 3e8453ff0ebf..0bcf8f8c5567 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -739,6 +739,9 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> bool: user_id: matrix user ID in the form @localpart:domain as a string. picture_https_url: HTTPS url for the picture image file. + + Returns: `True` if the user's avatar has been successfully set to the image at + `picture_https_url`. """ if not self._can_load_media_repo: logger.info( From 49937c83a31f686506482572ed740ccbb6559b06 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 19:03:06 +0400 Subject: [PATCH 65/75] skip assignment Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- synapse/handlers/sso.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 0bcf8f8c5567..6abc4af6c814 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -786,7 +786,6 @@ def is_allowed_mime_type(content_type: str) -> bool: if profile["avatar_url"] is not None: server_name = profile["avatar_url"].split("/")[-2] media_id = profile["avatar_url"].split("/")[-1] - media = None if server_name == self._server_name: media = await self._media_repo.store.get_local_media(media_id) if media is not None and upload_name == media["upload_name"]: From 5542a0f8a2325daadb11b75aa1c6757814b6979c Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 19:04:36 +0400 Subject: [PATCH 66/75] return true when skipping image since exact image is already set on the user profile --- synapse/handlers/sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 6abc4af6c814..223ad529fc52 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -790,7 +790,7 @@ def is_allowed_mime_type(content_type: str) -> bool: media = await self._media_repo.store.get_local_media(media_id) if media is not None and upload_name == media["upload_name"]: logger.info("skipping saving the user avatar") - return False + return True # store it in media repository avatar_mxc_url = await self._media_repo.create_content( From d00385ce69df7d6e2b6f38da15a56239fd9eb59d Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 19:05:21 +0400 Subject: [PATCH 67/75] better explanation for server configs Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- docs/usage/configuration/config_documentation.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 3b41f4f12a50..3bee43b43199 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3001,8 +3001,8 @@ Options for each entry include: Defaults to 'picture', which OpenID Connect compliant providers should provide and has to refer to a direct image file such as PNG, JPEG, or GIF image file. - Currently only supported in server configurations where media repository runs within - the Synapse process. + Currently only supported in monolithic (single-process) server configurations + where media the repository runs within the Synapse process. * `localpart_template`: Jinja2 template for the localpart of the MXID. If this is not set, the user will be prompted to choose their From a81b0f71a1c1c7c92bdaf85570c3d932186fbb0c Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 19:06:22 +0400 Subject: [PATCH 68/75] fix grammar --- docs/usage/configuration/config_documentation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 3bee43b43199..1ee166c68711 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3002,7 +3002,7 @@ Options for each entry include: and has to refer to a direct image file such as PNG, JPEG, or GIF image file. Currently only supported in monolithic (single-process) server configurations - where media the repository runs within the Synapse process. + where media repository runs within the Synapse process. * `localpart_template`: Jinja2 template for the localpart of the MXID. If this is not set, the user will be prompted to choose their From 9a89e9f0871b3a335581a47e983b65a9146b9500 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 19:09:57 +0400 Subject: [PATCH 69/75] fix tests to actually assert True/False, and not relying on get_success to handle that --- tests/handlers/test_sso.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 811ad3facdf1..1971eef8c01c 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -43,7 +43,7 @@ async def test_set_avatar(self) -> None: reg_handler = self.hs.get_registration_handler() user_id = self.get_success(reg_handler.register_user(approved=True)) - self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + self.assertTrue(self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"))) # Ensure avatar is set on this newly created user, # so no need to compare for the exact image @@ -59,7 +59,7 @@ async def test_set_avatar_too_big_image(self) -> None: # any random user works since image check is supposed to fail user_id = "@sso-user:test" - self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + self.assertFalse(self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"))) @unittest.override_config({"allowed_avatar_mimetypes": ["image/jpeg"]}) async def test_set_avatar_incorrect_mime_type(self) -> None: @@ -69,7 +69,7 @@ async def test_set_avatar_incorrect_mime_type(self) -> None: # any random user works since image check is supposed to fail user_id = "@sso-user:test" - self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"), False) + self.assertFalse(self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"))) async def test_skip_saving_avatar_when_not_changed(self) -> None: """Tests whether saving of avatar correctly skips if the avatar hasn't @@ -81,7 +81,7 @@ async def test_skip_saving_avatar_when_not_changed(self) -> None: user_id = self.get_success(reg_handler.register_user(approved=True)) # set avatar for the first time, should be a success - self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + self.assertTrue(self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"))) # get avatar picture for comparison after another attempt profile_handler = self.hs.get_profile_handler() @@ -89,7 +89,7 @@ async def test_skip_saving_avatar_when_not_changed(self) -> None: url_to_match = profile["avatar_url"] # set same avatar for the second time, should be a failure - self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"), False) + self.assertTrue(self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"))) # compare avatar picture's url from previous step profile = self.get_success(profile_handler.get_profile(user_id)) From 00af8337c376ae6daa16c7b36e5c9941571235ed Mon Sep 17 00:00:00 2001 From: Ashfame Date: Thu, 24 Nov 2022 19:12:09 +0400 Subject: [PATCH 70/75] fix formatting in tests --- tests/handlers/test_sso.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index 1971eef8c01c..a425d61e1a3e 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -43,7 +43,9 @@ async def test_set_avatar(self) -> None: reg_handler = self.hs.get_registration_handler() user_id = self.get_success(reg_handler.register_user(approved=True)) - self.assertTrue(self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"))) + self.assertTrue( + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + ) # Ensure avatar is set on this newly created user, # so no need to compare for the exact image @@ -59,7 +61,9 @@ async def test_set_avatar_too_big_image(self) -> None: # any random user works since image check is supposed to fail user_id = "@sso-user:test" - self.assertFalse(self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"))) + self.assertFalse( + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + ) @unittest.override_config({"allowed_avatar_mimetypes": ["image/jpeg"]}) async def test_set_avatar_incorrect_mime_type(self) -> None: @@ -69,7 +73,9 @@ async def test_set_avatar_incorrect_mime_type(self) -> None: # any random user works since image check is supposed to fail user_id = "@sso-user:test" - self.assertFalse(self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"))) + self.assertFalse( + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + ) async def test_skip_saving_avatar_when_not_changed(self) -> None: """Tests whether saving of avatar correctly skips if the avatar hasn't @@ -81,7 +87,9 @@ async def test_skip_saving_avatar_when_not_changed(self) -> None: user_id = self.get_success(reg_handler.register_user(approved=True)) # set avatar for the first time, should be a success - self.assertTrue(self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"))) + self.assertTrue( + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + ) # get avatar picture for comparison after another attempt profile_handler = self.hs.get_profile_handler() @@ -89,7 +97,9 @@ async def test_skip_saving_avatar_when_not_changed(self) -> None: url_to_match = profile["avatar_url"] # set same avatar for the second time, should be a failure - self.assertTrue(self.get_success(handler.set_avatar(user_id, "http://my.server/me.png"))) + self.assertTrue( + self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) + ) # compare avatar picture's url from previous step profile = self.get_success(profile_handler.get_profile(user_id)) From 1a37d82bd44d485109bf3133e3138db2c87ba225 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 19:28:23 +0400 Subject: [PATCH 71/75] fix grammar Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- docs/usage/configuration/config_documentation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 1ee166c68711..cfa5d7d72115 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3002,7 +3002,7 @@ Options for each entry include: and has to refer to a direct image file such as PNG, JPEG, or GIF image file. Currently only supported in monolithic (single-process) server configurations - where media repository runs within the Synapse process. + where the media repository runs within the Synapse process. * `localpart_template`: Jinja2 template for the localpart of the MXID. If this is not set, the user will be prompted to choose their From 8746b66cc1a7c585e4beb46b4fc8c62090f35771 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Thu, 24 Nov 2022 19:28:58 +0400 Subject: [PATCH 72/75] correct comment Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- tests/handlers/test_sso.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sso.py b/tests/handlers/test_sso.py index a425d61e1a3e..137deab138b5 100644 --- a/tests/handlers/test_sso.py +++ b/tests/handlers/test_sso.py @@ -96,7 +96,7 @@ async def test_skip_saving_avatar_when_not_changed(self) -> None: profile = self.get_success(profile_handler.get_profile(user_id)) url_to_match = profile["avatar_url"] - # set same avatar for the second time, should be a failure + # set same avatar for the second time, should be a success self.assertTrue( self.get_success(handler.set_avatar(user_id, "http://my.server/me.png")) ) From 81d834b48e71e110e96c8e95115c1a9f9ee947d4 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 25 Nov 2022 16:54:47 +0400 Subject: [PATCH 73/75] only load the media repo when its available --- synapse/handlers/sso.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index 57877c0aa615..cbd7a8537e30 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -199,8 +199,10 @@ def __init__(self, hs: "HomeServer"): self._error_template = hs.config.sso.sso_error_template self._bad_user_template = hs.config.sso.sso_auth_bad_user_template self._profile_handler = hs.get_profile_handler() - self._can_load_media_repo = hs.config.media.can_load_media_repo - self._media_repo = hs.get_media_repository() + if hs.config.media.can_load_media_repo: + self._media_repo = hs.get_media_repository() + else: + self._media_repo = None self._http_client = hs.get_proxied_blacklisted_http_client() # The following template is shown after a successful user interactive @@ -744,7 +746,7 @@ async def set_avatar(self, user_id: str, picture_https_url: str) -> bool: Returns: `True` if the user's avatar has been successfully set to the image at `picture_https_url`. """ - if not self._can_load_media_repo: + if self._media_repo is None: logger.info( "failed to set user avatar because out-of-process media repositories " "are not supported yet " From b32c2f7f9877ca127e204dea79d2e4dbe2381296 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 25 Nov 2022 17:48:39 +0400 Subject: [PATCH 74/75] define type for media_repo instance attribute --- synapse/handlers/sso.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index cbd7a8537e30..c469830544d1 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -55,6 +55,7 @@ from synapse.util.stringutils import random_string if TYPE_CHECKING: + from synapse.rest.media.v1.media_repository import MediaRepository from synapse.server import HomeServer logger = logging.getLogger(__name__) @@ -190,6 +191,8 @@ class SsoHandler: _MAPPING_SESSION_VALIDITY_PERIOD_MS = 15 * 60 * 1000 def __init__(self, hs: "HomeServer"): + self._media_repo: Optional[MediaRepository] + self._clock = hs.get_clock() self._store = hs.get_datastores().main self._server_name = hs.hostname @@ -199,10 +202,9 @@ def __init__(self, hs: "HomeServer"): self._error_template = hs.config.sso.sso_error_template self._bad_user_template = hs.config.sso.sso_auth_bad_user_template self._profile_handler = hs.get_profile_handler() - if hs.config.media.can_load_media_repo: - self._media_repo = hs.get_media_repository() - else: - self._media_repo = None + self._media_repo = ( + hs.get_media_repository() if hs.config.media.can_load_media_repo else None + ) self._http_client = hs.get_proxied_blacklisted_http_client() # The following template is shown after a successful user interactive From a14eea5a2358463dca3d49dfa15d9fbfb923e05a Mon Sep 17 00:00:00 2001 From: Ashfame Date: Fri, 25 Nov 2022 18:27:34 +0400 Subject: [PATCH 75/75] no need for explicit type annotation when ternary expression is used --- synapse/handlers/sso.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index c469830544d1..44e70fc4b874 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -55,7 +55,6 @@ from synapse.util.stringutils import random_string if TYPE_CHECKING: - from synapse.rest.media.v1.media_repository import MediaRepository from synapse.server import HomeServer logger = logging.getLogger(__name__) @@ -191,8 +190,6 @@ class SsoHandler: _MAPPING_SESSION_VALIDITY_PERIOD_MS = 15 * 60 * 1000 def __init__(self, hs: "HomeServer"): - self._media_repo: Optional[MediaRepository] - self._clock = hs.get_clock() self._store = hs.get_datastores().main self._server_name = hs.hostname