Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

add support for handling avatar with SSO login #13917

Merged
merged 78 commits into from
Nov 25, 2022
Merged
Show file tree
Hide file tree
Changes from 77 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
6c67c5a
add support for handling avatar with SSO login
ashfame Sep 23, 2022
0fd47d1
add changelog file
ashfame Sep 27, 2022
546ea47
change log message
ashfame Sep 28, 2022
4d74943
remove separate log call, just pass message while logging exception
ashfame Sep 28, 2022
d7285f7
ensure avatar image size is within the max_avatar_size allowed as per…
ashfame Sep 27, 2022
f1767d2
added missing period in changelog file
ashfame Sep 28, 2022
670237c
use synapse's instantiated http client rather than requests library f…
ashfame Sep 28, 2022
879f8ad
ensure content_type is checked against allowed mime types
ashfame Sep 28, 2022
c6de28d
oops: HEAD request is to be used and not OPTIONS request for getting …
ashfame Sep 28, 2022
93662ee
better way to obtain headers from response
ashfame Sep 28, 2022
586170d
define picture attribute as Optional[str] with default value as None …
ashfame Sep 28, 2022
6915096
Merge branch 'develop' into sso_avatar_support
ashfame Oct 27, 2022
25d2594
follow standard pattern for getting value out of userinfo with a defa…
ashfame Oct 31, 2022
b343a43
add picture_claim explanation to config docs
ashfame Oct 31, 2022
07cbe39
prefix media_repo attribute with underscore to indicate private usage…
ashfame Nov 1, 2022
f4c1d39
declare docstring for set_avatar()
ashfame Nov 1, 2022
1594a3f
add additional details to exceptions
ashfame Nov 1, 2022
3a18057
log as warning with any raised Exception inside set_avatar() and not …
ashfame Nov 1, 2022
06bbd44
handle reading http headers better
ashfame Nov 1, 2022
93ffe6a
improve exception messages
ashfame Nov 1, 2022
e432b53
enforce max size limit while downloading the image file
ashfame Nov 8, 2022
669c7b0
update avatar with every subsequent login too
ashfame Nov 8, 2022
f5379ed
add unit tests
ashfame Nov 9, 2022
fb823f5
Merge branch 'develop' into sso_avatar_support
ashfame Nov 9, 2022
b51b4f8
declare new variable instead of reusing as type is different
ashfame Nov 9, 2022
adf0640
remove unused imports
ashfame Nov 9, 2022
225be62
fix type issues reported by linter
ashfame Nov 9, 2022
dfd26f0
reorder imports
ashfame Nov 9, 2022
11ff1b4
fix semantic checks
ashfame Nov 10, 2022
1592d43
stream output correctly in test
ashfame Nov 10, 2022
c15e873
define return types on functions and define disallow_untyped_defs as …
ashfame Nov 10, 2022
c0861f8
improved description of function
ashfame Nov 10, 2022
7ff3320
Update tests/handlers/test_sso.py
ashfame Nov 10, 2022
70d3cb5
Update tests/handlers/test_sso.py
ashfame Nov 10, 2022
5271480
do not await in unit tests, use self.get_success()
ashfame Nov 11, 2022
e691a91
user registeration is sorted, remove TODO
ashfame Nov 11, 2022
7101fc0
consistently return FakeResponse and change return type with explanat…
ashfame Nov 11, 2022
2d46136
add await to ready_body_with_max_size()
ashfame Nov 11, 2022
b034ee3
move to alphabetical order
ashfame Nov 11, 2022
6d370b7
remove unnecessary cast
ashfame Nov 11, 2022
e2cc4bf
only use get_file() on http client to enforce size & mime type restri…
ashfame Nov 11, 2022
966edfd
simplify tests as per changed approach
ashfame Nov 11, 2022
19a417d
ensure avatar is set by querying from the profile handler
ashfame Nov 11, 2022
ef9af82
save hash as the upload name for avatar
ashfame Nov 11, 2022
0714752
bail early if the user already has the same avatar saved
ashfame Nov 11, 2022
ecdfe1a
switch to get_proxied_blacklisted_http_client() for blacklisting loca…
ashfame Nov 14, 2022
465bade
let default for picture be None instead of empty string
ashfame Nov 14, 2022
2f1819c
bail early if synapse is not running media repo in-process
ashfame Nov 14, 2022
006800f
Run linter
Nov 16, 2022
4a78052
grammar fixes in docstring
ashfame Nov 24, 2022
b1b0a91
sha256 hash the image instead of md5 hash
ashfame Nov 24, 2022
b710023
cleaner to destructure get_file() results than use indexing to access
ashfame Nov 24, 2022
f118e8b
rename function to include a verb
ashfame Nov 24, 2022
751855c
make set_avatar() return bool so that its easier to test
ashfame Nov 24, 2022
1ef48dd
don't hardcode size, use len() in tests
ashfame Nov 24, 2022
f815f3b
ensure test doesn't break even if SMALL_PNG is changed in future
ashfame Nov 24, 2022
fe8e1ba
fix grammar
ashfame Nov 24, 2022
a973e75
log info message since out-of-process media repos are not supported
ashfame Nov 24, 2022
7e165f4
add comment in respect to type annotations
ashfame Nov 24, 2022
2da03c2
ensure skipping of avatar updation is tested properly
ashfame Nov 24, 2022
7a29b06
fix grammar
ashfame Nov 24, 2022
4ebb1db
fix grammar
ashfame Nov 24, 2022
104eef8
add missing backticks
ashfame Nov 24, 2022
1e87ac4
ensure before skipping avatar updation we check for server_name to ma…
ashfame Nov 24, 2022
0526ec2
add comment for limited support for picture_claim
ashfame Nov 24, 2022
68eaef7
document return value
ashfame Nov 24, 2022
49937c8
skip assignment
ashfame Nov 24, 2022
5542a0f
return true when skipping image since exact image is already set on t…
ashfame Nov 24, 2022
d00385c
better explanation for server configs
ashfame Nov 24, 2022
a81b0f7
fix grammar
ashfame Nov 24, 2022
9a89e9f
fix tests to actually assert True/False, and not relying on get_succe…
ashfame Nov 24, 2022
00af833
fix formatting in tests
ashfame Nov 24, 2022
1a37d82
fix grammar
ashfame Nov 24, 2022
8746b66
correct comment
ashfame Nov 24, 2022
5ac6092
Merge branch 'develop' into sso_avatar_support
squahtx Nov 24, 2022
81d834b
only load the media repo when its available
ashfame Nov 25, 2022
b32c2f7
define type for media_repo instance attribute
ashfame Nov 25, 2022
a14eea5
no need for explicit type annotation when ternary expression is used
ashfame Nov 25, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13917.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Adds support for handling avatar in SSO login. Contributed by @ashfame.
9 changes: 8 additions & 1 deletion docs/usage/configuration/config_documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -2968,10 +2968,17 @@ 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.
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 monolithic (single-process) server configurations
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
own username (see the documentation for the `sso_auth_account_details.html`
Expand Down
4 changes: 3 additions & 1 deletion mypy.ini
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,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

Expand All @@ -137,7 +140,6 @@ disallow_untyped_defs = False
[mypy-tests.utils]
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:
Expand Down
7 changes: 7 additions & 0 deletions synapse/handlers/oidc.py
Original file line number Diff line number Diff line change
Expand Up @@ -1435,6 +1435,7 @@ class UserAttributeDict(TypedDict):
localpart: Optional[str]
confirm_localpart: bool
display_name: Optional[str]
picture: Optional[str] # may be omitted by older `OidcMappingProviders`
emails: List[str]


Expand Down Expand Up @@ -1520,6 +1521,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]
Expand All @@ -1539,6 +1541,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")
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved

