I've pastied my entire baby names controller - yes it's me again, the
baby names man - getting excited but of course the thing needs to be
secured before it's properly public! Would be really grateful for some
help here as I believe this can be simplified / improved but I can't
quite get my head around the logic.
In the first instance I'd like to secure everything so that only the
people who created the lists can see em' or do anything with em' - I may
allow no logged in users access to certain views later, but not until
later.
General improvements/refactor required + do I need to secure the sort
methods?
routes are this - I should comment out lines 58/59 right ? the default
routes.
Most importantly the baby_names controller where I'm looking for help
with the authorisation method Especially lines 104 to 119, is it the
right thing to do and can it be simplified - I'm aware that
authorisation should be the 1st filter as it stops the app having to do
unnecessary work.
I have not looked at your code but assuming that User has_many
:baby_names then only allow a get of baby names to find those for the
current user. So specify a condition of baby_names.user is current
user in the find.
I was thinking more along the lines of
@baby_names = BabyName.find( :all, :conditions => { user_id =>
current_user.id } )
to get all of them, or
@baby_name = BabyName.find(params[:id], :conditions => { user_id =>
current_user.id }) if params[:id]
if you want to get just one.
In addition if you use a named scope then you don't have to keep
putting the conditions in. Also you might like to look at the
find_by_... methods. Have a look at the rails guide on the Active
Record Query Interface at http://guides.rubyonrails.org/
I was thinking more along the lines of
@baby_names = BabyName.find( :all, :conditions => { user_id =>
current_user.id } )
to get all of them, or
@baby_name = BabyName.find(params[:id], :conditions => { user_id =>
current_user.id }) if params[:id]
if you want to get just one.
That's helpful thanks - I get that; the thing is though I'm trying to
understand *where* to do these finds and how to make them so they don't
error if no params are supplied or the wrong params are supplied. In
plain terms, with this app, at least initially, I'd like to secure the
thing so only users have access to *their own* baby_names both
individually and as a list. I see how you're code does that.
Would I be best doing this/these finds in a before_filter that calls a
method say find_baby_names where in that method I check for supplied
params then find, or perhaps in two seperate before_filters, one for the
index action that gets all the @baby_names and one for the times when
there's only one name (e.g. an edit). Or maybe I've got the wrong end of
the stick and I should do these finds in the different controller
actions, e.g in index do the @baby_names = BabyName.find( :all,
:conditions => { user_id => current_user.id } ).
I'm guessing that a single before_filter is the DRY way to do it and
inherently more secure as I'll have all the logic in one place and it'll
get called every time for every action.
The other questions I have, how to deal with the sort actions and if I
need the protected and private keywords at all.
I was thinking more along the lines of
@baby_names = BabyName.find( :all, :conditions => { user_id =>
current_user.id } )
to get all of them, or
@baby_name = BabyName.find(params[:id], :conditions => { user_id =>
current_user.id }) if params[:id]
if you want to get just one.
That's helpful thanks - I get that; the thing is though I'm trying to
understand *where* to do these finds and how to make them so they don't
error if no params are supplied or the wrong params are supplied. In
plain terms, with this app, at least initially, I'd like to secure the
thing so only users have access to *their own* baby_names both
individually and as a list. I see how you're code does that.
Would I be best doing this/these finds in a before_filter that calls a
method say find_baby_names where in that method I check for supplied
params then find, or perhaps in two seperate before_filters, one for the
index action that gets all the @baby_names and one for the times when
there's only one name (e.g. an edit). Or maybe I've got the wrong end of
the stick and I should do these finds in the different controller
actions, e.g in index do the @baby_names = BabyName.find( :all,
:conditions => { user_id => current_user.id } ).
I'm guessing that a single before_filter is the DRY way to do it and
inherently more secure as I'll have all the logic in one place and it'll
get called every time for every action.
You can't do the finds in a filter because what you want to do depends
on the action (find all for index but only one for edit for example).
I would firstly make sure that nothing can be done without logging in
by using a before filter to check that, something like
before_filter :require_user
in application_controller. Then provide appropriate named scopes for
BabyNames that enforce the conditions and always use those rather than
the generic find. If you are not sure what named scopes you need then
initially just put the finds in line with the conditions and when you
find yourself repeating a find then convert it to a named scope.
Remember that the code is under your control. If you have no find
operations in the code that do not specify the user conditions then
there is no way a find can be performed using your app that does not
have that condition. Do a global search in your app for 'find' and
check they all have appropriate conditions.
Don't forget in your automated tests to make sure nothing can be done
without logging in and that if an attempt is made to get a name that
should not be accessible then it fails.
You can't do the finds in a filter because what you want to do depends
on the action (find all for index but only one for edit for example).
ok, understood - and that might sound obvious but it wasn't, I had
visions of doing something programtically in the before filter to check
what action was being done at the time and that seemed like a very odd
thing to do, I can see now that's not the way to go. ta.
I would firstly make sure that nothing can be done without logging in
by using a before filter to check that, something like
before_filter :require_user
OK, great - I get this and I'm doing it already, albeit in the
baby_names_controller, I'll move it to application controller and skip
it for the welcome controllers index action (the homepage which is
public). Makes perfect sense. I can see how this way the app should be
secured at one level - you need to have an account and be logged in to
do stuff.
Then provide appropriate named scopes for
BabyNames that enforce the conditions and always use those rather than
the generic find. If you are not sure what named scopes you need then
initially just put the finds in line with the conditions and when you
find yourself repeating a find then convert it to a named scope.
Remember that the code is under your control. If you have no find
operations in the code that do not specify the user conditions then
there is no way a find can be performed using your app that does not
have that condition. Do a global search in your app for 'find' and
check they all have appropriate conditions.
That's fine, will do that - but the way you put it sounds like I'd have
a lot of named_scopes to do this, in this case I can only think of two,
the find for a specfic baby name and the one to get them all. Actually
hold on *CORRECTION*, sorry I type as I think! That's right I'm doing
other finds as well, e.g. finding a users boy baby names and girl baby
names - I guess I should secure those also on the basis of the
current_user's boy baby names. Basically are you saying move all find to
the model and make sure they're secured there? Hmm what if I want some
or other baby_name on my home page, no model for that -> guess I can do
a find for that in the welcome controller, that's ok. Is the idea to get
all the finds into the model so I can be certain the user authorisation
is in place any time someone tries to find a record / records?
Don't forget in your automated tests to make sure nothing can be done
without logging in and that if an attempt is made to get a name that
should not be accessible then it fails.
V good idea. Will put this in place in line with the new find code will
check it fails without the current_user.
Then provide appropriate named scopes for
BabyNames that enforce the conditions and always use those rather than
the generic find. If you are not sure what named scopes you need then
initially just put the finds in line with the conditions and when you
find yourself repeating a find then convert it to a named scope.
Remember that the code is under your control. If you have no find
operations in the code that do not specify the user conditions then
there is no way a find can be performed using your app that does not
have that condition. Do a global search in your app for 'find' and
check they all have appropriate conditions.
That's fine, will do that - but the way you put it sounds like I'd have
a lot of named_scopes to do this, in this case I can only think of two,
the find for a specfic baby name and the one to get them all. Actually
hold on *CORRECTION*, sorry I type as I think! That's right I'm doing
other finds as well, e.g. finding a users boy baby names and girl baby
names - I guess I should secure those also on the basis of the
current_user's boy baby names. Basically are you saying move all find to
the model and make sure they're secured there?
By using named scopes you are putting the logic for the find into the
model, you still have to call the named scope from the controller. So
instead of
BabyName.find(:all, :conditions.....)
you use
BabyName.find_all_for_current_user
or whatever, where find_all_for_user is the named scope. The
controller code does not have to know how to fetch records for the
current user, it leaves that to the named_scope in the model, which is
as it should be.
By the way, when you code up the named scope you will have to use a
'lamba' for the current user, so that the query is reconstructed each
time you call it (otherwise it will be constructed when the model is
loaded, with the current user at that instant, which is not what you
want). Read up on named_scope and you will see how that works.
Understood, thanks for taking the time to explain this in such detail.
I'll start by making sure my find conditions in the existing controller
are correct and limited to current_user, after that I'll look at moving
them to the model - and sorry yes, I appreciate I'll call them from the
controller. Particular thanks for the lamba reference - I wouldn't have
know that and it would have undoubtadly got me !
