Rails - Aug 2010 - authlogic and controllers - plus general advice

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.

http://pastie.org/1076934

models

http://pastie.org/1076943

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.

Here''s the controller as it stands

http://pastie.org/1076947
-- 
Posted via http://www.ruby-forum.com/.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

Also,

Protected versus Private at the end there...

What''s the difference, and in this case should I be doing either, none 
or one or the other?

http://pastie.org/1076947


bb
-- 
Posted via http://www.ruby-forum.com/.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

If I can ask a more direct question.

How do I secure my controller so only baby name owners can access their 
baby names? Am I on the right lines?
-- 
Posted via http://www.ruby-forum.com/.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

On 5 August 2010 13:44, bingo bob
<lists-fsXkhYbjdPsEEoCn2XhGlw@public.gmane.org>
wrote:
> If I can ask a more direct question.
>
> How do I secure my controller so only baby name owners can access their
> baby names? Am I on the right lines?
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.

Colin

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

like this then? I''m checking for existence of id to deal with the index
action, i think this might be OTT though?
---

def find_baby_name
  @baby_name = BabyName.find(params[:id]) if params[:id]

  if @baby_name

    if @baby_name.user == current_user
      true
    else
      flash[:notice] = "Not authorised."
      redirect_to root_url
      return false
    end

  end

end

-- 
Posted via http://www.ruby-forum.com/.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

On 5 August 2010 14:47, bingo bob
<lists-fsXkhYbjdPsEEoCn2XhGlw@public.gmane.org>
wrote:
> like this then? I''m checking for existence of id to deal with the
index
> action, i think this might be OTT though?
> ---
>
> def find_baby_name
>  @baby_name = BabyName.find(params[:id]) if params[:id]
>
>  if @baby_name
>
>    if @baby_name.user == current_user
>      true
>    else
>      flash[:notice] = "Not authorised."
>      redirect_to root_url
>      return false
>    end
>
>  end
>
> end
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/

Colin

Colin

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

> 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.

Very grateful for any insight.
-- 
Posted via http://www.ruby-forum.com/.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

On 6 August 2010 12:48, bingo bob
<lists-fsXkhYbjdPsEEoCn2XhGlw@public.gmane.org>
wrote:
>> 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.

Colin
>
> The other questions I have, how to deal with the sort actions and if I
> need the protected and private keywords at all.
>
> Very grateful for any insight.
> --
> Posted via http://www.ruby-forum.com/.
>
> --
> You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
> To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.
>
>
-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

> 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.

bb
-- 
Posted via http://www.ruby-forum.com/.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

>...
>> 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.

Colin

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

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 !
-- 
Posted via http://www.ruby-forum.com/.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

and i think it''s lambda - not that i know what that is - will found
out,
seen them mentioned, Llama anyone. :-)
-- 
Posted via http://www.ruby-forum.com/.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

On 6 August 2010 16:21, bingo bob
<lists-fsXkhYbjdPsEEoCn2XhGlw@public.gmane.org>
wrote:
> and i think it''s lambda - not that i know what that is - will
found out,
> seen them mentioned, Llama anyone. :-)
Yes, lambda, typo on my part.

Colin

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.

> 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]
Hi all, I actually couldn''t get this approach to work so I did
something
which I think amounts to the same thing, potentially better?

For the actions where I just need one baby name, e.g. show - I do this.

@baby_name = current_user.baby_names.find(params[:id])

For the actions where I need many baby names, e.g. index - I do this.

@baby_names_boys = current_user.baby_names.boys(:order =>
''position'')

Just want to check that''s an ok way to do it (perhaps it''s the
best way
to do it) before I work on moving them to named_scopes in the model?
-- 
Posted via http://www.ruby-forum.com/.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.