fix: provider claim mapping (Microsoft, GitHub) and inert usernameViaEmail option - #178
Open
heudev wants to merge 3 commits into
Open
fix: provider claim mapping (Microsoft, GitHub) and inert usernameViaEmail option#178heudev wants to merge 3 commits into
heudev wants to merge 3 commits into
Conversation
Microsoft's OIDC userinfo endpoint returns givenname and familyname rather than the standard given_name and family_name, leaving fullname empty for every user signing in through Microsoft.
editStrategy normalises every checkbox to 1 or 0 before saving, so the comparison against 'on' never matched and the option had no effect.
GitHub's /user response carries the account name in login and the avatar in avatar_url, so displayName fell through to the full name and no picture was ever synced.
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.
Three independent bugs found while wiring Microsoft (Entra ID / personal MSA) and GitHub up to this plugin. They are unrelated to each other, so they are separate commits.
1. Microsoft's
givenname/familynameclaims are ignoredhttps://graph.microsoft.com/oidc/userinforeturns the given/family name without underscores, and omitsname,nicknameandpreferred_usernameentirely. A real response:{ "sub": "AAAAAAAAAAAAAAAAAAAAAPxIiNbOEB2t7uk7_u06hig", "givenname": "Enes", "familyname": "Uysal", "email": "…@hotmail.com", "locale": "tr-TR", "picture": "https://graph.microsoft.com/v1.0/me/photo/$value" }parseUserReturnonly destructuresgiven_name/family_name, socombinedFullNameis empty and every Microsoft user ends up with a blankfullnameeven with "Sync fullname" enabled.Because
displayNameisnickname || preferred_username || name, it is alsoundefinedfor these accounts — which makesloadStrategiesreject the login withinsufficient-scope, a fairly misleading message given that the scopes are fine and the email is present. The fallback that exists for exactly this case is theusernameViaEmailoption, which brings us to the second bug.2.
usernameViaEmailnever takes effectControllers.editStrategynormalises every checkbox before persisting it:So the stored value is always
1or0, and the ACP template checks./usernameViaEmail == "1"accordingly — butparseUserReturncompares against the raw form value:which cannot match anything
editStrategywrites. The option is inert for anyone configuring it through the ACP. Every sibling setting is read withparseInt(…, 10); this one now matches.forceUsernameViaEmailis given the same treatment for consistency — it currently works only because0happens to be falsy as a number.3. GitHub's
loginandavatar_urlclaims are unmappedhttps://api.github.com/usernames those two fields differently from the OIDC conventions the plugin expects:{ "login": "heudev", "id": 74737994, "name": "Enes Uysal", "avatar_url": "https://avatars.githubusercontent.com/u/74737994" }loginis the account's actual username, but with nonicknameorpreferred_usernamepresentdisplayNamefell through toname— creating forum accounts literally calledEnes Uysal, spaces and all, instead ofheudev. And because the avatar is underavatar_urlrather thanpicture, "Sync picture" had nothing to read and no avatar was ever stored.loginis inserted ahead ofnamein thedisplayNamechain andavatar_urlis used as a fallback forpicture. Note this changes the username assigned to newly registered GitHub users; existing accounts keep the username they were created with.Testing
Verified against a live NodeBB 4.14.10 forum with both a Microsoft strategy (
userRoute=https://graph.microsoft.com/oidc/userinfo) and a GitHub one (https://api.github.com/user). Before: Microsoft logins failed withinsufficient-scope; GitHub logins produced a space-separated username and no avatar. After: accounts are created with sensible usernames,fullnameis populated from the provider's claims, and the GitHub avatar syncs.npx eslint .is clean.