before filter question

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

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

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

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.

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]

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.

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.

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

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.

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

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

See my first post.

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

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.