Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn on parameter whitelisting and get specs to pass #205

Merged
merged 1 commit into from Apr 24, 2012
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/models/forem/forum.rb
Expand Up @@ -11,6 +11,8 @@ class Forum < ActiveRecord::Base

validates :category, :title, :description, :presence => true

attr_accessible :category_id, :title, :description, :moderator_ids

def last_post_for(forem_user)
forem_user && forem_user.forem_admin? || moderator?(forem_user) ? posts.last : last_visible_post
end
Expand Down
2 changes: 2 additions & 0 deletions app/models/forem/group.rb
Expand Up @@ -5,6 +5,8 @@ class Group < ActiveRecord::Base
has_many :memberships
has_many :members, :through => :memberships, :class_name => Forem.user_class.to_s

attr_accessible :name

def to_s
name
end
Expand Down
2 changes: 2 additions & 0 deletions app/models/forem/membership.rb
Expand Up @@ -2,5 +2,7 @@ module Forem
class Membership < ActiveRecord::Base
belongs_to :group
belongs_to :member, :class_name => Forem.user_class.to_s

attr_accessible :member_id, :group_id
end
end
2 changes: 2 additions & 0 deletions app/models/forem/moderator_group.rb
Expand Up @@ -2,5 +2,7 @@ module Forem
class ModeratorGroup < ActiveRecord::Base
belongs_to :forum, :inverse_of => :moderator_groups
belongs_to :group

attr_accessible :group_id
end
end
2 changes: 2 additions & 0 deletions app/models/forem/post.rb
Expand Up @@ -15,6 +15,8 @@ class Post < ActiveRecord::Base
# Used in the moderation tools partial
attr_accessor :moderation_option

attr_accessible :text, :reply_to_id

belongs_to :topic
belongs_to :user, :class_name => Forem.user_class.to_s
belongs_to :reply_to, :class_name => "Post"
Expand Down
2 changes: 2 additions & 0 deletions app/models/forem/subscription.rb
Expand Up @@ -5,6 +5,8 @@ class Subscription < ActiveRecord::Base

validates :subscriber_id, :presence => true

attr_accessible :subscriber_id

def send_notification(post_id)
SubscriptionMailer.topic_reply(post_id, self.subscriber.id).deliver
end
Expand Down
2 changes: 2 additions & 0 deletions app/models/forem/view.rb
Expand Up @@ -8,6 +8,8 @@ class View < ActiveRecord::Base
validates :viewable_id, :presence => true
validates :viewable_type, :presence => true

attr_accessible :user, :current_viewed_at, :count

def viewed_at
updated_at
end
Expand Down
6 changes: 5 additions & 1 deletion db/seeds.rb
Expand Up @@ -4,9 +4,13 @@
:title => "Default",
:description => "Default forem created by install")

post = Forem::Post.find_or_initialize_by_text("Hello World")
post.user = user

topic = Forem::Topic.find_or_initialize_by_subject("Welcome to Forem")
topic.forum = forum
topic.user = user
topic.posts_attributes = [{:text => "Hello World", :user_id => user.id}]
topic.posts = [ post ]

topic.save!
end
5 changes: 5 additions & 0 deletions spec/lib/generators/forem/dummy/dummy_generator.rb
Expand Up @@ -56,6 +56,11 @@ def test_dummy_config
inject_into_file "#{dummy_path}/config/application.rb",
"\nrequire 'kaminari'\n",
:before => "module Dummy"

inject_into_file "#{dummy_path}/config/application.rb",
"\n config.active_record.whitelist_attributes = true\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strongly strongly strongly disagree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestions? You disagree with approach or turning whitelist_attributes on?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Disagree with turning this option on. If you turn this option on there's no way to know for certain that we have all attributes mass-assignable as required.

I'm about to head out to lunch but will look at this again when I come back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

K, I was just turning it on so that things actually break when running the specs. Otherwise everything can be mass assigned no problem (unless attr_accessible is called).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've now removed this with @35bd6fa. All the tests still pass. Nice work!

:before => "end\nend\n",
:verbose => false
end

protected
Expand Down
2 changes: 1 addition & 1 deletion spec/support/factories/topics.rb
Expand Up @@ -3,7 +3,7 @@
t.subject "FIRST TOPIC"
t.forum {|f| f.association(:forum) }
t.user {|u| u.association(:user) }
t.posts_attributes { [Factory.attributes_for(:post)] }
t.posts_attributes { [:text => "This is a brand new post"] }

trait :approved do
state 'approved'
Expand Down