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 2 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
10 changes: 10 additions & 0 deletions synapse/handlers/oidc.py
Original file line number Diff line number Diff line change
Expand Up @@ -1116,6 +1116,7 @@ class UserAttributeDict(TypedDict):
localpart: Optional[str]
confirm_localpart: bool
display_name: Optional[str]
picture: Optional[str]
ashfame marked this conversation as resolved.
Show resolved Hide resolved
emails: List[str]


Expand Down Expand Up @@ -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]
Expand All @@ -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")
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 @@ -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,
Expand Down Expand Up @@ -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 = ""
ashfame marked this conversation as resolved.
Show resolved Hide resolved

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

Expand Down
45 changes: 45 additions & 0 deletions synapse/handlers/sso.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -29,6 +30,7 @@
from urllib.parse import urlencode

import attr
import requests
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
from typing_extensions import NoReturn, Protocol

from twisted.web.iweb import IRequest
Expand Down Expand Up @@ -137,6 +139,7 @@ class UserAttributes:
localpart: Optional[str]
confirm_localpart: bool = False
display_name: Optional[str] = None
picture: str = ""
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
emails: Collection[str] = attr.Factory(list)


Expand Down Expand Up @@ -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()
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved

# 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 @@ -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:
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
try:
uid = UserID.from_string(user_id)

# download picture
http_response = requests.get(picture_https_url)
ashfame marked this conversation as resolved.
Show resolved Hide resolved
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(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the API instead of calling synapse internal code here, since the media repository can be outside of synapse, cf https://github.com/turt2live/matrix-media-repo.

Copy link
Contributor

@MatMaul MatMaul Sep 28, 2022

Choose a reason for hiding this comment

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

Following a talk on #synapse-dev, some more details:

  • we should use the public API and public facing endpoint, no internal routing should be needed
  • we need to authenticate the POST /upload. For that I think we can inject a temporary access token in synapse, do the upload with that, and then delete this temporary token.

Copy link
Member

Choose a reason for hiding this comment

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

we should use the public API and public facing endpoint, no internal routing should be needed

Hrm, tricky. On the one hand, we'll have to use the public API for integration with matrix-media-repo. On the other hand:

  • This isn't really consistent with our architecture, and I don't think it will work reliably: in some deployments it may require connection hairpinning which may not work. Having most inter-worker traffic go via internal APIs, and some go via public APIs, is unpleasant, architecturally speaking.
  • Using the public API means we need to implement authentication kludges like the one you suggest.

I think I agree that using the public API is the least bad, but as @dklimpel says, please make sure this new requirement is clearly documented.

(Also, if the media repo is in-process, we should go direct.)

Whatever happens, please put it in a separate "add this media to the media repo" function, rather than embedding it all in the SSO handler.

Copy link
Member

Choose a reason for hiding this comment

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

For that I think we can inject a temporary access token in synapse, do the upload with that, and then delete this temporary token.

Ugh. That sounds like a great way for us to end up with unused access tokens.

I think I agree that using the public API is the least bad

No, I'm changing my mind here. We need a hybrid approach:

  • If the media repo is in-process, do it directly
  • If the media repo is a separate synapse worker, use an internal HTTP replication API (like we have with event persisters)
  • If the media repo is a separate matrix-media-repo, use its configured api with a configured shared secret

We'll need to carefully consider what the configuration settings look like to make sure it is consistent with the other settings, and isn't limited to the SSO usecase.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe for now we just do the "in-process" solution (ie, not support it on worker-based deployments) and then extend it to the other mechanisms in later PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@richvdh Would you say this "in-process" solution is equivalent to how its currently being done via self.media_repo.create_content() call? I am not sure what should I try to do here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you say this "in-process" solution is equivalent to how its currently being done via self.media_repo.create_content() call?

Yes. We need to:

  • document that this isn't supported in worker deployments, or with external media repos,
  • ideally not call set_avatar in the first place if we are not on the main process, or if we're using an external media repo.

The first bullet is the most important. I'm not completely sure how to do the second bullet off the top of my head.

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)
ashfame marked this conversation as resolved.
Show resolved Hide resolved
except Exception as e:
logger.info("failed to save the user avatar via SSO: %s", user_id)
logger.exception(e)
ashfame marked this conversation as resolved.
Show resolved Hide resolved

async def complete_sso_ui_auth_request(
self,
auth_provider_id: str,
Expand Down