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.

Sign up for GitHub

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

Jump to bottom

Added default headers option #88

Merged
merged 1 commit into from
Feb 20, 2015
Merged

Added default headers option #88

merged 1 commit into from
Feb 20, 2015

Conversation

AlfonsoUceda
Copy link
Contributor

closes #87

AlfonsoUceda
AlfonsoUceda reviewed Feb 13, 2015
View reviewed changes
lib/lotus/action/head.rb
@@ -26,6 +26,8 @@ def finish

if _requires_no_body?
@_body = nil
else
@headers.merge!(configuration.default_headers)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this else for merging the default headers if the method isn't HEAD or the status code isn't HTTP_STATUSES_WITHOUT_BODY defined above

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.12%) to 99.0% when pulling a7e37bd on AlfonsoUceda:security_default_headers into 4cbeb03 on lotus:master.

jodosha
jodosha reviewed Feb 13, 2015
View reviewed changes
test/configuration_test.rb
end
end
end

describe 'duplicate' do
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add this to duplicate and reset! tests? TY!

@jodosha
Copy link
Member

jodosha commented Feb 13, 2015

@AlfonsoUceda This looks really good. I left one comment for extra tests for Configuration#duplicate and #reset.

Quoting #87 :

Developers MUST have the chance to override one or more headers in each single action.

This is still missing and it's really important for resource based headers.
Can you please ensure that it works with a test like this?

class MyAction
  include Lotus::Action

  # Default header in configuration is "Content-Security-Policy" => "script-src 'self'"
  def call(params)
    # ...
    headers['Content-Security-Policy'] = "script-src 'self' https://api.example.org"
  end
end

The header specified in the action should be the output.

@jodosha jodosha added this to the v0.4.0 milestone Feb 13, 2015
@jodosha jodosha self-assigned this Feb 17, 2015
runlevel5
runlevel5 reviewed Feb 17, 2015
View reviewed changes
lib/lotus/action/head.rb
@@ -26,6 +45,7 @@ def finish

if _requires_no_body?
@_body = nil
@headers.reject! {|header,_| !ENTITY_HEADERS.include?(header) }
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, add a space after {

@runlevel5
Copy link
Member

except above comments, 👍 to go

@jodosha jodosha merged commit 60bf6c2 into hanami:master Feb 20, 2015
jodosha added a commit that referenced this pull request Feb 20, 2015
@jodosha
Cleanup per #88
1bbd4f1
@jodosha
Copy link
Member

jodosha commented Feb 20, 2015

@AlfonsoUceda Thank you, merged 😉

jodosha added a commit that referenced this pull request Feb 27, 2015
@jodosha
Default HTTP headers: allow to set values multiple times, reject head…
956d655 
…ers with nil values, safely duplicate this setting. Ref #88
@AlfonsoUceda AlfonsoUceda deleted the security_default_headers branch May 15, 2015 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jodosha jodosha

Labels
feature
Projects
None yet
Milestone
v0.4.0
Development

Successfully merging this pull request may close these issues.

Default headers
4 participants
@AlfonsoUceda @coveralls @jodosha @runlevel5