Adding AuthorizePage & AuthorizeFolder without requiring a policy #5942

hishamco · 2017-03-10T01:58:36Z

This PR fixes issue #5936

hishamco · 2017-03-10T01:59:58Z

src/Microsoft.AspNetCore.Mvc.RazorPages/DependencyInjection/RazorPagesOptionsExtensions.cs

+                throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, nameof(path));
+            }
+
+            var authorizeFilter = new AuthorizeFilter(string.Empty);


I can use this instead

var authorizeFilter = new AuthorizeFilter(new[] { new AuthorizeAttribute() });

but IMHO passing string.Empty is much simpler in this case

Instead of doing this, just call the other overload:
public static RazorPagesOptions AuthorizePage(this RazorPagesOptions options, string path) => AuthorizePage(options, path, policy: string.Empty);

I was thinking last time do the exact thing, but I forget it 😄 , I will update it now ..

hishamco · 2017-03-10T02:02:33Z

...rosoft.AspNetCore.Mvc.RazorPages.Test/DependencyInjection/RazorPagesOptionsExtensionsTest.cs

        [Theory]
        [InlineData("/Users")]
        [InlineData("/Users/")]
-        public void AuthorizePage_AddsAuthorizeFilterToPagesUnderFolder(string folderName)
+        public void AuthorizePage_AddsAuthorizeFilterWithPolicyToPagesUnderFolder(string folderName)


We can add optional param for policy in this way we can merge both tests in one, let me know if I can go with it

I think the tests are fine the way they are now.

hishamco · 2017-03-10T02:19:13Z

src/Microsoft.AspNetCore.Mvc.RazorPages/DependencyInjection/RazorPagesOptionsExtensions.cs

@@ -43,7 +43,7 @@ public static RazorPagesOptions ConfigureFilter(this RazorPagesOptions options,
        /// <param name="path">The path of the Razor Page.</param>
        /// <param name="policy">The authorization policy.</param>
        /// <returns>The <see cref="RazorPagesOptions"/>.</returns>
-        public static RazorPagesOptions AuthorizePage(this RazorPagesOptions options, string path, string policy)
+        public static RazorPagesOptions AuthorizePage(this RazorPagesOptions options, string path, string policy = "")


I preferred to use optional parameter instead of creating another overload

We can use default(string) too if you don't like the double quotes

We generally don't use optional parameters in public APIs. Could you create another overload?

Here's a blog post that explains why it's a tricky to version these: http://haacked.com/archive/2010/08/10/versioning-issues-with-optional-arguments.aspx/

I think I saw this blog post long time back, ok I will revert the latest change

pranavkm · 2017-03-10T05:04:00Z

src/Microsoft.AspNetCore.Mvc.RazorPages/DependencyInjection/RazorPagesOptionsExtensions.cs

@@ -43,7 +43,7 @@ public static RazorPagesOptions ConfigureFilter(this RazorPagesOptions options,
        /// <param name="path">The path of the Razor Page.</param>
        /// <param name="policy">The authorization policy.</param>
        /// <returns>The <see cref="RazorPagesOptions"/>.</returns>
-        public static RazorPagesOptions AuthorizePage(this RazorPagesOptions options, string path, string policy)
+        public static RazorPagesOptions AuthorizePage(this RazorPagesOptions options, string path, string policy = "")


We generally don't use optional parameters in public APIs. Could you create another overload?

pranavkm · 2017-03-10T05:06:22Z

...rosoft.AspNetCore.Mvc.RazorPages.Test/DependencyInjection/RazorPagesOptionsExtensionsTest.cs

        [Theory]
        [InlineData("/Users")]
        [InlineData("/Users/")]
-        public void AuthorizePage_AddsAuthorizeFilterToPagesUnderFolder(string folderName)
+        public void AuthorizePage_AddsAuthorizeFilterWithPolicyToPagesUnderFolder(string folderName)


I think the tests are fine the way they are now.

hishamco · 2017-03-10T11:59:06Z

It's ok now

pranavkm · 2017-03-13T20:54:45Z

src/Microsoft.AspNetCore.Mvc.RazorPages/DependencyInjection/RazorPagesOptionsExtensions.cs

+                throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, nameof(path));
+            }
+
+            var authorizeFilter = new AuthorizeFilter(string.Empty);


Instead of doing this, just call the other overload:
public static RazorPagesOptions AuthorizePage(this RazorPagesOptions options, string path) => AuthorizePage(options, path, policy: string.Empty);

pranavkm · 2017-03-13T22:42:06Z

src/Microsoft.AspNetCore.Mvc.RazorPages/DependencyInjection/RazorPagesOptionsExtensions.cs

+        /// <param name="options">The <see cref="RazorPagesOptions"/> to configure.</param>
+        /// <param name="folderPath">The folder path.</param>
+        /// <returns>The <see cref="RazorPagesOptions"/>.</returns>
+        public static RazorPagesOptions AuthorizeFolder(this RazorPagesOptions options, string folderPath)


Same thing as AuthorizePage?

OMG something goes wrong in my local commits, sorry

Refactoring

hishamco · 2017-03-13T23:02:13Z

It should be ok now

pranavkm · 2017-03-13T23:02:49Z

Thanks for the update. I'll get this merged once Travis \ AppVeyor pass.

dnfclas added the cla-already-signed label Mar 10, 2017

hishamco commented Mar 10, 2017

View reviewed changes

hishamco force-pushed the authorization branch from 0921134 to 6db3b08 Compare March 10, 2017 02:17

hishamco commented Mar 10, 2017

View reviewed changes

pranavkm suggested changes Mar 10, 2017

View reviewed changes

hishamco force-pushed the authorization branch from 6db3b08 to e3dffdb Compare March 10, 2017 11:35

Eilon mentioned this pull request Mar 13, 2017

Adding AuthorizePage & AuthorizeFolder without requiring a policy #5936

Closed

pranavkm suggested changes Mar 13, 2017

View reviewed changes

hishamco force-pushed the authorization branch from e3dffdb to 5d2b253 Compare March 13, 2017 21:17

pranavkm reviewed Mar 13, 2017

View reviewed changes

Adding AuthorizePage & AuthorizeFolder without requiring a policy

3661e5a

Refactoring

hishamco force-pushed the authorization branch from 5d2b253 to 3661e5a Compare March 13, 2017 23:00

pranavkm approved these changes Mar 13, 2017

View reviewed changes

pranavkm merged commit e44d875 into aspnet:dev Mar 13, 2017

hishamco mentioned this pull request Mar 19, 2017

Adding AllowAnonymousToPage & AllowAnonymousToFolder #5987

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding AuthorizePage & AuthorizeFolder without requiring a policy #5942

Adding AuthorizePage & AuthorizeFolder without requiring a policy #5942

hishamco commented Mar 10, 2017

hishamco Mar 10, 2017

pranavkm Mar 13, 2017

hishamco Mar 13, 2017

hishamco Mar 10, 2017

pranavkm Mar 10, 2017

hishamco Mar 10, 2017

hishamco Mar 10, 2017

pranavkm Mar 10, 2017

pranavkm Mar 10, 2017

hishamco Mar 10, 2017

pranavkm Mar 10, 2017

pranavkm Mar 10, 2017

hishamco commented Mar 10, 2017

pranavkm Mar 13, 2017

pranavkm Mar 13, 2017

hishamco Mar 13, 2017

hishamco commented Mar 13, 2017

pranavkm commented Mar 13, 2017

Adding AuthorizePage & AuthorizeFolder without requiring a policy #5942

Adding AuthorizePage & AuthorizeFolder without requiring a policy #5942

Conversation

hishamco commented Mar 10, 2017

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

Choose a reason for hiding this comment

hishamco commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hishamco commented Mar 13, 2017

pranavkm commented Mar 13, 2017