fix(org): match account names and logins case-insensitively - #9064
Open
vbhanuchander-lang wants to merge 1 commit into
Open
fix(org): match account names and logins case-insensitively#9064vbhanuchander-lang wants to merge 1 commit into
vbhanuchander-lang wants to merge 1 commit into
Conversation
Emails were made case-insensitive in apache#9051, but the display name and provider login comparisons in ConnectUserAccountsExact still matched exactly. Provider logins are themselves case-insensitive, and a corporate git config and a provider profile routinely record the same display name with different capitalisation, so an account whose login differs from the users.csv name only in case is silently left unlinked - and every activity that attributes through that account goes missing. Towards apache#8698, where a GitHub account with no public email can only link through these name paths. Extends the org e2e fixtures with an account whose login differs from the user's name only in case; without the change it is not linked and the expected row count drops from 12 to 11.
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.
Summary
ConnectUserAccountsExactlinks a user to an account through three exact comparisons: email,FullName, andUserName. #9051 made the email comparison case-insensitive. The two namecomparisons were left matching exactly, and this makes them consistent with it.
The case that matters most is
UserName, which holds the provider login. Provider logins arethemselves case-insensitive — GitHub resolves
TyroneKCummingsandtyronekcummingsto the sameaccount — so comparing them case-sensitively against the
users.csvname is comparing on adistinction the provider does not make. Display names are folded for the same reason emails were: a
corporate git config and a provider profile routinely record the same name with different
capitalisation.
The consequence of a missed link is silent and easy to misread as missing data: the account is never
associated with the user, so every activity attributing through it disappears from work logs while
other activities for the same person keep appearing.
Does this close any open issues?
Towards #8698 — it does not close it. In that issue the GitHub account has no public email
(GitHub returns an empty string for users who have not made it public), so linkage can only happen
through the two name paths this PR fixes. That removes the case-mismatch failure, but a user whose
CSV name simply differs from their GitHub login is still unlinked. Closing #8698 properly needs a
way to declare the provider login rather than guess it, which is a larger change to the
crossdomain.Usermodel and worth discussing on the issue first —user_account_mapping.csvalready covers the declared case and may be the preferred answer.
Other Information
Tested by extending the existing org e2e fixtures with account
a12, whose loginN7differs fromuser
U007's namen7only in case, and with no email so the match is forced down the login path.With the change it links; reverting the change drops the expected
user_accountsrow count from 12to 11 and the test fails.
gofmtandgo vetclean. No documentation change: this corrects existing matching behaviourrather than adding a knob.