Best Practices: a strong case for attr_accessible part 2

Posted by trevor Wed, 24 May 2006 22:31:58 GMT

This is a followup of part 1 which you should read before continuing here.

So… are the requirements satisfied? No.

It’s possible for any user to delete a project that they don’t actually own. More specifically, it’s possible for a user to arbitrarily assume ownership of any project they know the id of. After that, the project is theirs to do with as they choose.

The code that allows you to assume project ownership is here:

class UserController < ApplicationController
  def save
    current_user.update_attributes(params[:user])
  end
end

class User < ActiveRecord::Base
  attr_protected :is_administrator
  has_many :projects
end

Spotted it yet?

Read more...

Posted in code | 9 comments

Best Practices: a strong case for [redacted] part 1

Posted by trevor Wed, 24 May 2006 03:10:00 GMT

Tonight I decided to tip over my blog-post idea pickle jar and see if anything interesting fell out. This is the first one that didn’t get crumpled up and thrown in the trash (gotta love that pickle jar).

Instead of just blathering on I’d like to show you a few lines of code and invite you to answer a pop quiz:

class UserController < ApplicationController
  def save
    current_user.update_attributes(params[:user])
  end
end

class ProjectController < ApplicationController
  def destroy
    @project = Project.find(params[:id])
    if ( @project.user_id == current_user.id ||
      current_user.is_administrator? )
      @project.destroy
    end
  end
end

class User < ActiveRecord::Base
  attr_protected :is_administrator
  has_many :projects
end

class Project < ActiveRecord::Base
  belongs_to :user
end

Before anyone gets too bent out of shape – don’t worry about the stuff you don’t see. Everything you need is there.

Here’s the requirements we’ve been given:

  1. Users should not be able to grant themselves administrative capability
  2. Only administrators or the project’s owner can delete a project

And here’s the pop quiz: are those requirements satisfied? Why or why not?

Bonus points (for what they’re worth) will be given if you can tell me what I’m going to make “a strong case for” in the title of this post.

I’ll post my answers tomorrow (Wednesday) at 10:00 PM GMT.

[Edit] someone asked so I’ll clarify: the current_user method in the controllers always returns the correct user.

[Edit] another clarification: we can assume that actions not shown in ProjectController such as edit/save perform the same checks as ProjectController#destroy before doing anything.

[Edit] Part 2 is available now.

Posted in code | 11 comments