def parse_template_config(option_name: str) -> Optional[Template]:
if option_name not in config:
Expand Down Expand Up @@ -1572,6 +1575,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,
Expand Down Expand Up @@ -1611,10 +1615,13 @@ def render_template_field(template: Optional[Template]) -> Optional[str]:
if email:
emails.append(email)

picture = userinfo.get("picture")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually use the picture_claim, see #14751.


return UserAttributeDict(
localpart=localpart,
display_name=display_name,
emails=emails,
picture=picture,
confirm_localpart=self._config.confirm_localpart,
)

Expand Down
114 changes: 114 additions & 0 deletions synapse/handlers/sso.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
# 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 (
TYPE_CHECKING,
Expand Down Expand Up @@ -53,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__)
Expand Down Expand Up @@ -138,6 +141,7 @@ class UserAttributes:
localpart: Optional[str]
confirm_localpart: bool = False
display_name: Optional[str] = None
picture: Optional[str] = None
emails: Collection[str] = attr.Factory(list)


Expand Down Expand Up @@ -187,6 +191,8 @@ class SsoHandler:
_MAPPING_SESSION_VALIDITY_PERIOD_MS = 15 * 60 * 1000

def __init__(self, hs: "HomeServer"):
self._media_repo: Optional[MediaRepository]

