diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/LdapUserName.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/LdapUserName.cs index 8c500a6..0d03e8a 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/LdapUserName.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/LdapUserName.cs @@ -2,8 +2,11 @@ namespace ZB.MOM.WW.Auth.Ldap.Internal; /// /// Reduces a typed login to the bare account name used by : -/// one spelling flows into the bind template, the search filter and the returned username, so audit -/// rows never carry DOMAIN\jdoe for one sign-in and jdoe for the next. +/// the qualifier is stripped before the name reaches the bind template and the search filter, so audit +/// rows never carry DOMAIN\jdoe for one sign-in and jdoe for the next. Case is NOT +/// normalised here: the returned username is canonicalised from the directory entry's +/// UserNameAttribute value, so JDoe and jdoe also yield one identity; this name is +/// only the fallback when the entry has no such value. /// internal static class LdapUserName { 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 676cbd6..927edd8 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 @@ -145,16 +145,19 @@ public sealed class LdapAuthService : ILdapAuthService // filter and the returned result (avoids two identities for one person). username = username.Trim(); - // 3b. User-bind mode guards, before any network call. An empty password must never reach - // the directory: AD answers a simple bind with an empty password as an UNAUTHENTICATED - // bind and reports success, which would admit anyone. The name is reduced to the bare - // account name so the bind template, the search filter and the result share one spelling. - if (_options.BindMode == LdapBindMode.User) + // 3b. An empty password must never reach the directory, in EITHER bind mode: AD answers a + // simple bind with an empty password as an UNAUTHENTICATED bind and reports success — + // the user bind (or the ServiceAccount-mode re-bind as the user's DN) would admit anyone. + // Refused here, before any network call, so no consumer depends on its own edge check. + if (string.IsNullOrEmpty(password)) + return LdapAuthResult.Fail(LdapAuthFailure.BadCredentials); + + // 3c. User-bind mode: reduce the name to the bare account name the bind template and the + // search filter share. (The returned username is canonicalised from the entry later.) + if (_options.BindMode == LdapBindMode.User + && !LdapUserName.TryNormalize(username, _options.AcceptDomainQualifiedNames, out username)) { - if (string.IsNullOrEmpty(password)) - return LdapAuthResult.Fail(LdapAuthFailure.BadCredentials); - if (!LdapUserName.TryNormalize(username, _options.AcceptDomainQualifiedNames, out username)) - return LdapAuthResult.Fail(LdapAuthFailure.BadCredentials); + return LdapAuthResult.Fail(LdapAuthFailure.BadCredentials); } // 4. Walk the endpoint list, preferred one first. A single-endpoint configuration @@ -278,7 +281,8 @@ public sealed class LdapAuthService : ILdapAuthService private LdapAuthResult CompleteWithServiceAccount(ZB.MOM.WW.Auth.Ldap.Internal.ILdapConnection conn, string username, string password) { // 6. Service-account bind so we can search for the user's DN. A bind failure here is a - // service-account misconfiguration — DISTINCT from a user-credential failure. + // service-account misconfiguration — DISTINCT from a user-credential failure — so it + // maps to ServiceAccountBindFailed. try { conn.Bind(_options.ServiceAccountDn, _options.ServiceAccountPassword); @@ -288,7 +292,9 @@ public sealed class LdapAuthService : ILdapAuthService throw new StageFailure(LdapAuthFailure.ServiceAccountBindFailed, "service-bind"); } - // 7–8. Exactly one match. Zero -> UserNotFound; two or more -> AmbiguousUser. + // 7–8. Require exactly one match. Zero -> UserNotFound; two or more -> AmbiguousUser. + // Both are authoritative answers from a healthy directory, so they end the walk. + // We never attempt a user bind against an ambiguous DN. var entries = SearchUser(conn, username); if (entries.Count == 0) return LdapAuthResult.Fail(LdapAuthFailure.UserNotFound); @@ -297,7 +303,9 @@ public sealed class LdapAuthService : ILdapAuthService var entry = entries[0]; - // 9. User bind verifies the password; a failure is the user's bad credentials and final. + // 9. User bind: re-bind as the resolved DN to verify the password. A bind failure here is + // the end user's bad credentials — the directory answered, so this is returned directly + // and never retried against another endpoint. try { conn.Bind(entry.Dn, password); @@ -344,6 +352,10 @@ public sealed class LdapAuthService : ILdapAuthService /// Step 7: search for the user entry; a search fault is system-side. private IReadOnlyList SearchUser(ZB.MOM.WW.Auth.Ldap.Internal.ILdapConnection conn, string username) { + // A search failure is infrastructure (directory unreachable / unhealthy) — system-side. + // NOTE: the LdapAuthFailure enum has no DirectoryUnavailable value, so a failed search maps + // to ServiceAccountBindFailed, the closest system-side bucket (same limitation as + // Connect), until a DirectoryUnavailable value exists. try { var filter = $"({_options.UserNameAttribute}={LdapEscaping.Filter(username)})"; @@ -355,7 +367,12 @@ public sealed class LdapAuthService : ILdapAuthService } } - /// Steps 10–11: fail-closed group extraction, then success. + /// + /// Steps 10–11: fail-closed group extraction, then success. The returned username is the + /// entry's own value (the directory's spelling), so + /// JDoe and jdoe sign in as one identity; the typed (normalised) name is the + /// fallback when the entry carries no such value. + /// private LdapAuthResult SucceedFromEntry(LdapSearchEntry entry, string username) { IReadOnlyList groups; @@ -371,7 +388,8 @@ public sealed class LdapAuthService : ILdapAuthService if (groups.Count == 0) return LdapAuthResult.Fail(LdapAuthFailure.GroupLookupFailed); - return LdapAuthResult.Success(username, ExtractDisplayName(entry, username), groups); + var canonical = ExtractUserName(entry, username); + return LdapAuthResult.Success(canonical, ExtractDisplayName(entry, canonical), groups); } /// @@ -393,16 +411,37 @@ public sealed class LdapAuthService : ILdapAuthService } /// - /// Builds the distinct attribute list requested from the directory. The display-name - /// and group attributes are de-duplicated so we never request the same attribute twice - /// when an operator configures them to the same value. + /// Builds the distinct attribute list requested from the directory: display name, groups and + /// the username attribute (read back as the canonical username). They are de-duplicated, + /// case-insensitively, so we never request the same attribute twice when an operator + /// configures two of them to the same value. /// private IReadOnlyList BuildSearchAttributes() { - if (string.Equals(_options.DisplayNameAttribute, _options.GroupAttribute, StringComparison.OrdinalIgnoreCase)) - return new[] { _options.DisplayNameAttribute }; + var attributes = new List(3); + foreach (var name in new[] { _options.DisplayNameAttribute, _options.GroupAttribute, _options.UserNameAttribute }) + { + if (!attributes.Contains(name, StringComparer.OrdinalIgnoreCase)) + attributes.Add(name); + } - return new[] { _options.DisplayNameAttribute, _options.GroupAttribute }; + return attributes; + } + + /// + /// Returns the first non-blank value of the configured username attribute — the directory's + /// canonical spelling — falling back to the (already normalised) typed username. + /// + private string ExtractUserName(LdapSearchEntry entry, string username) + { + if (entry.Attributes.TryGetValue(_options.UserNameAttribute, out var values) + && values.Count > 0 + && !string.IsNullOrWhiteSpace(values[0])) + { + return values[0]; + } + + return username; } /// diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs index ddb3e23..5075200 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs @@ -34,6 +34,9 @@ internal sealed class FakeLdapConnection : ILdapConnection /// public int BindAttempts { get; private set; } + /// The attribute list passed to the most recent call (the fake still returns every scripted attribute). + public IReadOnlyList? SearchAttributes { get; private set; } + /// Every call in order: connect, bind:<dn>, search:<filter>. public List Calls { get; } = new(); @@ -41,9 +44,14 @@ internal sealed class FakeLdapConnection : ILdapConnection /// /// Scripts a user entry that will be returned by the next call. - /// Builds a minimal attribute bag with memberOf and optional displayName. + /// Builds a minimal attribute bag with memberOf, optional displayName and any + /// (e.g. sAMAccountName) the test needs. /// - public FakeLdapConnection WithUserEntry(string dn, string[] memberOf, string? displayName = null) + public FakeLdapConnection WithUserEntry( + string dn, + string[] memberOf, + string? displayName = null, + IReadOnlyDictionary? extraAttributes = null) { var attrs = new Dictionary>(StringComparer.OrdinalIgnoreCase) { @@ -51,6 +59,11 @@ internal sealed class FakeLdapConnection : ILdapConnection }; if (displayName is not null) attrs["displayName"] = new[] { displayName }; + if (extraAttributes is not null) + { + foreach (var (name, values) in extraAttributes) + attrs[name] = values; + } _scriptedEntries.Add(new LdapSearchEntry(dn, attrs)); return this; @@ -177,6 +190,7 @@ internal sealed class FakeLdapConnection : ILdapConnection IReadOnlyList attributes) { Calls.Add($"search:{filter}"); + SearchAttributes = attributes; if (_throwOnSearch) throw new Novell.Directory.Ldap.LdapException( "Search failed", Novell.Directory.Ldap.LdapException.Unavailable, searchBase); 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 44ff334..91d54d4 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 @@ -147,4 +147,45 @@ public class LdapAuthServiceTests Assert.True(r.Succeeded); Assert.Equal(new[] { "Eng,ineers", "A,B" }, r.Groups); } + + [Fact] + public async Task Username_IsCanonicalisedFromTheEntry_InServiceAccountMode() + { + var fake = new FakeLdapConnection().WithUserEntry( + "cn=alice,dc=x", new[] { "cn=g,dc=x" }, "Alice", + new Dictionary { ["cn"] = new[] { "alice" } }); + var svc = new LdapAuthService(Opts(), new FakeLdapConnectionFactory(fake)); + + var r = await svc.AuthenticateAsync("ALICE", "pw", default); + + Assert.True(r.Succeeded); + Assert.Equal("alice", r.Username); + } + + [Fact] + public async Task Username_FallsBackToTheTypedName_InServiceAccountMode_WhenAttributeAbsent() + { + var fake = new FakeLdapConnection().WithUserEntry("cn=alice,dc=x", new[] { "cn=g,dc=x" }, "Alice"); + var svc = new LdapAuthService(Opts(), new FakeLdapConnectionFactory(fake)); + + var r = await svc.AuthenticateAsync("ALICE", "pw", default); + + Assert.True(r.Succeeded); + Assert.Equal("ALICE", r.Username); + } + + [Theory] + [InlineData("")] + [InlineData(null)] + public async Task EmptyPassword_IsRefused_InServiceAccountMode_WithoutContactingTheDirectory(string? password) + { + var factory = new SequenceLdapConnectionFactory( + new FakeLdapConnection().WithUserEntry("cn=alice,dc=x", new[] { "cn=g,dc=x" }, "Alice")); + var svc = new LdapAuthService(Opts(), factory); + + var r = await svc.AuthenticateAsync("alice", password!, default); + + Assert.Equal(LdapAuthFailure.BadCredentials, r.Failure); + Assert.Empty(factory.Created); + } } diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceUserBindTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceUserBindTests.cs index 1276c1a..7350867 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceUserBindTests.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceUserBindTests.cs @@ -178,4 +178,45 @@ public class LdapAuthServiceUserBindTests Assert.Equal(LdapAuthFailure.GroupLookupFailed, result.Failure); } + + [Fact] + public async Task Username_IsCanonicalisedFromTheEntry_NotTheTypedCase() + { + var factory = new SequenceLdapConnectionFactory(new FakeLdapConnection().WithUserEntry( + "CN=Jane Doe,OU=u,DC=x", new[] { "CN=UG_A,OU=g,DC=x" }, "Jane Doe", + new Dictionary { ["sAMAccountName"] = new[] { "jdoe" } })); + var result = await Login(new LdapAuthService(Opts(), factory), @"NAM\JDOE"); + + Assert.True(result.Succeeded); + Assert.Equal("jdoe", result.Username); + } + + [Fact] + public async Task Username_FallsBackToTheTypedName_WhenTheEntryLacksTheAttribute() + { + var factory = new SequenceLdapConnectionFactory(Jane()); + var result = await Login(new LdapAuthService(Opts(), factory), "JDOE"); + + Assert.True(result.Succeeded); + Assert.Equal("JDOE", result.Username); + } + + [Fact] + public async Task Search_RequestsTheUserNameAttribute() + { + var factory = new SequenceLdapConnectionFactory(Jane()); + await Login(new LdapAuthService(Opts(), factory), "jdoe"); + + Assert.Equal(new[] { "displayName", "memberOf", "sAMAccountName" }, factory.Created[0].SearchAttributes); + } + + [Fact] + public async Task Search_DeDuplicatesTheUserNameAttribute_CaseInsensitively() + { + var factory = new SequenceLdapConnectionFactory(Jane()); + var svc = new LdapAuthService(Opts() with { DisplayNameAttribute = "SAMACCOUNTNAME" }, factory); + await Login(svc, "jdoe"); + + Assert.Equal(new[] { "SAMACCOUNTNAME", "memberOf" }, factory.Created[0].SearchAttributes); + } }