JonPSmith / AuthPermissions.AspNetCore

This library provides extra authorization and multi-tenant features to an ASP.NET Core application.
https://www.thereformedprogrammer.net/finally-a-library-that-improves-role-authorization-in-asp-net-core/
MIT License
764 stars 155 forks source link

Fixed a bug where TenantRoles would give an exception if the _tenantRoles was null #43

Closed emorell96 closed 2 years ago

emorell96 commented 2 years ago

The TenantRoles would give an exception if the Tenant has no roles yet, since it calls .ToList() on something that may very well be null in this case.

Not sure if this is the right branch to merge into, maybe dev would be a better fit?

JonPSmith commented 2 years ago

Hi @emorell96,

Thanks for the pull request. Version 3.2.1 includes your fix, which is in NuGet now.

JonPSmith commented 2 years ago

Hi @emorell96,

Having run some tests I find that your pull request shouldn't have been added. The design of my applications are such that if a collection isn't loaded, then the collection should be null. This means if I forget to add an Include then the code will fail with a null exception. This approach has saved me many times.

It's my fault for not testing this change. I have reversed the pull request in the new version I am working on and will be released in version 3.2.2

emorell96 commented 2 years ago

Maybe you can tell me if the way I saw this was wrong but I had this exception when trying to adapt the librabry to be used with an API.

So I was thinking of creating an admin API, one that would allow my app admins to add users, add roles, add tenants etc...

But I started getting this issue when I added a user without roles, and later on I wanted to add roles to the users.

To do this I did the following:

[HttpPost]
        public async Task<ActionResult> PostAsync([FromBody] AuthUserDTO user, CancellationToken cancellationToken = default)
        {
            Guard.IsNotNull(user);
            Guard.IsNotNullOrEmpty(user.UserId);
            if(user.UserName == null && user.Email == null)
            {
                return BadRequest($"{nameof(user.UserName)} and {nameof(user.Email)} cannot both be null at the same time.");
            }

            var result = await authUsersAdminService.AddNewUserAsync(user.UserId, user.Email, user.UserName, user.RoleNames, user.TenantName, cancellationToken).ConfigureAwait(false);
            if (result.IsValid)
            {
                return Ok(result.Message);
            }
            return BadRequest(result.GetAllErrors());

        }

And then using another controller I would add a role:

[HttpPost("add")]
        public async Task<ActionResult> AddRoleToUserAsync([FromQuery][Required] string userId, [FromQuery][Required] string roleName, CancellationToken cancellationToken = default)
        {
//this would fail if the user doesn't have any roles at this point.
            var userResult = await authUsersAdminService.AddRoleToUserAsync(userId, roleName, cancellationToken);
            if (userResult.IsValid)
            {
                return Ok(userResult.Message);
            }
            return BadRequest(userResult.GetAllErrors());

        }

How would you achieve this without having an exception because the collection wasn't loaded at this point? Is there a better way to achieve this?

JonPSmith commented 2 years ago

For me to debug your code you need to send me: