feat: optionally download the profile picture with the access token - #180
Open
heudev wants to merge 1 commit into
Open
feat: optionally download the profile picture with the access token#180heudev wants to merge 1 commit into
heudev wants to merge 1 commit into
Conversation
…s token Some providers serve the picture claim from an endpoint that requires the access token, so storing the URL leaves users with an avatar that resolves to 401. Microsoft Graph's /me/photo/$value is one such case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an opt-in "Download the picture using the access token instead of storing its URL" option next to the existing "Picture" sync toggle.
Why
updateProfilestores thepictureclaim verbatim as the user's avatar URL. That works when the provider returns a publicly reachable image, but some return an API endpoint that is only readable with the access token. Microsoft's OIDC userinfo endpoint returns:NodeBB stores that string, the browser then requests it anonymously, Microsoft answers
401, and the user is left with a broken avatar that silently falls back to the letter icon. Nothing in the logs indicates a problem, because from the plugin's point of view the sync succeeded.What changed
getUserProfilenow carries the access token on the profile object. It was already available as an argument and simply discarded.syncPictureViaTokenfetches the picture with anAuthorization: Bearerheader and hands the bytes touser.uploadCroppedPicture, so the avatar ends up hosted by the forum instead of pointing at a URL that only worked during the login request.pictureis dropped from the fields passed touser.updateProfile, otherwise the raw URL would immediately overwrite the freshly uploaded avatar.The option is off by default and gated behind the existing
syncPicturetoggle, so no existing strategy changes behaviour.Two deliberate limits:
verbose, an exception atwarn, and the login proceeds. A missing profile picture should never block authentication.Testing
Verified on a live NodeBB 4.14.10 forum against a Microsoft strategy with
User.Readin scope — note that the OIDC scopes alone are not enough to read/me/photo/$value. Before:picturewas stored as the Graph URL and the avatar did not render. After: the image is fetched and stored under/assets/uploads/profile/uid-<uid>/…, and bothpictureanduploadedpicturepoint at it. Subsequent logins leave it untouched.npx eslint .is clean.