fix(auth): canonical username is the attribute value the user matched (Auth-009)
ExtractUserName returned the first value of a possibly multi-valued UserNameAttribute (default cn), so jsmith could sign in as "John Smith" depending on server ordering. It now prefers the value equal to the typed name ignoring case (still canonicalising case) and falls back to the first value only when none matches. (scadaproj#46) Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
This commit is contained in:
@@ -541,19 +541,29 @@ public sealed class LdapAuthService : ILdapAuthService
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Returns the first non-blank value of the configured username attribute — the directory's
|
||||
/// canonical spelling — falling back to the (already normalised) typed username.
|
||||
/// Returns the directory's spelling of the name the user matched on: the value of the configured
|
||||
/// username attribute that equals the typed (already normalised) name ignoring case. Only when no
|
||||
/// value does is the first non-blank value used, and only when there is none the typed name.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The attribute can be multi-valued — the default <c>cn</c> is, and OpenLDAP / 389-DS entries
|
||||
/// routinely carry both <c>cn: John Smith</c> and <c>cn: jsmith</c>. 0.4.1 returned whichever
|
||||
/// value the server listed first, so <c>jsmith</c> could sign in as <c>John Smith</c> depending on
|
||||
/// attribute ordering (Auth-009). Matching ordinal-ignore-case still canonicalises case
|
||||
/// (<c>JSMITH</c> → <c>jsmith</c>), which is what 0.4.1 set out to do.
|
||||
/// </remarks>
|
||||
private string ExtractUserName(LdapSearchEntry entry, string username)
|
||||
{
|
||||
if (entry.Attributes.TryGetValue(_options.UserNameAttribute, out var values)
|
||||
&& values.Count > 0
|
||||
&& !string.IsNullOrWhiteSpace(values[0]))
|
||||
if (!entry.Attributes.TryGetValue(_options.UserNameAttribute, out var values) || values.Count == 0)
|
||||
return username;
|
||||
|
||||
foreach (var value in values)
|
||||
{
|
||||
return values[0];
|
||||
if (string.Equals(value, username, StringComparison.OrdinalIgnoreCase))
|
||||
return value;
|
||||
}
|
||||
|
||||
return username;
|
||||
return string.IsNullOrWhiteSpace(values[0]) ? username : values[0];
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
||||
@@ -174,6 +174,54 @@ public class LdapAuthServiceTests
|
||||
Assert.Equal("ALICE", r.Username);
|
||||
}
|
||||
|
||||
// ---- Auth-009 (scadaproj#46): a multi-valued UserNameAttribute ----------------------------
|
||||
|
||||
[Fact]
|
||||
public async Task Username_FromAMultiValuedAttribute_IsTheValueTheUserMatched()
|
||||
{
|
||||
// OpenLDAP / 389-DS entries routinely carry both cn values; the server's ordering decides
|
||||
// which comes first. The search matched "jsmith", so that is the identity, not "John Smith".
|
||||
var fake = new FakeLdapConnection().WithUserEntry(
|
||||
"cn=jsmith,dc=x", new[] { "cn=g,dc=x" }, "John",
|
||||
new Dictionary<string, string[]> { ["cn"] = new[] { "John Smith", "jsmith" } });
|
||||
var svc = new LdapAuthService(Opts(), new FakeLdapConnectionFactory(fake));
|
||||
|
||||
var r = await svc.AuthenticateAsync("jsmith", "pw", default);
|
||||
|
||||
Assert.True(r.Succeeded);
|
||||
Assert.Equal("jsmith", r.Username);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Username_FromAMultiValuedAttribute_StillTakesTheDirectorysCase()
|
||||
{
|
||||
var fake = new FakeLdapConnection().WithUserEntry(
|
||||
"cn=jsmith,dc=x", new[] { "cn=g,dc=x" }, "John",
|
||||
new Dictionary<string, string[]> { ["cn"] = new[] { "John Smith", "JSmith" } });
|
||||
var svc = new LdapAuthService(Opts(), new FakeLdapConnectionFactory(fake));
|
||||
|
||||
var r = await svc.AuthenticateAsync("jsmith", "pw", default);
|
||||
|
||||
Assert.True(r.Succeeded);
|
||||
Assert.Equal("JSmith", r.Username);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Username_FallsBackToTheFirstValue_WhenNoValueMatchesTheTypedName()
|
||||
{
|
||||
// The directory matched on something the attribute values do not spell the same way
|
||||
// (a matching rule beyond case, say): keep 0.4.1's first-value behaviour.
|
||||
var fake = new FakeLdapConnection().WithUserEntry(
|
||||
"cn=jsmith,dc=x", new[] { "cn=g,dc=x" }, "John",
|
||||
new Dictionary<string, string[]> { ["cn"] = new[] { "J. Smith", "John Smith" } });
|
||||
var svc = new LdapAuthService(Opts(), new FakeLdapConnectionFactory(fake));
|
||||
|
||||
var r = await svc.AuthenticateAsync("jsmith", "pw", default);
|
||||
|
||||
Assert.True(r.Succeeded);
|
||||
Assert.Equal("J. Smith", r.Username);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData("")]
|
||||
[InlineData(null)]
|
||||
|
||||
@@ -10,7 +10,7 @@
|
||||
| Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) |
|
||||
| Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) |
|
||||
| Commit reviewed | `91fb2b4` |
|
||||
| Open findings | 3 |
|
||||
| Open findings | 2 |
|
||||
|
||||
## Summary
|
||||
|
||||
@@ -429,7 +429,7 @@ I verified this live against a SQL Server 2022 container:
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs:437` |
|
||||
|
||||
**Description**
|
||||
@@ -454,6 +454,20 @@ fake connection: typed `jsmith` returned `Username == "John Smith"`.
|
||||
Prefer the attribute value that equals the typed (normalised) name under `OrdinalIgnoreCase`, which still
|
||||
canonicalises case. Fall back to `values[0]` only when no value matches. Add a multi-valued-attribute test.
|
||||
|
||||
**Resolution**
|
||||
|
||||
Resolved 2026-11-09 (scadaproj#46, Auth `0.4.4`, unpublished). Re-verified through the fake connection: with
|
||||
`cn: ["John Smith", "jsmith"]`, typed `jsmith` returned `Username == "John Smith"`. `ExtractUserName` now returns the
|
||||
attribute value that equals the typed (normalised) name under `OrdinalIgnoreCase`. That keeps 0.4.1's case
|
||||
canonicalisation, so typed `jsmith` against `JSmith` still returns `JSmith`. Only when no value matches does it fall
|
||||
back to the first non-blank value, and then to the typed name, as 0.4.1 did. Both bind modes go through
|
||||
`SucceedFromEntry`, so both are covered.
|
||||
|
||||
Tests (`LdapAuthServiceTests`):
|
||||
- `Username_FromAMultiValuedAttribute_IsTheValueTheUserMatched` fails pre-fix.
|
||||
- `..._StillTakesTheDirectorysCase` fails pre-fix.
|
||||
- `Username_FallsBackToTheFirstValue_WhenNoValueMatchesTheTypedName` is a guard that passes both ways.
|
||||
|
||||
### Auth-010 — `ApiKeyVerifier` lets store exceptions escape despite its "only exception path is cancellation" contract
|
||||
|
||||
| | |
|
||||
|
||||
Reference in New Issue
Block a user