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

Support importing avatars from SSO identity provider #9357

Open
shaun-blake opened this issue Feb 9, 2021 · 11 comments · Fixed by #13917
Open

Support importing avatars from SSO identity provider #9357

shaun-blake opened this issue Feb 9, 2021 · 11 comments · Fixed by #13917
Labels
A-Social Login Login via external identity providers A-SSO Single Sign-On (maybe OIDC) T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.

Comments

@shaun-blake
Copy link

It would be nice to (optionally?) transfer the account picture/avatar of a user created through OpenID Connect (OIDC) to the Synapse user account. I'm using google as my provider. Their account pictures is in the picture attribute and is a url to the image.

@richvdh
Copy link
Member

richvdh commented Feb 9, 2021

Indeed it would. I thought we had an issue open to track this, but I can't find it :)

@richvdh richvdh changed the title Use OpenID Connect (OIDC) photo Support importing avatars from SSO identity provider Feb 9, 2021
@erikjohnston erikjohnston added the T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. label Feb 9, 2021
@richvdh richvdh added the A-Social Login Login via external identity providers label Feb 11, 2021
@richvdh richvdh removed their assignment Mar 29, 2021
@azmeuk
Copy link

azmeuk commented Aug 4, 2022

It looks like this would only be about calling set_avatar_url from complete_sso_login_request, and map the picture claim in JinjaOidcMappingProvider. Have I missed something?

@clokep
Copy link
Member

clokep commented Aug 4, 2022

It looks like this would only be about calling set_avatar_url from complete_sso_login_request, and map the picture claim in JinjaOidcMappingProvider. Have I missed something?

I think that's most of it, I believe avatar URLs need to be mxc URLs though so you might need to store the image first.

squahtx pushed a commit that referenced this issue Nov 25, 2022
This commit adds support for handling a provided avatar picture URL
when logging in via SSO.

Signed-off-by: Ashish Kumar <ashfame@users.noreply.github.com>

Fixes #9357.
@ashfame
Copy link
Contributor

ashfame commented Nov 25, 2022

@squahtx We should probably re-open this issue until we cover all server configurations where media repository is outside of the main process.

I understand when media repository is outside such as matrix-media-repo, this would happen using client server API (unsure whose credentials get used for those requests) and for cases when its in worker mode, I know nothing about that at all, so can use some guidance there.

@squahtx squahtx reopened this Nov 25, 2022
@squahtx
Copy link
Contributor

squahtx commented Nov 25, 2022

Thanks for pointing that out.

We still need

  • support for worker mode, when the media repository is running on another worker
  • support for out-of-Synapse media repositories, such as matrix-media-repo

As per Rich's comment here:

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

For examples of worker mode internal HTTP APIs, you can look for classes derived from ReplicationEndpoint. It may be necessary to define a new endpoint.

@clokep
Copy link
Member

clokep commented Nov 28, 2022

Also note that #13917 only does this for OIDC, not for SAML or CAS. Those could probably be filed as separate issues though if someone wants it.

@ashfame
Copy link
Contributor

ashfame commented Nov 29, 2022

If the media repo is a separate matrix-media-repo, use its configured api with a configured shared secret

@squahtx @richvdh I can only see the registration_shared_secret in Synapse nothing related to a shared secret meant for media. What would this secret be on Synapse side?

Based on previous discussions, it seems like using this secret we avoided the need to temporarily have an access token for uploading the image for the user. Found out that the media admin api, doesn't support uploading images. So, I am guessing its supposed to work more like register_new_matrix_user.py script? But would the images still be associated with the user? I believe, that's desirable for moderation standpoint, at least. I do see the capability of matrix-media-repo to be able to query current user by whoami endpoint but I am not certain how you are envisioning it to come together. Would appreciate an explanation :)

Lastly, would I need matrix-media-repo configured locally and running in order to work towards this or just relying on media upload client-server api is sufficient?

@squahtx
Copy link
Contributor

squahtx commented Nov 29, 2022

I can only see the registration_shared_secret in Synapse nothing related to a shared secret meant for media. What would this secret be on Synapse side?

I believe the proposal is to add a new config option that will hold the access token to use on the regular POST /_matrix/media/v3/upload endpoint. Note that if using a matrix-media-repo shared secret, the images would be associated with a dummy @sharedsecret user. If we want the image to be associated with the user logging in, then we have to use their access token to upload the image. I'm not sure whether we have one yet at the time of avatar upload.

It's best to test with a real instance of matrix-media-repo.

@ashfame
Copy link
Contributor

ashfame commented Nov 30, 2022

Ok, so the idea is to provide an access token for a user like @sharedsso in a new config option using which all avatars for SSO logins will be uploaded.

Are we sure that we don't want the actual user to be associated with the image upload? I think that's desirable and seems like a cleaner approach too?

Overall, this feels like a band-aid solution where we are interacting with our own public API. There should be a function which abstracts this mechanism away, ideally without a call to our own public API, by directly invoking the endpoint handler or the underlying code for that handler. Thoughts?

@ashfame
Copy link
Contributor

ashfame commented Dec 12, 2022

@squahtx Any thoughts on what I said above?

@squahtx
Copy link
Contributor

squahtx commented Dec 13, 2022

Ok, so the idea is to provide an access token for a user like @sharedsso in a new config option using which all avatars for SSO logins will be uploaded.

Yes, but only for the external matrix-media-repo case.

Are we sure that we don't want the actual user to be associated with the image upload? I think that's desirable and seems like a cleaner approach too?

If you had the user's access token, you could pass it to matrix-media-repo. The difficulty is that the user's access token has not been generated yet at the time set_avatar is called.

The above only applies to an external media repository, like matrix-media-repo. When the media repository is in another Synapse worker, you are free to define your own internal HTTP API to upload images as a given user.

H-Shay pushed a commit that referenced this issue Dec 13, 2022
This commit adds support for handling a provided avatar picture URL
when logging in via SSO.

Signed-off-by: Ashish Kumar <ashfame@users.noreply.github.com>

Fixes #9357.
@MadLittleMods MadLittleMods added the A-SSO Single Sign-On (maybe OIDC) label Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Social Login Login via external identity providers A-SSO Single Sign-On (maybe OIDC) T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants