Fix FormTagHelper to set flag indicating generation of AntiforgeryToken

[Fixes #4595] Better error message with extraneous @Html.AntiforgeryToken

src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs

@@ -187,7 +187,7 @@ public override void Process(TagHelperContext context, TagHelperOutput output)
                         routeValues = new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase);
                     }
 
-                    // Unconditionally replace any value from asp-route-area. 
+                    // Unconditionally replace any value from asp-route-area.
                     routeValues["area"] = Area;
                 }
 

src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs

@@ -156,6 +156,16 @@ public virtual IHtmlContent GenerateAntiforgery(ViewContext viewContext)
                 throw new ArgumentNullException(nameof(viewContext));
             }
 
+            // If we're inside a BeginForm/BeginRouteForm, the antiforgery token might have already been
+            // created and appended to the 'end form' content OR the form tag helper might have already generated
+            // an antiforgery token.
+            if (viewContext.FormContext.HasAntiforgeryToken)
+            {
+                return HtmlString.Empty;
+            }
+
+            viewContext.FormContext.HasAntiforgeryToken = true;
+
             return _antiforgery.GetHtml(viewContext.HttpContext);
         }
 
@@ -1468,7 +1478,7 @@ private IHtmlContent GenerateGroupsAndOptions(
             }
 
             // Group items in the SelectList if requested.
-            // The worst case complexity of this algorithm is O(number of groups*n). 
+            // The worst case complexity of this algorithm is O(number of groups*n).
             // If there aren't any groups, it is O(n) where n is number of items in the list.
             var optionGenerated = new bool[itemsList.Count];
             for (var i = 0; i < itemsList.Count; i++)

src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/HtmlHelper.cs

