New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding AuthorizePage & AuthorizeFolder without requiring a policy #5942

Merged
merged 1 commit into from Mar 13, 2017

Conversation

Projects
None yet
3 participants
@hishamco
Contributor

hishamco commented Mar 10, 2017

This PR fixes issue #5936

/cc @pranavkm

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

This comment has been minimized.

@hishamco

hishamco Mar 10, 2017

Contributor

I can use this instead

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

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

This comment has been minimized.

@pranavkm

pranavkm Mar 13, 2017

Member

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

This comment has been minimized.

@hishamco

hishamco Mar 13, 2017

Contributor

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

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

This comment has been minimized.

@hishamco

hishamco Mar 10, 2017

Contributor

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

This comment has been minimized.

@pranavkm

pranavkm Mar 10, 2017

Member

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

@@ -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 = "")

This comment has been minimized.

@hishamco

hishamco Mar 10, 2017

Contributor

I preferred to use optional parameter instead of creating another overload

This comment has been minimized.

@hishamco

hishamco Mar 10, 2017

Contributor

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

This comment has been minimized.

@pranavkm

pranavkm Mar 10, 2017

Member

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

This comment has been minimized.

@pranavkm

pranavkm Mar 10, 2017

Member

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/

This comment has been minimized.

@hishamco

hishamco Mar 10, 2017

Contributor

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

@@ -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 = "")

This comment has been minimized.

@pranavkm

pranavkm Mar 10, 2017

Member

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

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

This comment has been minimized.

@pranavkm

pranavkm Mar 10, 2017

Member

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

@hishamco

This comment has been minimized.

Contributor

hishamco commented Mar 10, 2017

It's ok now

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

This comment has been minimized.

@pranavkm

pranavkm Mar 13, 2017

Member

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

/// <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)

This comment has been minimized.

@pranavkm

pranavkm Mar 13, 2017

Member

Same thing as AuthorizePage?

This comment has been minimized.

@hishamco

hishamco Mar 13, 2017

Contributor

OMG something goes wrong in my local commits, sorry

@hishamco

This comment has been minimized.

Contributor

hishamco commented Mar 13, 2017

It should be ok now

@pranavkm

This comment has been minimized.

Member

pranavkm commented Mar 13, 2017

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

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment