Update/20260722 record dtos - #36
Merged
Merged
Conversation
Contributor
Reviewer's GuideThe PR migrates the AD user DTO to an immutable record, makes account creation require an explicit distinguished name, adds reusable parent-entry search and clearer error messages, and comprehensively realigns the mock LDAP dataset and tests with the Bitai AD namespace and Name/containerDN attribute model. Sequence diagram for explicit AD account creationsequenceDiagram
participant Caller
participant AccountManager
participant Searcher
participant LDAP
Caller->>AccountManager: CreateUserAccountForMsAD(userAccount)
AccountManager->>AccountManager: validate DistinguishedNameOfContainer
AccountManager->>AccountManager: validate DistinguishedName
AccountManager->>Searcher: SearchEntriesAsync(distinguishedNameFilter)
Searcher->>LDAP: SearchAsync
LDAP-->>Searcher: matching entries
Searcher-->>AccountManager: search result
AccountManager->>LDAP: AddEntryAsync(DistinguishedName)
LDAP-->>AccountManager: creation result
AccountManager-->>Caller: LDAPCreateMsADUserAccountResult
Sequence diagram for reusable parent-entry searchsequenceDiagram
participant Caller
participant Searcher
participant LDAP
Caller->>Searcher: SearchParentEntriesAsync(searchFilter, requiredEntryAttributes)
Searcher->>Searcher: SearchEntriesAsync(searchFilter, OnlyMemberOf)
Searcher->>Searcher: SelectAllMemberOfEntriesRecursively()
loop each parent entry
Searcher->>Searcher: SearchEntriesAsync(distinguishedNameFilter, requiredEntryAttributes)
Searcher->>LDAP: SearchAsync
LDAP-->>Searcher: parent entry
end
Searcher-->>Caller: LDAPSearchResult
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 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="adapters/Bitai.LDAPHelper.LdapAdapters.LdapHelperMock/MockLdapPersistentConnectionAdapter.cs" line_range="193-197" />
<code_context>
return MatchWildcard(entry.DistinguishedName, value);
}
- else if (filter.Contains("(cn="))
+ else if (filter.Contains("(Name="))
{
- var value = ExtractFilterValue(filter, "cn");
- var cn = entry.GetAttributeSet().GetAttribute("cn")?.StringValue;
+ var value = ExtractFilterValue(filter, "Name");
+ var cn = entry.GetAttributeSet().GetAttribute("Name")?.StringValue;
return MatchWildcard(cn, value);
}
else if (filter.Contains("(objectSid="))
</code_context>
<issue_to_address>
**issue (bug_risk):** The mock now handles CN filters only when the filter contains `(Name=...)` and then looks up the case-sensitive `Name` key, but seeded users and groups still add `cn` and lowercase `name`; those entries therefore return no match for name searches in the persistent mock.
**Triggers:** When the persistent mock is used to search seeded users or groups by the normal LDAP `cn` attribute.
**Suggested fix:** Support `(cn=...)` and read the actual `cn` attribute, or normalize mock attribute names case-insensitively and consistently seed the same attribute.
</issue_to_address>
### Comment 2
<location path="src/Bitai.LDAPHelper.DTO/LDAPMsADUserAccount.cs" line_range="32-33" />
<code_context>
+ /// <param name="distinguishedNameOfContainer">Container distinguished name of user account.</param>
+ public LDAPMsADUserAccount(string distinguishedNameOfContainer) : this()
+ {
+ DistinguishedNameOfContainer = distinguishedNameOfContainer
+ ?? throw new ArgumentNullException(nameof(distinguishedNameOfContainer));
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The single-argument constructor accepts an empty `distinguishedNameOfContainer`, whereas the previous constructor rejected both null and empty values; callers can now construct an invalid account that later fails during account creation.
**Triggers:** When a caller passes `string.Empty` as the container distinguished name.
**Suggested fix:** Use `string.IsNullOrEmpty(distinguishedNameOfContainer)` before throwing, preserving the previous contract.
```suggestion
DistinguishedNameOfContainer = string.IsNullOrEmpty(distinguishedNameOfContainer)
? throw new ArgumentNullException(nameof(distinguishedNameOfContainer))
: distinguishedNameOfContainer;
```
</issue_to_address>
### Comment 3
<location path="src/Bitai.LDAPHelper/AccountManager.cs" line_range="57-58" />
<code_context>
if (string.IsNullOrEmpty(newUserAccount.DistinguishedNameOfContainer))
throw new DataValidationException($"{nameof(newUserAccount.DistinguishedNameOfContainer)} is required.");
+ if (string.IsNullOrEmpty(newUserAccount.DistinguishedName))
+ throw new DataValidationException($"{nameof(newUserAccount.DistinguishedName)} is required. Set the value: CN={newUserAccount.Cn},{newUserAccount.DistinguishedNameOfContainer}");
+
if (string.IsNullOrEmpty(newUserAccount.Cn))
</code_context>
<issue_to_address>
**issue (broader_impact):** Account creation now rejects any account whose full distinguished name is missing instead of deriving `CN={Cn},{DistinguishedNameOfContainer}` as the removed normalization method did; existing callers that provide only a container DN receive a validation failure and no account is created.
**Triggers:** When callers use the existing container-only construction pattern and omit `DistinguishedName`.
**Suggested fix:** Either retain the automatic DN initialization before validation or make the new full-DN requirement an explicit breaking API change and update all callers.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and the DTO and account-creation changes alter LDAP account records, including requiring callers to supply the distinguished name and removing membership handling; a wrong value or placement could persist in the directory and require cleanup or recreation. Reverting restores the previous behavior, but it does not automatically repair accounts already created with incorrect attributes.
Blocking findings: adapters/Bitai.LDAPHelper.LdapAdapters.LdapHelperMock/MockLdapPersistentConnectionAdapter.cs:197, src/Bitai.LDAPHelper.DTO/LDAPMsADUserAccount.cs:33, src/Bitai.LDAPHelper/AccountManager.cs:58
Comment on lines
+193
to
197
| else if (filter.Contains("(Name=")) | ||
| { | ||
| var value = ExtractFilterValue(filter, "cn"); | ||
| var cn = entry.GetAttributeSet().GetAttribute("cn")?.StringValue; | ||
| var value = ExtractFilterValue(filter, "Name"); | ||
| var cn = entry.GetAttributeSet().GetAttribute("Name")?.StringValue; | ||
| return MatchWildcard(cn, value); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The mock now handles CN filters only when the filter contains (Name=...) and then looks up the case-sensitive Name key, but seeded users and groups still add cn and lowercase name; those entries therefore return no match for name searches in the persistent mock.
Triggers: When the persistent mock is used to search seeded users or groups by the normal LDAP cn attribute.
Suggested fix: Support (cn=...) and read the actual cn attribute, or normalize mock attribute names case-insensitively and consistently seed the same attribute.
Comment on lines
+32
to
+33
| DistinguishedNameOfContainer = distinguishedNameOfContainer | ||
| ?? throw new ArgumentNullException(nameof(distinguishedNameOfContainer)); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The single-argument constructor accepts an empty distinguishedNameOfContainer, whereas the previous constructor rejected both null and empty values; callers can now construct an invalid account that later fails during account creation.
Triggers: When a caller passes string.Empty as the container distinguished name.
Suggested fix: Use string.IsNullOrEmpty(distinguishedNameOfContainer) before throwing, preserving the previous contract.
Suggested change
| DistinguishedNameOfContainer = distinguishedNameOfContainer | |
| ?? throw new ArgumentNullException(nameof(distinguishedNameOfContainer)); | |
| DistinguishedNameOfContainer = string.IsNullOrEmpty(distinguishedNameOfContainer) | |
| ? throw new ArgumentNullException(nameof(distinguishedNameOfContainer)) | |
| : distinguishedNameOfContainer; |
Comment on lines
+57
to
+58
| if (string.IsNullOrEmpty(newUserAccount.DistinguishedName)) | ||
| throw new DataValidationException($"{nameof(newUserAccount.DistinguishedName)} is required. Set the value: CN={newUserAccount.Cn},{newUserAccount.DistinguishedNameOfContainer}"); |
Contributor
There was a problem hiding this comment.
issue (broader_impact): Account creation now rejects any account whose full distinguished name is missing instead of deriving CN={Cn},{DistinguishedNameOfContainer} as the removed normalization method did; existing callers that provide only a container DN receive a validation failure and no account is created.
Triggers: When callers use the existing container-only construction pattern and omit DistinguishedName.
Suggested fix: Either retain the automatic DN initialization before validation or make the new full-DN requirement an explicit breaking API change and update all callers.
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.
Summary by Sourcery
Modernize Active Directory DTOs and align LDAP search, account management, mock data, and tests with the updated Bitai directory model.
New Features:
Bug Fixes:
Enhancements:
Tests:
Chores: