Skip to content

Add optional OIDC avatar fetching from the picture claim #5429

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 24, 2025
Merged

Add optional OIDC avatar fetching from the picture claim #5429

merged 1 commit into from
May 24, 2025

Conversation

rubentalstra
Copy link

@rubentalstra rubentalstra commented Jan 20, 2025 •
edited
Loading

This update enables BookStack to optionally fetch user avatars from the OIDC picture claim. The implementation:

  1. Introduces a fetch_avatars config flag in config/oidc.php to toggle avatar retrieval.
  2. Uses UserAvatars->assignToUserFromUrl($user, $picture, $accessToken) to support both public and private (Bearer token-protected) endpoints.
  3. Fetches the picture claim from the user’s ID token or userinfo response, if provided.
  4. Works with various OIDC providers (Google, Okta, Keycloak, Azure, etc.), provided they include or allow retrieving a valid picture URL.
  5. Includes SSRF protection notes, advising users to only enable this if they trust the domains in the picture field.

This approach does not break existing behavior; avatar fetching is off by default. If enabled, BookStack will try to update a user’s avatar upon login, using the token to authenticate if necessary.

@rubentalstra rubentalstra changed the title Add optional OIDC avatar fetching from the “picture” claim Add optional OIDC avatar fetching from the picture claim Jan 20, 2025
@rubentalstra rubentalstra mentioned this pull request Jan 20, 2025
Closed
@ssddanbrown ssddanbrown added this to the Next Feature Release milestone Feb 12, 2025
@ssddanbrown
Copy link
Member

Thanks for providing this @rubentalstra.

Is the authorization header in the picture URL request based upon any actual need or standard/specification?
I'd be wary of sending the access token out, especially to other domains, but would prefer not to send at all if there's no need.

Also, I wouldn't look to have this run on each login, just the first registration/sync as per all existing details in support SSO options. We'll also need to add testing to cover the added functionality. I'm happy to make these changes/additions before merge, but I really just need to know about the authorization point above.

@rubentalstra
Copy link
Author

@ssddanbrown hi, thank you for taking the time. The header is for sure needed if you use Microsoft because it will call a global endpoint. And based on the header it will return the users profile.

@ssddanbrown
Copy link
Member

Thanks for this @rubentalstra.

I can confirm via testing.
Really not sure I want to go outside the spec just to suit Microsoft though.
Also, the origin between picture url and issuer is different so we can't put this behind a basic origin check.
Could have extra config options but that's getting dumb for a minor side features of an optional auth option.

I'm siding towards not sending any auth tokens, leaving any non-spec services like Entra/AzureAD use our logical theme hooks to work around their awkardness.

Another consideration for this PR: it's currently saving images as png, which probably will be the most common, but this is not assured; The spec does not confirm exact formats.

ssddanbrown added a commit that referenced this pull request May 24, 2025
@ssddanbrown
OIDC: Updated picture fetch implementation during review
f9dbbe5 
Review of #5429
@ssddanbrown ssddanbrown mentioned this pull request May 24, 2025
Merged
ssddanbrown added a commit that referenced this pull request May 24, 2025
@ssddanbrown
Merge pull request #5626 from BookStackApp/rubentalstra-development
d149b80 
Review of #5429, OIDC avatar fetching
@ssddanbrown ssddanbrown merged commit 05f7f4c into BookStackApp:development May 24, 2025
1 check failed
@ssddanbrown
Copy link
Member

This has now been merged for the next release, thanks again @rubentalstra.
I made some changes as could be seen via #5626. These included the removal of sending auth tokens as per my last comment.

@rubentalstra
Copy link
Author

This has now been merged for the next release, thanks again @rubentalstra.

I made some changes as could be seen via #5626. These included the removal of sending auth tokens as per my last comment.

@ssddanbrown thank you for reviewing my PR. I'm happy to see that it was not for nothing 😉

@rubentalstra rubentalstra mentioned this pull request May 24, 2025
Open
@rubentalstra
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
v25.05
Development

Successfully merging this pull request may close these issues.

OIDC: Support picture claim for use as user avatar
2 participants
@rubentalstra @ssddanbrown