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

Add basic support for access control headers to ActionDispatch::Static #19135

Merged
merged 1 commit into from Oct 13, 2015

Conversation

yuki24
Copy link
Contributor

@yuki24 yuki24 commented Feb 28, 2015

Now ActionDispatch::Static can accept access control headers such as Access-Control-Allow-Origin. These headers will be set as well as Cache-Control header when a response is delivered. Access control headers can be configured with #config in config/application.rb or config/environments/***.rb:

config.action_dispatch.access_control_headers = {
  "Access-Control-Allow-Origin"      => 'http://rubyonrails.org',
  "Access-Control-Expose-Headers"    => 'X-Other-Custom-Header',
  "Access-Control-Allow-Headers"     => 'X-Another-Custom-Header',
  "Access-Control-Max-Age"           => '42',
  "Access-Control-Allow-Credentials" => 'true',
  "Access-Control-Allow-Methods"     => 'GET'
}

cc/ @schneems

@@ -13,10 +16,10 @@ module ActionDispatch
# located at `public/assets/application.js` if the file exists. If the file
# does not exist a 404 "File not Found" response will be returned.
class FileHandler
def initialize(root, cache_control)
def initialize(root, cache_control, access_control_headers)
Copy link
Contributor Author

@yuki24 yuki24 Feb 28, 2015

Choose a reason for hiding this comment

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

Although this implementation works, I'm not sure about this interface since Cache-Control is also part of the HTTP headers and the third argument seems redundant. I guess this interface could be justdefault_headers.

@schneems
Copy link
Member

schneems commented Mar 2, 2015

Ultimately I think it looks good. Let's change some naming so that it's a little more generic. Instead of access_control_headers let's just use headers which is technically what it is (although you can use them as access control headers).

You can reduce the initialization logic to something like this:

headers ||= {}
headers['Cache-Control'] = cache_control

I would also change config.action_dispatch.access_control_headers to config.action_dispatch.asset_headers or something more accurate. We can then explicitly add a section in the guides that shows how to set access control headers http://guides.rubyonrails.org/asset_pipeline.html

I agree we should deprecate some interfaces here and clean some things up. Unfortunately as I understand, the deadline for getting deprecations in to Rails 5 was Rails 4.2.0

@yuki24 yuki24 force-pushed the access-control-support branch from ab1bc3f to 68e35a0 Compare Mar 3, 2015
@yuki24
Copy link
Contributor Author

yuki24 commented Mar 3, 2015

Updated the commit as suggested. Thanks @schneems for reviewing!

@yuki24
Copy link
Contributor Author

yuki24 commented Mar 3, 2015

forgot to add a CHANGELOG entry btw. I'll add it tonight.

@file_server = ::Rack::File.new(@root, headers)

headers ||= {}
@file_server = ::Rack::File.new(@root, headers.reverse_merge('Cache-Control' => cache_control))
Copy link
Member

@schneems schneems Mar 3, 2015

Choose a reason for hiding this comment

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

merge and reverse_merge are really expensive. Even though this only gets called once on initialization the code in this file is very performance sensitive and I don't want people copying existing style. Can you also factor out the merge?

Copy link
Contributor Author

@yuki24 yuki24 Mar 4, 2015

Choose a reason for hiding this comment

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

Thanks @schneems, removed the merge call,

@yuki24 yuki24 force-pushed the access-control-support branch from 68e35a0 to ef5c266 Compare Mar 4, 2015
@@ -24,6 +24,9 @@ class Railtie < Rails::Railtie # :nodoc:
'X-Content-Type-Options' => 'nosniff'
}

config.public_file_server = ActiveSupport::OrderedOptions.new
config.public_file_server.headers = { 'Cache-Control' => 'public, max-age=3600' }
Copy link
Contributor Author

@yuki24 yuki24 Mar 4, 2015

Choose a reason for hiding this comment

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

I'm not sure if this is the right place to define public_file_server. I'm open to suggestions.

@yuki24 yuki24 force-pushed the access-control-support branch from ef5c266 to 57e68ad Compare May 2, 2015
@yuki24 yuki24 mentioned this pull request May 15, 2015
@schneems
Copy link
Member

schneems commented May 21, 2015

What are we waiting on for this to go in? Anything a blocker here?

@yuki24 yuki24 force-pushed the access-control-support branch from 57e68ad to 582cca3 Compare Jun 10, 2015
headers = config.public_file_server.headers
headers['Cache-Control'] = config.static_cache_control if config.static_cache_control.present?

middleware.use ::ActionDispatch::Static, paths["public"].first, :not_set, config.static_index, headers: headers
Copy link
Contributor Author

@yuki24 yuki24 Jun 10, 2015

Choose a reason for hiding this comment

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

I needed to use :not_set explicitly due to this change: #20017. I think we should make the index arg a keyword argument so we don't have to care about the order of the args here. Alternatively, we can just merge the static_index config into the public_file_server namespace, which I actually prefer, but I'm open to suggestions.

cc/ @eliotsykes @jonatack

@yuki24 yuki24 force-pushed the access-control-support branch from 582cca3 to 8914567 Compare Jun 10, 2015
@@ -13,15 +13,13 @@ module ActionDispatch
# located at `public/assets/application.js` if the file exists. If the file
# does not exist, a 404 "File not Found" response will be returned.
class FileHandler
def initialize(root, cache_control, index = 'index')
def initialize(root, index = 'index', headers: )
Copy link
Member

@vipulnsward vipulnsward Jun 10, 2015

Choose a reason for hiding this comment

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

index can be kwarg too.

Copy link
Contributor Author

@yuki24 yuki24 Jun 10, 2015

Choose a reason for hiding this comment

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

Absolutely. See my comment above about it: #19135 (comment)

