Fix/20260608 exceptions messages - #27
Merged
Merged
Conversation
Renamed AccountManager MSAD methods for clarity and flexibility, now accepting explicit identifier attributes. Enhanced LDAP exception handling and made DataValidationException public. Refactored Searcher for extensibility and updated tests. Bumped version to 10.1.2.
Contributor
Reviewer's GuideThis PR refines LDAP helper exception messages and error handling, introduces a DataValidationException type, refactors some APIs to be identifier-based (DN or sAMAccountName), and updates tests and demos to match the new behavior and messages. Sequence diagram for SetMsADUserAccountPassword with refined error handlingsequenceDiagram
actor Client
participant AccountManager
participant Searcher
participant LDAPServer
participant Authenticator
Client->>AccountManager: SetMsADUserAccountPassword(identifierAttribute, identifierValue, password, requestLabel, postUpdateTestAuthentication)
AccountManager->>AccountManager: verifyMsADEntryAccountAuthenticity(identifierAttribute, identifierValue, true, requestLabel)
AccountManager->>Searcher: SearchEntriesAsync(searchFilterCombiner, RequiredEntryAttributes.Few, requestLabel)
Searcher->>LDAPServer: SearchAsync(SearchLimits, searchFilter, attributesToLoad, false)
LDAPServer-->>Searcher: searchQueue
loop each LDAP message
Searcher->>Searcher: GetEntryFromAttributeSet(attributeSet, requiredEntryAttributes, requestLabel)
end
Searcher-->>AccountManager: LDAPSearchResult
alt [searchResult unsuccessful]
Searcher-->>AccountManager: throw LdapException or Exception
else [no entries]
Searcher-->>AccountManager: throw EntryNotFoundException
else [entry not user]
AccountManager-->>AccountManager: throw DataValidationException
end
alt [entry resolved]
AccountManager->>LDAPServer: ModifyEntryAsync(entry.distinguishedName, modifications)
alt [postUpdateTestAuthentication]
AccountManager->>Authenticator: AuthenticateAsync(LDAPDistinguishedNameCredential, requestLabel)
Authenticator-->>AccountManager: LDAPDistinguishedNameAuthenticationResult
end
AccountManager-->>Client: LDAPPasswordUpdateResult (success)
else [EntryNotFoundException]
AccountManager-->>Client: LDAPPasswordUpdateResult("User account not found.")
else [DataValidationException]
AccountManager-->>Client: LDAPPasswordUpdateResult("Invalid data found.")
else [other Exception]
AccountManager-->>Client: LDAPPasswordUpdateResult("Unexpected error while attempting to replace password.")
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
CreateUserAccountForMsAD, the pre‑existence checks for DN and sAMAccountName use broadcatch (Exception)blocks that silently swallow all failures fromverifyMsADEntryAccountAuthenticity; consider distinguishing the "not found" case from other errors so unexpected LDAP or connectivity issues don’t get ignored and misreported as duplicates. - The new pre‑create duplicate checks for DN/sAMAccountName introduce a race window between the check and
AddEntryAsync; you may want to rely primarily on the directory’s own uniqueness constraint (e.g., by handling the LDAP duplicate error explicitly) or document/adjust the logic so it doesn’t assume the pre‑check guarantees uniqueness. - The new identifier‑based methods and validation (
SetMsADUserAccountPassword,DisableMsADUserAccount,RemoveMsADUserAccount,verifyMsADEntryAccountAuthenticity) mixArgumentException,ArgumentNullExceptionandDataValidationExceptionwith slightly different message styles; aligning on a single exception type and consistent messages for invalid caller input vs domain validation would make error handling more predictable for consumers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CreateUserAccountForMsAD`, the pre‑existence checks for DN and sAMAccountName use broad `catch (Exception)` blocks that silently swallow all failures from `verifyMsADEntryAccountAuthenticity`; consider distinguishing the "not found" case from other errors so unexpected LDAP or connectivity issues don’t get ignored and misreported as duplicates.
- The new pre‑create duplicate checks for DN/sAMAccountName introduce a race window between the check and `AddEntryAsync`; you may want to rely primarily on the directory’s own uniqueness constraint (e.g., by handling the LDAP duplicate error explicitly) or document/adjust the logic so it doesn’t assume the pre‑check guarantees uniqueness.
- The new identifier‑based methods and validation (`SetMsADUserAccountPassword`, `DisableMsADUserAccount`, `RemoveMsADUserAccount`, `verifyMsADEntryAccountAuthenticity`) mix `ArgumentException`, `ArgumentNullException` and `DataValidationException` with slightly different message styles; aligning on a single exception type and consistent messages for invalid caller input vs domain validation would make error handling more predictable for consumers.
## Individual Comments
### Comment 1
<location path="src/Bitai.LDAPHelper/AccountManager.cs" line_range="66" />
<code_context>
+ //Generate username DistinguishedName LDAP attribute
+ InitializeMissingMsADUserAccountDN(newUserAccount);
+
+ LDAPEntry checkUserAccount = null;
+ // Check whether an entry with the same distinguished name already exists in the directory before creating a new user. Since the distinguished name must be unique, a duplicate entry cannot be created. If one already exists, return an error or ask for a different name.
+ try
</code_context>
<issue_to_address>
**issue (bug_risk):** The duplicate-entry pre-checks swallow all exceptions from the verification method, potentially masking real directory errors.
Both duplicate checks wrap `verifyMsADEntryAccountAuthenticity` in a broad `try/catch (Exception)` and then only use `checkUserAccount != null` in `finally`. Any error (e.g. connectivity, LDAP, permission) is treated as “entry not found”, allowing the create to proceed and hiding real directory failures. Catch only the expected `EntryNotFoundException` (and optionally `DataValidationException` if invalid entries should be treated as missing) and let other exceptions propagate or be handled explicitly so genuine errors aren’t suppressed.
</issue_to_address>
### Comment 2
<location path="tests/Bitai.LDAPHelper.Tests/AccountManagerAdapterTests.cs" line_range="70" />
<code_context>
Assert.False(result.IsSuccessfulOperation);
- Assert.Contains("error creating user", result.OperationMessage.ToLower());
+ Assert.StartsWith("unable to create", result.OperationMessage, StringComparison.OrdinalIgnoreCase);
}
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that the error is surfaced as a DataValidationException when required attributes are missing.
Since this path now uses a custom `DataValidationException` for missing required fields, please extend the test to assert that `result.ErrorObject` is a `DataValidationException` and, optionally, that its message mentions the missing property (e.g., `DistinguishedNameOfContainer is required`). This will help prevent regressions and keep validation failures distinguishable from other error types.
Suggested implementation:
```csharp
var result = await accountManager.CreateUserAccountForMsAD(newUser, "TestCreate");
Assert.False(result.IsSuccessfulOperation);
var validationException = Assert.IsType<DataValidationException>(result.ErrorObject);
Assert.Contains("DistinguishedNameOfContainer", validationException.Message);
Assert.StartsWith("unable to create", result.OperationMessage, StringComparison.OrdinalIgnoreCase);
}
```
1. Ensure the correct namespace for `DataValidationException` is imported at the top of `AccountManagerAdapterTests.cs`, for example:
`using Bitai.LDAPHelper.Exceptions;` (or the actual namespace where `DataValidationException` is defined).
2. If a different property name is required in the validation message (instead of `DistinguishedNameOfContainer`), update the `Assert.Contains` string accordingly to match the real exception message.
</issue_to_address>
### Comment 3
<location path="tests/Bitai.LDAPHelper.Tests/AccountManagerAdapterTests.cs" line_range="99" />
<code_context>
- var userCredential = new LDAPDistinguishedNameCredential(mockUserEntry.DistinguishedName, "NewP@ssw0rd");
-
- var result = await accountManager.SetUserAccountPasswordForMsAD(userCredential, "TestPassword", postUpdateTestAuthentication: true);
+ var result = await accountManager.SetMsADUserAccountPassword(EntryAttribute.distinguishedName, mockUserEntry.DistinguishedName, "TestPassword", postUpdateTestAuthentication: true);
// Assert
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests covering the sAMAccountName-based path of SetMsADUserAccountPassword/DisableMsADUserAccount/RemoveMsADUserAccount.
The updated APIs support both `EntryAttribute.distinguishedName` and `EntryAttribute.sAMAccountName`, but the tests only cover the DN path. Please add tests that:
- Call `SetMsADUserAccountPassword(EntryAttribute.sAMAccountName, ...)` and verify password update (and successful authentication when `postUpdateTestAuthentication` is true).
- Call `DisableMsADUserAccount(EntryAttribute.sAMAccountName, ...)` and verify the account is disabled.
- Call `RemoveMsADUserAccount(EntryAttribute.sAMAccountName, ...)` and verify the account is removed.
This will exercise the sAMAccountName path and confirm both identifier types behave consistently.
Suggested implementation:
```csharp
Assert.False(result.IsSuccessfulOperation);
Assert.StartsWith("user account not found", result.OperationMessage, StringComparison.OrdinalIgnoreCase);
}
[Fact]
public async Task SetMsADUserAccountPassword_UsingSamAccountName_UpdatesPassword_AndAuthenticates()
{
// Arrange
var mockConnection = new MockLdapConnectionAdapter();
var accountManager = CreateAccountManagerAdapter(mockConnection);
var mockUserEntry = CreateMockUserEntry();
var userSearchFilter = BuildUserSearchFilter(mockUserEntry);
mockConnection.AddSearchResult(
userSearchFilter.ToString(),
new List<MockLdapEntryAdapter> { mockUserEntry });
// Act
var result = await accountManager.SetMsADUserAccountPassword(
EntryAttribute.sAMAccountName,
mockUserEntry.SamAccountName,
"NewP@ssw0rd",
postUpdateTestAuthentication: true);
// Assert
Assert.True(result.IsSuccessfulOperation);
}
[Fact]
public async Task DisableMsADUserAccount_UsingSamAccountName_DisablesAccount()
{
// Arrange
var mockConnection = new MockLdapConnectionAdapter();
var accountManager = CreateAccountManagerAdapter(mockConnection);
var mockUserEntry = CreateMockUserEntry();
var userSearchFilter = BuildUserSearchFilter(mockUserEntry);
mockConnection.AddSearchResult(
userSearchFilter.ToString(),
new List<MockLdapEntryAdapter> { mockUserEntry });
// Act
var result = await accountManager.DisableMsADUserAccount(
EntryAttribute.sAMAccountName,
mockUserEntry.SamAccountName);
// Assert
Assert.True(result.IsSuccessfulOperation);
}
[Fact]
public async Task RemoveMsADUserAccount_UsingSamAccountName_RemovesAccount()
{
// Arrange
var mockConnection = new MockLdapConnectionAdapter();
var accountManager = CreateAccountManagerAdapter(mockConnection);
var mockUserEntry = CreateMockUserEntry();
var userSearchFilter = BuildUserSearchFilter(mockUserEntry);
mockConnection.AddSearchResult(
userSearchFilter.ToString(),
new List<MockLdapEntryAdapter> { mockUserEntry });
// Act
var result = await accountManager.RemoveMsADUserAccount(
EntryAttribute.sAMAccountName,
mockUserEntry.SamAccountName);
// Assert
Assert.True(result.IsSuccessfulOperation);
}
```
Because only a small part of the file is visible, you will likely need to:
1. **Align helper usage**
- If `CreateAccountManagerAdapter`, `CreateMockUserEntry`, or `BuildUserSearchFilter` do not exist, replace those calls in the new tests with the same explicit arrange code used in the existing DN-based tests (e.g., direct construction of `MockLdapConnectionAdapter`, `AccountManagerAdapter`, user entries, and filters).
- If you already have common factory/helper methods for the DN tests, reuse those instead of creating duplicate logic.
2. **Confirm property names**
- Ensure the user object exposes the correct sAMAccountName property. If your mock entry uses a different name (e.g., `SAMAccountName`, `sAMAccountName`, or another property), update `mockUserEntry.SamAccountName` in all three tests accordingly.
3. **Ensure `EntryAttribute.sAMAccountName` enum value exists**
- If the enum member is named slightly differently (e.g., `EntryAttribute.sAMAccountName` vs `EntryAttribute.SamAccountName`), adjust the new tests to use the exact member name.
4. **Optional stronger assertions**
- If your existing DN-based tests assert more than just `IsSuccessfulOperation` (for example, verifying disable flags or delete/search absence), mirror those assertions in the new sAMAccountName tests so behavior is validated consistently across identifier types.
</issue_to_address>
### Comment 4
<location path="tests/Bitai.LDAPHelper.Tests/AccountManagerAdapterTests.cs" line_range="137" />
<code_context>
Assert.False(result.IsSuccessfulOperation);
- Assert.Contains("does not exist", result.OperationMessage.ToLower());
+ Assert.StartsWith("user account not found", result.OperationMessage, StringComparison.OrdinalIgnoreCase);
}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting the specific ErrorObject type (EntryNotFoundException) in not-found scenarios.
In these not-found tests, you now validate the updated operation message. To better lock in the behavior, also assert that `result.ErrorObject` is an `EntryNotFoundException`, so callers can reliably distinguish missing accounts from other failures and ensure the exception-to-result mapping remains correct.
Suggested implementation:
```csharp
Assert.False(result.IsSuccessfulOperation);
Assert.StartsWith("user account not found", result.OperationMessage, StringComparison.OrdinalIgnoreCase);
Assert.IsType<EntryNotFoundException>(result.ErrorObject);
}
```
If `EntryNotFoundException` is not already in scope in this test file, add the appropriate `using` directive at the top of `AccountManagerAdapterTests.cs`, for example:
- `using Bitai.LDAPHelper.Exceptions;`
or the correct namespace where `EntryNotFoundException` is defined.
</issue_to_address>
### Comment 5
<location path="tests/Bitai.LDAPHelper.Tests/SearcherAdapterTests.cs" line_range="114-116" />
<code_context>
var result = await accountManager.CreateUserAccountForMsAD(newUser, "TestCreate");
Assert.False(result.IsSuccessfulOperation);
- Assert.Contains("error creating user", result.OperationMessage.ToLower());
+ Assert.StartsWith("unable to create", result.OperationMessage, StringComparison.OrdinalIgnoreCase);
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that SearchParentEntriesAsync surfaces an EntryNotFoundException in ErrorObject for the empty case.
Since `SearchParentEntriesAsync` wraps `EntryNotFoundException` into `LDAPSearchResult`, please also assert that `result.HasErrorObject` is `true` and that `result.ErrorObject` is an `EntryNotFoundException`. This will exercise the error mapping and protect callers that rely on `ErrorObject` to detect this condition.
Suggested implementation:
```csharp
var result = await searcher.SearchParentEntriesAsync(expectedDummiestUserSearchFilter, RequiredEntryAttributes.Minimun, "TestRequest");
Assert.False(result.IsSuccessfulOperation);
Assert.Null(result.Entries);
Assert.StartsWith("nonexistent entry", result.OperationMessage, StringComparison.OrdinalIgnoreCase);
Assert.True(result.HasErrorObject);
Assert.IsType<EntryNotFoundException>(result.ErrorObject);
```
If `EntryNotFoundException` is not in the current namespace, add the appropriate `using` (for example, `using Bitai.LDAPHelper;`) or fully qualify the type (for example, `Assert.IsType<Bitai.LDAPHelper.EntryNotFoundException>(result.ErrorObject);`) to match where `EntryNotFoundException` is actually defined in your codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| Assert.False(result.IsSuccessfulOperation); | ||
| Assert.Contains("error creating user", result.OperationMessage.ToLower()); | ||
| Assert.StartsWith("unable to create", result.OperationMessage, StringComparison.OrdinalIgnoreCase); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Also assert that the error is surfaced as a DataValidationException when required attributes are missing.
Since this path now uses a custom DataValidationException for missing required fields, please extend the test to assert that result.ErrorObject is a DataValidationException and, optionally, that its message mentions the missing property (e.g., DistinguishedNameOfContainer is required). This will help prevent regressions and keep validation failures distinguishable from other error types.
Suggested implementation:
var result = await accountManager.CreateUserAccountForMsAD(newUser, "TestCreate");
Assert.False(result.IsSuccessfulOperation);
var validationException = Assert.IsType<DataValidationException>(result.ErrorObject);
Assert.Contains("DistinguishedNameOfContainer", validationException.Message);
Assert.StartsWith("unable to create", result.OperationMessage, StringComparison.OrdinalIgnoreCase);
}- Ensure the correct namespace for
DataValidationExceptionis imported at the top ofAccountManagerAdapterTests.cs, for example:
using Bitai.LDAPHelper.Exceptions;(or the actual namespace whereDataValidationExceptionis defined). - If a different property name is required in the validation message (instead of
DistinguishedNameOfContainer), update theAssert.Containsstring accordingly to match the real exception message.
|
|
||
| Assert.False(result.IsSuccessfulOperation); | ||
| Assert.Contains("does not exist", result.OperationMessage.ToLower()); | ||
| Assert.StartsWith("user account not found", result.OperationMessage, StringComparison.OrdinalIgnoreCase); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider asserting the specific ErrorObject type (EntryNotFoundException) in not-found scenarios.
In these not-found tests, you now validate the updated operation message. To better lock in the behavior, also assert that result.ErrorObject is an EntryNotFoundException, so callers can reliably distinguish missing accounts from other failures and ensure the exception-to-result mapping remains correct.
Suggested implementation:
Assert.False(result.IsSuccessfulOperation);
Assert.StartsWith("user account not found", result.OperationMessage, StringComparison.OrdinalIgnoreCase);
Assert.IsType<EntryNotFoundException>(result.ErrorObject);
}If EntryNotFoundException is not already in scope in this test file, add the appropriate using directive at the top of AccountManagerAdapterTests.cs, for example:
using Bitai.LDAPHelper.Exceptions;
or the correct namespace whereEntryNotFoundExceptionis defined.
Comment on lines
114
to
+116
| Assert.False(result.IsSuccessfulOperation); | ||
| Assert.Empty(result.Entries); | ||
| Assert.Contains("no one entry was found", result.OperationMessage.ToLower()); | ||
| Assert.Null(result.Entries); | ||
| Assert.StartsWith("nonexistent entry", result.OperationMessage, StringComparison.OrdinalIgnoreCase); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Also assert that SearchParentEntriesAsync surfaces an EntryNotFoundException in ErrorObject for the empty case.
Since SearchParentEntriesAsync wraps EntryNotFoundException into LDAPSearchResult, please also assert that result.HasErrorObject is true and that result.ErrorObject is an EntryNotFoundException. This will exercise the error mapping and protect callers that rely on ErrorObject to detect this condition.
Suggested implementation:
var result = await searcher.SearchParentEntriesAsync(expectedDummiestUserSearchFilter, RequiredEntryAttributes.Minimun, "TestRequest");
Assert.False(result.IsSuccessfulOperation);
Assert.Null(result.Entries);
Assert.StartsWith("nonexistent entry", result.OperationMessage, StringComparison.OrdinalIgnoreCase);
Assert.True(result.HasErrorObject);
Assert.IsType<EntryNotFoundException>(result.ErrorObject);If EntryNotFoundException is not in the current namespace, add the appropriate using (for example, using Bitai.LDAPHelper;) or fully qualify the type (for example, Assert.IsType<Bitai.LDAPHelper.EntryNotFoundException>(result.ErrorObject);) to match where EntryNotFoundException is actually defined in your codebase.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix library messages.
Summary by Sourcery
Improve LDAP helper error handling and messaging for search and account management operations, while introducing clearer validation and duplicate checks.
Bug Fixes:
Enhancements: