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 optional OIDC avatar fetching from the picture claim #5429

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

Conversation

rubentalstra
Copy link

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

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