Skip to content

Commit

Permalink
Never try to load things from CDN that won't be there
Browse files Browse the repository at this point in the history
fixes: CNVS-22486

there are 2 specific things that this fixes:
1.) the 403 error trying to load favicon.ico from
    the CDN
2.) if a school has custom js, on the mobile
    login screen, it calls include_account_js(raw:true)
    which would try to load:
    https://du11hjcvx0uqb.cloudfront.net/optimized/vendor/jquery-1.7.2.js?1439243240

this fixes both of those cases but it will more
generally fix it so we never try to load a file that
is un-revved or outside of /dist/brandable_css from
the CDN (since they won't be there)

test plan: with CDN enabled:
* open any page, the favicon should load and not
  have a 403 in console
* on a domain with new styles turned off, and from
  an account that has a custom js override file, go
  to the mobile login page (you can do this in desktop
  safari by picking Develop->User Agent->iPhone)
  and make sure jquery is loaded before their custom
  js loads

Change-Id: I877a8ef10a0b612fe7e43f47147e8148e7f98ff3
Reviewed-on: https://gerrit.instructure.com/60690
Tested-by: Jenkins
Reviewed-by: Jacob Fugal <[email protected]>
QA-Review: August Thornton <[email protected]>
Product-Review: Rob Orton <[email protected]>
  • Loading branch information
ryankshaw authored and roor0 committed Sep 4, 2015
1 parent 0ed15fc commit 2cb424f
Show file tree
Hide file tree
Showing 9 changed files with 244 additions and 82 deletions.
2 changes: 1 addition & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def js_env(hash = {})
unless @js_env
editor_css = view_context.stylesheet_path(css_url_for('what_gets_loaded_inside_the_tinymce_editor'))
@js_env = {
ASSET_HOST: asset_host,
ASSET_HOST: Canvas::Cdn.config.host,
active_brand_config: active_brand_config.try(:md5),
url_to_what_gets_loaded_inside_the_tinymce_editor_css: editor_css,
current_user_id: @current_user.try(:id),
Expand Down
17 changes: 5 additions & 12 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ def include_js_bundles
ary.concat(Canvas::RequireJs.extensions_for(bundle, 'plugins/')) unless use_optimized_js?
ary << "#{base_url}/compiled/bundles/#{bundle}.js"
end
javascript_include_tag *paths
javascript_include_tag(*paths)
end

def include_css_bundles
Expand Down Expand Up @@ -701,23 +701,16 @@ def include_account_js(options = {})
if includes.length > 0
if options[:raw]
includes.unshift("/optimized/vendor/jquery-1.7.2.js")
javascript_include_tag(includes)
javascript_include_tag(*includes)
else
str = <<-ENDSCRIPT
(function() {
var inject = function(src) {
require(['jquery'], function () {
#{includes.to_json}.forEach(function (src) {
var s = document.createElement('script');
s.src = src;
s.type = 'text/javascript';
document.body.appendChild(s);
};
var srcs = #{includes.to_json};
require(['jquery'], function() {
for (var i = 0, l = srcs.length; i < l; i++) {
inject(srcs[i]);
}
});
})();
});
ENDSCRIPT
javascript_tag(str)
end
Expand Down
17 changes: 15 additions & 2 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,20 @@
require File.expand_path('../boot', __FILE__)

unless CANVAS_RAILS3
require "rails/all"

# Yes, it doesn't seem DRY to list these both in the if and else
# but this used to be "require 'rails/all'" which included sprockets.
# I needed to explicitly opt-out of sprockets but since I'm not sure
# about the other frameworks, I left this so it would be exactly the same
# as "require 'rails/all'" but without sprockets--even though it is a little
# different then the rails 3 else block. If the difference is not intended,
# they can be pulled out of the if/else
require "active_record/railtie"
require "action_controller/railtie"
require "action_mailer/railtie"
# require "sprockets/railtie" # Do not enable the Rails Asset Pipeline
require "rails/test_unit/railtie"

Bundler.require(*Rails.groups)
else
require "active_record/railtie"
Expand Down Expand Up @@ -230,7 +243,7 @@ def call(env)
config.exceptions_app = ExceptionsApp.new

config.before_initialize do
config.action_controller.asset_host = Canvas::Cdn.config.host if Canvas::Cdn.config.host
config.action_controller.asset_host = Canvas::Cdn.method(:asset_host_for)
end
end
end
67 changes: 0 additions & 67 deletions config/initializers/make_rails_asset_helpers_use_gulp_filenames.rb

This file was deleted.

48 changes: 48 additions & 0 deletions config/initializers/revved_asset_urls.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# This is where we monkeypatch rails to look at the rev-manifest.json file we make in `gulp rev`
# instead of doing it's normal cache busting stuff on the url.
# eg: instead of '/images/whatever.png?12345', we want '/dist/images/whatever-<md5 of file>.png'.
# There is a different method that needs to be monkeypatched for rails 3 vs rails 4
if CANVAS_RAILS3
module ActionView
module Helpers
module AssetTagHelper
class AssetPaths
private

# Rails 3 expects us to override 'rewrite_asset_path' if we want to do something other than the
# default "/images/whatever.png?12345".
def rewrite_asset_path_with_rev_manifest(source, dir, options = nil)
# our brandable_css stylesheets are already fingerprinted, we don't need to do anything to them
return source if source =~ /^\/dist\/brandable_css/

key = (source[0] == '/') ? source : "#{dir}/#{source}"
Canvas::Cdn::RevManifest.url_for(key) || rewrite_asset_path_without_rev_manifest(source, dir, options)
end
alias_method_chain :rewrite_asset_path, :rev_manifest

end
end
end
end
else
require 'action_view/helpers/asset_url_helper'
module ActionView
module Helpers
module AssetUrlHelper

# Rails 4 leaves us 'path_to_asset' to override instead.
def path_to_asset_with_rev_manifest(source, options = {})
original_path = path_to_asset_without_rev_manifest(source, options)
revved_url = Canvas::Cdn::RevManifest.url_for(original_path)
if revved_url
File.join(compute_asset_host(revved_url, options).to_s, revved_url)
else
original_path
end
end
alias_method_chain :path_to_asset, :rev_manifest

end
end
end
end
11 changes: 11 additions & 0 deletions lib/canvas/cdn.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module Canvas
module Cdn
class << self

def config
@config ||= begin
config = ActiveSupport::OrderedOptions.new
Expand All @@ -15,6 +16,16 @@ def enabled?
config.enabled
end

def should_be_in_bucket?(source)
source.start_with?('/dist/brandable_css') || Canvas::Cdn::RevManifest.include?(source)
end

def asset_host_for(source)
return unless config.host # unless you've set a :host in the canvas_cdn.yml file, just serve normally
config.host if should_be_in_bucket?(source)
# Otherwise, return nil & use the same domain the page request came from, like normal.
end

def push_to_s3!(*args, &block)
return unless enabled?
uploader = Canvas::Cdn::S3Uploader.new(*args)
Expand Down
46 changes: 46 additions & 0 deletions lib/canvas/cdn/rev_manifest.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
require 'set'

# An interface to the manifest file created by `gulp rev`
module Canvas
module Cdn
module RevManifest
class << self
delegate :include?, to: :revved_urls

def manifest
load_data_if_needed
@manifest
end

def revved_urls
load_data_if_needed
@revved_urls
end

def url_for(source)
# remove the leading slash if there is one
source = source.sub(/^\//, '')
fingerprinted = manifest[source]
"/dist/#{fingerprinted}" if fingerprinted
end

private
def load_data_if_needed
# don't look this up every request in prduction
return if ActionController::Base.perform_caching && defined? @manifest
file = Rails.root.join('public', 'dist', 'rev-manifest.json')
if file.exist?
Rails.logger.debug "reading rev-manifest.json"
@manifest = JSON.parse(file.read).freeze
elsif Rails.env.production?
raise "you need to run `gulp rev` first"
else
@manifest = {}.freeze
end
@revved_urls = Set.new(@manifest.values.map{|s| "/dist/#{s}" }).freeze
end

end
end
end
end
31 changes: 31 additions & 0 deletions spec/selenium/brandable_css_js_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
require File.expand_path(File.dirname(__FILE__) + '/common')

describe "brandableCss JS integration specs" do
include_context "in-process server selenium tests"

EXAMPLE_CDN_HOST = 'https://somecdn.example.com'

it "sets ENV.asset_host correctly" do
Canvas::Cdn.config.expects(:host).at_least_once.returns(EXAMPLE_CDN_HOST)
get "/login/canvas"
expect(driver.execute_script("return ENV.ASSET_HOST")).to eq(EXAMPLE_CDN_HOST)
end

it "loads css from handlebars correctly" do
admin_logged_in
get "/accounts/#{Account.default.id}/permissions?account_roles=1"

css_bundle = 'jst/roles/rolesOverrideIndex'
fingerprint = BrandableCSS.fingerprint_for(css_bundle, 'legacy_normal_contrast')
css_url = "#{Canvas::Cdn.config.host}/dist/brandable_css/legacy_normal_contrast/#{css_bundle}-#{fingerprint}.css"
expect(fj("head link[rel='stylesheet'][data-loaded-by-brandableCss][href*='#{css_bundle}']")['href']).to match(css_url)

driver.execute_script("window.ENV.ASSET_HOST = '#{EXAMPLE_CDN_HOST}'")
EXAMPLE_CSS_BUNDLE = 'jst/some/css/bundle-12345.css'
require_exec('compiled/util/brandableCss', "brandableCss.loadStylesheet('#{EXAMPLE_CSS_BUNDLE}')")
css_url = "#{EXAMPLE_CDN_HOST}/dist/brandable_css/legacy_normal_contrast/#{EXAMPLE_CSS_BUNDLE}.css"
expect(fj("head link[rel='stylesheet'][data-loaded-by-brandableCss][href*='jst/some/css/bundle']")['href']).to eq(css_url)
end

end

87 changes: 87 additions & 0 deletions spec/selenium/cdn_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
# Why are these in spec/selenium?
# ===============================
# Although these tests don't use selenium at all, they need to be have assets
# compiled in order to work. eg: `gulp rev` and `brandable_css` need to run first.
# By putting them in spec/selenium, our build server will run them with the rest
# of the browser specs, after it has compiled assets.

require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')

RE_SHORT_MD5 = /\A[a-f0-9]{10}\z/ # 10 chars of an MD5

EXAMPLE_CDN_HOST = 'https://somecdn.example.com'

describe 'Stuff related to how we load stuff from CDN and use brandable_css' do

describe BrandableCSS do

describe 'fingerprint_for' do

it 'finds the right fingerprints for normal bundles, plugins & handlebars' do
sample_bundles = {
'bundles/common' => true,
'plugins/analytics/analytics' => true, # to test that it works with plugins
'jst/tinymce/EquationEditorView' => false # to test that it works with handlebars-loaded css
}
sample_bundles.each do |bundle_name, bundle_loads_variables|
fingerprints = BrandableCSS.variants.map do |variant|
fingerprint = BrandableCSS.fingerprint_for(bundle_name, variant)
expect(fingerprint).to match(RE_SHORT_MD5)
fingerprint
end

expect(fingerprints.length).to eq(4), 'We have 4 variants'
msg = 'make sure the conbined results match the result of all_fingerprints_for'
expect(fingerprints).to eq(BrandableCSS.all_fingerprints_for(bundle_name).values), msg

msg = "all variants should outupt the same css if a bundle doesn't pull in
the variables file. If it does, there should be some that are different"
expect(fingerprints.uniq.length).to(bundle_loads_variables ? (be > 1) : eq(1), msg)
end
end
end
end


def check_css(bundle_name)
fingerprint = BrandableCSS.fingerprint_for(bundle_name, 'legacy_normal_contrast')
expect(fingerprint).to match(RE_SHORT_MD5)
assert_tag(tag: 'link', attributes: {
rel: 'stylesheet',
href: "#{EXAMPLE_CDN_HOST}/dist/brandable_css/legacy_normal_contrast/#{bundle_name}-#{fingerprint}.css"
})
end

def check_asset(tag, asset_path)
revved_path = Canvas::Cdn::RevManifest.url_for(asset_path)
expect(revved_path).to be_present
attributes = {}
attributes[(tag == 'link') ? :href : :src] = "#{EXAMPLE_CDN_HOST}#{revved_path}"
assert_tag(tag: tag, attributes: attributes)
end

describe 'urls for script tag and stylesheets on actual pages', :type => :request do

it 'has the right stuff on the login page' do
Canvas::Cdn.config.expects(:host).at_least_once.returns(EXAMPLE_CDN_HOST)
get '/login/canvas'

['bundles/common', 'bundles/login'].each { |bundle| check_css(bundle) }
['images/favicon-yellow.ico', 'images/apple-touch-icon.png'].each { |i| check_asset('link', i) }
js_base_url = ENV['USE_OPTIMIZED_JS'] == 'true' ? '/optimized' : '/javascripts'
['vendor/require.js', 'compiled/bundles/login.js'].each { |s| check_asset('script', "#{js_base_url}/#{s}") }
end

it "loads custom js 'raw' on mobile login screen" do
js_url = 'https://example.com/path/to/some/file.js'
Account.default.settings[:global_includes] = true
Account.default.settings[:global_javascript] = js_url
Account.default.save!

get '/login/canvas', {}, { 'HTTP_USER_AGENT' => 'iphone' }
# match /optimized/vendor/jquery-1.7.2.js?1440111591 or /optimized/vendor/jquery-1.7.2.js
assert_tag(tag: 'script', attributes: { src: /^\/optimized\/vendor\/jquery-1.7.2.js(\?\d+)*$/})
assert_tag(tag: 'script', attributes: { src: js_url})
end
end
end

0 comments on commit 2cb424f

Please sign in to comment.