Finalize the Download Command changes (#203)

* Refactor parameter parsing in Download.cs, add more checks

* Handle Download#unsecureAuth as boolean

* Parse Enums in a case-insensitive mode, propagate error correctly

* Add tests for the newly introduced functionality

* Update the configuration sample to reflect the recent changes

* Update the sample text according to the proposal from @nightman68

doc/xmlConfigFile.md

@@ -154,7 +154,7 @@ For servers requiring authentication some parameters must be specified depending
 * `basic`: Basic authentication, sub-parameters:
 	* `username=“UserName”`
 	* `password=“Passw0rd”`
-	* `unsecureAuth=“enabled”: default=“disabled"`
+	* `unsecureAuth=“true”: default=“false"`
 
 The parameter “unsecureAuth” is only effective when the transfer protocol is HTTP - unencrypted data transfer. This is a security vulnerability because the credentials are send in clear text! For a SSPI authentication this is not relevant because the authentication tokens are encrypted.
 
@@ -176,8 +176,8 @@ Examples:
               auth="basic" username="aUser" password="aPassw0rd" />
 
     <download from="http://example.com/some.dat" to="%BASE%\some.dat"
-              auth="basic" unsecureAuth=“enabled”
-              username="aUser" password=“aPassw0rd" />
+              auth="basic" unsecureAuth="true"
+              username="aUser" password="aPassw0rd" />
 ```
 
 This is another useful building block for developing a self-updating service.

examples/sample-allOptions.xml

@@ -251,18 +251,21 @@ SECTION: Environment setup
   -->
   <!--
   <download from="http://www.google.com/" to="%BASE%\index.html" />
-  <download from="http://www.nosuchhostexists.com/" to="%BASE%\dummy.html" />
+  
+  Download and fail the service startup on Error:
+  <download from="http://www.nosuchhostexists.com/" to="%BASE%\dummy.html" failOnError="true"/>
 
   An example for unsecure Basic authentication because the connection is not encrypted:
   <download from="http://example.com/some.dat" to="%BASE%\some.dat"
-            auth="basic" unsecureAuth=“enabled”
+            auth="basic" unsecureAuth=“true”
             username="aUser" password=“aPassw0rd" />
 
   Secure Basic authentication via HTTPS:
   <download from="https://example.com/some.dat" to="%BASE%\some.dat"
             auth="basic" username="aUser" password="aPassw0rd" />
 
-  Secure authentication when the target server and the client are members of domain:
+  Secure authentication when the target server and the client are members of the same domain or 
+  the server domain and the client domain belong to the same forest with a trust:
   <download from="https://example.com/some.dat" to="%BASE%\some.dat" auth="sspi" />
   -->
 
@@ -297,4 +300,4 @@ More info is available here: https://github.com/kohsuke/winsw/blob/master/doc/ex
 </extensions>
 -->
 
-</configuration>
+</configuration>

src/Core/WinSWCore/Download.cs

@@ -3,6 +3,7 @@ using System.IO;
 using System.Net;
 using System.Text;
 using System.Xml;
+using winsw.Util;
 
 namespace winsw
 {
@@ -19,72 +20,64 @@ namespace winsw
         public readonly AuthType Auth = AuthType.none;
         public readonly string Username;
         public readonly string Password;
-        public readonly bool UnsecureAuth = false;
+        public readonly bool UnsecureAuth;
         public readonly bool FailOnError;
 
-        public Download(string from, string to, bool failOnError = false)
+        public string ShortId { get { return String.Format("(download from {0})", From); } }
+
+        public Download(string from, string to, bool failOnError = false, AuthType auth = AuthType.none, 
+            string username = null, string password = null, bool unsecureAuth = false)
         {
             From = from;
             To = to;
             FailOnError = failOnError;
+            Auth = auth;
+            Username = username;
+            Password = password;
+            UnsecureAuth = unsecureAuth;
         }
 
-        internal Download(XmlNode n)
+        /// <summary>
+        /// Constructs the download setting sfrom the XML entry
+        /// </summary>
+        /// <param name="n">XML element</param>
+        /// <exception cref="InvalidDataException">The required attribute is missing or the configuration is invalid</exception>
+        internal Download(XmlElement n)
         {
-            From = Environment.ExpandEnvironmentVariables(n.Attributes["from"].Value);
-            To = Environment.ExpandEnvironmentVariables(n.Attributes["to"].Value);
-            
-            var failOnErrorNode = n.Attributes["failOnError"];
-            FailOnError = failOnErrorNode != null ? Boolean.Parse(failOnErrorNode.Value) : false;
+            From = XmlHelper.SingleAttribute<String>(n, "from");
+            To = XmlHelper.SingleAttribute<String>(n, "to");
 
-            string tmpStr = "";
-            try
-            {
-                tmpStr = Environment.ExpandEnvironmentVariables(n.Attributes["auth"].Value);
-            }
-            catch (Exception)
-            {
-            }
-            Auth = tmpStr != "" ? (AuthType)Enum.Parse(typeof(AuthType), tmpStr) : AuthType.none;
+            // All arguments below are optional
+            FailOnError = XmlHelper.SingleAttribute<bool>(n, "failOnError", false);
 
-            try
-            {
-                tmpStr = Environment.ExpandEnvironmentVariables(n.Attributes["username"].Value);
-            }
-            catch (Exception)
-            {
-            }
-            Username = tmpStr;
-
-            try
-            {
-                tmpStr = Environment.ExpandEnvironmentVariables(n.Attributes["password"].Value);
-            }
-            catch (Exception)
-            {
-            }
-            Password = tmpStr;
-
-            try
-            {
-                tmpStr = Environment.ExpandEnvironmentVariables(n.Attributes["unsecureAuth"].Value);
-            }
-            catch (Exception)
-            {
-            }
-            UnsecureAuth = tmpStr == "enabled" ? true : false;
+            Auth = XmlHelper.EnumAttribute<AuthType>(n, "auth", AuthType.none);
+            Username = XmlHelper.SingleAttribute<String>(n, "user", null);
+            Password = XmlHelper.SingleAttribute<String>(n, "password", null);
+            UnsecureAuth = XmlHelper.SingleAttribute<bool>(n, "unsecureAuth", false);
 
             if (Auth == AuthType.basic)
             {
-                if (From.StartsWith("http:") && UnsecureAuth == false)
+                // Allow it only for HTTPS or for UnsecureAuth 
+                if (!From.StartsWith("https:") && !UnsecureAuth)
                 {
-                    throw new Exception("Warning: you're sending your credentials in clear text to the server. If you really want this you must enable this in the configuration!");
+                    throw new InvalidDataException("Warning: you're sending your credentials in clear text to the server " + ShortId + 
+                                                   "If you really want this you must enable 'unsecureAuth' in the configuration");
+                }
+
+                // Also fail if there is no user/password
+                if (Username == null)
+                {
+                    throw new InvalidDataException("Basic Auth is enabled, but username is not specified " + ShortId);
+                }
+                if (Password == null)
+                {
+                    throw new InvalidDataException("Basic Auth is enabled, but password is not specified " + ShortId);
                 }
             }
         }
 
         // Source: http://stackoverflow.com/questions/2764577/forcing-basic-authentication-in-webrequest
-        public void SetBasicAuthHeader(WebRequest request, String username, String password)
+        private void SetBasicAuthHeader(WebRequest request, String username, String password)
         {
             string authInfo = username + ":" + password;
             authInfo = Convert.ToBase64String(Encoding.GetEncoding("ISO-8859-1").GetBytes(authInfo));
@@ -103,6 +96,10 @@ namespace winsw
 
             switch (Auth)
             {
+                case AuthType.none:
+                    // Do nothing
+                    break;
+
                 case AuthType.sspi:
                     req.UseDefaultCredentials = true;
                     req.PreAuthenticate = true;
@@ -112,6 +109,9 @@ namespace winsw
                 case AuthType.basic:
                     SetBasicAuthHeader(req, Username, Password);
                     break;
+
+                default:
+                    throw new WebException("Code defect. Unsupported authentication type: " + Auth);
             }
 
             WebResponse rsp = req.GetResponse();
@@ -123,15 +123,6 @@ namespace winsw
             File.Move(To + ".tmp", To);
         }
 
-        /// <summary>
-        /// Produces the XML configuuration entry.
-        /// </summary>
-        /// <returns>XML String for the configuration file</returns>
-        public String toXMLConfig()
-        {
-            return "<download from=\"" + From + "\" to=\"" + To + "\" failOnError=\"" + FailOnError + "\"/>";
-        }
-
         private static void CopyStream(Stream i, Stream o)
         {
             byte[] buf = new byte[8192];

src/Core/WinSWCore/ServiceDescriptor.cs

@@ -563,7 +563,11 @@ namespace winsw
                 List<Download> r = new List<Download>();
                 foreach (XmlNode n in xmlNodeList)
                 {
-                    r.Add(new Download(n));
+                    XmlElement el = n as XmlElement;
+                    if (el != null)
+                    {
+                        r.Add(new Download(el));
+                    }
                 }
                 return r;
             }

src/Core/WinSWCore/Util/XmlHelper.cs

@@ -71,5 +71,32 @@ namespace winsw.Util
              var value = (TAttributeType)Convert.ChangeType(substitutedValue, typeof(TAttributeType));
              return value;
         }
+
+        /// <summary>
+        /// Retireves a single enum attribute
+        /// </summary>
+        /// <typeparam name="TAttributeType">Type of the enum</typeparam>
+        /// <param name="node">Parent node</param>
+        /// <param name="attributeName">Attribute name</param>
+        /// <param name="defaultValue">Default value</param>
+        /// <returns>Attribute value (or default)</returns>
+        /// <exception cref="InvalidDataException">Wrong enum value</exception>
+        public static TAttributeType EnumAttribute<TAttributeType>(XmlElement node, string attributeName, TAttributeType defaultValue)
+        {
+            if (!node.HasAttribute(attributeName)) return defaultValue;
+
+            string rawValue = node.GetAttribute(attributeName);
+            string substitutedValue = Environment.ExpandEnvironmentVariables(rawValue);
+            try
+            {
+                var value = Enum.Parse(typeof(TAttributeType), substitutedValue, true);
+                return (TAttributeType)value;
+            }
+            catch (Exception ex) // Most likely ArgumentException
+            {
+                throw new InvalidDataException("Cannot parse <" +  attributeName + "> Enum value from string '" + substitutedValue +
+                    "'. Enum type: " + typeof(TAttributeType), ex);
+            }
+        }
     }
 }

src/Test/winswTests/DownloadTest.cs

@@ -1,6 +1,7 @@
 using NUnit.Framework;
 using System;
 using System.Collections.Generic;
+using System.IO;
 using System.Text;
 using winsw;
 using winswTests.Util;
@@ -10,9 +11,87 @@ namespace winswTests
     [TestFixture]
     class DownloadTest
     {
-        private const string From = "http://www.nosuchhostexists.foo.myorg/foo.xml";
+        private const string From = "https://www.nosuchhostexists.foo.myorg/foo.xml";
         private const string To = "%BASE%\\foo.xml";
 
+        [Test]
+        public void Roundtrip_Defaults()
+        {
+            // Roundtrip data
+            Download d = new Download(From, To);
+            var sd = ConfigXmlBuilder.create()
+                .WithDownload(d)
+                .ToServiceDescriptor(true);
+            var loaded = getSingleEntry(sd);
+
+            // Check default values
+            Assert.That(loaded.FailOnError, Is.EqualTo(false));
+            Assert.That(loaded.Auth, Is.EqualTo(Download.AuthType.none));
+            Assert.That(loaded.Username, Is.Null);
+            Assert.That(loaded.Password, Is.Null);
+            Assert.That(loaded.UnsecureAuth, Is.EqualTo(false));
+        }
+
+        [Test]
+        public void Roundtrip_BasicAuth()
+        {
+            // Roundtrip data
+            Download d = new Download(From, To, true, Download.AuthType.basic, "aUser", "aPassword", true);
+            var sd = ConfigXmlBuilder.create()
+                .WithDownload(d)
+                .ToServiceDescriptor(true);
+            var loaded = getSingleEntry(sd);
+
+            // Check default values
+            Assert.That(loaded.FailOnError, Is.EqualTo(true));
+            Assert.That(loaded.Auth, Is.EqualTo(Download.AuthType.basic));
+            Assert.That(loaded.Username, Is.EqualTo("aUser"));
+            Assert.That(loaded.Password, Is.EqualTo("aPassword"));
+            Assert.That(loaded.UnsecureAuth, Is.EqualTo(true));
+        }
+
+        [Test]
+        public void Roundtrip_SSPI()
+        {
+            // Roundtrip data
+            Download d = new Download(From, To, false, Download.AuthType.sspi);
+            var sd = ConfigXmlBuilder.create()
+                .WithDownload(d)
+                .ToServiceDescriptor(true);
+            var loaded = getSingleEntry(sd);
+
+            // Check default values
+            Assert.That(loaded.FailOnError, Is.EqualTo(false));
+            Assert.That(loaded.Auth, Is.EqualTo(Download.AuthType.sspi));
+            Assert.That(loaded.Username, Is.Null);
+            Assert.That(loaded.Password, Is.Null);
+            Assert.That(loaded.UnsecureAuth, Is.EqualTo(false));
+        }
+        
+        [TestCase("http://")]
+        [TestCase("ftp://")]
+        [TestCase("file:///")]
+        [TestCase("jar://")]
+        [TestCase("\\\\")] // UNC
+        public void ShouldReject_BasicAuth_with_UnsecureProtocol(String protocolPrefix)
+        {
+            var d = new Download(protocolPrefix + "myServer.com:8080/file.txt", To,
+                auth: Download.AuthType.basic, username: "aUser", password: "aPassword");
+            assertInitializationFails(d, "you're sending your credentials in clear text to the server");
+        }
+
+        public void ShouldRejectBasicAuth_without_username()
+        {
+            var d = new Download(From, To, auth: Download.AuthType.basic, username: null, password: "aPassword");
+            assertInitializationFails(d, "Basic Auth is enabled, but username is not specified");
+        }
+
+        public void ShouldRejectBasicAuth_without_password()
+        {
+            var d = new Download(From, To, auth: Download.AuthType.basic, username: "aUser", password: null);
+            assertInitializationFails(d, "Basic Auth is enabled, but password is not specified");
+        }
+
         /// <summary>
         /// Ensures that the fail-on-error field is being processed correctly.
         /// </summary>
@@ -46,11 +125,49 @@ namespace winswTests
             Assert.That(loaded.FailOnError, Is.False);
         }
 
+        [TestCase("sspi")]
+        [TestCase("SSPI")]
+        [TestCase("SsPI")]
+        [TestCase("Sspi")]
+        public void AuthType_Is_CaseInsensitive(String authType)
+        {
+            var sd = ConfigXmlBuilder.create()
+                    .WithRawEntry("<download from=\"http://www.nosuchhostexists.foo.myorg/foo.xml\" to=\"%BASE%\\foo.xml\" auth=\"" + authType + "\"/>")
+                    .ToServiceDescriptor(true);
+            var loaded = getSingleEntry(sd);
+            Assert.That(loaded.Auth, Is.EqualTo(Download.AuthType.sspi));
+        }
+
+        [Test]
+        public void Should_Fail_On_Unsupported_AuthType()
+        {
+            // TODO: will need refactoring once all fields are being parsed on startup
+            var sd = ConfigXmlBuilder.create()
+                    .WithRawEntry("<download from=\"http://www.nosuchhostexists.foo.myorg/foo.xml\" to=\"%BASE%\\foo.xml\" auth=\"digest\"/>")
+                    .ToServiceDescriptor(true);
+
+            ExceptionHelper.assertFails("Cannot parse <auth> Enum value from string 'digest'", typeof(InvalidDataException), delegate {
+                var d = getSingleEntry(sd);
+            });
+        }
+
         private Download getSingleEntry(ServiceDescriptor sd)
         {
             var downloads = sd.Downloads.ToArray();
             Assert.That(downloads.Length, Is.EqualTo(1), "Service Descriptor is expected to have only one entry");
             return downloads[0];
         }
+
+        private void assertInitializationFails(Download download, String expectedMessagePart = null, Type expectedExceptionType = null)
+        {
+            var sd = ConfigXmlBuilder.create()
+                .WithDownload(download)
+                .ToServiceDescriptor(true);
+
+            ExceptionHelper.assertFails(expectedMessagePart, expectedExceptionType ?? typeof(InvalidDataException), delegate
+            {
+                var d = getSingleEntry(sd);
+            });
+        }
     }
 }

src/Test/winswTests/Util/ConfigXmlBuilder.cs

@@ -122,7 +122,27 @@ namespace winswTests.Util
 
         public ConfigXmlBuilder WithDownload(Download download)
         {
-            return WithRawEntry(download.toXMLConfig());
+            StringBuilder str = new StringBuilder();
+            str.AppendFormat("<download from=\"{0}\" to=\"{1}\" failOnError=\"{2}\"", new Object[] { download.From, download.To, download.FailOnError});
+            
+            // Authentication
+            if (download.Auth != Download.AuthType.none)
+            {
+                str.AppendFormat(" auth=\"{0}\"", download.Auth);
+                if (download.Auth == Download.AuthType.basic)
+                {
+                    str.AppendFormat(" user=\"{0}\" password=\"{1}\"", new Object[] { download.Username, download.Password });
+                }
+
+                if (download.UnsecureAuth)
+                {
+                    str.AppendFormat(" unsecureAuth=\"true\"");
+                }
+            }
+
+            str.Append("/>");
+
+            return WithRawEntry(str.ToString());
         }
     }
 }

src/Test/winswTests/Util/ExceptionHelper.cs

@@ -0,0 +1,36 @@
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace winswTests.Util
+{
+    class ExceptionHelper
+    {
+        public static void assertFails(String expectedMessagePart, Type expectedExceptionType, ExceptionHelperExecutionBody body)
+        {
+            try
+            {
+                body();
+            }
+            catch (Exception ex)
+            {
+                Console.Out.WriteLine("Caught exception: " + ex);
+                Assert.That(ex, Is.InstanceOf(expectedExceptionType ?? typeof(Exception)), "Wrong exception type");
+                if (expectedMessagePart != null)
+                {
+                    Assert.That(ex.Message, Is.StringContaining(expectedMessagePart), "Wrong error message");
+                }
+
+                // Else the exception is fine
+                return;
+            }
+
+            Assert.Fail("Expected exception " + expectedExceptionType + " to be thrown by the operation");
+        }
+
+        
+    }
+
+    public delegate void ExceptionHelperExecutionBody();
+}

src/Test/winswTests/winswTests.csproj

@@ -61,6 +61,7 @@
     <Compile Include="Extensions\RunawayProcessKillerTest.cs" />
     <Compile Include="Extensions\SharedDirectoryMapperTest.cs" />
     <Compile Include="MainTest.cs" />
+    <Compile Include="Util\ExceptionHelper.cs" />
     <Compile Include="Util\FilesystemTestHelper.cs" />
     <Compile Include="Util\ProcessHelperTest.cs" />
     <Compile Include="Properties\AssemblyInfo.cs" />