Copy link
Contributor Author

@yuki24 yuki24 Jun 11, 2015

Choose a reason for hiding this comment

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

I just sent a separate pull request for this: #20520. Now we need to wait on it to be merged before merging this PR.

Copy link
Contributor

@jonatack jonatack Jun 11, 2015

Choose a reason for hiding this comment

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

The comment states optional HTTP headers but the headers keyword is required. Perhaps headers: needs a default value?

Copy link
Contributor Author

@yuki24 yuki24 Jun 12, 2015

Choose a reason for hiding this comment

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

Headers are totally optional. I changed it to use a {} by default.

Copy link
Contributor

@jonatack jonatack Jun 12, 2015

Choose a reason for hiding this comment

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

Great.

This PR will break the interface for other services/gems that hook into it, for instance the Heroku-deflator gem here but seems unavoidable for adding this feature.

Copy link
Contributor Author

@yuki24 yuki24 Jun 12, 2015

Choose a reason for hiding this comment

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

oh, I didn't know that ActionDispatch::FileHandler is a public class. I don't think it's ok to break it.

Copy link
Contributor Author

@yuki24 yuki24 Jun 13, 2015

Choose a reason for hiding this comment

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

I did a little investigation but I'm not sure if we should treat ActionDispatch::FileHandler as a public class. It's basically an inner class. Does it still have to be public?

@yuki24
Copy link
Contributor Author

yuki24 commented Jun 10, 2015

@schneems updated the commit. I'm a bit concerned about the arguments of the ActionDispatch::Static initializer, but everything else should be good now.

@@ -52,6 +52,13 @@ def initialize(*)
@x = Custom.new
end

def static_cache_control=(value)
ActiveSupport::Deprecation.warn("static_cache_control is deprecated and will be removed in Rails 5.1. " \
"Please use `config.public_file_server.headers['Cache-Control'] = #{value} instead.")
Copy link
Member

@vipulnsward vipulnsward Jun 10, 2015

Choose a reason for hiding this comment

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

Would be good to have this as-

config.public_file_server.headers= {'Cache-Control' => #{value}}

This would suggest the options being passed to headers have been expanded.

Copy link
Contributor Author

@yuki24 yuki24 Jun 11, 2015

Choose a reason for hiding this comment

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

updated.

@vipulnsward
Copy link
Member

vipulnsward commented Jun 10, 2015

@yuki24 Thanks a ton for this, I was looking to implement something similar due to this - https://twitter.com/vipulnsward/status/603866969201373184

@yuki24 yuki24 force-pushed the access-control-support branch 5 times, most recently from f235974 to 3223d80 Compare Jun 12, 2015
Now ActionDispatch::Static can accept HTTP headers so that developers
will have control of returning arbitrary headers like
'Access-Control-Allow-Origin' when a response is delivered. They can
be configured through `#config.public_file_server.headers`:

  config.public_file_server.headers = {
    "Cache-Control"               => "public, max-age=60",
    "Access-Control-Allow-Origin" => "http://rubyonrails.org"
  }

Also deprecate `config.static_cache_control` in favor of
`config.public_file_server.headers`.
@yuki24 yuki24 force-pushed the access-control-support branch from 3223d80 to 5226058 Compare Jun 13, 2015
@yuki24
Copy link
Contributor Author

yuki24 commented Jun 13, 2015

I believe everything should be good and we are ready to merge.

@schneems
Copy link
Member

schneems commented Oct 13, 2015

I spoke to @jeremy, I think it just fell off his radar. Can you rebase, this is good to merge.

@jeremy
Copy link
Member

jeremy commented Oct 13, 2015

👍 Looks like a changelog conflict only, no need to rebase.

@jeremy jeremy merged commit 5226058 into rails:master Oct 13, 2015
jeremy added a commit that referenced this pull request Oct 13, 2015
Add basic support for access control headers to ActionDispatch::Static
@yuki24
Copy link
Contributor Author

yuki24 commented Oct 15, 2015

@schneems @jeremy Thanks! ❤️ 💟 💛 💙 ❤️ 💟 💛 💙

@yuki24 yuki24 deleted the access-control-support branch Oct 15, 2015
@kaspth
Copy link
Contributor

kaspth commented Nov 3, 2015

@yuki24 nice! Have you sent a pull request to enable public_file_server as mentioned in the outdated diffs yet? 😁

@yuki24
Copy link
Contributor Author

yuki24 commented Nov 3, 2015

@kaspth Not yet. I'm not sure if I can work on it anytime soon, so feel free to send a PR if you could.

@schneems
Copy link
Member

schneems commented Nov 3, 2015

What was the idea?

kaspth added a commit to kaspth/rails that referenced this pull request Nov 3, 2015
As discussed in rails#19135 (comment).

Replaces `serve_static_files` to unify the static options under the `public_file_server` wing.

Deprecates `serve_static_files` accessors, but make them use the newer config internally.
@kaspth
Copy link
Contributor

kaspth commented Nov 3, 2015

@yuki24 @schneems added a pull request at #22173 😁

@@ -1,3 +1,22 @@
* Deprecate `config.static_cache_control` in favor of
Copy link
Contributor

@kaspth kaspth Nov 3, 2015

Choose a reason for hiding this comment

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

Feels like this should be in the Railties CHANGELOG file. I will change it at #22173.

kaspth added a commit that referenced this pull request Nov 5, 2015
It was deprecated in #19135.

We're now favoring `public_file_server.headers`.
shamozhixing pushed a commit to shamozhixing/gitlab-ce that referenced this pull request Aug 23, 2016
…ions.

Deprecation of static_cache_control: rails/rails#19135

Deprecation of serve_static_files: rails/rails#22173
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants