Feature/inf 143

[ctucker3]

Adds system roles to bi-api. POST /users accepts roles with the request, or no roles with the request. A new endpoint, PUT /users/{userId}/roles has been added for modifying user system roles. The argument for the PUT system roles endpoint is the new state of the user's roles (Ex. If an empty list is passed, all roles for that user are deleted).

A new exception was added AuthorizationException for the case where the user tries to modify their own roles.

Disclaimers

This branch introduces breaking changes for bi-web. The program roles endpoints where changed to be /programs/roles instead of /roles. The fixes for bi-web will be done in the next feature, INF-94:

https://trello.com/c/h0aJaCnM/197-inf-94-admin-management-improvements

This card is dependent on INF-95 and will need changes once INF-95 is merged.

Review

• Do the endpoints behave as expected?
• Is test coverage complete?
• Do the tests run?
• General code flow and clarity

[nickpalladino]

Will all users have a system role? I see right now there's just an admin role and absence would indicate they aren't an admin. I understand the issues with trying to manage the roles in one endpoint and think breaking it out as you have makes sense. Looks like you used the database tables we were talking about and that all seems good.

[ctucker3]

Will all users have a system role? I see right now there's just an admin role and absence would indicate they aren't an admin. I understand the issues with trying to manage the roles in one endpoint and think breaking it out as you have makes sense. Looks like you used the database tables we were talking about and that all seems good.

Yeah I know we talked about having a 'User' and 'Admin' role. But the 'User' role seemed redundant given that we are adding deactivation in the future. It's definitely something we can add in the future if we find that we need it.

[timparsons]

Will all users have a system role? I see right now there's just an admin role and absence would indicate they aren't an admin. I understand the issues with trying to manage the roles in one endpoint and think breaking it out as you have makes sense. Looks like you used the database tables we were talking about and that all seems good.

Yeah I know we talked about having a 'User' and 'Admin' role. But the 'User' role seemed redundant given that we are adding deactivation in the future. It's definitely something we can add in the future if we find that we need it.

From a clarity standpoint, I think it would be nice to have every user have a system role, even if it is a minimum of user.

[timparsons]

In regards to separating out updates to the User from updates to the user's Role, I agree with that approach for update only. The POST to /users, however, should also include the Role. This eliminates an unneeded dependency on the network/user's browser in between creating the user and updating their role, thus eliminating the potential for invalid state in which a user exists in the system, but doesn't have a role.

[ctucker3]

Will all users have a system role? I see right now there's just an admin role and absence would indicate they aren't an admin. I understand the issues with trying to manage the roles in one endpoint and think breaking it out as you have makes sense. Looks like you used the database tables we were talking about and that all seems good.

Yeah I know we talked about having a 'User' and 'Admin' role. But the 'User' role seemed redundant given that we are adding deactivation in the future. It's definitely something we can add in the future if we find that we need it.

From a clarity standpoint, I think it would be nice to have every user have a system role, even if it is a minimum of user.

Do you additional thoughts on the additional clarity that having the user role for every users would add? I think for the program users have a 'member' role made sense because it assigned a program user to a program. But for the system user, I'm still not seeing the benefit of having user role in addition to the activate/deactivate field.

[ctucker3]

INF-143 - Admin User Management system role support

[nickpalladino]

Some of the program controller integration and units tests are failing for me

[nickpalladino]

Still reviewing but a couple comments so far

[ctucker3]

Some of the program controller integration and units tests are failing for me

My IntelliJ must have been in a weird state where it was using an old version of the application. I'm getting the errors in the tests now. I will fix those tests and get them up.

[timparsons]

Will all users have a system role? I see right now there's just an admin role and absence would indicate they aren't an admin. I understand the issues with trying to manage the roles in one endpoint and think breaking it out as you have makes sense. Looks like you used the database tables we were talking about and that all seems good.

Yeah I know we talked about having a 'User' and 'Admin' role. But the 'User' role seemed redundant given that we are adding deactivation in the future. It's definitely something we can add in the future if we find that we need it.

From a clarity standpoint, I think it would be nice to have every user have a system role, even if it is a minimum of user.

Do you additional thoughts on the additional clarity that having the user role for every users would add? I think for the program users have a 'member' role made sense because it assigned a program user to a program. But for the system user, I'm still not seeing the benefit of having user role in addition to the activate/deactivate field.

@ctucker3

By having a role for every user, I was thinking that it'd be more explicit to know what roles a user has. When defining role-based access for resources in our controllers, we can say what explicit roles are allowed to access a resource, i.e. @Secured(Role.ADMIN) for admin-only resources or @Secured(Role.USER) for authenticated user resources (both admin and user roles would be able to access the resource). As I was thinking through this response, I do realize that @Secured(SecurityRule.IS_AUTHENTICATED) is equivalent to @Secured(Role.USER), so I can see the case for using that instead of having every user have a user role.

[ctucker3]

By having a role for every user, I was thinking that it'd be more explicit to know what roles a user has. When defining role-based access for resources in our controllers, we can say what explicit roles are allowed to access a resource, i.e. @Secured(Role.ADMIN) for admin-only resources or @Secured(Role.USER) for authenticated user resources (both admin and user roles would be able to access the resource). As I was thinking through this response, I do realize that @Secured(SecurityRule.IS_AUTHENTICATED) is equivalent to @Secured(Role.USER), so I can see the case for using that instead of having every user have a user role.

@timparsons

Yeah that was my thought with the user role, is that it isn't conveying a role or permissions, but rather that they are a user, which is already done with them having a user model and being active. It will also be one less thing to manage (albeit, it's a tiny thing to manage) to not have a required user role. Are you okay with leaving the required user role out for now and revisiting later if necessary?

[timparsons]

By having a role for every user, I was thinking that it'd be more explicit to know what roles a user has. When defining role-based access for resources in our controllers, we can say what explicit roles are allowed to access a resource, i.e. @Secured(Role.ADMIN) for admin-only resources or @Secured(Role.USER) for authenticated user resources (both admin and user roles would be able to access the resource). As I was thinking through this response, I do realize that @Secured(SecurityRule.IS_AUTHENTICATED) is equivalent to @Secured(Role.USER), so I can see the case for using that instead of having every user have a user role.

@timparsons

Yeah that was my thought with the user role, is that it isn't conveying a role or permissions, but rather that they are a user, which is already done with them having a user model and being active. It will also be one less thing to manage (albeit, it's a tiny thing to manage) to not have a required user role. Are you okay with leaving the required user role out for now and revisiting later if necessary?

@ctucker3 That works for me!

[dmeidlin]

The user creation endpoint with a system role as admin works as described in the docs and the tests pass. Looks good!