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

Fixes #7230, #12021 - Upgrade to Rails 4.1.5 #2870

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,28 @@ require File.expand_path('../lib/regexp_extensions', FOREMAN_GEMFILE)

source 'https://rubygems.org'

gem 'rails', '3.2.22'
gem 'rack-cache', '< 1.3.0'
gem 'rails', '4.1.5'
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a newer version? I thought 4.1.5 was the latest, but it isn't by a mile now - it's vulnerable to a number of security issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be newer than that, I pinned it to 4.1.5 though because it's what the SCL offered - https://www.softwarecollections.org/repos/rhscl/rh-ror41/epel-6-x86_64/rh-ror41-rubygem-rails-4.1.5-3.el6/ . I can relax it a bit ~> 4.1, 4.2 would bring some incompatibilities.

Copy link
Contributor

Choose a reason for hiding this comment

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

~> 4.1.5 or a specific version would be fine, yep. develop works this way now, the version in the SCL is just an RPM packaging change, so for Bundler users it should be the current release to get security fixes.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep it pinned to a specific version, as there are sometimes API changes between patch-level version bumps in rails.

gem 'json', '~> 1.5'
gem 'rest-client', '~> 1.6.0', :require => 'rest_client'
gem 'audited-activerecord', '3.0.0'
gem 'audited-activerecord', '~> 4.0'
gem 'will_paginate', '~> 3.0'
gem 'ancestry', '~> 2.0'
gem 'scoped_search', '~> 3.0'
gem 'scoped_search', '>= 3.2.2', '< 4'
gem 'ldap_fluff', '>= 0.3.5', '< 1.0'
gem 'net-ldap', '>= 0.8.0'
gem 'apipie-rails', '~> 0.2.5'
gem 'apipie-rails', '~> 0.3.4'
gem 'rabl', '~> 0.11'
gem 'oauth', '~> 0.4'
gem 'deep_cloneable', '~> 2.0'
gem 'foreigner', '~> 1.4'
gem 'validates_lengths_from_database', '~> 0.2'
gem 'friendly_id', '~> 4.0'
gem 'friendly_id', '~> 5.0'
Copy link
Member Author

Choose a reason for hiding this comment

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

Breaking changes on 5.0 force us to call .friendly on relations to find using friendly_id. We could add something in the models to avoid this, but it looks like defaulting .find to .friendly.find is going to be removed in future versions.

gem 'secure_headers', '~> 1.3'
gem 'safemode', '~> 1.2'
gem 'fast_gettext', '~> 0.8'
gem 'gettext_i18n_rails', '~> 1.0'
gem 'i18n', '~> 0.6.4'
gem 'rails-i18n', '~> 3.0.0'
gem 'rails-i18n', '~> 4.0.0'
gem 'turbolinks', '~> 2.5'
gem 'logging', '>= 1.8.0', '< 3.0.0'
gem 'fog-core', '1.34.0'
Expand All @@ -36,6 +35,9 @@ if RUBY_VERSION.start_with? '1.9.'
else
gem 'net-ssh'
end
gem 'activerecord-session_store', '~> 0.1.1'
gem 'rails-observers', '~> 0.1'
gem 'protected_attributes', '~> 1.1.1'

