Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Add ForbiddenResult #3443

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 113 additions & 0 deletions src/Microsoft.AspNet.Mvc.Core/ForbiddenResult.cs
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add param docs

: this(new[] { authenticationScheme })
{
}

/// <summary>
/// Initializes a new instance of <see cref="ForbiddenResult"/> with the
/// specified authentication schemes.
/// </summary>
public ForbiddenResult(IList<string> authenticationSchemes)
Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gets or sets (all of em)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this say authentication schemes? Is there anything easy we can put here to say what this means?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AuthenticationSchemes?.Count > 0 #PranavPoints ®️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I care about unnecessary boxing.

Copy link
Member

Choose a reason for hiding this comment

The 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal. Yes, really.

Copy link
Member

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really ugly. just use the objectively better normal syntax.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

named param for null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

named param for null

Go away :trollface:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

}
}
121 changes: 121 additions & 0 deletions test/Microsoft.AspNet.Mvc.Core.Test/ForbiddenResultTest.cs
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();
}

Copy link
Member

Choose a reason for hiding this comment

The 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();
}
}
}