@@ -258,14 +258,6 @@ public IHtmlContent ActionLink(
         /// <inheritdoc />
         public IHtmlContent AntiForgeryToken()
         {
-            // If we're inside a BeginForm/BeginRouteForm, the antiforgery token might have already been
-            // created and appended to the 'end form' content.
-            if (ViewContext.FormContext.HasAntiforgeryToken)
-            {
-                return HtmlString.Empty;
-            }
-
-            ViewContext.FormContext.HasAntiforgeryToken = true;
             var html = _htmlGenerator.GenerateAntiforgery(ViewContext);
             return html ?? HtmlString.Empty;
         }
@@ -900,7 +892,6 @@ protected virtual MvcForm GenerateForm(
             if (shouldGenerateAntiforgery)
             {
                 ViewContext.FormContext.EndOfFormContent.Add(_htmlGenerator.GenerateAntiforgery(ViewContext));
-                ViewContext.FormContext.HasAntiforgeryToken = true;
             }
 
             return CreateForm();
@@ -957,7 +948,6 @@ protected virtual MvcForm GenerateRouteForm(
             if (shouldGenerateAntiforgery)
             {
                 ViewContext.FormContext.EndOfFormContent.Add(_htmlGenerator.GenerateAntiforgery(ViewContext));
-                ViewContext.FormContext.HasAntiforgeryToken = true;
             }
 
             return CreateForm();

test/Microsoft.AspNetCore.Mvc.FunctionalTests/TagHelpersTest.cs

@@ -87,6 +87,42 @@ public async Task CanRenderViewsWithTagHelpersAndUnboundDynamicAttributes_Encode
 #endif
         }
 
+        [Fact]
+        public async Task ReregisteringAntiforgeryTokenInsideFormTagHelper_DoesNotAddDuplicateAntiforgeryTokenFields()
+        {
+            // Arrange
+            var expectedMediaType = MediaTypeHeaderValue.Parse("text/html; charset=utf-8");
+            var outputFile = "compiler/resources/TagHelpersWebSite.Employee.DuplicateAntiforgeryTokenRegistration.html";
+            var expectedContent =
+                await ResourceFile.ReadResourceAsync(_resourcesAssembly, outputFile, sourceFile: false);
+
+            // Act
+            var response = await Client.GetAsync("http://localhost/Employee/DuplicateAntiforgeryTokenRegistration");
+            var responseContent = await response.Content.ReadAsStringAsync();
+
+            // Assert
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            Assert.Equal(expectedMediaType, response.Content.Headers.ContentType);
+
+            responseContent = responseContent.Trim();
+
+            var forgeryToken = AntiforgeryTestHelper.RetrieveAntiforgeryToken(
+                responseContent, "/Employee/DuplicateAntiforgeryTokenRegistration");
+
+#if GENERATE_BASELINES
+            // Reverse usual substitution and insert a format item into the new file content.
+            responseContent = responseContent.Replace(forgeryToken, "{0}");
+            ResourceFile.UpdateFile(_resourcesAssembly, outputFile, expectedContent, responseContent);
+#else
+            expectedContent = string.Format(expectedContent, forgeryToken);
+            // Mono issue - https://github.com/aspnet/External/issues/19
+            Assert.Equal(
+                PlatformNormalizer.NormalizeContent(expectedContent.Trim()),
+                responseContent,
+                ignoreLineEndingDifferences: true);
+#endif
+        }
+
         public static TheoryData TagHelpersAreInheritedFromViewImportsPagesData
         {
             get

test/Microsoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/TagHelpersWebSite.Employee.DuplicateAntiforgeryTokenRegistration.html

@@ -0,0 +1,6 @@
+<html>
+<body>
+    <form action="/Employee/DuplicateAntiforgeryTokenRegistration" method="post"><input name="__RequestVerificationToken" type="hidden" value="{0}" /></form>
+</body>
+</html>
+

test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Rendering/HtmlHelperFormTest.cs

@@ -396,7 +396,6 @@ public void BeginForm_EndForm_RendersAntiforgeryToken()
             // Act & Assert
             using (var form = htmlHelper.BeginForm())
             {
-                Assert.True(viewContext.FormContext.HasAntiforgeryToken);
             }
 
             Assert.Equal(
@@ -435,50 +434,6 @@ public void BeginForm_EndForm_RendersAntiforgeryTokenWhenMethodIsPost()
             // Act & Assert
             using (var form = htmlHelper.BeginForm(FormMethod.Post, antiforgery: null, htmlAttributes: null))
             {
-                Assert.True(viewContext.FormContext.HasAntiforgeryToken);
-            }
-
-            Assert.Equal(
-                "<form><antiforgery></antiforgery></form>",
-                writer.GetStringBuilder().ToString());
-        }
-
-        // This is an integration for the implicit antiforgery token added by BeginForm.
-        [Fact]
-        public void BeginForm_EndForm_RendersAntiforgeryToken_WithExplicitCallToAntiforgery()
-        {
-            // Arrange
-            var htmlGenerator = new Mock<IHtmlGenerator>(MockBehavior.Strict);
-            htmlGenerator
-                .Setup(g => g.GenerateForm(
-                    It.IsAny<ViewContext>(),
-                    It.IsAny<string>(),
-                    It.IsAny<string>(),
-                    It.IsAny<object>(),
-                    It.IsAny<string>(),
-                    It.IsAny<object>()))
-                .Returns(new TagBuilder("form"));
-
-            htmlGenerator
-                .Setup(g => g.GenerateAntiforgery(It.IsAny<ViewContext>()))
-                .Returns(new TagBuilder("antiforgery"));
-
-            var htmlHelper = DefaultTemplatesUtilities.GetHtmlHelper(htmlGenerator.Object);
-            var serviceProvider = new Mock<IServiceProvider>();
-            serviceProvider.Setup(s => s.GetService(typeof(HtmlEncoder))).Returns(new HtmlTestEncoder());
-            var viewContext = htmlHelper.ViewContext;
-            viewContext.HttpContext.RequestServices = serviceProvider.Object;
-
-            var writer = viewContext.Writer as StringWriter;
-            Assert.NotNull(writer);
-
-            // Act & Assert
-            using (var form = htmlHelper.BeginForm())
-            {
-                Assert.True(viewContext.FormContext.HasAntiforgeryToken);
-
-                // This call will no-op
-                Assert.Same(HtmlString.Empty, htmlHelper.AntiForgeryToken());
             }
 
             Assert.Equal(
@@ -518,7 +473,6 @@ public void BeginForm_EndForm_SuppressAntiforgeryToken()
             // Act & Assert
             using (var form = htmlHelper.BeginForm(FormMethod.Post, antiforgery: false, htmlAttributes: null))
             {
-                Assert.False(viewContext.FormContext.HasAntiforgeryToken);
             }
 
             Assert.Equal(
@@ -557,7 +511,6 @@ public void BeginForm_EndForm_SuppressAntiforgeryTokenWhenMethodIsGet()
             // Act & Assert
             using (var form = htmlHelper.BeginForm(FormMethod.Get, antiforgery: null, htmlAttributes: null))
             {
-                Assert.False(viewContext.FormContext.HasAntiforgeryToken);
             }
 
             Assert.Equal(
@@ -598,7 +551,6 @@ public void BeginForm_EndForm_DoesNotSuppressAntiforgeryTokenWhenAntiforgeryIsTr
             // Act & Assert
             using (var form = htmlHelper.BeginForm(method, antiforgery: true, htmlAttributes: null))
             {
-                Assert.True(viewContext.FormContext.HasAntiforgeryToken);
             }
 
             Assert.Equal(
@@ -638,8 +590,6 @@ public void BeginForm_EndForm_SuppressAntiforgeryToken_WithExplicitCallToAntifor
             // Act & Assert
             using (var form = htmlHelper.BeginForm(FormMethod.Post, antiforgery: false, htmlAttributes: null))
             {
-                Assert.False(viewContext.FormContext.HasAntiforgeryToken);
-
                 // This call will ouput a token.
                 Assert.Equal("antiforgery", Assert.IsType<TagBuilder>(htmlHelper.AntiForgeryToken()).TagName);
             }
@@ -680,7 +630,6 @@ public void BeginRouteForm_EndForm_RendersAntiforgeryToken()
             // Act & Assert
             using (var form = htmlHelper.BeginRouteForm(routeValues: null))
             {
-                Assert.True(viewContext.FormContext.HasAntiforgeryToken);
             }
 
             Assert.Equal(
@@ -723,7 +672,6 @@ public void BeginRouteForm_EndForm_RendersAntiforgeryTokenWhenMethodIsPost()
                 antiforgery: null,
                 htmlAttributes: null))
             {
-                Assert.True(viewContext.FormContext.HasAntiforgeryToken);
             }
 
             Assert.Equal(
@@ -767,7 +715,6 @@ public void BeginRouteForm_EndForm_SuppressAntiforgeryToken()
                 antiforgery: false,
                 htmlAttributes: null))
             {
-                Assert.False(viewContext.FormContext.HasAntiforgeryToken);
             }
 
             Assert.Equal(
@@ -810,7 +757,6 @@ public void BeginRouteForm_EndForm_SuppressAntiforgeryTokenWhenMethodIsGet()
                 antiforgery: null,
                 htmlAttributes: null))
             {
-                Assert.False(viewContext.FormContext.HasAntiforgeryToken);
             }
 
             Assert.Equal(
@@ -855,7 +801,6 @@ public void BeginRouteForm_EndForm_DoesNotSuppressAntiforgeryTokenWhenAntiforger
                 antiforgery: true,
                 htmlAttributes: null))
             {
-                Assert.True(viewContext.FormContext.HasAntiforgeryToken);
             }
 
             Assert.Equal(

test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/DefaultHtmlGeneratorTest.cs

@@ -13,6 +13,7 @@
 using Microsoft.AspNetCore.Mvc.ModelBinding;
 using Microsoft.AspNetCore.Mvc.Rendering;
 using Microsoft.AspNetCore.Mvc.Routing;
+using Microsoft.AspNetCore.Mvc.TestCommon;
 using Microsoft.AspNetCore.Mvc.ViewEngines;
 using Microsoft.AspNetCore.Routing;
 using Microsoft.Extensions.Options;
@@ -636,21 +637,48 @@ public void GetCurrentValues_ValueConvertedAsExpected(
             Assert.Equal<string>(expected, result);
         }
 
+        [Theory]
+        [InlineData(true, "")]
+        [InlineData(false, "<input name=\"formFieldName\" type=\"hidden\" value=\"requestToken\" />")]
+        public void GenerateAntiforgery_GeneratesAntiforgeryFieldsOnlyIfRequired(
+            bool hasAntiforgeryToken,
+            string expectedAntiforgeryHtmlField)
+        {
+            // Arrange
+            var metadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
+            var htmlGenerator = GetGenerator(metadataProvider);
+            var viewContext = GetViewContext<Model>(model: null, metadataProvider: metadataProvider);
+            viewContext.FormContext.HasAntiforgeryToken = hasAntiforgeryToken;
+
+            // Act
+            var result = htmlGenerator.GenerateAntiforgery(viewContext);
+
+            // Assert
+            var antiforgeryField = HtmlContentUtilities.HtmlContentToString(result, HtmlEncoder.Default);
+            Assert.Equal(expectedAntiforgeryHtmlField, antiforgeryField);
+        }
+
         // GetCurrentValues uses only the IModelMetadataProvider passed to the DefaultHtmlGenerator constructor.
         private static IHtmlGenerator GetGenerator(IModelMetadataProvider metadataProvider)
         {
             var mvcViewOptionsAccessor = new Mock<IOptions<MvcViewOptions>>();
             mvcViewOptionsAccessor.SetupGet(accessor => accessor.Value).Returns(new MvcViewOptions());
             var htmlEncoder = Mock.Of<HtmlEncoder>();
-            var antiforgery = Mock.Of<IAntiforgery>();
+            var antiforgery = new Mock<IAntiforgery>();
+            antiforgery
+                .Setup(mock => mock.GetAndStoreTokens(It.IsAny<DefaultHttpContext>()))
+                .Returns(() =>
+                {
+                    return new AntiforgeryTokenSet("requestToken", "cookieToken", "formFieldName", "headerName");
+                });
 
             var optionsAccessor = new Mock<IOptions<MvcOptions>>();
             optionsAccessor
                 .SetupGet(o => o.Value)
                 .Returns(new MvcOptions());
 
             return new DefaultHtmlGenerator(
-                antiforgery,
+                antiforgery.Object,
                 mvcViewOptionsAccessor.Object,
                 metadataProvider,
                 new UrlHelperFactory(),

test/WebSites/BasicWebSite/Views/Antiforgery/Login.cshtml

@@ -10,6 +10,7 @@
         <section id="loginForm">
             @using (Html.BeginForm("Login", "Antiforgery", FormMethod.Post, new { @class = "form-horizontal", role = "form" }))
             {
+                @* BeginForm already registers antiforgery tokens. This Html helper call should be no-op *@
                 @Html.AntiForgeryToken()
                 <h4>Use a local account to log in.</h4>
                 <hr />
@@ -39,6 +40,7 @@
 
 @using (Html.BeginForm("UseFacebookLogin", "Antiforgery", FormMethod.Post, new { @class = "form-horizontal", role = "form" }))
 {
+    @* BeginForm already registers antiforgery tokens. This Html helper call should be no-op *@
     @Html.AntiForgeryToken()
     <h4>Use Facebook login.</h4>
     <hr />

test/WebSites/TagHelpersWebSite/Controllers/EmployeeController.cs

@@ -31,5 +31,11 @@ public IActionResult Create(Employee employee)
             }
             return View(employee);
         }
+
+        // GET: Employee/DuplicateAntiforgeryTokenRegistration
+        public IActionResult DuplicateAntiforgeryTokenRegistration()
+        {
+            return View();
+        }
     }
 }

test/WebSites/TagHelpersWebSite/Views/Employee/DuplicateAntiforgeryTokenRegistration.cshtml

@@ -0,0 +1,11 @@
+@addTagHelper *, Microsoft.AspNetCore.Mvc.TagHelpers
+@{
+    Layout = null;
+}
+<html>
+<body>
+    @*Form tag helper already registers antiforgery token. The Html helper call should be no-op*@
+    <form asp-antiforgery="true" asp-action="DuplicateAntiforgeryTokenRegistration">@Html.AntiForgeryToken()</form>
+</body>
+</html>
+