Allow IgnoreAntiForgeryToken applied on Razor Page models to work #7907

pranavkm · 2018-06-12T20:03:30Z

dougbu

Looking good but does this actually fix the original #7795? That bug was about [IgnoreAntiforgeryToken] on handler methods, not the overall page model. (Discussion of how to apply the attribute at the page model level diverted the discussion.)

At least need a test involving the attribute on page handlers.

dougbu · 2018-06-12T22:14:59Z

src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs

@@ -189,7 +189,7 @@ public AuthorizeFilter(string policy)
            var authenticateResult = await policyEvaluator.AuthenticateAsync(effectivePolicy, context.HttpContext);

            // Allow Anonymous skips all authorization
-            if (context.Filters.Any(item => item is IAllowAnonymousFilter))
+            if (HasAllowAnonymous(context.Filters))


To confirm:

The later ToArray() calls are the only thing preventing removal of the Linq dependency?

This change is just for perf and consistency and no behaviours change?

This change is just for perf and consistency and no behaviours change?

Yup. I noticed this unnecessary Linq use when I was making a similar change to the Antiforgery filter.

dougbu · 2018-06-12T22:32:18Z

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

-            var reader = new StringReader(htmlContent);
-            var htmlDocument = XDocument.Load(reader);
+            var parser = new HtmlParser();
+            var htmlDocument = parser.Parse(htmlContent);


This parser should handle implied CDATA elements e.g. <script> and DTD declarations better than XDocument. But, I'm curious if anything elsewhere in this PR broke the previous approach?

I backed out a couple of tests that passed the IHtmlDocument in, but in general seems like the correct thing to use a Html parser to parse html.

Agreed. Just confirming.

dougbu · 2018-06-12T22:36:23Z

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

+
+            var responseBody = await response.Content.ReadAsStringAsync();
+            var formToken = AntiforgeryTestHelper.RetrieveAntiforgeryToken(responseBody);
+            var cookie = AntiforgeryTestHelper.RetrieveAntiforgeryCookie(response);


Suggest using fixture.CreateClient(...) in this class to avoid need to manually copy the cookie over. CreateDefaultClient(...) should be avoided unless specifically testing redirects.

CreateDefaultClient(...) should be avoided unless specifically testing redirects.

We use it in all our functional tests. Leaving this code as is.

We use it in all our functional tests

Not completely true -- we use both methods on a seemingly-random basis. Should move toward CreateClient() in a similar fashion to moving to realz internal classes.

dougbu · 2018-06-12T22:37:58Z

test/WebSites/RazorPagesWebSite/Pages/Antiforgery/AntiforgeryDefault.cshtml.cs

+
+        public void OnPost()
+        {
+        }


Remove these boilerplate methods.

dougbu · 2018-06-12T22:38:44Z

test/WebSites/RazorPagesWebSite/Pages/Antiforgery/IgnoreAntiforgery.cshtml.cs

+
+        public void OnPost()
+        {
+        }


Remove these methods.

Fixes #7795

pranavkm · 2018-06-13T17:21:46Z

That bug was about [IgnoreAntiforgeryToken] on handler methods,

Spoke to @dougbu offline. We specifically do not support any filters on page handler methods. For 2.2.0, we added an analyzer that complains if you do: #7684

pranavkm · 2018-06-13T17:21:59Z

🆙 📅

mkArtakMSFT · 2018-06-13T17:27:35Z

src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs

+
+        private static bool HasAllowAnonymous(IList<IFilterMetadata> filters)
+        {
+            for (var i = 0; i < filters.Count; i++)


Is speed the main concern here to not use the following instead:
return filters.OfType<IAllowAnonymousFilter>().Any();

filters.OfType<IAllowAnonymousFilter>().Any() results in needless allocations, writing a bespoke code avoids this.

I agree with that, but then you have to do the same in the other place, where you used exactly that:) (the linq expression): https://github.com/aspnet/Mvc/pull/7907/files#diff-50e1233b5c1f8070fd737eac5239cea1R34

@mkArtakMSFT filters execute in every request. No need to optimize application model providers because they run just once.

Ahh right. This one's part of the auth filter so it's on a hot path (runs per request for all actions with auth). The other one's an app model provider, which in production runs once for the lifetime of the application.

mkArtakMSFT · 2018-06-13T17:29:28Z

src/Microsoft.AspNetCore.Mvc.RazorPages/AutoValidateAntiforgeryPageApplicationModelProvider.cs

+    internal class AutoValidateAntiforgeryPageApplicationModelProvider : IPageApplicationModelProvider
+    {
+        // The order is set to execute after the DefaultPageApplicationModelProvider.
+        public int Order => -1000 + 10;


Relying on the hardcoded values for orders isn't good in terms of maintaining relative orders. Why won't we define constants (or static readonly fields) to reference instead?

For one, this code was moved as is (I changed the namespace from .Internal -> internal class) which git really doesn't seem to track as a rename. That said, we only ever have one constant for all our providers - viz the default is -1000. We really don't have any significant numbers outside of it. There really isn't a lot of value in turning that one digit in to a constant.

dougbu

Please do not squash commits before the review is done. Good to see the updates as we go.

dougbu · 2018-06-13T17:44:03Z

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

-            var reader = new StringReader(htmlContent);
-            var htmlDocument = XDocument.Load(reader);
+            var parser = new HtmlParser();
+            var htmlDocument = parser.Parse(htmlContent);


Agreed. Just confirming.

dougbu · 2018-06-13T17:46:19Z

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

+
+            var responseBody = await response.Content.ReadAsStringAsync();
+            var formToken = AntiforgeryTestHelper.RetrieveAntiforgeryToken(responseBody);
+            var cookie = AntiforgeryTestHelper.RetrieveAntiforgeryCookie(response);


We use it in all our functional tests

Not completely true -- we use both methods on a seemingly-random basis. Should move toward CreateClient() in a similar fashion to moving to realz internal classes.

pranavkm assigned dougbu Jun 12, 2018

pranavkm requested a review from rynowak June 12, 2018 20:03

dougbu suggested changes Jun 12, 2018

View reviewed changes

Allow IgnoreAntiForgeryToken applied on Razor Page models to work

329f1c4

Fixes #7795

pranavkm force-pushed the prkrishn/auto-antiforgery branch from 4f6782a to 329f1c4 Compare June 13, 2018 17:20

mkArtakMSFT reviewed Jun 13, 2018

View reviewed changes

mkArtakMSFT approved these changes Jun 13, 2018

View reviewed changes

dougbu approved these changes Jun 13, 2018

View reviewed changes

pranavkm merged commit 287a3c5 into dev Jun 13, 2018

dougbu deleted the prkrishn/auto-antiforgery branch July 15, 2018 03:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow IgnoreAntiForgeryToken applied on Razor Page models to work #7907

Allow IgnoreAntiForgeryToken applied on Razor Page models to work #7907

pranavkm commented Jun 12, 2018

dougbu left a comment

dougbu Jun 12, 2018

pranavkm Jun 13, 2018

dougbu Jun 12, 2018

pranavkm Jun 13, 2018

dougbu Jun 13, 2018

dougbu Jun 12, 2018

pranavkm Jun 13, 2018

dougbu Jun 13, 2018

dougbu Jun 12, 2018

dougbu Jun 12, 2018

pranavkm commented Jun 13, 2018

pranavkm commented Jun 13, 2018

mkArtakMSFT Jun 13, 2018

pranavkm Jun 13, 2018

mkArtakMSFT Jun 13, 2018

dougbu Jun 13, 2018

pranavkm Jun 13, 2018

mkArtakMSFT Jun 13, 2018

pranavkm Jun 13, 2018

dougbu left a comment

dougbu Jun 13, 2018

dougbu Jun 13, 2018

Allow IgnoreAntiForgeryToken applied on Razor Page models to work #7907

Allow IgnoreAntiForgeryToken applied on Razor Page models to work #7907

Conversation

pranavkm commented Jun 12, 2018

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Jun 13, 2018

pranavkm commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment