LDAP Property Processor Refactor and Testing #96
Conversation
src/CommonLib/Extensions.cs
Outdated
| @@ -393,6 +393,14 @@ public static Label GetLabel(this SearchResultEntry entry) | |||
| return objectType; | |||
| } | |||
|
|
|||
| public static void AddRange<T, U>(this Dictionary<T, U> me, IEnumerable<KeyValuePair<T, U>> keyValuePairs) | |||
There was a problem hiding this comment.
Maybe I should rename to AddMany?
There was a problem hiding this comment.
caveat: not a c# expert
I tend to agree that AddRange is a little unintuitive about what this function is doing. Some additional ideas off the top of my head:
AddEntriesAssignAddToDict
That said, whatever you end up calling it, it's probably a good idea to document the API so folks can rely on their intellisense to figure out what it does.
| @@ -148,45 +106,84 @@ public async Task<UserProperties> ReadUserProperties(ISearchResultEntry entry) | |||
| { | |||
There was a problem hiding this comment.
For the top-level ReadUserProperties and ReadComputerProperties methods especially I'm trying to pull a lot of the "how" out and make these methods more compositional.
| @@ -589,6 +489,167 @@ public async Task<ComputerProperties> ReadComputerProperties(ISearchResultEntry | |||
|
|
|||
| return props; | |||
| } | |||
|
|
|||
| public static Dictionary<string, object> GetProperties(string ldapProperty, ISearchResultEntry entry) | |||
| { | |||
There was a problem hiding this comment.
The goal here is single-truth LDAP property mapping. It provides a dictionary of sorts of our available properties (the "how", see above) and tries to enforce that properties get added to props through our "vetted" source.
| /// <returns></returns> | ||
| /// <exception cref="ArgumentException"></exception> | ||
| public static string GetPropertyName(string ldapProperty) | ||
| { |
There was a problem hiding this comment.
I'm not certain this will live here or that it should take this form as a method instead of a static class providing static properties, but this provides a glossary of LDAP properties and our own internal names for them.
| props.Add(PropertyMap.GetUacPropertyName(UacFlags.TrustedForDelegation), allFlags[UacFlags.TrustedForDelegation]); | ||
| props.Add(PropertyMap.GetUacPropertyName(UacFlags.DontExpirePassword), allFlags[UacFlags.DontExpirePassword]); | ||
| // Note that we flip the flag for Account Disable ("enabled" by resolved name) | ||
| props.Add(PropertyMap.GetUacPropertyName(UacFlags.AccountDisable), !allFlags[UacFlags.AccountDisable]); |
There was a problem hiding this comment.
This property breaks the mold and devalues the benefits of the new pattern a bit because it's an inverse of the LDAP property. I'm not super jazzed that this new pattern doesn't so gracefully address this so if you have any ideas or concerns here please let me know.
| case LDAPProperties.ServicePrincipalNames: | ||
| var spn = entry.GetArrayProperty(LDAPProperties.ServicePrincipalNames); | ||
| props.Add(PropertyMap.GetPropertyName(ldapProperty), spn); | ||
| props.Add("hasspn", spn.Length > 0); |
There was a problem hiding this comment.
I don't like creating a precedent here for wild-west property names (that aren't vetted through PropertyMap) from the get out of a refactor, but I have no better solutions for this many-to-one property map at the moment.
Because we're still vetting through our single-truth mapping method I'm inclined to let it slide, but I might be the only one.
| props.AddRange(GetProperties(LDAPProperties.OperatingSystem, entry)); | ||
|
|
||
| var sidHistory = ReadSidHistory(entry); | ||
| props.Add(PropertyMap.GetPropertyName(LDAPProperties.SIDHistory), sidHistory.History.ToArray()); |
There was a problem hiding this comment.
SidHistory breaks the new design a little because it provides dual functionality that can't be split without duplicating efforts (see ReadSidHistory below), and so we map the property value here instead of within GetProperties as the others. Not sure how I want to resolve this.
There was a problem hiding this comment.
Cleaned this up a bit.
definitelynotagoblin
changed the title
test/unit/LDAPPropertyTests.cs
Outdated
|
|
||
| // [Fact] | ||
| // public void LDAPPropertyProcessor_ReadAllowedToActPrincipals_ReturnsPopulatedList() | ||
| // { |
There was a problem hiding this comment.
This may be a limitation of platform, need to test further, but failed to produce mocks for ActiveDirectorySecurityDescriptor for this test. Will remove before merging if this can't be resolved.
Description
Refactoring LDAPPropertyProcessor to provide constraints and prepare for more granular testing.
Note that this expands upon the changes made for ESC6.
Motivation and Context
The goal is to prepare LDAP Property Processor for growth and continued development.
How Has This Been Tested?
Project unit tests insofar, with plans to expand testing.
Screenshots (if appropriate):
Types of changes
Checklist: