-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add ForbiddenResult #3443
Add ForbiddenResult #3443
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNet.Http.Authentication; | ||
using Microsoft.AspNet.Mvc.Logging; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Logging; | ||
|
||
namespace Microsoft.AspNet.Mvc | ||
{ | ||
/// <summary> | ||
/// An <see cref="ActionResult"/> that on execution issues a 403 forbidden response | ||
/// if the authentication challenge is unacceptable. | ||
/// </summary> | ||
public class ForbiddenResult : ActionResult | ||
{ | ||
/// <summary> | ||
/// Initializes a new instance of <see cref="ForbiddenResult"/>. | ||
/// </summary> | ||
public ForbiddenResult() | ||
: this(new string[] { }) | ||
{ | ||
} | ||
|
||
/// <summary> | ||
/// Initializes a new instance of <see cref="ForbiddenResult"/> with the | ||
/// specified authentication scheme. | ||
/// </summary> | ||
public ForbiddenResult(string authenticationScheme) | ||
: this(new[] { authenticationScheme }) | ||
{ | ||
} | ||
|
||
/// <summary> | ||
/// Initializes a new instance of <see cref="ForbiddenResult"/> with the | ||
/// specified authentication schemes. | ||
/// </summary> | ||
public ForbiddenResult(IList<string> authenticationSchemes) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add param docs |
||
: this(authenticationSchemes, properties: null) | ||
{ | ||
} | ||
|
||
/// <summary> | ||
/// Initializes a new instance of <see cref="ForbiddenResult"/> with the | ||
/// specified <paramref name="properties"/>. | ||
/// </summary> | ||
public ForbiddenResult(AuthenticationProperties properties) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add param docs |
||
: this(new string[] { }, properties) | ||
{ | ||
} | ||
|
||
/// <summary> | ||
/// Initializes a new instance of <see cref="ForbiddenResult"/> with the | ||
/// specified authentication scheme and <paramref name="properties"/>. | ||
/// </summary> | ||
public ForbiddenResult(string authenticationScheme, AuthenticationProperties properties) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add param docs |
||
: this(new[] { authenticationScheme }, properties) | ||
{ | ||
} | ||
|
||
/// <summary> | ||
/// Initializes a new instance of <see cref="ForbiddenResult"/> with the | ||
/// specified authentication schemes and <paramref name="properties"/>. | ||
/// </summary> | ||
public ForbiddenResult(IList<string> authenticationSchemes, AuthenticationProperties properties) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add param docs |
||
{ | ||
AuthenticationSchemes = authenticationSchemes; | ||
Properties = properties; | ||
} | ||
|
||
/// <summary> | ||
/// The list of authentication components that should handle the authentication challenge | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gets or sets (all of em) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Tratcher was the one that came up with the term authentication components. Schemes seems a bit easier to follow |
||
/// invoked by this instance of <see cref="ForbiddenResult"/>. | ||
/// </summary> | ||
public IList<string> AuthenticationSchemes { get; set; } | ||
|
||
/// <summary> | ||
/// <see cref="AuthenticationProperties"/> used to perform authentication. | ||
/// </summary> | ||
public AuthenticationProperties Properties { get; set; } | ||
|
||
/// <inheritdoc /> | ||
public override async Task ExecuteResultAsync(ActionContext context) | ||
{ | ||
if (context == null) | ||
{ | ||
throw new ArgumentNullException(nameof(context)); | ||
} | ||
|
||
var loggerFactory = context.HttpContext.RequestServices.GetRequiredService<ILoggerFactory>(); | ||
var logger = loggerFactory.CreateLogger<ForbiddenResult>(); | ||
|
||
var authentication = context.HttpContext.Authentication; | ||
|
||
if (AuthenticationSchemes != null && AuthenticationSchemes.Count > 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well I care about unnecessary boxing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👊 - there's some unnecessary boxing for you. |
||
{ | ||
for (var i = 0; i < AuthenticationSchemes.Count; i++) | ||
{ | ||
await authentication.ForbidAsync(AuthenticationSchemes[i], Properties); | ||
} | ||
} | ||
else | ||
{ | ||
await authentication.ForbidAsync(Properties); | ||
} | ||
|
||
logger.ForbiddenResultExecuting(AuthenticationSchemes); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using Microsoft.AspNet.Mvc.Core; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's this for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed |
||
using Microsoft.Extensions.Logging; | ||
|
||
namespace Microsoft.AspNet.Mvc.Logging | ||
{ | ||
/// <summary> | ||
/// Extensions methods for logging <see cref="ForbiddenResult"/> instances. | ||
/// </summary> | ||
public static class ForbiddenResultLoggerExtensions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. internal. Yes, really. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for docs on these FYI |
||
{ | ||
private static readonly Action<ILogger, string[], Exception> _resultExecuting = | ||
LoggerMessage.Define<string[]>( | ||
LogLevel.Information, | ||
eventId: 1, | ||
formatString: $"Executing {nameof(ForbiddenResult)} with authentication schemes ({{Schemes}})."); | ||
|
||
public static void ForbiddenResultExecuting(this ILogger logger, IList<string> authenticationSchemes) | ||
=> _resultExecuting(logger, authenticationSchemes.ToArray(), null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is really ugly. just use the objectively better normal syntax. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. named param for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Go away There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNet.Http; | ||
using Microsoft.AspNet.Http.Authentication; | ||
using Microsoft.AspNet.Mvc.Abstractions; | ||
using Microsoft.AspNet.Mvc.Internal; | ||
using Microsoft.AspNet.Routing; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Logging; | ||
using Microsoft.Extensions.Logging.Testing; | ||
using Moq; | ||
using Xunit; | ||
|
||
namespace Microsoft.AspNet.Mvc | ||
{ | ||
public class ForbiddenResultTest | ||
{ | ||
[Fact] | ||
public async Task ExecuteResultAsync_InvokesForbiddenAsyncOnAuthenticationManager() | ||
{ | ||
// Arrange | ||
var authenticationManager = new Mock<AuthenticationManager>(); | ||
authenticationManager | ||
.Setup(c => c.ForbidAsync("", null)) | ||
.Returns(TaskCache.CompletedTask) | ||
.Verifiable(); | ||
var httpContext = new Mock<HttpContext>(); | ||
httpContext.Setup(c => c.RequestServices).Returns(CreateServices()); | ||
httpContext.Setup(c => c.Authentication).Returns(authenticationManager.Object); | ||
var result = new ForbiddenResult("", null); | ||
var routeData = new RouteData(); | ||
|
||
var actionContext = new ActionContext( | ||
httpContext.Object, | ||
routeData, | ||
new ActionDescriptor()); | ||
|
||
// Act | ||
await result.ExecuteResultAsync(actionContext); | ||
|
||
// Assert | ||
authenticationManager.Verify(); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would like to see a case with an empty list of schemes. I almost gave the feedback to oversimplify that loop, so it seems worth having. |
||
[Fact] | ||
public async Task ExecuteResultAsync_InvokesForbiddenAsyncOnAllConfiguredSchemes() | ||
{ | ||
// Arrange | ||
var authProperties = new AuthenticationProperties(); | ||
var authenticationManager = new Mock<AuthenticationManager>(); | ||
authenticationManager | ||
.Setup(c => c.ForbidAsync("Scheme1", authProperties)) | ||
.Returns(TaskCache.CompletedTask) | ||
.Verifiable(); | ||
authenticationManager | ||
.Setup(c => c.ForbidAsync("Scheme2", authProperties)) | ||
.Returns(TaskCache.CompletedTask) | ||
.Verifiable(); | ||
var httpContext = new Mock<HttpContext>(); | ||
httpContext.Setup(c => c.RequestServices).Returns(CreateServices()); | ||
httpContext.Setup(c => c.Authentication).Returns(authenticationManager.Object); | ||
var result = new ForbiddenResult(new[] { "Scheme1", "Scheme2" }, authProperties); | ||
var routeData = new RouteData(); | ||
|
||
var actionContext = new ActionContext( | ||
httpContext.Object, | ||
routeData, | ||
new ActionDescriptor()); | ||
|
||
// Act | ||
await result.ExecuteResultAsync(actionContext); | ||
|
||
// Assert | ||
authenticationManager.Verify(); | ||
} | ||
|
||
public static TheoryData ExecuteResultAsync_InvokesForbiddenAsyncWithAuthPropertiesData => | ||
new TheoryData<AuthenticationProperties> | ||
{ | ||
null, | ||
new AuthenticationProperties() | ||
}; | ||
|
||
[Theory] | ||
public async Task ExecuteResultAsync_InvokesForbiddenAsyncWithAuthProperties(AuthenticationProperties expected) | ||
{ | ||
// Arrange | ||
var authenticationManager = new Mock<AuthenticationManager>(); | ||
authenticationManager | ||
.Setup(c => c.ForbidAsync(expected)) | ||
.Returns(TaskCache.CompletedTask) | ||
.Verifiable(); | ||
var httpContext = new Mock<HttpContext>(); | ||
httpContext.Setup(c => c.RequestServices).Returns(CreateServices()); | ||
httpContext.Setup(c => c.Authentication).Returns(authenticationManager.Object); | ||
var result = new ForbiddenResult(expected); | ||
var routeData = new RouteData(); | ||
|
||
var actionContext = new ActionContext( | ||
httpContext.Object, | ||
routeData, | ||
new ActionDescriptor()); | ||
|
||
// Act | ||
await result.ExecuteResultAsync(actionContext); | ||
|
||
// Assert | ||
authenticationManager.Verify(); | ||
} | ||
|
||
private static IServiceProvider CreateServices() | ||
{ | ||
return new ServiceCollection() | ||
.AddInstance<ILoggerFactory>(NullLoggerFactory.Instance) | ||
.BuildServiceProvider(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add param docs