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 | | |