Clean Up Identity

[PedroVentura235]

Clean up Role Claims Service, and think of use user service or role Service.
Check if the save, delete methods are really necessary

[fretje]

While we're at it, I think the whole Identity feature is in need of some cleanup...

• What exactly is the difference between UserService and IdentityService (or their respective controllers)? What are their responsibilities? Seems like there's quite some overlap between the two. Maybe we should merge them? Or otherwise split them up differently and call IdentityService something else as IdentityService is a really bad name in this context...

• UserService has a GetRolesAsync(string userid) method and RolesService has a GetUserRolesAsync(string userId) method... the latter doesn't seem to be used.

• UserService.GetPermissionsAsync uses ApplicationDbContext while it might as well use RoleManager (or roleservice for that matter).

• Everywhere where ApplicationDbContext is used in RoleService can be replaced with using RoleManager in stead.

• RoleService.UpdatePermissionsAsync checks if the current user is in the admin role by getting the user from the usermanager and then checking with usermanager again if that user is in the admin role. Isn't this what _currentUser.IsInRole is made for? Why jumping through all those hoops?

• Make routes / method names more consistent (not necessarily only identity but across all controllers/services):

  • register => Create (generally use Create for POST calls)
  • getall/search => GetList (generaly use GetList for GET calls)
  • update calls should take in id on route and compare it with id in dto (like brands and products controllers do now)
  • remove => Delete (generally use Delete for DELETE calls)

• And while we're at it, do the same for the permissions

  • register => create
  • search => list
  • remove => delete

• there are still some places where cancellationtokens should be passed

The way I see it, RoleService should only use RoleManager and UserService should only use UserManager and if UserService needs something from RoleManager it should delegate to the RoleService service and vice versa... keeping responsabilities separate ;-)

[fretje]

Just remarked by robsworld on discord:

api/users/{id}/permissions

has [MustHavePermission(FSHPermissions.RoleClaims.View)]

but this call is executed after the user logs in to retrieve its permissions, so as a new user doesn't have the FSHPermissions.RoleClaims.View permission, this call fails.

My first response was: There's 2 solutions... adding FSHPermissions.RoleClaims.View to the base role permissions, or simply not requiring a permission for this method (which is the better choice IMO as a user should always have permission to retrieve its own permissions, right?)

On second thought, this method should probably not take in a userId and take the userId from the currentUser, similar like how Identity.GetProfileDetailsAsync works.

I have an idea about the names... why don't we rename IdentityService and -controller to AccountService and -controller? Move some stuff over to the usercontroller and keep only the stuff that's related to a specific user account in there?

This UserService.GetPermissions method would then move to AccountService and -controller.

[fretje]

Also: IdentityController.RegisterAsync has [MustHavePermission(FSHPermissions.Identity.Register)].

Shouldn't that also allow anonymous, if you want to enable a "register as new user" button on the login screen?

That would mean building in extra security measures (as a captcha for instance) as you wouldn't want that endpoint to be spammed (not sure how that works for an api)?
And that would also probably mean creating another endpoint (in the usercontroller) for creating new users from the admin view?

Maybe this issue should be renamed to "Clean Up Identity"... I've gathered quite some points here already... if they all could have some attention, I think we can make a very nice and clean integration here ;-)

[fretje]

RoleService.DeleteRoleAsync

gets a list of all (!) users and then checks for each user if userManager.IsInRoleAsync(user, rolename) (and then doesn't even shortcuts when it finds one that still uses it).

I don't think this is gonna scale very well when the list of users gets bigger...

Hint: UserManager also has a GetUsersInRoleAsync method ;-)

[fretje]

Having the Description property on the ApplicationRoleClaim entity doesn't really make sense...

See also fullstackhero/blazor-wasm-boilerplate#94

[xlogex]

Also: IdentityController.RegisterAsync has [MustHavePermission(FSHPermissions.Identity.Register)].

Shouldn't that also allow anonymous, if you want to enable a "register as new user" button on the login screen?

That would mean building in extra security measures (as a captcha for instance) as you wouldn't want that endpoint to be spammed (not sure how that works for an api)? And that would also probably mean creating another endpoint (in the usercontroller) for creating new users from the admin view?

Maybe this issue should be renamed to "Clean Up Identity"... I've gathered quite some points here already... if they all could have some attention, I think we can make a very nice and clean integration here ;-)

it could be a good idea to add external new registration (in anonymos mode) ...

[snax4a]

I agree with all Freije's points and I think this Identity clean up / refactoring should have some high priority before 0.0.6-rc / 1.0 release.

[CanMehmetK]

For Tenants permissions may differ ... also writing in to code that permissions costantly is good but not nicest way.

