It’s time for everyone’s favorite game show, What’s Wrong with this Code! Today we are going to look at some code that is supposed to check if the logged in Vault user is an administrator or not.
Have a look at the below code and try to spot all the things wrong with it.
|
// C# public bool IsAdmin(WebServiceManager mgr) { long userid = mgr.SecurityService.SecurityHeader.UserId;
Role[] roles = mgr.AdminService.GetRolesByUserId(userid);
return roles.Any(n => n.Name == "Administrator"); } |
|
' VB.NET Public Function IsAdmin(mgr As WebServiceManager) As Boolean
Dim userid As Long = mgr.SecurityService.SecurityHeader.UserId
Dim roles As Role() = mgr.AdminService.GetRolesByUserId(userid)
Return roles.Any(Function(n) n.Name = "Administrator") End Function |
You can assume good data is being passed in. You can also assume that unexpected errors, such as network failures, are handled in a higher level catch block.
Continue on when you think you found all the errors…
Error 1: There should be a check to see if userid is valid. In the case of an anonymous connection, userid will be 0. All valid User IDs are greater than 0.
Error 2: GetRolesByUserId will fail unless the user is logged in as an administrator.
Error 3: For non-English installs, the role name will probably not be “Administrator”.
I’ll be the first to admit that correct code is not very elegant. The core Vault framework doesn’t really have an administrator concept. Instead it just understands that users have groups of permissions. “Administrator” is a higher level concept, and is a collection of almost every permission.
So the permission set is what we need to look at, and we do that with GetPermissionsByUserId. Non-admins can call this function as long as they are calling it for their own user ID. Once we get the permission set, we check for a permission that only the admin group has. Permission IDs don’t change between vaults, so this is one of the few cases where it’s OK hard-code an ID.
Here is the correct code:
|
// C# public bool IsAdmin(WebServiceManager mgr) { long userid = mgr.SecurityService.SecurityHeader.UserId; if (userid <= 0) return false;
Permis[] permissions = mgr.AdminService.GetPermissionsByUserId(userid);
// check for AdminUserRead permission ID=82 return permissions.Any(n => n.Id == 82); } |
|
' VB.NET Public Function IsAdmin(mgr As WebServiceManager) As Boolean Dim userid As Long = mgr.SecurityService.SecurityHeader.UserId If userid <= 0 Then Return False End If
Dim permissions As Permis() = _ mgr.AdminService.GetPermissionsByUserId(userid)
' check for AdminUserRead permission ID=82 Return permissions.Any(Function(n) n.Id = 82) End Function |
Leave a Reply