Skip to content

Commit

Permalink
Changed ActiveRecord attributes to respect access control.
Browse files Browse the repository at this point in the history
Signed-off-by: Michael Koziarski <michael@koziarski.com>
[#1084 state:committed]
  • Loading branch information
Adam Milligan authored and NZKoz committed Sep 24, 2008
1 parent a78ec93 commit 4d9a7ab
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 5 deletions.
8 changes: 7 additions & 1 deletion activerecord/lib/active_record/attribute_methods.rb
Expand Up @@ -232,6 +232,10 @@ def evaluate_attribute_method(attr_name, method_definition, method_name=attr_nam
def method_missing(method_id, *args, &block)
method_name = method_id.to_s

if self.class.private_method_defined?(method_name)
raise NoMethodError("Attempt to call private method", method_name, args)
end

# If we haven't generated any methods yet, generate them, then
# see if we've created the method we're looking for.
if !self.class.generated_methods?
Expand Down Expand Up @@ -334,10 +338,12 @@ def query_attribute(attr_name)
# <tt>person.respond_to?(:name=)</tt>, and <tt>person.respond_to?(:name?)</tt>
# which will all return +true+.
alias :respond_to_without_attributes? :respond_to?
def respond_to?(method, include_priv = false)
def respond_to?(method, include_private_methods = false)
method_name = method.to_s
if super
return true
elsif self.private_methods.include?(method_name) && !include_private_methods
return false
elsif !self.class.generated_methods?
self.class.define_attribute_methods
if self.class.generated_methods.include?(method_name)
Expand Down
51 changes: 47 additions & 4 deletions activerecord/test/cases/attribute_methods_test.rb
Expand Up @@ -58,19 +58,19 @@ def test_should_unserialize_attributes_for_frozen_records

def test_kernel_methods_not_implemented_in_activerecord
%w(test name display y).each do |method|
assert_equal false, ActiveRecord::Base.instance_method_already_implemented?(method), "##{method} is defined"
assert !ActiveRecord::Base.instance_method_already_implemented?(method), "##{method} is defined"
end
end

def test_primary_key_implemented
assert_equal true, Class.new(ActiveRecord::Base).instance_method_already_implemented?('id')
assert Class.new(ActiveRecord::Base).instance_method_already_implemented?('id')
end

def test_defined_kernel_methods_implemented_in_model
%w(test name display y).each do |method|
klass = Class.new ActiveRecord::Base
klass.class_eval "def #{method}() 'defined #{method}' end"
assert_equal true, klass.instance_method_already_implemented?(method), "##{method} is not defined"
assert klass.instance_method_already_implemented?(method), "##{method} is not defined"
end
end

Expand All @@ -80,7 +80,7 @@ def test_defined_kernel_methods_implemented_in_model_abstract_subclass
abstract.class_eval "def #{method}() 'defined #{method}' end"
abstract.abstract_class = true
klass = Class.new abstract
assert_equal true, klass.instance_method_already_implemented?(method), "##{method} is not defined"
assert klass.instance_method_already_implemented?(method), "##{method} is not defined"
end
end

Expand Down Expand Up @@ -228,6 +228,40 @@ def test_setting_time_zone_conversion_for_attributes_should_write_value_on_class
assert_equal [:field_b], Minimalistic.skip_time_zone_conversion_for_attributes
end

def test_read_attributes_respect_access_control
privatize("title")

topic = @target.new(:title => "The pros and cons of programming naked.")

This comment has been minimized.

Copy link
@mislav

mislav Sep 24, 2008

Member

Oh, there are cons?

assert !topic.respond_to?(:title)
assert_raise(NoMethodError) { topic.title }
topic.send(:title)
end

def test_write_attributes_respect_access_control
privatize("title=(value)")

topic = @target.new
assert !topic.respond_to?(:title=)
assert_raise(NoMethodError) { topic.title = "Pants"}
topic.send(:title=, "Very large pants")
end

def test_question_attributes_respect_access_control
privatize("title?")

topic = @target.new(:title => "Isaac Newton's pants")
assert !topic.respond_to?(:title?)
assert_raise(NoMethodError) { topic.title? }
assert topic.send(:title?)
end

def test_bulk_update_respects_access_control
privatize("title=(value)")

assert_raise(ActiveRecord::UnknownAttributeError) { topic = @target.new(:title => "Rants about pants") }
assert_raise(ActiveRecord::UnknownAttributeError) { @target.new.attributes = { :title => "Ants in pants" } }
end

private
def time_related_columns_on_topic
Topic.columns.select{|c| [:time, :date, :datetime, :timestamp].include?(c.type)}.map(&:name)
Expand All @@ -244,4 +278,13 @@ def in_time_zone(zone)
Time.zone = old_zone
ActiveRecord::Base.time_zone_aware_attributes = old_tz
end

def privatize(method_signature)
@target.class_eval <<-private_method
private
def #{method_signature}
"I'm private"
end
private_method
end
end

15 comments on commit 4d9a7ab

@NZKoz
Copy link
Member

@NZKoz NZKoz commented on 4d9a7ab Sep 25, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it’s frowned upon at community code drives or in the enterprise.

Programming naked isn’t enterprise ready

@supaspoida
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cons: moment of panic as you rush to get clothed when the doorbell rings.

@nbibler
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cons: the look you receive from your wife when she returns home from work at the end of the day, and there you are.

@smart
Copy link

@smart smart commented on 4d9a7ab Sep 25, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cons: forgetting that your skype was set to video chat….

@nickh
Copy link
Contributor

@nickh nickh commented on 4d9a7ab Sep 25, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cons: pair programming and standup meetings

@rotor
Copy link

@rotor rotor commented on 4d9a7ab Sep 25, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cons: a vinyl chair

@amyhoy
Copy link

@amyhoy amyhoy commented on 4d9a7ab Sep 25, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cons: crack-sweat + chair.

I win.

(Being non-enterprise-ready is a pro, in my book.)

@masterkain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cons: having adeona running, your macbook gets stolen and police look at photos taken to identify the thief.

@ggoodale
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cons: Near nuclear temperatures emanating from the bottom of your MacBook Pro when Time Machine kicks in.

@mtodd
Copy link
Contributor

@mtodd mtodd commented on 4d9a7ab Sep 25, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pros: air flow!

Someone had to.

@randito
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pros: My plugin hit your mencache.
Cons: It was a miss.

@raggi
Copy link
Contributor

@raggi raggi commented on 4d9a7ab Sep 25, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cons:

def test_something
send :something
end

describe ‘something’ do
before do
@obj.module_eval { public :something }
end
it ‘is a pain in the ass to test’ { true.should.eql(true) }
it ….
end

;-P

@augustl
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pros: Makes tabbing to porn sites when tests are running a lot more convenient

@henrik
Copy link
Contributor

@henrik henrik commented on 4d9a7ab Sep 26, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leethal: Those are supposed to be swords: http://xkcd.com/303/

@lenary
Copy link
Contributor

@lenary lenary commented on 4d9a7ab Oct 9, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cons: pubes in your keyboard!
Pros: Easier to have sex during tests!

Please sign in to comment.