Async Authorization Check

[pbros]

Hello,

I've started implementing authorization in the navigation tree, and I noticed that the ShouldAllowView() method in INavigationNodePermissionResolver is a sync method making async calls.

var authResult = _authorizationService.AuthorizeAsync(_httpContextAccessor.HttpContext.User, menuNode.Value.AuthorizationPolicy).GetAwaiter().GetResult();

Since this is called a lot when building the navigation, would it not be more performant to have this method async? I haven't yet looked at the impact of making this async, but it might trickle up to a lot of other methods which should be async as well.

This might be a can of worms, but I was wondering if it had been considered.

Thank you

[joeaudette]

yeah it was considered, it would be a major breaking change, therefore a can of worms. Originally we only had filtering by roles so use of authorization policy was added later and role checks didn't need async, think it would be very chaotic to try and change it to async at this point. I will review/reconsider it again when I get a chance, but I recall deciding against that change. The tree is cached after it is built and perf seems fine to me.

[joeaudette]

after more consideration I've decided to make this change which will be a major breaking change that will require anyone who is using custom navigation views to update their views to await methods that are now async.

Specifically the changes are:
INavigationNodePermissionResolver ShouldAllowView method is now Task of bool instead of bool.
In addition the following methods on NavigationViewModel change from bool to Task of bool and must be awaited when invoked from the views:
ShouldAllowView
HasVisibleChildren

to make it clear this is a major update to the library I will bump the version up to 3.0.0

[pbros]

What made you decide to make everything async after all?

[joeaudette]

Well, because I just watched this week asp.net community standup and they were talking about how bad it is to do sync over async https://www.youtube.com/watch?v=wPWqTw2GB3U&feature=em-lbcastemail

even though it has not seemed to cause any bottlenecks in my projects I just think for the longer term it is the right thing to do to make this change.