Skip to content

Commit 6868d7f

Browse files
author
Juho Hanhimäki
authored
Fix AuthorizeViewCore.cs invalid rendering logic (#31794)
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.
1 parent fe4df69 commit 6868d7f

File tree

3 files changed

+106
-9
lines changed

3 files changed

+106
-9
lines changed

src/Components/Authorization/src/AuthorizeViewCore.cs

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ namespace Microsoft.AspNetCore.Components.Authorization
1515
public abstract class AuthorizeViewCore : ComponentBase
1616
{
1717
private AuthenticationState currentAuthenticationState;
18-
private bool isAuthorized;
18+
private bool? isAuthorized;
1919

2020
/// <summary>
2121
/// The content that will be displayed if the user is authorized.
@@ -54,11 +54,11 @@ protected override void BuildRenderTree(RenderTreeBuilder builder)
5454
{
5555
// We're using the same sequence number for each of the content items here
5656
// so that we can update existing instances if they are the same shape
57-
if (currentAuthenticationState == null)
57+
if (isAuthorized == null)
5858
{
5959
builder.AddContent(0, Authorizing);
6060
}
61-
else if (isAuthorized)
61+
else if (isAuthorized == true)
6262
{
6363
var authorized = Authorized ?? ChildContent;
6464
builder.AddContent(0, authorized?.Invoke(currentAuthenticationState));
@@ -85,13 +85,10 @@ protected override async Task OnParametersSetAsync()
8585
throw new InvalidOperationException($"Authorization requires a cascading parameter of type Task<{nameof(AuthenticationState)}>. Consider using {typeof(CascadingAuthenticationState).Name} to supply this.");
8686
}
8787

88-
// First render in pending state
89-
// If the task has already completed, this render will be skipped
90-
currentAuthenticationState = null;
88+
// Clear the previous result of authorization
89+
// This will cause the Authorizing state to be displayed until the authorization has been completed
90+
isAuthorized = null;
9191

92-
// Then render in completed state
93-
// Importantly, we *don't* call StateHasChanged between the following async steps,
94-
// otherwise we'd display an incorrect UI state while waiting for IsAuthorizedAsync
9592
currentAuthenticationState = await AuthenticationState;
9693
isAuthorized = await IsAuthorizedAsync(currentAuthenticationState.User);
9794
}

src/Components/Authorization/test/AuthorizeViewTest.cs

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,68 @@ public void RendersNothingUntilAuthorizationCompleted()
292292
});
293293
}
294294

295+
[Fact]
296+
public void RendersAuthorizingUntilAuthorizationCompletedAsync()
297+
{
298+
// Covers https://github.com/dotnet/aspnetcore/pull/31794
299+
// Arrange
300+
var @event = new ManualResetEventSlim();
301+
var authorizationService = new TestAsyncAuthorizationService();
302+
authorizationService.NextResult = AuthorizationResult.Success();
303+
var renderer = CreateTestRenderer(authorizationService);
304+
renderer.OnUpdateDisplayComplete = () => { @event.Set(); };
305+
var rootComponent = WrapInAuthorizeView(
306+
authorizing: builder => builder.AddContent(0, "Auth pending..."),
307+
authorized: context => builder => builder.AddContent(0, $"Hello, {context.User.Identity.Name}!"));
308+
309+
var authTcs = new TaskCompletionSource<AuthenticationState>();
310+
// Complete the authentication beforehand
311+
authTcs.SetResult(CreateAuthenticationState("Monsieur").Result);
312+
313+
rootComponent.AuthenticationState = authTcs.Task;
314+
315+
// Act/Assert 1: Auth pending
316+
renderer.AssignRootComponentId(rootComponent);
317+
rootComponent.TriggerRender();
318+
var batch1 = renderer.Batches.Single();
319+
var authorizeViewComponentId = batch1.GetComponentFrames<AuthorizeView>().Single().ComponentId;
320+
var diff1 = batch1.DiffsByComponentId[authorizeViewComponentId].Single();
321+
Assert.Collection(diff1.Edits, edit =>
322+
{
323+
Assert.Equal(RenderTreeEditType.PrependFrame, edit.Type);
324+
AssertFrame.Text(
325+
batch1.ReferenceFrames[edit.ReferenceFrameIndex],
326+
"Auth pending...");
327+
});
328+
329+
// Act/Assert 2: Auth process completes asynchronously
330+
@event.Reset();
331+
332+
// Wait for authorization to complete asynchronously
333+
@event.Wait(Timeout);
334+
335+
Assert.Equal(2, renderer.Batches.Count);
336+
var batch2 = renderer.Batches[1];
337+
var diff2 = batch2.DiffsByComponentId[authorizeViewComponentId].Single();
338+
Assert.Collection(diff2.Edits, edit =>
339+
{
340+
Assert.Equal(RenderTreeEditType.UpdateText, edit.Type);
341+
Assert.Equal(0, edit.SiblingIndex);
342+
AssertFrame.Text(
343+
batch2.ReferenceFrames[edit.ReferenceFrameIndex],
344+
"Hello, Monsieur!");
345+
});
346+
347+
// Assert: The IAuthorizationService was given expected criteria
348+
Assert.Collection(authorizationService.AuthorizeCalls, call =>
349+
{
350+
Assert.Equal("Monsieur", call.user.Identity.Name);
351+
Assert.Null(call.resource);
352+
Assert.Collection(call.requirements,
353+
req => Assert.IsType<DenyAnonymousAuthorizationRequirement>(req));
354+
});
355+
}
356+
295357
[Fact]
296358
public void RendersAuthorizingUntilAuthorizationCompleted()
297359
{
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using System.Security.Claims;
7+
using System.Threading.Tasks;
8+
using Microsoft.AspNetCore.Authorization;
9+
10+
namespace Microsoft.AspNetCore.Components.Authorization
11+
{
12+
public class TestAsyncAuthorizationService : IAuthorizationService
13+
{
14+
public AuthorizationResult NextResult { get; set; }
15+
= AuthorizationResult.Failed();
16+
17+
public List<(ClaimsPrincipal user, object resource, IEnumerable<IAuthorizationRequirement> requirements)> AuthorizeCalls { get; }
18+
= new List<(ClaimsPrincipal user, object resource, IEnumerable<IAuthorizationRequirement> requirements)>();
19+
20+
public async Task<AuthorizationResult> AuthorizeAsync(ClaimsPrincipal user, object resource, IEnumerable<IAuthorizationRequirement> requirements)
21+
{
22+
AuthorizeCalls.Add((user, resource, requirements));
23+
24+
// Make Authorization run asynchronously
25+
await Task.Yield();
26+
27+
// The TestAuthorizationService doesn't actually apply any authorization requirements
28+
// It just returns the specified NextResult, since we're not trying to test the logic
29+
// in DefaultAuthorizationService or similar here. So it's up to tests to set a desired
30+
// NextResult and assert that the expected criteria were passed by inspecting AuthorizeCalls.
31+
return NextResult;
32+
}
33+
34+
public Task<AuthorizationResult> AuthorizeAsync(ClaimsPrincipal user, object resource, string policyName)
35+
=> throw new NotImplementedException();
36+
37+
}
38+
}

0 commit comments

Comments
 (0)