From 9ec0da1b2ca48441cfabc6a96d016e8260db84be Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 15:18:25 -0500 Subject: [PATCH] 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 --- .../ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs | 24 +++++++--- .../LdapAuthServiceTests.cs | 48 +++++++++++++++++++ code-reviews/Auth/findings.md | 18 ++++++- 3 files changed, 81 insertions(+), 9 deletions(-) diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs index 7643a0d..35f9e17 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs @@ -541,19 +541,29 @@ public sealed class LdapAuthService : ILdapAuthService } /// - /// 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. /// + /// + /// The attribute can be multi-valued — the default cn is, and OpenLDAP / 389-DS entries + /// routinely carry both cn: John Smith and cn: jsmith. 0.4.1 returned whichever + /// value the server listed first, so jsmith could sign in as John Smith depending on + /// attribute ordering (Auth-009). Matching ordinal-ignore-case still canonicalises case + /// (JSMITH → jsmith), which is what 0.4.1 set out to do. + /// 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]; } /// diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceTests.cs index 91d54d4..1286f16 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceTests.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceTests.cs @@ -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 { ["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 { ["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 { ["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)] diff --git a/code-reviews/Auth/findings.md b/code-reviews/Auth/findings.md index 4e8e846..17a31a7 100644 --- a/code-reviews/Auth/findings.md +++ b/code-reviews/Auth/findings.md @@ -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 | | |