-1
\$\begingroup\$

This is my first WIP backend asp.net application, which I will pair with react to create a full stack app. The thing I'm mostly worried about is model conversion to DTO and custom policy authorization, as It has 2 contexts I feel like it's bloated, and hacked. I will be grateful for any tips, from experienced developers, as I have no real commercial experience, and I am self-taught.

Custom authorization:

public class FamilyHeadOnlyHandler : AuthorizationHandler<FamilyHeadOnlyRequirement> {


        private readonly IHttpContextAccessor _httpContextAccessor;
        private readonly FamilyManDbContext _databaseContext;
        private readonly UserManager<ApplicationUser> _userManager;

        public FamilyHeadOnlyHandler(
            IHttpContextAccessor httpContextAccessor,
            FamilyManDbContext databaseContext,
            UserManager<ApplicationUser> userManager
        ) {
            _httpContextAccessor = httpContextAccessor;
            _databaseContext = databaseContext;
            _userManager = userManager;
        }

        protected override async Task HandleRequirementAsync(
            AuthorizationHandlerContext context, FamilyHeadOnlyRequirement requirement) {

            var currentUserId = context.User.FindFirst(c => c.Type == ClaimTypes.NameIdentifier)?.Value;

            var requestedFamilyId = _httpContextAccessor.HttpContext!.GetRouteValue("id")!.ToString();
            var requestedFamily = await _databaseContext.Families!.Include("Head").FirstOrDefaultAsync(f => f.Id.ToString() == requestedFamilyId);
            if (requestedFamily == null) {
                throw new NotFoundException("Family not found.");
            }


            if (requestedFamily!.Head!.Id == currentUserId) {
                context.Succeed(requirement);
            }

        }
       
    }

I'm aware, that most people use Allman indentation convention, but I'm used to C, C++ and JavaScript's K&R standard. And I might need to start Allman in my C# code.

Link to the repo, which contains full code: Github repo

Edit: AuthorizationHandlerContext context is default context of overriden method, it has currentUser property, but lacks things such as route value, ability to manipulate cookies, and so on, unlike HttpContextAccessor. Now the second thing is, userManager is part of IdentityContext, which is in my databaseContext. I probably could use _dataBaseContext instead of _userManager to manipulate user, but the _userManager is built in, whereas databaseContext is my own context. It feels like bloated code to me. I would like to know, if this is good by design, or should I change it. Is the code readable?

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Could you tell us more about these two contexts like what they are for etc? I'd also like to point that we only review the code inside the question. The link to the repository is fine for reference but if there are any other code parts you'd like us to look at, please add them to the question. \$\endgroup\$
    – t3chb0t
    Jan 3 at 20:39
  • \$\begingroup\$ @t3chb0t i updated my question \$\endgroup\$ Jan 3 at 21:03
0
\$\begingroup\$

First of all welcome to CodeReview!

Most of the time the Authorization handlers are not issuing any kind of external (either database or service) calls since they are time consuming and fragile (due to transient network issues).

On the other hand they are usually checking the existence of a particular Claim or Fact of the provided Identity. Like in case of Role based authorization they are checking whether or not the Identity does possess the required role or permission to execute the requested action.


If you need to perform some lookups because your access control is more granular then consider to utilize some caching to speed up subsequent requests. The first request will issue an external / internal call but the following requests could be served based on cached value.

But be caution with this. It might happen that based on your cached value you can say that the requester can gain access but if you would perform a database/service lookup then his or her request would be denied.

Based on your requirements (like how frequently does the authorization data change) and based on your cache invalidation logic you should be able to determine whether or not caching is a viable option for you. You might also need to put into the account whether the requester can cause harm if you false postively grants access to him/her.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.