Dir["#{File.dirname(FOREMAN_GEMFILE)}/bundler.d/*.rb"].each do |bundle|
self.instance_eval(Bundler.read_file(bundle))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,20 +118,21 @@ function submit_modal_form() {
function build_modal(element, url) {
var url = url + "?" + $.param({host_ids: $.foremanSelectedHosts});
var title = $(element).attr('data-dialog-title');
$('#confirmation-modal .modal-header h4').text(title);
$('#confirmation-modal .modal-body').empty().append("<img class='modal-loading' src='/assets/spinner.gif'>");
$('#confirmation-modal').modal();
$("#confirmation-modal .modal-body").load(url + " #content",
function(response, status, xhr) {
$("#loading").hide();
$('#submit_multiple').val('');
var b = $("#confirmation-modal .btn-primary");
if ($(response).find('#content form select').length > 0)
b.addClass("disabled").attr("disabled", true);
else
b.removeClass("disabled").attr("disabled", false);
});
return false;
$('#confirmation-modal .modal-header h4').text(title);
$('#confirmation-modal .modal-body').empty()
.append("<img class='modal-loading' src='<%= asset_path('spinner.gif') %>'");
Copy link
Member Author

Choose a reason for hiding this comment

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

Same reason as https://github.com/theforeman/foreman/pull/2870/files#r47890054 , note the .erb extension. Only this line changed

$('#confirmation-modal').modal();
$("#confirmation-modal .modal-body").load(url + " #content",
function(response, status, xhr) {
$("#loading").hide();
$('#submit_multiple').val('');
var b = $("#confirmation-modal .btn-primary");
if ($(response).find('#content form select').length > 0)
b.addClass("disabled").attr("disabled", true);
else
b.removeClass("disabled").attr("disabled", false);
});
return false;

}

Expand Down
2 changes: 1 addition & 1 deletion app/assets/javascripts/topbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ $(document).on('mouseleave','.loc-submenu', function(){
function mark_active_menu() {
var menus = $('.menu_tab_dropdown'),
path = window.location.pathname + window.location.search,
link = $("[href='%s'".replace('%s', path));
link = $("[href='%s']".replace('%s', path));

menus.removeClass('active');
$("[class^='menu_tab_']").removeClass('active');
Expand Down
20 changes: 12 additions & 8 deletions app/assets/stylesheets/application.scss
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,11 @@ a.config_group_name {
}

.editable {
background: url("edit.png") no-repeat scroll 98% 6px transparent;
background: image-url("edit.png") no-repeat scroll 98% 6px transparent;
Copy link
Member Author

Choose a reason for hiding this comment

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

All of these changes make the url fetch the digested asset (edit.png3820910893120893) instead of the undigested asset. Rails 4 does not serve undigested assets anymore.

cursor: pointer;
padding: 4px 26px 4px 0;
&:hover {
background: url("edit-hover.png") no-repeat scroll 98% 6px #F2F2F2;
background: image-url("edit-hover.png") no-repeat scroll 98% 6px #F2F2F2;
}
}

Expand Down Expand Up @@ -282,23 +282,23 @@ table {
}

.sorting {
background: url('sort_both.png') no-repeat center right;
background: image-url('sort_both.png') no-repeat center right;
}

.sorting_asc {
background: url('sort_asc.png') no-repeat center right;
background: image-url('sort_asc.png') no-repeat center right;
}

.sorting_desc {
background: url('sort_desc.png') no-repeat center right;
background: image-url('sort_desc.png') no-repeat center right;
}

.sorting_asc_disabled {
background: url('sort_asc_disabled.png') no-repeat center right;
background: image-url('sort_asc_disabled.png') no-repeat center right;
}

.sorting_desc_disabled {
background: url('sort_desc_disabled.png') no-repeat center right;
background: image-url('sort_desc_disabled.png') no-repeat center right;
}
}

Expand Down Expand Up @@ -340,7 +340,7 @@ table {
.spinner-placeholder {
width: 16px;
height: 16px;
background: url('spinner.gif');
background: image-url('spinner.gif');
text-indent: 20px;
white-space: nowrap;
margin: 20% 30% 0 40%;
Expand Down Expand Up @@ -537,3 +537,7 @@ span.glyphicon.host-status {
word-wrap: break-word; /* IE */
word-break: break-all;
}

.ms-container {
background: transparent image-url('switch.png') no-repeat 270px 80px;
}
4 changes: 2 additions & 2 deletions app/assets/stylesheets/charts.scss
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,9 @@
}

.statistics-pie.small .overlay {
background: url(/assets/pie_overlay.png) no-repeat;
background: image-url('/assets/pie_overlay.png') no-repeat;
background-size: cover;
-ms-behavior: url(/assets/background-size.htc);
-ms-behavior: asset-url('/assets/background-size.htc');
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as in the other css classes - this allows us to serve these assets as digested assets

z-index: 1;
}

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v1/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def destroy
param :id, :identifier, :required => true

def last
conditions = { :host_id => Host.authorized(:view_hosts).find(params[:host_id]).try(:id) } if params[:host_id].present?
conditions = { :host_id => Host.authorized(:view_hosts).friendly.find(params[:host_id]).try(:id) } if params[:host_id].present?
max_id = resource_scope.where(conditions).maximum(:id)
@report = resource_scope.includes(:logs => [:message, :source]).find(max_id)
render :show
Expand Down
4 changes: 3 additions & 1 deletion app/controllers/api/v2/config_reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ def destroy
param :id, :identifier, :required => true

def last
conditions = { :host_id => Host.authorized(:view_hosts).find(params[:host_id]).try(:id) } if params[:host_id].present?
if params[:host_id].present?
conditions = { :host_id => resource_finder(Host.authorized(:view_hosts), params[:host_id]).try(:id) }
end
max_id = resource_scope.where(conditions).maximum(:id)
@config_report = resource_scope.includes(:logs => [:message, :source]).find(max_id)
render :show
Expand Down
6 changes: 4 additions & 2 deletions app/controllers/api/v2/host_classes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ def destroy
# overwrite resource_name so it's host and and not host_class, since we want to return @host
def find_host
not_found and return false if params[:host_id].blank?
@host = Host.find(params[:host_id]) if Host::Managed.respond_to?(:authorized) &&
Host::Managed.authorized("view_host", Host::Managed)
if Host::Managed.respond_to?(:authorized) &&
Host::Managed.authorized("view_host", Host::Managed)
@host = resource_finder(Host.authorized(:view_hosts), params[:host_id])
end
end
end
end
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/api/v2/models_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ def update
def destroy
process_response @model.destroy
end

private

def find_resource
@model = Model.friendly.find(params[:id]) || super
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use FindCommon helper here like everywhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC this is here because some models could have names like 101 that if we use from_param or friendly search first, we might return the wrong Model, @unorthodoxgeek ?

Copy link
Member

Choose a reason for hiding this comment

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

that's about right. names can be numbers for Model. in which case, find_resource will look for ID instead of starting with name,

end
end
5 changes: 3 additions & 2 deletions app/controllers/api/v2/parameters_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,9 @@ def allowed_nested_id
def find_parameter
# nested_obj is required, so no need to check here
@parameters = nested_obj.send(parameters_method)
@parameter = @parameters.find(params[:id])
return @parameter if @parameter
@parameter = @parameters.from_param(params[:id])
@parameter ||= @parameters.friendly.find(params[:id])
return @parameter if @parameter.present?
not_found
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v2/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def destroy
param :id, :identifier, :required => true

def last
conditions = { :host_id => Host.authorized(:view_hosts).find(params[:host_id]).try(:id) } if params[:host_id].present?
conditions = { :host_id => resource_finder(Host.authorized(:view_hosts), params[:host_id]).try(:id) } if params[:host_id].present?
max_id = resource_scope.where(conditions).maximum(:id)
@report = resource_scope.includes(:logs => [:message, :source]).find(max_id)
render :show
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ def remote_user_provided?
end

def display_layout?
return nil if two_pane?
return false if two_pane?
Copy link
Member Author

Choose a reason for hiding this comment

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

layout changed on Rails 4 - returning nil as we did before, means to return the default layout, so two-pane won't work.

We have to return false to ensure the 'edit' methods in two-pane are loaded in the two-pane instead of as a fullpage.

"application"
end

Expand Down
59 changes: 23 additions & 36 deletions app/controllers/concerns/api/v2/lookup_keys_common_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,6 @@ module Api::V2::LookupKeysCommonController
before_filter :return_if_smart_mismatch, :only => [:show, :update, :destroy]
end

def puppetclass_id?
params.keys.include?('puppetclass_id')
end

def environment_id?
params.keys.include?('environment_id')
end

def host_id?
params.keys.include?('host_id')
end

def hostgroup_id?
params.keys.include?('hostgroup_id')
end

def smart_variable_id?
params.keys.include?('smart_variable_id') || controller_name.match(/smart_variables/)
end
Expand All @@ -43,28 +27,22 @@ def smart_class_parameter_id?
params.keys.include?('smart_class_parameter_id') || controller_name.match(/smart_class_parameters/)
end

def find_puppetclass
@puppetclass = Puppetclass.authorized(:view_puppetclasses).find(params['puppetclass_id'])
rescue ActiveRecord::RecordNotFound
not_found({ :error => { :message => (_("Puppet class with id '%{id}' was not found") % { :id => params['puppetclass_id'] }) } })
end
[Puppetclass, Environment, Host::Base, Hostgroup].each do |model|
model_string = model.to_s.split('::').first.downcase

def find_environment
@environment = Environment.authorized(:view_environments).find(params['environment_id'])
rescue ActiveRecord::RecordNotFound
not_found({ :error => { :message => (_("Environment with id '%{id}' was not found") % { :id => params['environment_id'] }) } })
end

def find_host
@host = Host::Base.authorized(:view_hosts).find(params['host_id'])
rescue ActiveRecord::RecordNotFound
not_found({ :error => { :message => (_("Host with id '%{id}' was not found") % { :id => params['host_id'] }) } })
end
define_method("#{model_string}_id?") do
params.keys.include?("#{model_string}_id")
end

def find_hostgroup
@hostgroup = Hostgroup.authorized(:view_hostgroups).find(params['hostgroup_id'])
rescue ActiveRecord::RecordNotFound
not_found({ :error => { :message => (_("Hostgroup with id '%{id}' was not found") % { :id => params['hostgroup_id'] }) } })
define_method("find_#{model_string}") do
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a refactor of find_host, find_environment, find_puppetclass, find_hostgroup, that uses FindCommon to find these attributes, rather than implementing our own find methods again.

scope = model.authorized(:"view_#{model_string.pluralize}")
begin
instance_variable_set("@#{model_string}",
resource_finder(scope, params["#{model_string}_id"]))
rescue ActiveRecord::RecordNotFound
model_not_found(model_string)
end
end
end

def find_smart_variable
Expand Down Expand Up @@ -144,4 +122,13 @@ def return_if_smart_mismatch
not_found "#{obj} not found by id '#{id}'"
end
end

private

def model_not_found(model)
error_message = (
_("%{model} with id '%{id}' was not found") %
{ :id => params["#{model}_id"], :model => model.capitalize } )
not_found(:error => { :message => error_message } )
end
end
16 changes: 11 additions & 5 deletions app/controllers/concerns/find_common.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
# this mixin is used by both ApplicationController and Api::BaseController
# searches for an object based on its id, name, label, etc and assign it to an instance variable
# This mixin is used by both ApplicationController and Api::BaseController
# Searches for an object based on its id, name, label, etc and assign it to an instance variable
# friendly_id performs the logic if params[:id] is 'id' or 'id-name' or 'name'

module FindCommon
# example: @host = Host.find(params[:id])
def find_resource
not_found and return if params[:id].blank?
instance_variable_set("@#{resource_name}", resource_scope.find(params[:id]))
instance_variable_set("@#{resource_name}",
resource_finder(resource_scope, params[:id]))
Copy link
Member

Choose a reason for hiding this comment

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

Why we don't raise 404 anymore here? Not following.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do, a few lines below we raise ActiveRecord::NotFound when the scope is empty - https://github.com/theforeman/foreman/blob/develop/app/controllers/application_controller.rb#L9 , I can also check for params[:id] if needed, but a not_found is going to be raised if the resource is not found.

end

def resource_finder(scope, id)
Copy link
Member Author

Choose a reason for hiding this comment

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

This mimics the way the class worked on Rails 3, find_resource finds by param, then by friendly id, and then resorts to the regular find if no other way is available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

done

raise ActiveRecord::RecordNotFound if scope.empty?
result = scope.from_param(id) if scope.respond_to?(:from_param)
result ||= scope.friendly.find(id) if scope.respond_to?(:friendly)
Copy link
Contributor

Choose a reason for hiding this comment

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

What scopes wouldn't respond to these?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Same for from_param, not all models include Parameterized::By*

Copy link
Member

Choose a reason for hiding this comment

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

actually all include it for some whacky reason, and I really don't like that (ActiveRecord::Base includes Parametrize::ById)

Copy link
Contributor

Choose a reason for hiding this comment

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

actually all include it for some whacky reason, and I really don't like that (ActiveRecord::Base includes Parametrize::ById)

In case you were thinking of it, I'd not suggest removing it as it could unexpectedly affect plugins.

Copy link
Member

Choose a reason for hiding this comment

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

result || scope.find(id)
end

def resource_name(resource = controller_name)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/config_reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def index
def show
# are we searching for the last report?
if params[:id] == "last"
conditions = { :host_id => Host.authorized(:view_hosts).find(params[:host_id]).try(:id) } if params[:host_id].present?
conditions = { :host_id => resource_finder(Host.authorized(:view_hosts), params[:host_id]).try(:id) } if params[:host_id].present?
params[:id] = resource_base.where(conditions).maximum(:id)
end

Expand Down
6 changes: 3 additions & 3 deletions app/controllers/hosts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def puppetclass_parameters
def externalNodes
certname = params[:name]
@host ||= resource_base.find_by_certname certname
@host ||= resource_base.find certname
@host ||= resource_base.friendly.find certname
not_found and return unless @host

begin
Expand Down Expand Up @@ -352,7 +352,7 @@ def update_multiple_parameters
skipped = []
params[:name].each do |name, value|
next if value.empty?
if (host_param = host.host_parameters.find(name))
if (host_param = host.host_parameters.friendly.find(name))
counter += 1 if host_param.update_attribute(:value, value)
else
skipped << name
Expand Down Expand Up @@ -698,7 +698,7 @@ def taxonomy_scope
# overwrite application_controller
def find_resource
not_found and return false if (id = params[:id]).blank?
@host = resource_base.find(id)
@host = resource_base.friendly.find(id)
@host ||= resource_base.find_by_mac params[:host][:mac] if params[:host] && params[:host][:mac]

not_found and return(false) unless @host
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/settings_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def index
end

def update
@setting = Setting.find(params[:id])
@setting = Setting.friendly.find(params[:id])
if @setting.parse_string_value(params[:setting][:value]) && @setting.save
render :json => @setting
else
Expand Down
Loading