Added extra validation of WebHook instances during registration. As not all instances come in through the ASP.NET Web API registration controller, we have to run validation explicitly to verify that we have a correct WebHook registration.

Henrik Frystyk Nielsen · Henrik Frystyk Nielsen · commit 690b54e93bcc · 2016-02-27T21:30:29.000-08:00
To validate a WebHook instance, call IWebHookManager.VerifyWebHookAsync which will also verify that the registered WebHook URI is valid.
diff --git a/src/Microsoft.AspNet.WebHooks.Custom/Properties/CustomResources.Designer.cs b/src/Microsoft.AspNet.WebHooks.Custom/Properties/CustomResources.Designer.cs
diff --git a/src/Microsoft.AspNet.WebHooks.Custom/Properties/CustomResources.resx b/src/Microsoft.AspNet.WebHooks.Custom/Properties/CustomResources.resx
@@ -159,6 +159,9 @@
   <data name="Notification_KeyNotFound" xml:space="preserve">
     <value>No Notification setting was found with key '{0}'.</value>
   </data>
+  <data name="Sender_BadWorkItem" xml:space="preserve">
+    <value>Invalid '{0}' instance: '{1}' cannot be null.</value>
+  </data>
   <data name="WebHook_InvalidSecret" xml:space="preserve">
     <value>The WebHook secret key parameter must be between 32 and 64 characters long.</value>
   </data>
diff --git a/src/Microsoft.AspNet.WebHooks.Custom/WebHooks/WebHookManager.cs b/src/Microsoft.AspNet.WebHooks.Custom/WebHooks/WebHookManager.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.ComponentModel.DataAnnotations;
 using System.Globalization;
 using System.Linq;
 using System.Net.Http;
@@ -72,6 +73,10 @@ public async Task VerifyWebHookAsync(WebHook webHook)
                 throw new ArgumentNullException("webHook");
             }
 
