That seems sort of backwards to me. These aren’t the user’s invoices,
right? They’re just invoices which the user happens to be allowed to
see? Chaining it this way makes it look like the invoices belong to
the role, and seems put the user up front instead of the invoices.
You also have conditional branching with hardcoded values, making the
controller brittle, and some redundancy with the controller asking the
model for a value and then passing the value right back to the model.
Can a model have more than one role? If it can, you have a problem
here because the ‘if’ will only ever act on one role. If it can’t,
life gets simple:
[controller]
def index @invoices = Invoice.by_role(user)
rescue AccessDenied
flash[:warning] = “Nope. Sorry. Nice try.”
redirect_to :back
end
[Invoice model]
def by_role(user)
case user.role
when “admin”
[whatever]
when “associate”
[whatever]
else
raise AccessDenied
end
end
…That could probably still be made more elegant. I’m not a huge fan
of case statements, and I have in my head some idea that you could
have named scopes for each role and use “send” to pick the right
scope. But that could be overdesigning it, and in any case I don’t
really know what each role has to return in your business case.
The important takeaway here is that the Invoice is responsible for
figuring out what to return, based on user data passed to it at
runtime; the User doesn’t have to have special logic to know how to
query every other model in the system.
those requirements are met.
method of User.
In our controller, we could have something like:
For me it’s a little win, that results in big ways.
Forgot to mention what we did do. We ended up with the following…
def index
if user.has_role?(“admin”)
user.in_role(“admin”).invoices
elsif user.has_role?(“associate”)
user.in_role(“associate”).invoices
else
raise AccessDenied
end
end
To us, the change here is subtle, but important. The controller is
allowed to ask for invoices from each role, but is not allowed to know
how find the invoices, that’s the behaviour of the role.
Half of my problem right now is that I’m not even sure what layer to put
model specific authentication! If it’s in the controller layer, it’s
repeated logic in every controller that touches the model in question. If
it’s in the model, the logic is centralized, but now your model needs not
only to know about Users in general, it needs a specific user.
My two cents:
1.) I feel authorization belongs in two places: models and views.
Models need to know what they’re allowed to do. Authorization becomes
a scope on reads and a validation on updates. Views (specifically
helper methods) need to know whether they’re allowed to show that
“Edit” button, etc. That’s not critical path, that’s navigation,
maybe one step up from cosmetics. I don’t see a reason why
controllers need to know as long as they can handle nils coming back
from the models.
2.) In both cases, you need to know who the current user is, and
that’s fine. Figuring it out is the job of authentication, not
authorization. Your authentication stack just needs to give you a
hook where you can ask it for the user, and your authorization stack
should handle it sensibly if the answer is ‘nil.’ The
restful_authentication plugin implements current_user as a global
Application method, but that’s not the only way to do it.
3.) Consider separating the authorization stuff from the core business
logic of your app, and implementing it as a module on the authorizable
classes instead. Then if your basic authz behavior changes, you
(ideally) only have to change it in one place. And it doesn’t mess up
the readability of your main behavior.
…And because everyone else is doing it, here are my design notes on
my own overcomplicated authorization system (which, caveat, has yet to
be built, but I wrote this all down anyway to get it out of my head):
On Mon, Mar 2, 2009 at 11:35 PM, Stephen E. [email protected] wrote:
raise AccessDenied
end
end
That seems sort of backwards to me. These aren’t the user’s invoices,
right? They’re just invoices which the user happens to be allowed to
see? Chaining it this way makes it look like the invoices belong to
the role, and seems put the user up front instead of the invoices.
I agree, it could be better.
You also have conditional branching with hardcoded values, making the
controller brittle, and some redundancy with the controller asking the
model for a value and then passing the value right back to the model.
I would like to push down the conditional logic to a lower part of the
app and there probably is a way that I haven’t found yet with the
technique I’m using. Right now I’m exploring some trade-offs. Put more
emphasis on the responsibility of a role or on the responsibility of
the model? I’ve been down the model route, and would like to see where
the role route takes me.
redirect_to :back
raise AccessDenied
end
end
…That could probably still be made more elegant. I’m not a huge fan
of case statements, and I have in my head some idea that you could
have named scopes for each role and use “send” to pick the right
scope. But that could be overdesigning it, and in any case I don’t
really know what each role has to return in your business case.
I’ve been down this path many times. It has worked well when the
privilege/role set was limited and fairly simple, but seems to leave
models convoluted for anything else. That’s what sparked me to explore
concretely identifying the Role in my app, and allowing it to make
decisions that are unique to it, rather than sprinkling them
throughout the models.
The important takeaway here is that the Invoice is responsible for
figuring out what to return, based on user data passed to it at
runtime; the User doesn’t have to have special logic to know how to
query every other model in the system.
To clarify, the User doesn’t know how-to query anything. All it knows
is if it can fulfill a Role. If it can it returns the appropriate role
object. Each role is responsible for knowing what it can access, but
it doesn’t do the nitty gritty work. It calls methods on other models.
For the Invoice example, the associate role calls
Invoice.by_area(user.area), whereas the Admin role calls Invoice.all
in each of their respective #invoices methods.
Some of what has piqued my interest in exploring apps that place more
emphasis on Roles and Privileges is that it it’s difficult to
understand what it means to be an admin when what it means to be able
to to be an “admin” or “supervisor” or a “team supervisor” or an
“employee” is sprinkled throughout the app. So far I have enjoyed
having the responsibility of an admin in one spot, even though that
Admin object doesn’t deal with the details. It just knows how-to make
a decision and then it tells another object to do it. It’s kind of
like inserting a thin-layer of roles and privileges between the
application layer and the domain layer. I’d say these sit on the top
end of the domain layer.
As you pointed out earlier, the API for #in_role could use a little
love.
It doesn’t seem right to me that invoices know about users and roles.
I would try something like
user.role.invoices
Heh. Which is what Zach said he wanted to do, and it isn’t wrong.
But it doesn’t seem right to me that roles know about invoices.
8-> As I see it, if you go that path you end up having to inform
roles about every other model, and consolidating all your business
logic in one class. At which point you’re risking drifting away from
object-oriented programming and heading back to big procedures and
spaghetti code.
But that’s a kneejerk response, and it needn’t be that bad. It really
comes down to taste. To-mae-to, to-mah-to. Sooner or later it’s all
ones and zeros.
It doesn’t seem right to me that invoices know about users and roles.
I think of invoices are being closer to the metal – closer to the
essence of the application – than petty concerns like authorization.
I would try something like
user.role.invoices
where Role is a model that does the traffic-cop work of deciding what
invoices are available to it.
Heh. Which is what Zach said he wanted to do, and it isn’t wrong.
Actually, I thought Zach was talking about a method on User called
in_role.
But it doesn’t seem right to me that roles know about invoices.
Roles know about access to invoices. That’s what their purpose is - to
let people do some things and not let them do others.
8-> As I see it, if you go that path you end up having to inform
roles about every other model, and consolidating all your business
logic in one class.
You do consolidate all your access logic in one class, just as you
might consolidate all your sales tax knowledge in another class. That
way you have one source of responsibility for that behavior.
Otherwise, if you added, changed or deleted a role, you’d have to
change every model.
This is basically the Mediator pattern. Pluses and minuses, to be sure.
I think this discussion has gone backwards a bit. Here is what I think
the
index method in the invoices controller should be like
def index
begin
invoice.get_collection
rescue
# decide what to do if we can’t get collection
end
end
Now clearly this needs some work to get it to work …
What is ‘invoice’
Rails by default ties ‘invoice’ to a class in app/model. Usually this
ActiveRecord model class, but it doesn’t have to be. We can always put
another layer inbetween (e.g. Presenter) if it makes our code simpler
Authentication parameters
Clearly these need to be passed through to get_collection. This can be
done
by parameters or by making the authentication available in a wider
context.
Exceptions
We need an exception hierarchy. NotAuthorised, NotFound etc.
All the controller should do is get the collection and deal with
exceptions
if the collection is not available. (n.b. the collection being empty is
not
exceptional.)
Rails historically has corrupted (compromised, polluted …) MVC by
allowing
concerns of how we get the collection to be included in the controller.
RESTful design has highlighted the problems with this and now we end up
with
this situation where things like authentication and authorisation don’t
really have an obvious place.
These things - authentication, authorisation and the exception handling
(for
the resource) - are services which all resources need access to. They
need
to be seperated and applied in a cross-cutting manner. Perhaps we could
do
things more elegantly with an Aspect Orientated solution.
Now clearly this needs some work to get it to work …
What is ‘invoice’
Rails by default ties ‘invoice’ to a class in app/model. Usually this
ActiveRecord model class, but it doesn’t have to be. We can always put
another layer inbetween (e.g. Presenter) if it makes our code simpler
Presenters are for presentation logic, not for authorization concerns.
Authentication parameters
Clearly these need to be passed through to get_collection. This can be done
by parameters or by making the authentication available in a wider context.
Authentication isn’t the issue here, authorization is.
concerns of how we get the collection to be included in the controller.
RESTful design has highlighted the problems with this and now we end up with
this situation where things like authentication and authorisation don’t
really have an obvious place.
It is up to the controllers to know how-to ask for something, it
should not know how the internals of that request work. For a
controller to know what role can access a particular resource is
completely aligned with a layered architecture, keeping application
concerns in the right layer, and domain logic in another layer.
These things - authentication, authorisation and the exception handling (for
the resource) - are services which all resources need access to. They need
to be seperated and applied in a cross-cutting manner. Perhaps we could do
things more elegantly with an Aspect Orientated solution.
Is some cases yes, but I fail to see where it benefits or adds more
value then concretely identifying a role that has particular behavior.
You do consolidate all your access logic in one class, just as you
might consolidate all your sales tax knowledge in another class. That
way you have one source of responsibility for that behavior.
Otherwise, if you added, changed or deleted a role, you’d have to
change every model.
This is basically the Mediator pattern. Pluses and minuses, to be sure.
This is not where I am spending my time at the moment, but it is an area
that i am going to confront sooner than later. So, I am interested in
this subject.
If I understand aright, it is agreed that authorization is best
determined by the user model, either as a full-blown authorization
method wholly contained in the User class or as a call to a subordinate
Authorization class which does the heavy lifting and then returns the
result to the User class. The controllers (and perhaps models) that
require authorization simply call the equivalent to an “authorized?”
method on the current_user passing the context, whether this be a named
role or a control/action pair or a model/attribute pair. The
User.authorized? method simply replies whether this current_user belongs
to the named role or otherwise has the privilege of performing the
desired action.
If there is an Authorization class then this is where the work is done
(lookups on models or tables as the case may be).
Does this capture the essence?
The question that I have is: If one implements an authorization system
as a model or models then would this best be implemented in the user as
an association rather than as a separate Authorization class?
That seems sort of backwards to me. These aren’t the user’s invoices,
right? They’re just invoices which the user happens to be allowed to
see? Chaining it this way makes it look like the invoices belong to
the role, and seems put the user up front instead of the invoices.
You also have conditional branching with hardcoded values, making the
controller brittle, and some redundancy with the controller asking the
model for a value and then passing the value right back to the model.
By saying that models need the following case statement
def by_role(role
case role
when “associate”
…
when “admin”
…
end
your implying that the model is represented in a different way depending
on
the role. So in your case of invoices the associate’s view is different
from
the administrators. If this is the case then you have identified two
distinct resources!
Currently you are representing these two resources with one url e.g.
/invoices
However if you think of these as seperate resources then you have two
url’s
/admin_invoices /associatie_invoices
Other url schems come fo mind /admin/invoices /invoices/admin etc.
Point is that you can implement these seperate resources in the standard
rails way and remove any need for case statements
class AdminInvoicesController
before_filter must_be_admin
def index
Invoices.find # specify admin criteria here
end
end
class AssociateInvoices
before_filter must_be_associate
def index
Invoices.find # specify associate criteria here
end
end
I agree I’m taking a step back to try to make two steps forward. The
heart of the exploration is more than the conditional in the action
(which simply states which roles are allowed to access that action, I
just made it explicit rather than using a #restrict_to call). To me
this discussion is about where the behaviour for a role should go and
how-to access that behaviour.
The flip side of this is that models end up with redundant conditional
branches to do x for RoleA or y for RoleB (for every model thats
affected by a Role). I would like to collapse these conditional
branches and attempt a more polymorphic approach by extracting a class
representative of each role, and simply calling the method in
question. e.g:
Rather than the following approach. Keep in mind that roles hardly
ever are limited to one model. Consider having the case statement in
20 models. Where’s the redundancy now?
Invoice.by_role(<role_name>)
def by_role(role
case role
when “associate”
…
when “admin”
…
end
