Recently at work I was tasked to fix a bug that was occuring when a user would get assigned new roles. In our product we use roles to dictate what actions a user can do within the app. A user obtains a role either via an administrator assigning them a role or they can request a role and that request can later be approved by an administrator. Whenever a role gets assigned, the user's role requests for that role should also be approved. We have 2 main workflows where a user may be assigned a role:
- An administrator edits the user and assigns the role to a user via the user interface.
- An administrator can import a csv file with the user's email and their roles to be assigned.
The bug we were encountering was that in the bulk import workflow the user's requests were not getting auto approved. The product would then say the user had the role but was also requesting it. I discovered that both of the workflows were implemented individually and were both directly modifying the backing entity records. There was no role assignment workflow which would encapsulate all of the logic.
First, I want to go through the history of how these features were implemented and then show you how we can implement this better to prevent similar bugs in the future. The first thing to be implemented was the single user role assignment via our UI. I've left out some error handling and other things just for brevity. First here is our User class, it is a standard JPA entity with an Id and a List of roles that they have:
class User {
private Long id;
private List<Role> roles;
public void setRoles(List<Role> roles) {
this.roles = roles;
}
public List<Role> getRoles() {
return this.roles;
}
}Next here is the UI controller that did the user saving and role assignment:
class UserController {
public Result saveUser(Request request, long id) {
User currentUser = currentUser();
boolean isAdmin = currentUser.getRoles()
.stream()
.anyMatch(Role::isAdmin);
if (!isAdmin)
return badRequest();
var user = findUserById(id);
if (user == null)
return badRequest();
if (currentUser.equals(user))
return badRequest();
var roles = parseRolesFromRequest(request);
user.setRoles(roles);
user.save();
return ok();
}
}As you can see from this controller code there are a few requirements:
- You must be an administrator to assign roles.
- You cannot assign roles to yourself.
- You must have a user to assign roles to (this is an obvious one).
This feature was shipped to production and was working exactly to the business specification. The business then realized that since this was a brand new application and we would be onboarding thousands of users they needed an easier way to create users in bulk. This is when the bulk import feature was created. Below is the code that was written for it:
class AdministrationController {
public Result bulkImportUsers(Request request) {
User currentUser = currentUser();
boolean isAdmin = currentUser.getRoles()
.stream()
.anyMatch(ProductRole::isAdmin);
if (!isAdmin)
return badRequest();
Collection<User> users = new ArrayList<User>();
for (Row row : csvFromRequest(request)) {
String email = row.get("email");
String roles = row.get("roles");
User user = findByEmail(email);
if (user == null) {
user = new User();
user.setEmail(email);
}
// Users can't assign roles to themselves.
if (currentUser.equals(user))
continue;
user.setRoles(parseRoles(roles));
user.add(user);
}
Database.saveAll(users);
}
}As you can see the second developer wrote very similar code as the first to enforce the system requirements of only allowing an administrator to assign roles and not allowing them to assign them to themselves. As of now there are no bugs between the features because they are doing the exact same thing. Next our business wanted to provide the ability for users to request roles for themselves and an administrator can see a marker in the ui and assign them the role. When a role is assigned any requests for the user should be marked as approved. Here is how that was implemented by a third developer:
class User {
private Long id;
private List<Role> roles;
private List<RoleRequest> roleRequests;
public List<Role> getRoles() {
return this.roles;
}
public void setRoles(List<Role> roles) {
this.roles = roles;
}
public List<RoleRequest> getRoleRequests() {
return this.roleRequests;
}
public void setRoleRequests(List<RoleRequest> roleRequests) {
this.roleRequests = roleRequests;
}
}
class RoleRequest {
private Long id;
private User user;
private Role role;
private Status status;
enum Status {
PENDING, DENIED, APPROVED;
}
public void setUser(User user) {
this.user = user;
}
public User getUser() {
return this.user;
}
public void setRole(Role role) {
this.role = role;
}
public Role getRole() {
return this.role;
}
public void setStatus(Status status) {
this.status = status;
}
public Status getStatus() {
return this.status;
}
}
class UserController {
public Result saveUser(Request request, long id) {
User currentUser = currentUser();
boolean isAdmin = currentUser.getRoles()
.stream()
.anyMatch(Role::isAdmin);
if (!isAdmin)
return unauthorized();
var user = findUserById(id);
if (user == null)
return badRequest();
if (currentUser.equals(user))
return badRequest();
var roles = parseRolesFromRequest(request);
user.setRoles(roles);
for (var request : user.getRoleRequests()) {
if (roles.contains(request.getRole()))
request.setStatus(Status.APPROVED);
}
user.save();
return ok();
}
}This code was shipped to production without knowledge of the bulk import way of assigning roles, resulting in the bug of requests not being auto approved [0]. I think we could have prevented this by using object oriented techniques and encapsulating the business roles within the type system. If you remember when the role assignment feature was first developed we had 2 rules:
- You must be an administrator to assign roles.
- You cannot assign roles to yourself.
When the first developer encountered these business rules they should have encapsulated them:
class User {
private Long id;
private List<Role> roles;
public List<Role> getRoles() {
// DESIGN DECISION: We can't let anyone outside of our class modify the roles
// because that could break the encapsulation of our business rules so an
// immutable list is returned. Additionally, the raw setRoles function is removed.
return Collections.unmodifiableList(roles);
}
// DESIGN DECISION: Since only another user can assign roles, the function to assign roles goes on the User class.
// This is a natural spot because it matches the language the business would use when describing it: "When a user assign roles..."
// and it is a natural spot to check the users priviledges.
// DESIGN DECISION: SelfAssignmentException and AuthorizationException are checked exceptions so the compiler enforces callers
// to handle them. We'll see how this plays out in the controller code later.
public void assignRoles(User user, List<Role> roles) throws SelfAssignmentException, AuthorizationException {
// RULE #1: You must be an admin to assign roles.
if (!this.isAdmin())
throw new AuthorizationException();
// RULE #2 you cannot assign roles to yourself
if (this.equals(user))
throw new SelfAssignmentException();
user.roles = roles;
}
public boolean isAdmin() {
return this.roles.stream().anyMatch(Role::isAdmin);
}
}
class UserController {
public Result saveUser(Request request) {
var user = findUserById(id);
if (user == null)
return badRequest();
var roles = parseRolesFromRequest(request);
var currentUser = currentUser();
try {
currentUser.assignRoles(user, roles);
} catch (SelfAssignmentException ex) {
return badRequest();
} catch (AuthorizationException ex) {
return unauthorized();
}
user.save();
return ok();
}
}Now that the rules are encapsulated the second developer wouldn't have had to reimplement them and there would be no way for them not to enforce them. They only had to worry about what happens if one of those rules were violated:
class AdministrationController {
public Result bulkImportUsers(Request request) {
User currentUser = currentUser();
Collection<User> users = new ArrayList<User>();
for (Row row : csvFromRequest(request)) {
String email = row.get("email");
String roles = row.get("roles");
User user = findByEmail(email);
if (user == null) {
user = new User();
user.setEmail(email);
}
try {
currentUser.assignRoles(user, parseRoles(roles));
} catch (SelfAssignmentException ex) {
return badRequest();
} catch (AuthorizationException ex) {
return unauthorized();
}
user.add(user);
}
Database.saveAll(users);
}
}Finally the third developer can come along and implement their request approval feature without introducing a bug into the system:
class User {
private Long id;
private List<Role> roles;
private List<RoleRequest> roleRequests;
public List<Role> getRoles() {
return Collections.unmodifiableList(roles);
}
public void assignRoles(User user, List<Role> roles) throws SelfAssignmentException, AuthorizationException {
// RULE #1: You must be an admin to assign roles.
if (!this.isAdmin())
throw new AuthorizationException();
// RULE #2 you cannot assign roles to yourself
if (this.equals(user))
throw new SelfAssignmentException();
user.roles = roles;
// RULE #3 All requests with matching roles must be set to approved.
// The single user assignment and bulk assignment workflows both get this logic for free
// because we encapsulated the role assignment workflow in this function.
for (var request : this.roleRequests) {
if (roles.contains(request.getRole())
request.setStatus(Status.APPROVED);
}
}
public boolean isAdmin() {
return this.roles.stream().anyMatch(Role::isAdmin);
}
}[0] There were actually other bugs. We had previously added analytics around what roles were being assigned, more rules around who can assign roles, and there were several bugs in the bulk import around user creation, but thats a topic for a different post.