before filter question

11155 · September 2, 2011, 8:35am

I have a correct_user() before filter that passes all my tests:

class UsersController < ApplicationController before_filter :authenticate, :only => [:edit, :update] before_filter :correct_user, :only => [:edit, :update]

... ...

private def correct_user user = User.find(params[:id]) redirect_to(root_path) unless get_user_from_session == user end

But if I change the before filter to this:

def correct_user redirect_to(root_path) unless get_user_from_session.id.to_s == params[:id] end

all kinds of things start failing. Here's an example:

1) UsersController GET edit should be successful Failure/Error: response.should be_success expected success? to return true, got false # ./spec/controllers/users_controller_spec.rb:15:in `block (3 levels) in <top (required)>'

and the test:

describe UsersController do render_views

describe "GET edit" do before(:each) do @user = Factory(:user) test_sign_in(@user) end

it "should be successful" do get :edit, :id => @user response.should be_success end

What is the difference between:

def correct_user user = User.find(params[:id]) redirect_to(root_path) unless get_user_from_session == user end

and:

def correct_user redirect_to(root_path) unless get_user_from_session.id.to_s == params[:id] end

Jim_Ruther_Nill · September 2, 2011, 10:06am

I have a correct_user() before filter that passes all my tests:

class UsersController < ApplicationController

before_filter :authenticate, :only => [:edit, :update]

before_filter :correct_user, :only => [:edit, :update]

…

…

private
def correct_user

  user = User.find(params[:id])

  redirect_to(root_path) unless get_user_from_session == user

end
But if I change the before filter to this:

def correct_user
  redirect_to(root_path) unless get_user_from_session.id.to_s ==
params[:id]
end
all kinds of things start failing. Here’s an example:

UsersController GET edit should be successful

Failure/Error: response.should be_success

expected success? to return true, got false