+            // Validate data annotations explicitly as the WebHook may not have gone through deserialization
+            ValidationContext context = new ValidationContext(webHook);
+            Validator.ValidateObject(webHook, context, validateAllProperties: true);
+
             // Check that WebHook URI is either 'http' or 'https'
             if (!(webHook.WebHookUri.IsHttp() || webHook.WebHookUri.IsHttps()))
             {
diff --git a/src/Microsoft.AspNet.WebHooks.Custom/WebHooks/WebHookSender.cs b/src/Microsoft.AspNet.WebHooks.Custom/WebHooks/WebHookSender.cs
@@ -154,14 +154,19 @@ protected virtual JObject CreateWebHookRequestBody(WebHookWorkItem workItem)
         /// HTTP header to the <see cref="HttpRequestMessage"/> along with the entity body.
         /// </summary>
         /// <param name="workItem">The current <see cref="WebHookWorkItem"/>.</param>
-        /// <param name="request">The request to add t</param>
-        /// <param name="body">The body to sign and add to the request</param>
+        /// <param name="request">The request to add the signature to.</param>
+        /// <param name="body">The body to sign and add to the request.</param>
         protected virtual void SignWebHookRequest(WebHookWorkItem workItem, HttpRequestMessage request, JObject body)
         {
             if (workItem == null)
             {
                 throw new ArgumentNullException("workItem");
             }
+            if (workItem.WebHook == null)
+            {
+                string msg = string.Format(CultureInfo.CurrentCulture, CustomResources.Sender_BadWorkItem, this.GetType().Name, "WebHook");
+                throw new ArgumentException(msg, "workItem");
+            }
             if (request == null)
             {
                 throw new ArgumentNullException("request");
diff --git a/test/Microsoft.AspNet.WebHooks.Custom.Test/Microsoft.AspNet.WebHooks.Custom.Test.csproj b/test/Microsoft.AspNet.WebHooks.Custom.Test/Microsoft.AspNet.WebHooks.Custom.Test.csproj
@@ -29,6 +29,7 @@
       <Private>True</Private>
     </Reference>
     <Reference Include="System" />
+    <Reference Include="System.ComponentModel.DataAnnotations" />
     <Reference Include="System.Core" />
     <Reference Include="System.Net.Http" />
     <Reference Include="System.Net.Http.Formatting, Version=5.2.2.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
diff --git a/test/Microsoft.AspNet.WebHooks.Custom.Test/WebHooks/WebHookManagerTests.cs b/test/Microsoft.AspNet.WebHooks.Custom.Test/WebHooks/WebHookManagerTests.cs
@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
 using System.Collections.Specialized;
+using System.ComponentModel.DataAnnotations;
 using System.Globalization;
 using System.Linq;
 using System.Net;
@@ -43,6 +44,24 @@ public WebHookManagerTests()
             _response = new HttpResponseMessage();
         }
 
+        public static TheoryData<WebHook, string> WebHookValidationData
+        {
+            get
+            {
+                Uri webHookUri = new Uri("http://localhost");
+                string secret = new string('a', 32);
+                return new TheoryData<WebHook, string>
+                {
+                    { new WebHook(), "The WebHookUri field is required." },
+                    { new WebHook { WebHookUri = null, Secret = secret }, "The WebHookUri field is required." },
+                    { new WebHook { WebHookUri = webHookUri, Secret = null }, "The Secret field is required." },
+                    { new WebHook { WebHookUri = webHookUri, Secret = string.Empty }, "The Secret field is required." },
+                    { new WebHook { WebHookUri = webHookUri, Secret = "a" }, "The WebHook secret key parameter must be between 32 and 64 characters long." },
+                    { new WebHook { WebHookUri = webHookUri, Secret = new string('a', 65) }, "The WebHook secret key parameter must be between 32 and 64 characters long." },
+                };
+            }
+        }
+
         public static TheoryData<IEnumerable<WebHook>, NotificationDictionary> FilterSingleNotificationData
         {
             get
@@ -84,6 +103,20 @@ public static TheoryData<IEnumerable<WebHook>, IEnumerable<NotificationDictionar
             }
         }
 
+        [Theory]
+        [MemberData("WebHookValidationData")]
+        public async Task VerifyWebHookAsync_Throws_IfInvalidWebHook(WebHook webHook, string expected)
+        {
+            // Arrange
+            _manager = new WebHookManager(_storeMock.Object, _senderMock.Object, _loggerMock.Object, _httpClient);
+
+            // Act
+            ValidationException ex = await Assert.ThrowsAsync<ValidationException>(() => _manager.VerifyWebHookAsync(webHook));
+
+            // Assert
+            Assert.Equal(expected, ex.Message);
+        }
+
         [Theory]
         [InlineData("ftp://localhost")]
         [InlineData("telnet://localhost")]
@@ -94,7 +127,7 @@ public async Task VerifyWebHookAsync_Throws_IfNotHttpOrHttpsUri(string webHookUr
             // Arrange
             _manager = new WebHookManager(_storeMock.Object, _senderMock.Object, _loggerMock.Object, _httpClient);
             WebHook webHook = CreateWebHook();
-            webHook.WebHookUri = new Uri(webHookUri);
+            webHook.WebHookUri = webHookUri != null ? new Uri(webHookUri) : null;
 
             // Act
             InvalidOperationException ex = await Assert.ThrowsAsync<InvalidOperationException>(() => _manager.VerifyWebHookAsync(webHook));
diff --git a/test/Microsoft.AspNet.WebHooks.Custom.Test/WebHooks/WebHookSenderTests.cs b/test/Microsoft.AspNet.WebHooks.Custom.Test/WebHooks/WebHookSenderTests.cs
@@ -68,6 +68,22 @@ public void CreateWebHookRequestBody_CreatesExpectedBody()
             Assert.Equal(SerializedWebHook, actual.ToString());
         }
 
+        [Fact]
+        public void SignWebHookRequest_HandlesNullWebHook()
+        {
+            WebHookWorkItem workItem = CreateWorkItem();
+            HttpRequestMessage request = new HttpRequestMessage();
+            _sender = new WebHookSenderMock(_loggerMock.Object);
+            JObject body = _sender.CreateWebHookRequestBody(workItem);
+            workItem.WebHook = null;
+
+            // Act
+            ArgumentException ex = Assert.Throws<ArgumentException>(() => _sender.SignWebHookRequest(workItem, request, body));
+
+            // Assert
+            Assert.StartsWith("Invalid 'WebHookSenderMock' instance: 'WebHook' cannot be null.", ex.Message);
+        }
+
         [Fact]
         public async Task SignWebHookRequest_SignsBodyCorrectly()
         {
diff --git a/test/Microsoft.AspNet.WebHooks.Custom.Test/WebHooks/WebHookTests.cs b/test/Microsoft.AspNet.WebHooks.Custom.Test/WebHooks/WebHookTests.cs
@@ -3,6 +3,8 @@
 
 using System;
 using System.Collections.Generic;
+using System.ComponentModel.DataAnnotations;
+using System.Linq;
 using Microsoft.TestUtilities;
 using Newtonsoft.Json;
 using Xunit;
@@ -19,6 +21,31 @@ public WebHookTests()
             _webHook = new WebHook();
         }
 
+        public enum ValidationOutcome
+        {
+            Valid = 0,
+            Required,
+            Invalid
+        }
+
+        public static TheoryData<string, ValidationOutcome> WebHookSecretData
+        {
+            get
+            {
+                return new TheoryData<string, ValidationOutcome>
+                {
+                    { string.Empty, ValidationOutcome.Required },
+                    { " ", ValidationOutcome.Required },
+                    { "\r\n", ValidationOutcome.Required },
+                    { new string('a', 31), ValidationOutcome.Invalid },
+                    { new string('a', 65), ValidationOutcome.Invalid },
+                    { new string('a', 32), ValidationOutcome.Valid },
+                    { new string('a', 64), ValidationOutcome.Valid },
+                    { "你好世界你好世界你好世界你好世界你好世界你好世界你好世界你好世界", ValidationOutcome.Valid },
+                };
+            }
+        }
+
         [Fact]
         public void Id_Roundtrips()
         {
@@ -38,6 +65,38 @@ public void Secret_Roundtrips()
             PropertyAssert.Roundtrips(_webHook, w => w.Secret, PropertySetter.NullRoundtrips, roundtripValue: "你好世界");
         }
 
+        [Theory]
+        [MemberData("WebHookSecretData")]
+        public void Secret_Validates(string secret, ValidationOutcome expected)
+        {
+            // Arrange
+            WebHook webHook = new WebHook { Secret = secret };
+            var validationResults = new List<ValidationResult>();
+            var context = new ValidationContext(webHook) { MemberName = "Secret" };
+
+            // Act
+            bool actual = Validator.TryValidateProperty(webHook.Secret, context, validationResults);
+
+            // Assert
+            switch (expected)
+            {
+                case ValidationOutcome.Valid:
+                    Assert.True(actual);
+                    break;
+
+                case ValidationOutcome.Required:
+                    Assert.False(actual);
+                    Assert.Equal("The Secret field is required.", validationResults.Single().ErrorMessage);
+                    Assert.Equal("Secret", validationResults.Single().MemberNames.Single());
+                    break;
+
+                case ValidationOutcome.Invalid:
+                    Assert.Equal("The WebHook secret key parameter must be between 32 and 64 characters long.", validationResults.Single().ErrorMessage);
+                    Assert.Equal("Secret", validationResults.Single().MemberNames.Single());
+                    break;
+            }
+        }
+
         [Fact]
         public void Description_Roundtrips()
         {

Original file line number	Diff line number	Diff line change
`@@ -154,14 +154,19 @@ protected virtual JObject CreateWebHookRequestBody(WebHookWorkItem workItem)`
`154`	`154`	`/// HTTP header to the <see cref="HttpRequestMessage"/> along with the entity body.`
`155`	`155`	`/// </summary>`
`156`	`156`	`/// <param name="workItem">The current <see cref="WebHookWorkItem"/>.</param>`
`157`		`- /// <param name="request">The request to add t</param>`
`158`		`- /// <param name="body">The body to sign and add to the request</param>`
	`157`	`+ /// <param name="request">The request to add the signature to.</param>`
	`158`	`+ /// <param name="body">The body to sign and add to the request.</param>`
`159`	`159`	`protected virtual void SignWebHookRequest(WebHookWorkItem workItem, HttpRequestMessage request, JObject body)`
`160`	`160`	`{`
`161`	`161`	`if (workItem == null)`
`162`	`162`	`{`
`163`	`163`	`throw new ArgumentNullException("workItem");`
`164`	`164`	`}`
	`165`	`+ if (workItem.WebHook == null)`
	`166`	`+ {`
	`167`	`+ string msg = string.Format(CultureInfo.CurrentCulture, CustomResources.Sender_BadWorkItem, this.GetType().Name, "WebHook");`
	`168`	`+ throw new ArgumentException(msg, "workItem");`
	`169`	`+ }`
`165`	`170`	`if (request == null)`
`166`	`171`	`{`
`167`	`172`	`throw new ArgumentNullException("request");`