From c2c37bf6873b529da725003cabdd688d945ca644 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juho=20Hanhim=C3=A4ki?= Date: Wed, 14 Apr 2021 12:53:39 +0300 Subject: [PATCH 1/4] Fix AuthorizeViewCore.cs invalid rendering logic Fixes #24381 Component render would occur between lines 95 and 96 if the AuthenticationState Task had already been completed before it was awaited on line 95. https://github.com/dotnet/aspnetcore/blob/d70439cc82253ac32cc3eaf94ce965bab1ac9d37/src/Components/Authorization/src/AuthorizeViewCore.cs#L73-97 That leads to rendering of NotAuthorized state in BuildRenderTree function which is unexpected behaviour. NotAuthorized state should only ever be rendered after IsAuthorizedAsync has been completed. https://github.com/dotnet/aspnetcore/blob/d70439cc82253ac32cc3eaf94ce965bab1ac9d37/src/Components/Authorization/src/AuthorizeViewCore.cs#L53-70 If the AuthenticationState had not been completed before it was awaited the rendering would have occurred before line 95 and no render would occur between lines 95 and 96. In that case the AuthorizeViewCore works as expected. In this fix the isAuthorized field is made nullable and it is used to determine the state to display in the BuildRenderTree function. Until the authorization has completed the isAuthorized is null the Authorizing state is displayed as expected. --- .../Authorization/src/AuthorizeViewCore.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/Components/Authorization/src/AuthorizeViewCore.cs b/src/Components/Authorization/src/AuthorizeViewCore.cs index b225bb19bdef..614e40a316ca 100644 --- a/src/Components/Authorization/src/AuthorizeViewCore.cs +++ b/src/Components/Authorization/src/AuthorizeViewCore.cs @@ -15,7 +15,7 @@ namespace Microsoft.AspNetCore.Components.Authorization public abstract class AuthorizeViewCore : ComponentBase { private AuthenticationState currentAuthenticationState; - private bool isAuthorized; + private bool? isAuthorized; /// /// The content that will be displayed if the user is authorized. @@ -54,7 +54,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) { // We're using the same sequence number for each of the content items here // so that we can update existing instances if they are the same shape - if (currentAuthenticationState == null) + if (isAuthorized == null) { builder.AddContent(0, Authorizing); } @@ -85,13 +85,10 @@ protected override async Task OnParametersSetAsync() throw new InvalidOperationException($"Authorization requires a cascading parameter of type Task<{nameof(AuthenticationState)}>. Consider using {typeof(CascadingAuthenticationState).Name} to supply this."); } - // First render in pending state - // If the task has already completed, this render will be skipped - currentAuthenticationState = null; + // Clear the previous result of authorization + // This will cause the Authorizing state to be displayed until the authorization has been completed + isAuthorized = null; - // Then render in completed state - // Importantly, we *don't* call StateHasChanged between the following async steps, - // otherwise we'd display an incorrect UI state while waiting for IsAuthorizedAsync currentAuthenticationState = await AuthenticationState; isAuthorized = await IsAuthorizedAsync(currentAuthenticationState.User); } From fc026f83e1f66d67cd1857527880f2360bbdc217 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juho=20Hanhim=C3=A4ki?= Date: Wed, 14 Apr 2021 13:14:29 +0300 Subject: [PATCH 2/4] Fix error that prevented the build --- src/Components/Authorization/src/AuthorizeViewCore.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Components/Authorization/src/AuthorizeViewCore.cs b/src/Components/Authorization/src/AuthorizeViewCore.cs index 614e40a316ca..458966059707 100644 --- a/src/Components/Authorization/src/AuthorizeViewCore.cs +++ b/src/Components/Authorization/src/AuthorizeViewCore.cs @@ -58,7 +58,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) { builder.AddContent(0, Authorizing); } - else if (isAuthorized) + else if (isAuthorized == true) { var authorized = Authorized ?? ChildContent; builder.AddContent(0, authorized?.Invoke(currentAuthenticationState)); From d5b4a4f35be3be641aad20ba189cb2be86a1a1a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juho=20Hanhim=C3=A4ki?= Date: Wed, 14 Apr 2021 23:30:48 +0300 Subject: [PATCH 3/4] Add test --- .../Authorization/test/AuthorizeViewTest.cs | 62 +++++++++++++++++++ .../test/TestAsyncAuthorizationService.cs | 38 ++++++++++++ 2 files changed, 100 insertions(+) create mode 100644 src/Components/Authorization/test/TestAsyncAuthorizationService.cs diff --git a/src/Components/Authorization/test/AuthorizeViewTest.cs b/src/Components/Authorization/test/AuthorizeViewTest.cs index a28b8808c43a..4f224ddb07e0 100644 --- a/src/Components/Authorization/test/AuthorizeViewTest.cs +++ b/src/Components/Authorization/test/AuthorizeViewTest.cs @@ -292,6 +292,68 @@ public void RendersNothingUntilAuthorizationCompleted() }); } + [Fact] + public void RendersAuthorizingWhenAuthenticationIsCompletedBeforehandAndAuthorizationIsAsync() + { + // Covers https://github.com/dotnet/aspnetcore/pull/31794 + // Arrange + var @event = new ManualResetEventSlim(); + var authorizationService = new TestAsyncAuthorizationService(); + authorizationService.NextResult = AuthorizationResult.Success(); + var renderer = CreateTestRenderer(authorizationService); + renderer.OnUpdateDisplayComplete = () => { @event.Set(); }; + var rootComponent = WrapInAuthorizeView( + authorizing: builder => builder.AddContent(0, "Auth pending..."), + authorized: context => builder => builder.AddContent(0, $"Hello, {context.User.Identity.Name}!")); + + var authTcs = new TaskCompletionSource(); + // Complete the authentication beforehand + authTcs.SetResult(CreateAuthenticationState("Monsieur").Result); + + rootComponent.AuthenticationState = authTcs.Task; + + // Act/Assert 1: Auth pending + renderer.AssignRootComponentId(rootComponent); + rootComponent.TriggerRender(); + var batch1 = renderer.Batches.Single(); + var authorizeViewComponentId = batch1.GetComponentFrames().Single().ComponentId; + var diff1 = batch1.DiffsByComponentId[authorizeViewComponentId].Single(); + Assert.Collection(diff1.Edits, edit => + { + Assert.Equal(RenderTreeEditType.PrependFrame, edit.Type); + AssertFrame.Text( + batch1.ReferenceFrames[edit.ReferenceFrameIndex], + "Auth pending..."); + }); + + // Act/Assert 2: Auth process completes asynchronously + @event.Reset(); + + // Wait for authorization to complete asynchronously + @event.Wait(Timeout); + + Assert.Equal(2, renderer.Batches.Count); + var batch2 = renderer.Batches[1]; + var diff2 = batch2.DiffsByComponentId[authorizeViewComponentId].Single(); + Assert.Collection(diff2.Edits, edit => + { + Assert.Equal(RenderTreeEditType.UpdateText, edit.Type); + Assert.Equal(0, edit.SiblingIndex); + AssertFrame.Text( + batch2.ReferenceFrames[edit.ReferenceFrameIndex], + "Hello, Monsieur!"); + }); + + // Assert: The IAuthorizationService was given expected criteria + Assert.Collection(authorizationService.AuthorizeCalls, call => + { + Assert.Equal("Monsieur", call.user.Identity.Name); + Assert.Null(call.resource); + Assert.Collection(call.requirements, + req => Assert.IsType(req)); + }); + } + [Fact] public void RendersAuthorizingUntilAuthorizationCompleted() { diff --git a/src/Components/Authorization/test/TestAsyncAuthorizationService.cs b/src/Components/Authorization/test/TestAsyncAuthorizationService.cs new file mode 100644 index 000000000000..ac273a4b087b --- /dev/null +++ b/src/Components/Authorization/test/TestAsyncAuthorizationService.cs @@ -0,0 +1,38 @@ +// 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.Security.Claims; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authorization; + +namespace Microsoft.AspNetCore.Components.Authorization +{ + public class TestAsyncAuthorizationService : IAuthorizationService + { + public AuthorizationResult NextResult { get; set; } + = AuthorizationResult.Failed(); + + public List<(ClaimsPrincipal user, object resource, IEnumerable requirements)> AuthorizeCalls { get; } + = new List<(ClaimsPrincipal user, object resource, IEnumerable requirements)>(); + + public async Task AuthorizeAsync(ClaimsPrincipal user, object resource, IEnumerable requirements) + { + AuthorizeCalls.Add((user, resource, requirements)); + + // Make Authorization run asynchronously + await Task.Yield(); + + // The TestAuthorizationService doesn't actually apply any authorization requirements + // It just returns the specified NextResult, since we're not trying to test the logic + // in DefaultAuthorizationService or similar here. So it's up to tests to set a desired + // NextResult and assert that the expected criteria were passed by inspecting AuthorizeCalls. + return NextResult; + } + + public Task AuthorizeAsync(ClaimsPrincipal user, object resource, string policyName) + => throw new NotImplementedException(); + + } +} From ff492e5ba0e0de96274cf682704325ae973a2c71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juho=20Hanhim=C3=A4ki?= Date: Thu, 15 Apr 2021 09:45:44 +0300 Subject: [PATCH 4/4] Improved test name --- src/Components/Authorization/test/AuthorizeViewTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Components/Authorization/test/AuthorizeViewTest.cs b/src/Components/Authorization/test/AuthorizeViewTest.cs index 4f224ddb07e0..ce84e611cc42 100644 --- a/src/Components/Authorization/test/AuthorizeViewTest.cs +++ b/src/Components/Authorization/test/AuthorizeViewTest.cs @@ -293,7 +293,7 @@ public void RendersNothingUntilAuthorizationCompleted() } [Fact] - public void RendersAuthorizingWhenAuthenticationIsCompletedBeforehandAndAuthorizationIsAsync() + public void RendersAuthorizingUntilAuthorizationCompletedAsync() { // Covers https://github.com/dotnet/aspnetcore/pull/31794 // Arrange