ashfame marked this conversation as resolved.
Show resolved Hide resolved
self._clock = hs.get_clock()
self._store = hs.get_datastores().main
self._server_name = hs.hostname
Expand All @@ -196,6 +202,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._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
# authentication session. It tells the user they can close the window.
Expand Down Expand Up @@ -495,6 +505,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)
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved

await self._auth_handler.complete_sso_login(
user_id,
Expand Down Expand Up @@ -703,8 +715,110 @@ 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) -> bool:
"""Set avatar of the user.

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
the hash of the file in the `upload_name` of the avatar image.

Currently, it only supports server configurations which run the media repository
within the same process.

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.

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`.
"""
ashfame marked this conversation as resolved.
Show resolved Hide resolved
if self._media_repo is None:
logger.info(
"failed to set user avatar because out-of-process media repositories "
"are not supported yet "
)
return False

try:
uid = UserID.from_string(user_id)

def is_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()

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=is_allowed_mime_type,
)

if code != 200:
raise Exception(
"GET request to download sso avatar image returned {}".format(code)
)

# 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.sha256(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:
server_name = profile["avatar_url"].split("/")[-2]
media_id = profile["avatar_url"].split("/")[-1]
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 True

# store it in media repository
avatar_mxc_url = await self._media_repo.create_content(
media_type=headers[b"Content-Type"][0].decode("utf-8"),
upload_name=upload_name,
content=picture,
content_length=content_length,
auth_user=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")
return True
except Exception:
logger.warning("failed to save the user avatar")
return False

async def complete_sso_ui_auth_request(
self,
auth_provider_id: str,
Expand Down
145 changes: 145 additions & 0 deletions tests/handlers/test_sso.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
# 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 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 synapse.api.errors import Codes, SynapseError
from synapse.http.client import RawHeaders
from synapse.server import HomeServer
from synapse.util import Clock

from tests import unittest
from tests.test_utils import SMALL_PNG, FakeResponse


class TestSSOHandler(unittest.HomeserverTestCase):
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
)
return hs

async def test_set_avatar(self) -> None:
"""Tests successfully setting the avatar of a newly created user"""
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))

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
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": 1})
async def test_set_avatar_too_big_image(self) -> None:
"""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
user_id = "@sso-user:test"

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:
"""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
user_id = "@sso-user:test"

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
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))

# 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"))
)

# 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 success
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))
self.assertEqual(profile["avatar_url"], url_to_match)


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]:

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": [str(len(SMALL_PNG))]}
),
body=SMALL_PNG,
)

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,),
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 len(SMALL_PNG), {b"Content-Type": [b"image/png"]}, "", 200