./spec/controllers/users_controller_spec.rb:15:in `block (3

levels) in <top (required)>’

and the test:

describe UsersController do

render_views

describe “GET edit” do
before(:each) do

  @user = Factory(:user)

  test_sign_in(@user)

end



it "should be successful" do

  get :edit, :id => @user

  response.should be_success

end
What is the difference between:
def correct_user

  user = User.find(params[:id])

  redirect_to(root_path) unless get_user_from_session == user

end
and:

def correct_user
  redirect_to(root_path) unless get_user_from_session.id.to_s ==
params[:id]
end

params[:id] may not always be an integer, it can be ‘123-user’

11155 · September 2, 2011, 11:07am

Jim ruther Nill wrote in post #1019766:

params[:id] may not always be an integer, it can be '123-user'

I don't understand how that is relevant.

11155 · September 2, 2011, 11:26am

My logic in the correct_user() before filter is: why should I have to search the database for the user:

user = User.find(params[:id])

when I can just compare the id of the user in the session:

get_user_from_session.id.to_s

to the id in the url:

params[:id]

Jim_Ruther_Nill · September 2, 2011, 11:48am

My logic in the correct_user() before filter is: why should I have to

search the database for the user:

user = User.find(params[:id])

when I can just compare the id of the user in the session:

get_user_from_session.id.to_s

to the id in the url:

params[:id]

i’m assuming that get_user_from_session returns a User object.

get_user_from_session.id.to_s # User with ID = 1

User.find(‘1-user-name’) # User with ID = 1

so comparing params[:id] with the User ID converted to string will not always be equal.

11155 · September 2, 2011, 11:58am

Jim ruther Nill wrote in post #1019785:

to the id in the url:

params[:id]

i'm assuming that get_user_from_session returns a User object.

Yes.

get_user_from_session.id.to_s # User with ID = 1 User.find('1-user-name') # User with ID = 1

so comparing params[:id] with the User ID converted to string will not always be equal.

I understand what you are saying, I just don't see how it's relevant to my problem.

Frederick_Cheung · September 2, 2011, 12:17pm

>>> get_user_from_session.id.to_s # User with ID = 1 >>> User.find('1-user-name') # User with ID = 1

> so comparing params[:id] with the User ID converted to string will not > always be equal.

I understand what you are saying, I just don't see how it's relevant to my problem.

I suspect that if you inspected the two things you are comparing you'd see the relevance. I assume this is not using rails 3.1 ?

Fred

11155 · September 2, 2011, 12:49pm

Frederick Cheung wrote in post #1019792:

>>> get_user_from_session.id.to_s # User with ID = 1 >>> User.find('1-user-name') # User with ID = 1

> so comparing params[:id] with the User ID converted to string will not > always be equal.

I understand what you are saying, I just don't see how it's relevant to my problem.

I suspect that if you inspected the two things you are comparing you'd see the relevance. I assume this is not using rails 3.1 ?

Fred

rails 3.0.9

Your response does point out a difference in the two code snippets I posted:

case 1: get_user_from_session.id.to_s == params[:id]

***user1 signs in, which puts his id in the session***

If a malicious user goes to the edit page, and see this url in the address bar:

http://localhost:3000/users/1/edit

and changes that to:

http://localhost:300/users/1-user-name/edit

and refreshes the page, he will be **redirected** because:

get_user_from_session.id.to_s will equal '1'

and:

params[:id] will equal '1-user-name'

case 2: get_user_from_session == User.find(params[:id])

***user1 signs in, which puts his id in the session***

If a malicious user goes to the edit page, and see this url in the address bar:

http://localhost:3000/users/1/edit

and changes that to:

http://localhost:300/users/1-user-name/edit

and refreshes the page, he will be **shown his own edit** page again because:

get_user_from_session will return user1

and:

User.find(params[:id]) will return user1

I really don't care what happens in that case, and I don't understand why that would cause my tests to fail.

With both versions of the code, if the user changes the url from:

http://localhost:3000/users/1/edit

to:

http://localhost:3000/users/4/edit

then he will be redirected and not allowed to edit user4's info.

Frederick_Cheung · September 2, 2011, 1:12pm

User.find(params[:id]) will return user1

I really don't care what happens in that case, and I don't understand why that would cause my tests to fail.

You're missing the point. The point is that in your tests params[:id] isn't a string (because pre rails 3.1 rails didn't force this and would allow you to create params that couldn't exist in the real world)

Fred

11155 · September 3, 2011, 5:14am

Frederick Cheung wrote in post #1019805:

User.find(params[:id]) will return user1

I really don't care what happens in that case, and I don't understand why that would cause my tests to fail.

You're missing the point.

Apparently.

The point is that in your tests params[:id] isn't a string

The term params[:id] doesn't appear in my tests, so I have no idea what you are talking about. And in the before filter, I've showed that params[:id] is a string:

Session id: ****1**** Sesssion id class: Fixnum Params id: ****3**** Params id class: String

Frederick_Cheung · September 3, 2011, 8:03am

Frederick Cheung wrote in post #1019805:

>> User.find(params[:id]) will return user1

>> I really don't care what happens in that case, and I don't understand >> why that would cause my tests to fail.

> You're missing the point.

Apparently.

> The point is that in your tests params[:id] > isn't a string

The term params[:id] doesn't appear in my tests, so I have no idea what you are talking about.

I was talking about the value of params[:id] in your controller action when the test is run.

And in the before filter, I've showed that params[:id] is a string:

Session id: ****1**** Sesssion id class: Fixnum Params id: ****3**** Params id class: String

Was this output produced when running the tests or when using the page from a browser?

Fred

11155 · September 4, 2011, 12:01pm

See my first post.

11155 · September 7, 2011, 2:23pm

Anyone have any explanation for the behavior I'm seeing?

Colin_Law1 · September 7, 2011, 2:51pm

What behaviour? You have not quoted any previous message. Looking back at the thread though I see you do not appear to have answered Fred's last questions. If he cannot help you then I doubt whether anyone here can.