RoleService UpdatePermissions

[fretje]

https://github.com/fullstackhero/dotnet-webapi-boilerplate/blob/54d25010fb2e316adc7cf36bef8f6d4f8f6a9500/src/Infrastructure/Identity/RoleService.cs#L156

What exactly does that method do?

• (lines 186-190) It first deletes all permission claims from the role
• (lines 192-202) then it adds the selectedpermissions as claims to the role again (using an extension method which should probably just be in the service as it's only used here, or not even be a separate method at al?)
• (lines 204-212) then it goes over all the selected permissions again and checks if they are in the permissions of the role (they all should be as they were just added in the previous step?) and in that case they are saved once more using the roleclaimservice??

Isn't this last step redundant? This is doing 2 times the same thing, right? Or am I missing something?

@iammukeshm You can hopefully shed some light on this?

Also while we're at the subject, and also in light of PR #377 ... I think the role api should change a bit:

• GET api/role/{id}: would return the permissions included inside the RoleDto (we will probably need a new dto for that, or rename the existing one to RoleListDto). Just a List<string>. No need for the Enabled property.
• UpdatePermissionsRequest would also just contain a List<string> of permissions.

Side thought: We could even include the updatepermissions (optionally) into the PUT api/role/{id} call maybe?

That together with the static FSHPermissions strings (in the shared project) is enough to build the whole UI for it I think. No need to send "Enabled" properties over the wire... that's 100% a UI concern, right?

I even think we can eliminate the roleclaimservice. What else is that used for other than adding and removing permissions from roles? I would roll (no pun intended) all that functionality into the roleservice (which practically already is there) and just eliminate the roleclaimservice and -api?

To put it into DDD terms: it would be the "Role aggregate", which contains the permissions (roleclaims).

[PedroVentura235]

Hi, you can assign it into me , I can change it.

• Remove roleClaimService
• You mean remove the GetPermissions, and include this one in the main Get?
• UpdatePermission would only receive a List that would be the ones to have. Yes maybe is better. The code of the method were already there, but we can refactor it(the UpdatePermissions)

[fretje]

I can't assign issues myself... but you can go ahead... can use the existing pr... will review when ready ;-)

You mean remove the GetPermissions, and include this one in the main Get?

Yes, I think the get is only used to see detailed info of a role (otherwise the listdto (RoleDto now) is used to create/update roles atm) so I think it would fit perfectly...

Mind you, everything here is still up for discussion... just throwing my 2cts around ;-)

But I think we can all agree that the UpdatePermissions method is in need of a serious refactoring.

[iammukeshm]

@fretje , Yes, the last step is not needed there. maybe it's a piece of old code that went unnoticed. We can remove that.

Do you mean, a single request to update both the role details and the associated claim (permission)? Not sure of that. Currently, it's split up into 2 requests keeping security in mind. Someone who has access to view the Role details shouldn't be able to see the permissions too, in case.

[PedroVentura235]

@iammukeshm I Will review all the UpdatePermissions method, because i didnt touch it too much because i guess was already verified

[PedroVentura235]

@fretje Yes, is better to have a method only to GetPermissions, and another to GetRole, because in the Basic/Admin role we cant edit, but if we create another roles, we can edit them(name and description), so is better have seperated methods

[PedroVentura235]

PR updated

[fretje]

Ok, I understand that it need to be separate methods... but I would rename the GetPermissions method to GetByIdWithPermissions and return the same result but wrapped in a RoleDto... then there's no need to do 2 calls on the client. And still have the same protection by the FSHPermissions.RoleClaims.View permission...

[PedroVentura235]

Ok, that is a good way to do it yes