my suggestion would be dynamic claim permission management with predefined one
Still wee have routing,controller,action definitions with authorize decorator and we can detect it at runtime..

So give sceens with a decription (easyly can read xml documentaion at runtime also that we created while building)

Here is our old imlementation ... sorry for language... it is Turkish

ApplicationController.txt

[fretje]

Please keep this issue for cleanup only... new feature requests should go into their own issue...

I just saw by reviewing #437 that the whole AssignRolesAsync method has a bit of the same issues - design wise - like we had with the AddPermissionsAsync method.
To keep things consistent, we should also remove the "enabled" property from the roledto, and only send over the roles which you want to assign to the user...

[fretje]

How about we model the permissions something like this:

public static class FSHAction
{
    public const string View = nameof(View);
    public const string Search = nameof(Search);
    public const string Create = nameof(Create);
    public const string Update = nameof(Update);
    public const string Delete = nameof(Delete);
    public const string Export = nameof(Export);
    public const string UpgradeSubscription = nameof(UpgradeSubscription);
}

public static class FSHResource
{
    public const string Tenants = nameof(Tenants);
    public const string Dashboard = nameof(Dashboard);
    public const string AuditLogs = nameof(AuditLogs);
    public const string Hangfire = nameof(Hangfire);
    public const string Identity = nameof(Identity);
    public const string Users = nameof(Users);
    public const string Roles = nameof(Roles);
    public const string Products = nameof(Products);
    public const string Brands = nameof(Brands);
}

public record FSHPermission(string Name, string Action, string Resource, bool IsBasic = false, bool IsRoot = false);

public static class FSHPermissions
{
    private static readonly FSHPermission[] _all = new FSHPermission[]
    {
        new("View Dashboard", FSHAction.View, FSHResource.Dashboard),
        new("View AuditLogs", FSHAction.View, FSHResource.AuditLogs),
        new("View Hangfire", FSHAction.View, FSHResource.Hangfire),
        new("Create Identity", FSHAction.Create, FSHResource.Identity),
        new("View Users", FSHAction.View, FSHResource.Users),
        new("Search Users", FSHAction.Search, FSHResource.Users),
        new("Create Users", FSHAction.Create, FSHResource.Users),
        new("Update Users", FSHAction.Update, FSHResource.Users),
        new("Delete Users", FSHAction.Delete, FSHResource.Users),
        new("Export Users", FSHAction.Export, FSHResource.Users),
        new("View Roles", FSHAction.View, FSHResource.Roles),
        new("Create Roles", FSHAction.Create, FSHResource.Roles),
        new("Update Roles", FSHAction.Update, FSHResource.Roles),
        new("Delete Roles", FSHAction.Delete, FSHResource.Roles),
        new("View Products", FSHAction.View, FSHResource.Products, IsBasic: true),
        new("Search Products", FSHAction.View, FSHResource.Products, IsBasic: true),
        new("Create Products", FSHAction.View, FSHResource.Products),
        new("Update Products", FSHAction.View, FSHResource.Products),
        new("Delete Products", FSHAction.View, FSHResource.Products),
        new("View Brands", FSHAction.View, FSHResource.Brands, IsBasic: true),
        new("Search Brands", FSHAction.Search, FSHResource.Brands, IsBasic: true),
        new("Create Brands", FSHAction.Create, FSHResource.Brands),
        new("Update Brands", FSHAction.Update, FSHResource.Brands),
        new("Delete Brands", FSHAction.Delete, FSHResource.Brands),
        new("View Tenants", FSHAction.View, FSHResource.Tenants, IsRoot: true),
        new("Create Tenants", FSHAction.Create, FSHResource.Tenants, IsRoot: true),
        new("Update Tenants", FSHAction.Update, FSHResource.Tenants, IsRoot: true),
        new("Delete Tenants", FSHAction.Delete, FSHResource.Tenants, IsRoot: true),
        new("Upgrade Tenant Subscription", FSHAction.UpgradeSubscription, FSHResource.Tenants, IsRoot: true)
    };

    public static IReadOnlyList<FSHPermission> Root { get; } = new ReadOnlyCollection<FSHPermission>(_all.Where(p => p.IsRoot).ToArray());
    public static IReadOnlyList<FSHPermission> Admin { get; } = new ReadOnlyCollection<FSHPermission>(_all.Where(p => !p.IsRoot).ToArray());
    public static IReadOnlyList<FSHPermission> Basic { get; } = new ReadOnlyCollection<FSHPermission>(_all.Where(p => p.IsBasic).ToArray());
}

Would be much easier to maintain I think... also no need to use reflection to get to the descriptions...

I'll have a look if it's a lot of work to change the rest of the code to support this...

We can then probably do something like [MustHavePermission(FSHAction.Delete, FSHResource.Brands)]...