Whitelisting parameters required for successful display of posts. #90

Merged
merged 3 commits into from Sep 29, 2013

Projects

None yet

2 participants

@gaelian
Collaborator
gaelian commented Sep 22, 2013

The move to Rails 4 and strong parameters necessitates whitelisting of
parameters being processed by the controller. The exclusion of the
:tag_list, :published_at_natural and :slug parameters from the
whitelist means that these data are not being saved to the db and hence
no new post will be displayed on the front end, no new tags will be
saved and the post slug cannot be updated after initial creation. This
commit fixes these issues.

@gaelian gaelian Whitelisting parameters required for successful display of posts.
The move to Rails 4 and strong parameters necessitates whitelisting of
parameters being processed by the controller. The exclusion of the
:tag_list, :published_at_natural and :slug parameters from the
whitelist means that these data are not being saved to the db and hence
no new post will be displayed on the front end, no new tags will be
saved and the post slug cannot be updated after initial creation. This
commit fixes these issues.
f7dbc3c
@xaviershay
Owner

Thanks! Before I merge, is there an easy spec for this?

@gaelian
Collaborator
gaelian commented Sep 22, 2013

I had the same thought. Looking at the relevant posts_controller_spec.rb file, I couldn't think of an easy spec, but I was also running out of time to spend on this. :P

If you want to hold off, I can have another think about it, probably later in the week.

gaelian added some commits Sep 28, 2013
@gaelian gaelian Fixes related to correct whitelisting of attributes for comments.
Start making use of the comment_params method in
admin/comments_controller.rb.

Removed author_url and author_email from the params whitelist in
comments_controller.rb as according to the spec, these should not be
able to be set from the front end anyway.
d930136
@gaelian gaelian Spec coverage for Rails 4 strong params functionality.
Added factory_girl factories for use with update specs on:

admin/comments_controller_spec.rb
admin/posts_controller_spec.rb
48185aa
@gaelian
Collaborator
gaelian commented Sep 28, 2013

OK, so these new commits are what I've ended up with.

I found that in the front end app/controllers/comments_controller.rb, the comment_params method appears to be somewhat superfluous because this same functionality was already present - pre-Rails 4 - through the Comment.protected_attribute? method, so behaviour is the same with or without the comment_params method it would seem. But I have none the less limited the permitted attributes to :author and :body within comment_params. Let me know if this is not correct.

@xaviershay xaviershay commented on the diff Sep 28, 2013
spec/controllers/admin/posts_controller_spec.rb
@@ -112,6 +139,27 @@ def do_put
end
end
+ describe 'handling POST to create with expected whitelisted attributes present' do
+ it 'allows whitelisted attributes as expected' do
@xaviershay
xaviershay Sep 28, 2013 Owner

It's probably more interesting to verify that it doesn't allow certain attributes. I think this happy-path behaviour is already tested in features?

@gaelian
Collaborator
gaelian commented Sep 29, 2013

In the browsing feature? Yes, it would seem that way. However, seeing as the initial impetus for my pull request was a failure of the happy path, I'd suggest that we could do with a little more coverage of it in rspec, along side cucumber.

On the admin side of things, there's also not a lot that we're restricting of course, but I can certainly tack on some checks of the unhappy path if you'd like?

@xaviershay
Owner

eh, probably fine. Will sleep on it an merge tomorrow.

@gaelian
Collaborator
gaelian commented Sep 29, 2013

OK no worries. After thinking about it a bit more myself, I'm more convinced that a bit more detailed testing of the happy path is in order because parts of the failure I'm addressing were quite subtle, i.e. the post slug. As things currently stand on the master branch, If the post slug is created automatically along with the initial post creation, this will work. But if you manually enter a slug at post creation or if you update it after post creation, you won't get the result you were expecting. The browsing feature is testing for the presence of a post and associated tag on a high level as one would expect, but it's not testing for correct slug behaviour.

But like I said, still happy to whip up some extra tests of the unhappy path if you think there's value there.

By the way, "happy-path behaviour", I like that. I might have to steal that one. :)

@xaviershay
Owner

Going to merge this now, since it does fix broken behaviour on master. We can add anything else in another PR :)

Thanks again!

@xaviershay xaviershay merged commit c8e4c75 into xaviershay:master Sep 29, 2013

1 check passed

default The Travis CI build passed
Details
@gaelian gaelian deleted the gaelian:strong-params-fix branch Sep 30, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment