Searching for matching shares

Question

We're working on some code that involves validating certain UNC paths (specifically looking for things that appear to be subdirectories of other things), and we've gone back and forth on how to best do this.

One step of that process looks like this:

• Given a UNC path such as \\localhost\c$\some\great\directory
• Transform that to the appropriate rooted path, e.g. C:\some\great\directory

You can assume that the UNC path is validated to not be null, empty, whitespace, or to a location that does not exist in a previous step. You can also assume that ParseUncPath is perfect; the contents of that are not relevant to the review, but you can assume that given an input of \\localhost\c$\some\great\path we end up with

• hostname -> localhost
• sharename -> c$
• path -> \some\great\path

Our existing implementation uses System.Management APIs to accomplish this like so:

private static string CheckIfUncPathExistsV1(string fullpath)
{    
    ParseUncPath(fullpath, out string hostname, out string sharename, out string path);

    var scope = new ManagementScope(string.Format(@"\\{0}\root\cimv2", hostname));
    var query = new SelectQuery(string.Format("SELECT * FROM Win32_Share WHERE name = '{0}'", sharename));

    using (var search = new ManagementObjectSearcher(scope, query))
    {
        return search.Get()
                     .Cast<ManagementBaseObject>()
                     .Select(item => item["path"].ToString() + path)
                     .First();
    }
}

This is pretty straightforward, but poking around on MSDN reveals a few potential issues:

• System.Management is apparently slower and not supported relative to Microsoft.Management.Infrastructure
• Its possible that root/CIMv2 is not the correct WMI namespace on all machines
• It just feels kinda gross to me

I dig some digging, and figured out that we could use the Microsoft.Management.Infrastructure APIs instead, which are the hip thing apparently. The only problem is that those get really, really gross:

private static string CheckIfUncPathExistsV2(string fullpath)
{
    // https://docs.microsoft.com/en-us/windows/win32/cimwin32prov/win32-share
    const string wmiClassName = "Win32_Share";

    // https://docs.microsoft.com/en-us/windows/win32/wmisdk/constructing-a-moniker-string
    const string defaultWmiNamespace = "root/CIMv2";
    const string registryKeyNamespace = @"HKEY_LOCAL_MACHINE\Software\Microsoft\WBEM\Scripting";
    const string registryKey = "Default Namespace";

    ParseUncPath(fullpath, out string hostname, out string sharename, out string path);

    string wmiNamespace = Registry.GetValue(registryKeyNamespace, registryKey, defaultWmiNamespace) as string;
    using (var session = CimSession.Create(hostname))
    {
        using (var search = new CimInstance(wmiClassName, wmiNamespace))
        {
            var property = CimProperty.Create("Name", sharename, CimFlags.Key);
            search.CimInstanceProperties.Add(property);

            using (var searchResults = session.GetInstance(wmiNamespace, search))
            {
                if (searchResults == null)
                {
                    throw new Exception(string.Format("No share on host {0} could be found with name {1}", hostname, sharename));
                }

                return searchResults.CimInstanceProperties["Path"].Value.ToString() + path;
            }
        }
    }
}

It seems that the interactions between all of the Cim* classes is really tricky (the fact that you need two CimInstances to get anything blows my mind), so we decided not to use it. I wanted to see if I could make this work better, and came up with the following way to encapsulate the interactions with WMI. First, I made a base class that hides all of the general nastiness:

public class Win32WmiClass: IDisposable
{
    #region Wmi Magic
    // https://docs.microsoft.com/en-us/windows/win32/wmisdk/constructing-a-moniker-string
    static private string DEFAULT_WMI_NAMESPACE = "root/CIMv2";
    static private string REGISTRY_KEY_NAMESPACE = @"HKEY_LOCAL_MACHINE\Software\Microsoft\WBEM\Scripting"; 
    static private string REGISTRY_KEY_VALUE_NAME = "Default Namespace";
    protected T GetCimInstanceProperty<T>(string key)
    {
        return (T)SearchResults.CimInstanceProperties[key].Value;
    }

    private CimSession _session = null;
    private CimSession Session
    {
        get
        {
            if (_session == null)
            {
                _session = CimSession.Create(HostName);
            }
            return _session;
        }
    }

    private string _wmiNamespace;
    private string WmiNamespace
    {
        get
        {
            if (string.IsNullOrWhiteSpace(_wmiNamespace))
            {
                _wmiNamespace = Registry.GetValue(REGISTRY_KEY_NAMESPACE, REGISTRY_KEY_VALUE_NAME, DEFAULT_WMI_NAMESPACE) as string;
            }

            return _wmiNamespace;
        }
    }

    private CimInstance _search = null;
    private CimInstance Search
    {
        get
        {
            if (_search == null)
            {
                _search = new CimInstance(WmiClassName, WmiNamespace);
                _search.CimInstanceProperties.Add(CimProperty.Create(WmiKeyPropertyName, WmiKeySearchCriteria, CimFlags.Key));
            }
            return _search;
        }
    }

    private CimInstance _results = null;
    private CimInstance SearchResults
    {
        get
        {
            if (_results == null)
            {
                _results = Session.GetInstance(WmiNamespace, Search);
            }
            return _results;
        }
    }
    #endregion

    #region Needs Override
    virtual protected string WmiClassName { get; }
    virtual protected string WmiKeyPropertyName { get; }
    virtual protected string WmiKeySearchCriteria { get; }
    virtual public string HostName { get; protected set; }
    #endregion

    #region IDisposable Support
    private bool disposedValue = false; // To detect redundant calls

    protected virtual void Dispose(bool disposing)
    {
        if (!disposedValue)
        {
            if (disposing)
            {
                // TODO: dispose managed state (managed objects).
                Session?.Dispose();
                Search?.Dispose();
                SearchResults?.Dispose();
            }

            // TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
            // TODO: set large fields to null.

            disposedValue = true;
        }
    }

    // TODO: override a finalizer only if Dispose(bool disposing) above has code to free unmanaged resources.
    // ~Win32Share() {
    //   // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
    //   Dispose(false);
    // }

    // This code added to correctly implement the disposable pattern.
    public void Dispose()
    {
        // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
        Dispose(true);
        // TODO: uncomment the following line if the finalizer is overridden above.
        // GC.SuppressFinalize(this);
    }
    #endregion
}

This now handles a few things:

• We grab the appropriate namespace from the registry, and fall back to the normal default value
• We assume that everything will search using a single key property
• This implements IDisposable, so now we don't need our deeply nested usings
• We don't need to know how all of the CIM classes interact under the hood.

Then we make a child class to represent the specific WMI class we need, Win32_Share:

public class Win32Share: Win32WmiClass
{
    #region Override
    override protected string WmiClassName { get; } = "Win32_Share";
    override protected string WmiKeyPropertyName { get; } = "Name";
    override protected string WmiKeySearchCriteria { get { return ShareName; } }
    #endregion

    #region Properties
    public string ShareName { get; set; }
    public string Caption { get { return GetCimInstanceProperty<string>("Caption"); } }
    public string Description { get { return GetCimInstanceProperty<string>("Description"); } }
    public DateTime InstallDate { get { return GetCimInstanceProperty<DateTime>("InstallDate"); } }
    public string Status { get { return GetCimInstanceProperty<string>("Status"); } }
    public uint AccessMask { get { return GetCimInstanceProperty<uint>("AccessMask"); } }
    public bool AllowMaximum{ get { return GetCimInstanceProperty<bool>("AllowMaximum"); } }
    public uint MaximumAllowed { get { return GetCimInstanceProperty<uint>("MaximumAllowed"); } }
    public string Name { get { return GetCimInstanceProperty<string>("Name"); } }
    public string Path { get { return GetCimInstanceProperty<string>("Path"); } }
    public uint Type { get { return GetCimInstanceProperty<uint>("Type"); } }
    #endregion


    public Win32Share(string hostname, string sharename)
    {
        HostName = hostname;
        ShareName = sharename;
    }

}

This exposes the different instance properties through normal, strongly-typed properties and doesn't require a developer to remember how to grab the instance properties. We could add methods/system properties as well, but that isn't necessary for our use-case. Additionally, this becomes very easy to use for any developer who needs it, like so:

private static string CheckIfUncPathExistsV3(string fullpath)
{
    ParseUncPath(fullpath, out string hostname, out string sharename, out string path);

    using (var share = new Win32Share(hostname, sharename))
    {
        return share.Path + path;
    }
}

The main things I'd be interested in are:

• General C# tips; it isn't my main language, but we do use it a fair bit so if there are cleaner ways to express myself I'd love to hear it
• Specific System.Management or Microsoft.Management.Infrastructure tips, or if I'm completely misusing/misunderstanding something. It works in all of our tests so far, but if there is something I'm missing please let me know.
• Encapsulation - am I providing a reasonable base class that would be extendable to any other WMI classes I'm just not using right now?
• Usability - if you were to get onboarded to this project tomorrow, which of these three approaches would be easiest to understand, use, and modify without having to have MSDN open the entire time?

Answer 1

API #3

Several legacy Windows WMI API's (LAN adapters, CIM, installed products, ..) have extended state available in the Registry. It's a good idea to provide a wrapper class for consumers, keeping all the magic inside this class. I'm not convinced though about the name Win32WmiClass, it suggest a WMI-only interface, but you also use the Registry.

---

Naming Concentions (C#)

• Constants should be PascalCased: DEFAULT_WMI_NAMESPACE -> DefaultWmiNamespace

---

General Guidelines

Use arrow notation to write more clean and compact code.

public string Caption { get { return GetCimInstanceProperty<string>("Caption"); }}

   public string Caption => GetCimInstanceProperty<string>("Caption");

Use built-in path concatenation functions.

return share.Path + path;

return System.IO.Path.Combine(share.Path, path); // namespace could be using statement

Use built-in memoization support. The problem with the code below, is that it will always retrieve the value from the Registry on each get, if the value is an empty string.

private string WmiNamespace
{
    get
    {
        if (string.IsNullOrWhiteSpace(_wmiNamespace))
        {
            _wmiNamespace = Registry.GetValue(REGISTRY_KEY_NAMESPACE, 
                REGISTRY_KEY_VALUE_NAME, DEFAULT_WMI_NAMESPACE) as string;
        }

        return _wmiNamespace;
    }
}

You only want initialise-once support instead.

private Lazy<string> _wmiNamespace = new Lazy<string>(
    () => Registry.GetValue(REGISTRY_KEY_NAMESPACE, REGISTRY_KEY_VALUE_NAME, 
        DEFAULT_WMI_NAMESPACE) as string);

public string WmiNamespace => _wmiNamespace.Value; // only once initialized

---

OO-Design

• Your base class is not abstract, but perhaps it should be. Is there a use case to instantiate the base class, I tend to think not? Your virtual properties are glorified constants. It's better, in my opinion, to make a protected constructor with the values of the properties for derived classes to set. This allows for better encapsulation.

I would do something like this:

public abstract class Win32WmiClass: IDisposable
{
    protected string WmiClassName { get; }
    protected string WmiKeyPropertyName { get; }
    protected string WmiKeySearchCriteria { get; }
    public string HostName { get; }

    protected Win32WmiClass(string wmiClassName, string wmiKeyPropertyName, 
        string wmiKeySearchCriteria, string hostName) 
    {
        WmiClassName = wmiClassName;
        WmiKeyPropertyName = wmiKeyPropertyName;
        WmiKeySearchCriteria = wmiKeySearchCriteria;
        HostName = hostName;
    }

    // .. CR: other code omitted for brevity
}

If Win32Share itself could have derived classes, also provide the protected constructor below. If not, take it out and seal the class.

public class Win32Share: Win32WmiClass
{
    public string ShareName { get; }

    protected Win32Share(string wmiClassName, string wmiKeyPropertyName, 
            string wmiKeySearchCriteria, string hostName, string shareName) 
        : base (wmiClassName, wmiKeyPropertyName, wmiKeySearchCriteria, hostName)
    {
        ShareName = shareName;
    }

    public Win32Share(string hostName, string shareName) 
        : this("Win32_Share", "Name", hostName, shareName)
    { 
    }

    // .. CR: other code omitted for brevity
}