Identity: external login ProviderKey equality is delegated to the database collation
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 276
Description
## Summary
`Microsoft.AspNetCore.Identity.EntityFrameworkCore` resolves an external login by comparing the stored `ProviderKey` with the supplied one using an EF Core `==` that is translated to a bare SQL `=`. The equivalence relation for that comparison is therefore whatever the column collation defines, and nothing in managed code verifies that the row returned actually matches the requested key ordinally. `ProviderKey` is an opaque, externally supplied identifier, so its equality should not depend on the database collation.
## What is wrong
* **Broken invariant:** for an opaque external identifier, `FindByLoginAsync(provider, key)` should return a user only when the stored `(LoginProvider, ProviderKey)` is *ordinally* equal to the arguments. Today it returns whatever row the database considers equal.
* **Where it originates:** the EF store expresses the lookup as a LINQ predicate (`userLogin.LoginProvider == loginProvider && userLogin.ProviderKey == providerKey`). EF Core translates simple equality to the database's native `=` and, as documented, makes no attempt to force a case-sensitive comparison. The Identity EF model does not configure a collation on these columns, so they inherit the database default.
* **Consequences of the inherited relation:**
* Case-insensitive collations are the default on SQL Server, Azure SQL Database and MySQL. On SQL Server LocalDB the instance collation is `SQL_Latin1_General_CP1_CI_AS` and cannot be changed.
* MySQL 8's default `utf8mb4_0900_ai_ci` is additionally accent-insensitive, widening the equivalence class further.
* On SQL Server, `=` treats trailing spaces as insignificant regardless of collation, including binary collations.
* **No post-query verification** exists on any of the three `FindByLoginAsync` implementations.
## Why it matters (defense in depth)
* **Correctness, independent of any adversary.** `ProviderKey` carries the OpenID Connect `sub`. OIDC Core defines `sub` as a case-sensitive string and requires claim comparisons to be performed as Unicode code-point equality. Delegating that comparison to a general-text collation is not conformant.
* **`ProviderKey` is the only Identity lookup column that is not pre-normalized.** `NormalizedUserName` and `NormalizedEmail` are passed through `ILookupNormalizer` (upper-invariant) before both storage and lookup, so a case-insensitive collation is effectively a no-op for them. `ProviderKey` is stored verbatim, which makes the collation load-bearing for its semantics alone.
* **Store implementations are not interchangeable.** The in-memory reference store keys its dictionary on the provider and key with ordinal semantics, while the EF store defers to the database. The same application can therefore resolve logins differently depending on which store is configured, and no existing test would catch the divergence.
* **Behavior varies by engine and by operator configuration** for a value the framework treats as opaque. Hardening this narrows the surface where an external identity assertion could be matched to an unintended local account.
## Affected code
* `src/Identity/Extensions.Stores/src/UserStoreBase.cs:426-437` - `FindByLoginAsync` returns `FindUserAsync(userLogin.UserId, ...)` with no verification of the returned row's `LoginProvider`/`ProviderKey`.
* `src/Identity/EntityFrameworkCore/src/UserOnlyStore.cs:472-483` - `FindByLoginAsync` **override**; duplicates the base logic, so a base-only fix would not apply here.
* `src/Identity/EntityFrameworkCore/src/UserStore.cs:589-600` - same override.
* `src/Identity/EntityFrameworkCore/src/UserOnlyStore.cs:323-326` - `FindUserLoginAsync(loginProvider, providerKey, ct)`; the database-translated predicate.
* `src/Identity/EntityFrameworkCore/src/UserStore.cs:350-353` - same predicate.
* `src/Identity/EntityFrameworkCore/src/UserOnlyStore.cs:311-314` and `src/Identity/EntityFrameworkCore/src/UserStore.cs:338-341` - the three-argument overload used by `RemoveLoginAsync`; same collation dependence.
* `src/Identity/EntityFrameworkCore/src/IdentityUserContext.cs:245-256`, `:355-366`, `:450-461` - `IdentityUserLogin` model configuration. `HasKey(l => new { l.LoginProvider, l.ProviderKey })` and `HasMaxLength(maxKeyLength)` only; no collation is configured. A repository-wide search finds no `UseCollation` or `Collate` usage anywhere in `src/Identity`.
## Recommended fix
**Selected approach.** Add a managed ordinal verification after the database filter, so the returned row is accepted only when `string.Equals(stored, requested, StringComparison.Ordinal)` holds for both `LoginProvider` and `ProviderKey`; otherwise return `null`. Apply it to all three `FindByLoginAsync` implementations listed above - the two EF overrides are functionally identical to the base apart from `ConfigureAwait(false)`, so an alternative is to delete them and fix `UserStoreBase` alone. Use the static `string.Equals` overload for null safety. This is provider-agnostic, requires no schema change, keeps the existing index-sargable query as the filter, and also closes the trailing-whitespace case that a collation change does not. Consider whether the three-argument `FindUserLoginAsync` path used by `RemoveLoginAsync` warrants the same treatment for consistency.
**Alternatives considered.**
1. *Configure a binary collation on the `LoginProvider`/`ProviderKey` columns* (`UseCollation("Latin1_General_BIN2")` or equivalent). Rejected as the primary fix: it requires a schema migration for every existing deployment, is not uniformly expressible across providers (SQLite supports only `BINARY`/`NOCASE`/`RTRIM`, and the collation names differ per engine), does not address SQL Server's trailing-space insignificance, and `EF.Functions.Collate` applied in a predicate disables index usage. Reasonable as *operator guidance*, not as the framework fix.
2. *Normalize `ProviderKey` on write*, as is done for `NormalizedUserName`. Rejected: `ProviderKey` must round-trip verbatim; normalizing it would be lossy and a breaking data change.
3. *Verify in `UserManager.FindByLoginAsync`*. Rejected: `UserManager` receives only the resolved `TUser` and has no visibility into the stored key, so the check is only expressible in the store.
**Compatibility, migration, versioning.**
* This is a behavior change. On a case-insensitive collation, a lookup that previously succeeded for a key differing only by case (or, on SQL Server, by trailing whitespace) will now return `null`. The realistic affected population is deployments where the external provider changed the casing of a returned key over time, or where an application stored a case-folded key.
* No public API change. All modified members are existing `virtual`/`override` methods.
* Suggest targeting `main` only. Emit a distinct `ILogger` event on the reject path so operators can detect and diagnose the change. If this is ever considered for servicing, gate it behind an `AppContext` switch.
* Third-party `IUserStore` implementations that do not derive from `UserStoreBase` are unaffected and should be called out in release notes as still owning this behavior.
* A repository-wide search found no existing tests covering `ProviderKey` casing, so this is not locked by current coverage.
## Acceptance criteria
* [ ] `FindByLoginAsync` returns `null` when the stored `LoginProvider` or `ProviderKey` is not ordinally equal to the supplied value, regardless of database collation or provider.
* [ ] The three `FindByLoginAsync` implementations behave identically (or the redundant EF overrides are removed).
* [ ] Test: against a case-insensitive collation, a key differing only by case does not resolve a user.
* [ ] Test: on SQL Server, a key differing only by trailing whitespace does not resolve a user.
* [ ] Test: exact-match lookups, `AddLoginAsync`, `RemoveLoginAsync` and `GetLoginsAsync` continue to behave as before.
* [ ] Test: the EF store and the in-memory reference store agree on lookup semantics for the same inputs.
* [ ] Release notes document the behavior change and the recommendation for third-party store implementations.
Contributor guide
Research direction
Start with FindByLoginAsync in src/Identity/Extensions.Stores/src/UserStoreBase.cs and its overrides in src/Identity/EntityFrameworkCore/src/UserOnlyStore.cs and UserStore.cs, then inspect the FindUserLoginAsync predicates and IdentityUserContext model configuration. Run the existing Identity store tests and add coverage for ordinal mismatches, exact matches, trailing whitespace, and agreement with the in-memory store; done also includes the requested release-note update.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- authentication, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100