It’s AWESOME for authorization in phoenix 1.3. Highly recommended.
I do have one question that’s more about the docs and the 1.3 Phoenix release than anything else though.
Where do you CALL your authorization logic? The examples for bodyguard suggest doing so in the controller. That’s how things were done before, and it certainly keeps methods that otherwise don’t have any need for the user object to not require it. But it also makes it really easy to skip your authorization calls (inside or outside of your phoenix app). So it seems like forcing authorization would be ideal.
One other option would be to look at authorization from an AoP point of view. To do that effectively in phoenix, I think you’d need to find a way to store the current user on each call, and then wrap all of your auth-requiring methods in macros that will check your policies before executing the underlying code.
The negative to this approach is just that part about having to set ‘invisible’ data as context to a function. It never bothered me in OOP land, but it feels anti-functional here.
I perform validation right on the line before accessing a DB call or refining data based on some permission. I have a lot of lines like this scattered about (copy/pasted):
conn =
conn
|> Perms.verify(%Perms.SomeSection.Report{action: :view, report: report})
|> ensure!()
# Strip other things working on `conn`
That would assume a fairly standard, RESTful layout, but for most web apps, it wouldn’t be far off. However, in a context world I’m pretty sure the right direction is to enforce security at your context boundaries.
Ok, here’s a compromise option between AoP and functional.
defauth create_user(user_params) do
User.changeset(%User{}, user_params) |> Repo.insert
end
This would get compiled to this:
@spec create_user(User.t, map)
def create_user(%User{}=user, user_params) do
with :ok <- UserPolicy.authorize(:create_user, user, user_params) do
User.changeset(%User{}, user_params) |> Repo.insert
end
end
That way, the controller and everyone else is forced to provide that user for authorization, but doesn’t hide the user parameter and obfuscate how things work. And the context method gets to have a single responsibility.
Oh I don’t have it in the controller’s, it is in the modules that handle that necessary work. ^.^
Yeah that would not even remotely work here. I have to verify they have access to very specific things. Like even when I get rows back from a database I have to verify that they have permission to access specific rows so I Enum.filter a lot. Plugs entirely fail there.
Just to add to the point: I feel like I’ve gained superpowers the day I discovered I could check for permissions wherever I wanted and that a permission check is just an function that returns true or false (this was a long time ago, in python-land).
“Declarative permissions” and friends and all fine and good when it makes sense, but sometimes you just have to ask (the DB, the rules, the world, etc) if this user can do that, and often you only know exactly which question to ask just before you ask it.
Hi @jeffdeville – you’ve raised a good question. I went back-and-forth internally for a while about this. I think we can at least agree that, whether called internally or externally, the authorize/3 callback should exist directly on the context module itself, since it’s a context-specific API that determines what user can do.
Off the top of my head, here are some arguments for performing authorization in a controller action:
Reinforces the concept of controllers being the interface INTO your app – the first line of defense
Easier to call context methods from a privileged position (e.g. scheduled task, testing, etc) where we don’t care about authorization
Don’t need to pass the user (e.g. conn.assigns[:current_user]) into every context function if not needed (although lots of the time, we end up doing that anyway)
Easier to compose multiple context actions together
Leaner context functions that just “do the thing”
… and some arguments for performing authorization in a context function:
Strictly enforces authorization rules at application level – can’t skip it, no matter what. Arguably this is better design, and forces you to think about non-user-account cases like “guest” or “background task” users
Leaner controller actions
More flexibility to perform complex authorization rules, or change authorization rules without having to track down every context caller
So while I did pick controller-level authorization for the code examples, I don’t think it’s the One True Way, and the overall design is certainly up to you.
I think there is room in Bodyguard for a design element that gives you the best of both worlds – a way to perform authorization from within a context function (better design) but doesn’t require repetitive auth checks and passing the user model around everywhere (more convenient). I haven’t thought it through so I’m open to suggestions.
@schrockwell, if I took a stab at the macro described above (msg 4), and had it check the policies the way you have laid out, would you be interested in incorporating it into bodyguard? I believe it would meet the pros of both strategies with out the cons of either.
Implementation question. I can implement this as 1 method where the auth is baked in as in my description above, or I can implement it like this:
defauth create_user(user_params), do: stuff
compiling to:
def create_user(%User{} = user, user_params) do
with :ok <- authorize(:create_user, user, user_params) do
__create_user(user_params)
end
end
def __create_user__(user_params) do
User.changeset(%User{}, user_params) |> Repo.insert
end
Pros
Testing is easier, because you can write specs against __create_user__ for that functionality, and test the policy for the security separately.
Cons
It is possible to skip the auth check, but it’s pretty obvious you’re doing it.
I’m leaning toward this approach because I don’t worry about malicious developers using my libraries, and it feels like a reasonable balance of productivity and security, but I’m open to critiques.
There’s plenty of cleanup to do and docs to write, but the macro itself is the part I’m least confident of. Specifically, why did I have to var!(user) on line 80?
Is this for per-function testing before the body is called? I often have to check permissions inside the body, not knowing what I can check for until I get the query/queries/remote_response/etc back.
I think it is pretty common to have multiple layers of security. You’ve got high-level perimeter security - is this a known person and can he even access these resource types - and then you may have business logic that governs what you can do with an individual record. For example in an HR system - you are a manager so you can access the compensation page - but which employees can you see? Just the employees who report up to you.
Putting perimeter security in the controller - where we deserialize and sanitize all the untrusted input coming from the world - doesn’t sound like the wrong thing to me.
What about when you need to do the same security on a websocket connection? Or a TCP connection? Or something else? You start duplicating code. Better to put all work into a context (as phoenix calls it) including the authentication, then just call those.
They make some very good points about keeping it simple—even to the point where they don’t use frameworks at all. Particularly, I like:
Don’t create an endpoint that you don’t need. Eg. if you want to be sure the user can only see their own account, don’t create a GET /accounts/:id endpoint.
Build a separate app for different audiences. Eg. public facing app and a separate customer service app.
You got it.
