Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for OIDC picture #5177

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

jasonpincin
Copy link

This is a naive first attempt at adding support for OIDC pictures. It works (for me) which is about all I can say about it.

Tossing it up there in case it's useful, or for feedback. Happy to develop the PR further if there's broader interest in the feature. If I were to develop it further, I'd be curious if there's thoughts on better ways of plumbing the picture path through, and if there's any mechanism for refreshing the user's avatar that would need to be updated. It wasn't immediately evident to me that this existed.

@jasonpincin
Copy link
Author

This relates to a feature request captured here: #4271

@ssddanbrown
Copy link
Member

Thanks for offering this @jasonpincin, and sorry for my delayed response. The code here looks good, but there's a few things I think we'd need to consider, or some changes to the code to avoid complexity.

High Level Review

  • Fetching resources from an external domain can be dangerous (Open to server-side-request exploitation) so this needs to be done in a considered approach, rather than being default enabled. I thought about maybe being able to define "trusted origins" for this, but that might be over complicating scope. Might just be best to have an extra simple boolean config option to whether avatars are fetched from OIDC or not. Then we can add commentary to that option in documentation regarding the risks (eg. Only enable if picture URLs would be considered trusted).

Code Review

  • A lot of methods here are gaining added complexity just to carry across the $picture through the whole default registration process. Instead I think it would be better if this can the be left as-is, and and extra assignToUserFromUrl method be added to the UserAvatars class, much like this method which is what we use for LDAP avatar images. The UserAvatars class can then be injected into the OidcService and used directly.
  • As with all added back-end features, we need to ensure it's covered with testing. There's some guidance for this here, with existing OIDC tests here, but this can get complex with all the OIDC and external HTTP call mocking required so don't stress with this if it's not familiar or difficult, I can add that after if needed.

Would you be happy to take changes following the above? Otherwise I can continue this if needed so don't stress on it. Just let me know if anything is unclear or if you need extra guidance on anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants