add support for handling avatar with SSO login

[ashfame]

This PR adds support for handling avatar picture url provided when logging in via SSO.

Happy to address any feedback given so that we can land this change.

Signed-off-by: Ashish Kumar [email protected]

fixes #9357

[MatMaul]

Thanks for your contribution!

[MatMaul]

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.

[MatMaul]

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.

[richvdh]

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.

[richvdh]

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.

[richvdh]

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?

[ashfame]

@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.

[DMRobertson]

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.

[richvdh]

a few more drive-by comments

[DMRobertson]

I have left some thoughts.

We would like to see some tests for this and documentation updates. (Have you been able to test this manually?)

[DMRobertson]

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.

[ashfame]

@DMRobertson Thank you for your detailed feedback! I have addressed almost all of them. Let me know if they meet your expectations. I am happy to make further changes to land this.

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.

I am not sure where to even begin for that. Could you give me some pointers?

Have you been able to test this manually?

Yes! This works with a custom OIDC provider I configured and is able to set avatars correctly when supplied in user claims.

Tests

I will work on that and get back to you.

[DMRobertson]

For cross-referencing, there is some additional discussion of tests here.

[squahtx]

Thanks for your patience and for updating the PR. It looks good to merge now.

changes made

[squahtx]

I just realised why CI is failing. We're erroring out in the worker configuration:

  frontend_proxy1 | 2022-11-24 20:32:23,784 - synapse.app._base - 257 - CRITICAL - sentinel - Error during startup
  Traceback (most recent call last):
    File "/usr/local/lib/python3.9/site-packages/synapse/app/_base.py", line 242, in wrapper
      await cb(*args, **kwargs)
    File "/usr/local/lib/python3.9/site-packages/synapse/app/_base.py", line 543, in start
      hs.start_listening()
    File "/usr/local/lib/python3.9/site-packages/synapse/app/generic_worker.py", line 305, in start_listening
      self._listen_http(listener)
    File "/usr/local/lib/python3.9/site-packages/synapse/app/generic_worker.py", line 207, in _listen_http
      login.register_servlets(self, resource)
    File "/usr/local/lib/python3.9/site-packages/synapse/rest/client/login.py", line 668, in register_servlets
      LoginRestServlet(hs).register(http_server)
    File "/usr/local/lib/python3.9/site-packages/synapse/rest/client/login.py", line 114, in __init__
      self._sso_handler = hs.get_sso_handler()
    File "/usr/local/lib/python3.9/site-packages/synapse/server.py", line 181, in _get
      dep = builder(self)
    File "/usr/local/lib/python3.9/site-packages/synapse/server.py", line 492, in get_sso_handler
      return SsoHandler(self)
    File "/usr/local/lib/python3.9/site-packages/synapse/handlers/sso.py", line 203, in __init__
      self._media_repo = hs.get_media_repository()
    File "/usr/local/lib/python3.9/site-packages/synapse/server.py", line 181, in _get
      dep = builder(self)
    File "/usr/local/lib/python3.9/site-packages/synapse/server.py", line 627, in get_media_repository
      return MediaRepository(self)
    File "/usr/local/lib/python3.9/site-packages/synapse/rest/media/v1/media_repository.py", line 85, in __init__
      self.max_upload_size = hs.config.media.max_upload_size
  AttributeError: 'ContentRepositoryConfig' object has no attribute 'max_upload_size'
  Error during startup:
  Traceback (most recent call last):
    File "/usr/local/lib/python3.9/site-packages/synapse/app/_base.py", line 242, in wrapper
      await cb(*args, **kwargs)
    File "/usr/local/lib/python3.9/site-packages/synapse/app/_base.py", line 543, in start
      hs.start_listening()
    File "/usr/local/lib/python3.9/site-packages/synapse/app/generic_worker.py", line 305, in start_listening
      self._listen_http(listener)
    File "/usr/local/lib/python3.9/site-packages/synapse/app/generic_worker.py", line 207, in _listen_http
      login.register_servlets(self, resource)
    File "/usr/local/lib/python3.9/site-packages/synapse/rest/client/login.py", line 668, in register_servlets
      LoginRestServlet(hs).register(http_server)
    File "/usr/local/lib/python3.9/site-packages/synapse/rest/client/login.py", line 114, in __init__
      self._sso_handler = hs.get_sso_handler()
    File "/usr/local/lib/python3.9/site-packages/synapse/server.py", line 181, in _get
      dep = builder(self)
    File "/usr/local/lib/python3.9/site-packages/synapse/server.py", line 492, in get_sso_handler
      return SsoHandler(self)
    File "/usr/local/lib/python3.9/site-packages/synapse/handlers/sso.py", line 203, in __init__
      self._media_repo = hs.get_media_repository()
    File "/usr/local/lib/python3.9/site-packages/synapse/server.py", line 181, in _get
      dep = builder(self)
    File "/usr/local/lib/python3.9/site-packages/synapse/server.py", line 627, in get_media_repository
      return MediaRepository(self)
    File "/usr/local/lib/python3.9/site-packages/synapse/rest/media/v1/media_repository.py", line 85, in __init__
      self.max_upload_size = hs.config.media.max_upload_size
  AttributeError: 'ContentRepositoryConfig' object has no attribute 'max_upload_size'

[ashfame]

AttributeError: 'ContentRepositoryConfig' object has no attribute 'max_upload_size'

But it does have it -

synapse/synapse/config/repository.py

Line 137 in 39cde58

self.max_upload_size = self.parse_size(config.get("max_upload_size", "50M"))

[squahtx]

can_load_media_repo is False on that worker, so the config is never populated.
We can try this:

def __init__:

-         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

def set_avatar:

- if not self._can_load_media_repo:
+ if self._media_repo is None:

[squahtx]

Seems mypy didn't like that. We have to give self._media_repo an explicit type annotation.
eg. self._media_repo: Optional[MediaRepository].

[ashfame]

@squahtx Finally good to go! :)

[squahtx]

Thanks for taking the time to make CI pass!

[clokep]

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