List: My question/concern is obvious, but just follow allow so we''re all on the same page. --------- The login generator handles all of my login magic and suggests adding this filter to controllers as necessary: before_filter :login_required, :only => [:create, :new, :edit, :destroy] This is great but a second level of security needs to be addressed. What happens if a user, who is logged in, tries to edit/update/destroy a record that is not theirs? (Of course such a link won''t exist but that doesn''t stop them from entering a URL manually.) Ie: User #1 is signed in: /blog/destroy/10 ## Ok, they own this /blog/destroy/11 ## They do not own this In these cases, the ''before_fitler :login_required'' isn''t enough. There needs to be a check on the ownership of the record and the logged in user. Right now, I had a second filter: before_filter :ownership_required, :only => [:edit, :update, :destroy] (defined in application.rb for the time being and not complete, but you get the idea) def ownership_required @object = eval( @params[:controller].capitalize ).find( @params[:id] ) if( @session[:user] != @object.user ) access_denied return false else return true end end This does the job... but is it clean (or possibly dangerous)? I figure an all-in-one solution probably cannot exist because the way developers tie ownership to models will differ. I''d rather not do an ownership check in each edit/destroy/update method either if it can be efficiently handled at a higher level as illustrated above. Second, instead of duplicating the two filter lines in every controller: before_filter :login_required, :only => [:create, :new, :edit, :destroy] before_filter :ownership_required, :only => [:edit, :update, :destroy] ...can I just toss those in Application.rb (Every controller''s CRUD methods should be validated/authenticated/authorized in the same way). I don''t see why this would be a problem (in terms of a best practice), but I''m new to all this. What do you think? How do others handle this? Thanks Adam _______________________________________________ Rails mailing list Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org http://lists.rubyonrails.org/mailman/listinfo/rails
Michael Schuerig
2005-Sep-29 20:02 UTC
Re: Best practice for security check on edit/update/destroy
On Thursday 29 September 2005 20:34, Adam Roth wrote:> What do you think? How do others handle this?Have a look. http://dev.rubyonrails.com/file/trunk/actionpack/lib/action_controller/verification.rb That may be -- part of -- what you need. I can''t find a link to the docs right now. Michael -- Michael Schuerig Failures to use one''s frontal lobes mailto:michael-q5aiKMLteq4b1SvskN2V4Q@public.gmane.org can result in the loss of them. http://www.schuerig.de/michael/ --William H. Calvin
Benjamin Curtis
2005-Sep-30 13:22 UTC
Re: Best practice for security check on edit/update/destroy
One way that you can do this that isn''t global (i.e., adding something to application.rb to handle all cases generically) but is pretty readable is to use the user object to find the related object. For example, if a User has_many Articles, you could do userobject.articles.find(:id), which will throw ActiveRecord::RecordNotFound if the article isn''t associated with the user, even if Article.find(:id) does find it. On Sep 29, 2005, at 11:34 AM, Adam Roth wrote:> List: My question/concern is obvious, but just follow allow so > we''re all on the same page. > --------- > > The login generator handles all of my login magic and suggests > adding this filter to controllers as necessary: > > before_filter :login_required, :only => > [:create, :new, :edit, :destroy] > > This is great but a second level of security needs to be addressed. > What happens if a user, who is logged in, tries to edit/update/ > destroy a record that is not theirs? (Of course such a link won''t > exist but that doesn''t stop them from entering a URL manually.) Ie: > > User #1 is signed in: > > /blog/destroy/10 ## Ok, they own this > /blog/destroy/11 ## They do not own this > > In these cases, the ''before_fitler :login_required'' isn''t enough. > There needs to be a check on the ownership of the record and the > logged in user. > > Right now, I had a second filter: > > before_filter :ownership_required, :only => > [:edit, :update, :destroy] > > (defined in application.rb for the time being and not complete, but > you get the idea) > > def ownership_required > @object = eval( @params[:controller].capitalize ).find( @params > [:id] ) > if( @session[:user] != @object.user ) > access_denied > return false > else > return true > end > end > > This does the job... but is it clean (or possibly dangerous)? I > figure an all-in-one solution probably cannot exist because the way > developers tie ownership to models will differ. I''d rather not do > an ownership check in each edit/destroy/update method either if it > can be efficiently handled at a higher level as illustrated above. > > Second, instead of duplicating the two filter lines in every > controller: > > before_filter :login_required, :only => > [:create, :new, :edit, :destroy] > before_filter :ownership_required, :only => > [:edit, :update, :destroy] > > ...can I just toss those in Application.rb (Every controller''s > CRUD methods should be validated/authenticated/authorized in the > same way). I don''t see why this would be a problem (in terms of a > best practice), but I''m new to all this. > > What do you think? How do others handle this? > > Thanks > Adam > _______________________________________________ > Rails mailing list > Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org > http://lists.rubyonrails.org/mailman/listinfo/rails >
Tom Mornini
2005-Sep-30 15:46 UTC
Re: Best practice for security check on edit/update/destroy
I posted this to the list a few weeks ago with zero response:> Subject: Link security via crypto hash? > > in view: > > <%= link_to ''Delete'', :controller => ''users'', > :action => :delete, > :id => @user, > :secure => true %> > > Will generate a link similar to this: > > http://0.0.0.0:3000/users/delete/1? > security=7382038b2148cdc312c5a3fbaaf6bd427cd42511 > > And, if that controller and method have > > before_filter :check_security > > Then the link would only be allowed if it had not been > changed by the user, else it would redirect to a fixed > controller and action. > > That eliminates the need to be obsessive about verifying > that the session identified user actually owns the > account number 1, since the end user could not have > modified the URL or generated a new one of their own. > > This is just proto code, it''s just feedback on the idea > that I''m interested in. > > Thanks!Code: application.rb> # Before filter > > def check_security(*args) > args[0] ||= ''security'' > args[1] ||= :violation > > set_session_security > > path_info = @request.env[''PATH_INFO''] > sess_sec = session[:security] > > p_sec = params[:security] || '''' > u_sec = Digest::SHA1.hexdigest(path_info + sess_sec) > > if p_sec != u_sec > redirect_to :controller => args[0], :action => args[1] > end > end > > # Override for Rails frameword method > > def url_for(args) > set_session_security > if args[:secure] > args.delete(:secure) > url = super > url += ''?security='' + Digest::SHA1.hexdigest(url + session > [:security]) > else > super > end > end > > def set_session_security > if session[:security].nil? > key = Time.now.to_s > 1.upto(8) { key += rand.to_s } > session[:security] = Digest::SHA1.hexdigest(key) > end > end-- -- Tom Mornini On Sep 30, 2005, at 6:22 AM, Benjamin Curtis wrote:> One way that you can do this that isn''t global (i.e., adding > something to application.rb to handle all cases generically) but is > pretty readable is to use the user object to find the related object. > > For example, if a User has_many Articles, you could do > userobject.articles.find(:id), which will throw > ActiveRecord::RecordNotFound if the article isn''t associated with > the user, even if Article.find(:id) does find it. > > On Sep 29, 2005, at 11:34 AM, Adam Roth wrote: > > >> List: My question/concern is obvious, but just follow allow so >> we''re all on the same page. >> --------- >> >> The login generator handles all of my login magic and suggests >> adding this filter to controllers as necessary: >> >> before_filter :login_required, :only => >> [:create, :new, :edit, :destroy] >> >> This is great but a second level of security needs to be >> addressed. What happens if a user, who is logged in, tries to edit/ >> update/destroy a record that is not theirs? (Of course such a link >> won''t exist but that doesn''t stop them from entering a URL >> manually.) Ie: >> >> User #1 is signed in: >> >> /blog/destroy/10 ## Ok, they own this >> /blog/destroy/11 ## They do not own this >> >> In these cases, the ''before_fitler :login_required'' isn''t enough. >> There needs to be a check on the ownership of the record and the >> logged in user. >> >> Right now, I had a second filter: >> >> before_filter :ownership_required, :only => >> [:edit, :update, :destroy] >> >> (defined in application.rb for the time being and not complete, >> but you get the idea) >> >> def ownership_required >> @object = eval( @params[:controller].capitalize ).find( @params >> [:id] ) >> if( @session[:user] != @object.user ) >> access_denied >> return false >> else >> return true >> end >> end >> >> This does the job... but is it clean (or possibly dangerous)? I >> figure an all-in-one solution probably cannot exist because the >> way developers tie ownership to models will differ. I''d rather not >> do an ownership check in each edit/destroy/update method either if >> it can be efficiently handled at a higher level as illustrated above. >> >> Second, instead of duplicating the two filter lines in every >> controller: >> >> before_filter :login_required, :only => >> [:create, :new, :edit, :destroy] >> before_filter :ownership_required, :only => >> [:edit, :update, :destroy] >> >> ...can I just toss those in Application.rb (Every controller''s >> CRUD methods should be validated/authenticated/authorized in the >> same way). I don''t see why this would be a problem (in terms of a >> best practice), but I''m new to all this. >> >> What do you think? How do others handle this? >> >> Thanks >> Adam >> _______________________________________________ >> Rails mailing list >> Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org >> http://lists.rubyonrails.org/mailman/listinfo/rails >> >> > > _______________________________________________ > Rails mailing list > Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org > http://lists.rubyonrails.org/mailman/listinfo/rails >