I have a server with many methods that can be requested by a user (or other service). I want to implement a role based access control. I can think of 2 ways to do it.
-
Inline: Each method has
checkRole(req.role, 'some role')as its first line. -
Centralized: I have an object called
RoleMethodsthat has, for each role, the list of allowed methods. Each request, before being routed to the method, will first callcheckRoleAndMethod(req.role, req.method).
(I’m guessing these are the 2 ways to do it, if they are bad and there’s another better way please tell)
- Pros of inline: each method explicitly says what role it requires, making it clear and readable
- Cons of inline: devs have to include the line in every method, causing some repetition and they may forget it causing vulnerabilities.
- Pros of centralized: avoids the cons of inline by centralizing into one step
- Cons of centralized: the role for each method is defined outside of it, making it harder to grasp and follow the logic. You’ll need extra middleware to extract and check the method name (instead of just routing using a lib like apollo server). Devs may forget to include a method name, leading to the method being unusable.
Which way is considered best practice among professionals? Are there any other pros and cons I’m missing?
I personally prefer inline because I think the permission level is something that belongs in the method itself, and having to keep track of an extra RoleMethods object is annoying.
3
The problem I see with variation #1 is the use of stringly-typed code. This isn’t completely fatal to your application, though. Some application frameworks practically mandate a string. For example, the AuthorizeAttribute in C# allows you to decorate a controller method with the names of the roles allowed to execute that method. The C# language requires a compile-time constant, and the framework literally asks for a string. There are use cases for passing a string for the role.
Variation #2 is better. Presumably you are using enums or constants, but the problem I see here is how messy the code gets if all invocations of checkRoleAndMethod are being passed hard-coded values. If you are not careful, passing constants or enums can allow a caller to accidentally pass invalid combinations of roles and methods when the intent is to allow authorization.
I’ve found it useful to create explicit methods for authorizing a user. These methods would be named after the operation you are authorizing, for example, “canEditBlogPost”. The arguments you pass to these methods would be some minimal information about the user so you can access the claims or roles they have. The “canEditBlogPost” method can hard-code the names of the roles it requires inside the mthod body. Callers should remain largely ignorant of which roles are needed. They simply ask this authorizer whether the specified user can perform a specific action.
This structure allows you to organize your code into cohesive classes to encapsulate permission checks. It prevents callers from passing a bunch of constants and enums around, and certainly prevents blatantly invalid combinations of roles and actions. A good IDE will guide the code author using auto-complete. Pulling these permission checks into their own classes gives you a way to write proper unit tests, provided you inject interfaces for dependencies that require file system, network, or database access.
This does not work so well if a framework or use case requires the name of the role or method to be parameterized. Note that having a method with parameters doesn’t mean it needs to be parameterized. If you are calling checkRoleAndMethod(RoleMethod.editBlogPost, Role.foo), then the code author has already made the decision about which role and method is being checked. It is much easier and more expressive to simply write canEditBlogPost(user).
An ideal solution should:
- Reduce or eliminate the need for hard-coding constants or enum values.
- Facilitate unit testing and mocking.
- Prevent code authors from accidentally specifying the wrong roles for a particular action.
- Make it easy and expressive for callers.