This page now only allows for searching latest inspect reports for all nodes for a particular file matching a particular content, with an option whether to show matching, unmatching, or all nodes. This makes the UI much simpler, and its behavior more well-defined.
Also refactored some scopes to remove redundant joins, and extra queries. Paired-With: Matt Robinson Signed-off-by: Nick Lewis <[email protected]> --- Local-branch: ticket/next/5865 app/controllers/application_controller.rb | 1 + app/controllers/files_controller.rb | 2 +- app/controllers/reports_controller.rb | 42 +++++------ app/helpers/string_helper.rb | 5 + app/models/report.rb | 2 +- app/models/resource_status.rb | 19 ++--- app/views/reports/search.html.haml | 49 +++++++------ public/stylesheets/application.css | 4 +- public/stylesheets/sass/application.scss | 34 ++++---- spec/controllers/reports_controller_spec.rb | 106 +++++++++++++-------------- spec/helpers/string_helper_spec.rb | 25 ++++++ spec/models/resource_status_spec.rb | 6 -- 12 files changed, 157 insertions(+), 138 deletions(-) create mode 100644 app/helpers/string_helper.rb create mode 100644 spec/helpers/string_helper_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 9efbde9..57d01d6 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -4,6 +4,7 @@ class ApplicationController < ActionController::Base include InheritedResources::DSL include PaginateScopeHelper + include StringHelper helper :all # include all helpers, all the time protect_from_forgery # See ActionController::RequestForgeryProtection for details diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb index 10f4b14..4b2dfa5 100644 --- a/app/controllers/files_controller.rb +++ b/app/controllers/files_controller.rb @@ -6,7 +6,7 @@ class FilesController < ApplicationController end [params[:file1], params[:file2]].each do |md5| - unless md5 =~ /^[0-9a-f]{32}$/ + unless is_md5?(md5) render :text => "Invalid md5: #{md5.inspect}", :content_type => 'text/plain', :status => 400 return end diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index bd6f85a..9ed00db 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -51,35 +51,31 @@ class ReportsController < InheritedResources::Base end def search - if params[:search_all_inspect_reports] - inspected_resources = ResourceStatus.inspections - else - inspected_resources = ResourceStatus.latest_inspections - end - inspected_resources = inspected_resources.order("reports.time DESC") + flash[:errors] = [] + flash[:notices] = [] + inspected_resources = ResourceStatus.latest_inspections.order("nodes.name") - if params[:file_title].present? and params[:file_content].present? - @files = inspected_resources.by_file_title(params[:file_title]) - if params[:content_match] == "negative" - @files = @files.without_file_content(params[:file_content]) - else - @files = @files.by_file_content(params[:file_content]) - end + @files = [] - elsif params[:file_title].present? - @files = inspected_resources.by_file_title(params[:file_title]) + @title = params[:file_title].to_s.strip + @content = params[:file_content].to_s.strip - elsif params[:file_content].present? - if params[:content_match] == "negative" - @files = inspected_resources.in_a_report_without_content(params[:file_content]) + if @title.present? then + @files = case params[:search_results] + when 'unmatching' then + inspected_resources.by_file_title(@title).without_file_content(@content) + when 'matching' then + inspected_resources.by_file_title(@title).by_file_content(@content) + when 'all' then + inspected_resources.by_file_title(@title) else - @files = inspected_resources.by_file_content(params[:file_content]) + [] end - - else - @files = nil - return end + + flash[:errors] << "Please specify the file title to search for" if params[:search_results].present? + flash[:notices] << "#{@content} is not a valid md5 checksum" if @content.present? and !is_md5?(@content) + @files = paginate_scope @files end diff --git a/app/helpers/string_helper.rb b/app/helpers/string_helper.rb new file mode 100644 index 0000000..4639b63 --- /dev/null +++ b/app/helpers/string_helper.rb @@ -0,0 +1,5 @@ +module StringHelper + def is_md5?(str) + !!(str =~ /^[0-9a-f]{32}$/) + end +end diff --git a/app/models/report.rb b/app/models/report.rb index 858c657..f0c0051 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -136,7 +136,7 @@ class Report < ActiveRecord::Base end def baseline? - self.node.baseline_report == self + self.node.baseline_report_id == self.id end def baseline! diff --git a/app/models/resource_status.rb b/app/models/resource_status.rb index 69b8835..17b63bb 100644 --- a/app/models/resource_status.rb +++ b/app/models/resource_status.rb @@ -1,5 +1,5 @@ class ResourceStatus < ActiveRecord::Base - belongs_to :report + belongs_to :report, :include => :node has_many :events, :class_name => "ResourceEvent", :dependent => :destroy accepts_nested_attributes_for :events @@ -9,33 +9,28 @@ class ResourceStatus < ActiveRecord::Base named_scope :inspections, { :joins => :report, :conditions => "reports.kind = 'inspect'" } named_scope :latest_inspections, { - :joins => "INNER JOIN reports ON resource_statuses.report_id = reports.id INNER JOIN nodes on reports.id = nodes.last_inspect_report_id", - :conditions => "reports.kind = 'inspect' and reports.id = nodes.last_inspect_report_id" + :conditions => "nodes.last_inspect_report_id = resource_statuses.report_id", + :include => [:report => :node], } named_scope :by_file_content, lambda {|content| { :conditions => ["resource_statuses.resource_type = 'File' AND resource_events.property = 'content' AND resource_events.previous_value = ?", "{md5}#{content}"], - :joins => :events, + :include => :events, } } named_scope :without_file_content, lambda {|content| { :conditions => ["resource_statuses.resource_type = 'File' AND resource_events.property = 'content' AND resource_events.previous_value != ?", "{md5}#{content}"], - :joins => :events, - } - } - - named_scope :in_a_report_without_content, lambda {|content| - { - :conditions => ["resource_statuses.report_id NOT IN (SELECT report_id FROM resource_statuses rs JOIN resource_events re ON re.resource_status_id = rs.id WHERE rs.resource_type = 'File' AND re.property = 'content' AND re.previous_value = ?)", "{md5}#{content}"], + :include => :events, } } named_scope :by_file_title, lambda {|title| { - :conditions => ["resource_type = 'File' AND title = ?", title], + :conditions => ["resource_statuses.resource_type = 'File' AND resource_statuses.title = ?", title], + :include => :events, } } diff --git a/app/views/reports/search.html.haml b/app/views/reports/search.html.haml index a16b25c..f4769b7 100644 --- a/app/views/reports/search.html.haml +++ b/app/views/reports/search.html.haml @@ -3,37 +3,43 @@ .header %h2 - Search Reports + Search Latest Inspect Reports .item + - [:errors, :notices].each do |message_type| + - if flash[message_type].present? + - flash[message_type].each do |messages| + %div{:class => "section #{message_type.to_s.singularize}"} + %h3= message_type.to_s.capitalize + %p + - messages.each do |message| + %li= message + .section - form_tag url_for(:action => :search), :method => :get do = label_tag 'file_title', 'File title' - = text_field_tag 'file_title', params[:file_title], :size => 100 - = label_tag 'file_content', 'File checksum' - = text_field_tag 'file_content', params[:file_content], :size => 100 - = label_tag 'content_match_positive', 'Files matching checksum' - = radio_button_tag 'content_match', "positive", params[:content_match] != "negative" - = label_tag 'content_match_negative', 'Files differing from checksum' - = radio_button_tag 'content_match', "negative", params[:content_match] == "negative" - = label_tag 'search_all_inspect_reports', 'Search all reports?' - = check_box_tag 'search_all_inspect_reports', 'true', params[:search_all_inspect_reports] + = text_field_tag 'file_title', @title, :size => 100 + = label_tag 'file_content', 'File checksum (md5)' + = text_field_tag 'file_content', @content, :size => 100 + %br + = radio_button_tag 'search_results', 'unmatching', (params[:search_results] == 'unmatching' or params[:search_results].nil?) + = label_tag 'search_results_unmatching', 'unmatching', :class => :inline + = radio_button_tag 'search_results', 'matching', params[:search_results] == 'matching' + = label_tag 'search_results_matching', 'matching', :class => :inline + = radio_button_tag 'search_results', 'all', params[:search_results] == 'all' + = label_tag 'search_results_all', 'all', :class => :inline %br %button Search - .section - - if @files.nil? - - # blank - - elsif @files.empty? - No matching files found. - - else + - if @files.present? + .section + %h4 Found #{pluralize(@files.count, "node")} %table.inspector %thead %tr %th.status - %th Time ↓ - %th Host - %th Filename + %th Time + %th Host ↑ %th Checksum %tbody - @files.each do |file| @@ -41,10 +47,9 @@ = report_status_td(file.report) %td= link_to file.report.time, file.report %td= link_to h(file.report.host), file.report.node - %td= file.title - %td= file.events.find_by_property("content").previous_value rescue "not recorded" + %td= file.events.detect {|event| event.property == "content"}.previous_value.gsub("{md5}","") rescue "not recorded" - if @files.total_pages > 1 %tfoot %tr - %td{:colspan => 5} + %td{:colspan => 4} = pagination_for(@files) diff --git a/public/stylesheets/application.css b/public/stylesheets/application.css index fc07d5e..7745a5a 100644 --- a/public/stylesheets/application.css +++ b/public/stylesheets/application.css @@ -293,7 +293,7 @@ body { body .section.error h3 { margin: -10px -10px .5em; padding: .5em 10px .5em 26px; - background: #ffcccc url("../images/icons/failure.png") 5px center no-repeat; + background: #ffcccc url("../images/icons/failed.png") 5px center no-repeat; color: #443333; } body .section.error p:last-child { padding-bottom: 0; @@ -309,7 +309,7 @@ body { body .section.failure h3 { margin: -10px -10px .5em; padding: .5em 10px .5em 26px; - background: #ffcccc url("../images/icons/failure.png") 5px center no-repeat; + background: #ffcccc url("../images/icons/failed.png") 5px center no-repeat; color: #443333; } body .section.failure p:last-child { padding-bottom: 0; diff --git a/public/stylesheets/sass/application.scss b/public/stylesheets/sass/application.scss index 3e657c4..5ffadf8 100644 --- a/public/stylesheets/sass/application.scss +++ b/public/stylesheets/sass/application.scss @@ -471,37 +471,37 @@ body { margin-bottom: 1.5em; &.error { - @include icony_section('failure', - #433, #FCC, - #433, #FDD, + @include icony_section('failed', + #433, #FCC, + #433, #FDD, #FCC); } &.failure { - @include icony_section('failure', - #433, #FCC, - #433, #FDD, + @include icony_section('failed', + #433, #FCC, + #433, #FDD, #FCC); } &.warning { - @include icony_section('warning', - #444, #DCC, - #433, #EDD, + @include icony_section('warning', + #444, #DCC, + #433, #EDD, #DCC); } &.success { - @include icony_section('warning', - #444, #CDC, - #343, #DED, + @include icony_section('warning', + #444, #CDC, + #343, #DED, #CDC); } &.notice { - @include icony_section('notice', - #444, #CCC, - #333, #DDD, + @include icony_section('notice', + #444, #CCC, + #333, #DDD, #CCC); } @@ -569,7 +569,7 @@ body { ul.token-input-list-facebook { width: 100%; } - + div.token-input-dropdown-facebook { &.token-input-dropdown-facebook { background-color: #FFFFED; @@ -578,7 +578,7 @@ body { ul { li { - &.token-input-dropdown-item-facebook, + &.token-input-dropdown-item-facebook, &.token-input-dropdown-item2-facebook { background-color: #FFFFED; list-style-type: none; diff --git a/spec/controllers/reports_controller_spec.rb b/spec/controllers/reports_controller_spec.rb index 8b020ea..a6e9e4f 100644 --- a/spec/controllers/reports_controller_spec.rb +++ b/spec/controllers/reports_controller_spec.rb @@ -84,82 +84,80 @@ describe ReportsController do get('search') response.code.should == '200' response.should render_template("reports/search") - assigns[:files].should == nil + assigns[:files].should == [] end describe "when searching for files" do before do @matching_report = Report.create!(:host => "foo", :time => 1.week.ago.to_date, :status => "unchanged", :kind => "inspect") @matching_report.resource_statuses.create!(:resource_type => "File", :title => "/etc/hosts", :events_attributes => [{:property => "content", :previous_value => "{md5}ab07acbb1e496801937adfa772424bf7"}]) - @matching_earlier_report = Report.create!(:host => "foo", :time => 10.weeks.ago.to_date, :status => "unchanged", :kind => "inspect") - @matching_earlier_report.resource_statuses.create!(:resource_type => "File", :title => "/etc/hosts", :events_attributes => [{:property => "content", :previous_value => "{md5}ab07acbb1e496801937adfa772424bf7"}]) - @unmatching_report = Report.create!(:host => "foo", :time => 2.weeks.ago.to_date, :status => "unchanged", :kind => "inspect") - @unmatching_report.resource_statuses.create!(:resource_type => "File", :title => "/etc/sudoers", :events_attributes => [{:property => "content", :previous_value => "{md5}aa876288711c4198cfcda790b58d7e95"}]) - @doubly_matching_report = Report.create!(:host => "foo", :time => 3.weeks.ago.to_date, :status => "unchanged", :kind => "inspect") - @doubly_matching_report.resource_statuses.create!(:resource_type => "File", :title => "/etc/hosts", :events_attributes => [{:property => "content", :previous_value => "{md5}aa876288711c4198cfcda790b58d7e95"}]) - @doubly_matching_report.resource_statuses.create!(:resource_type => "File", :title => "/etc/sudoers", :events_attributes => [{:property => "content", :previous_value => "{md5}ab07acbb1e496801937adfa772424bf7"}]) - end - describe "in latest reports " do - describe "by title" do - it "should find the correct reports" do - get('search', :file_title => "/etc/hosts", :file_content => '') - assigns[:files].to_a.should =~ @matching_report.resource_statuses - end - end + @other_matching_report = Report.create!(:host => "bar", :time => 1.week.ago.to_date, :status => "unchanged", :kind => "inspect") + @other_matching_report.resource_statuses.create!(:resource_type => "File", :title => "/etc/hosts", :events_attributes => [{:property => "content", :previous_value => "{md5}ab07acbb1e496801937adfa772424bf7"}]) - describe "by content" do - it "should find the correct reports" do - get('search', :file_title => '', :file_content => "ab07acbb1e496801937adfa772424bf7") - assigns[:files].to_a.should =~ @matching_report.resource_statuses - end - end + @unmatching_file_report = Report.create!(:host => "baz", :time => 1.week.ago.to_date, :status => "unchanged", :kind => "inspect") + @unmatching_file_report.resource_statuses.create!(:resource_type => "File", :title => "/etc/sudoers", :events_attributes => [{:property => "content", :previous_value => "{md5}ab07acbb1e496801937adfa772424bf7"}]) - describe "by both title and content" do - it "should find the correct reports" do - get('search', :file_title => "/etc/hosts", :file_content => "ab07acbb1e496801937adfa772424bf7") - assigns[:files].to_a.should =~ @matching_report.resource_statuses - end - end + @unmatching_content_report = Report.create!(:host => "banana", :time => 1.week.ago.to_date, :status => "unchanged", :kind => "inspect") + @unmatching_content_report.resource_statuses.create!(:resource_type => "File", :title => "/etc/hosts", :events_attributes => [{:property => "content", :previous_value => "{md5}aa876288711c4198cfcda790b58d7e95"}]) end - describe "in all reports " do - describe "by title" do - it "should find the correct reports" do - get('search', :file_title => "/etc/hosts", :file_content => '', :search_all_inspect_reports => true) - assigns[:files].to_a.should =~ @matching_report.resource_statuses + @matching_earlier_report.resource_statuses + [@doubly_matching_report.resource_statuses.first] - end + describe "when both file title and content are specified" do + it "should return only matching nodes when requested" do + get('search', :file_title => "/etc/hosts", :file_content => "ab07acbb1e496801937adfa772424bf7", :search_results => "matching") + assigns[:files].to_a.should =~ @matching_report.resource_statuses + @other_matching_report.resource_statuses + flash[:notices].should be_empty + end + + it "should return only unmatching nodes when requested" do + get('search', :file_title => "/etc/hosts", :file_content => "ab07acbb1e496801937adfa772424bf7", :search_results => "unmatching") + assigns[:files].to_a.should =~ @unmatching_content_report.resource_statuses + flash[:notices].should be_empty end - describe "by content" do - it "should find the correct reports" do - get('search', :file_title => '', :file_content => "ab07acbb1e496801937adfa772424bf7", :search_all_inspect_reports => true) - assigns[:files].to_a.should =~ @matching_report.resource_statuses + @matching_earlier_report.resource_statuses + [@doubly_matching_report.resource_statuses.last] - end + it "should return all nodes containing the file when requested" do + get('search', :file_title => "/etc/hosts", :file_content => "ab07acbb1e496801937adfa772424bf7", :search_results => "all") + assigns[:files].to_a.should =~ @matching_report.resource_statuses + + @other_matching_report.resource_statuses + + @unmatching_content_report.resource_statuses + flash[:notices].should be_empty end - describe "by both title and content" do - it "should find the correct reports" do - get('search', :file_title => "/etc/hosts", :file_content => "ab07acbb1e496801937adfa772424bf7", :search_all_inspect_reports => true) - assigns[:files].to_a.should =~ @matching_report.resource_statuses + @matching_earlier_report.resource_statuses - end + it "should warn but still search if the specified md5 is not valid" do + get('search', :file_title => "/etc/hosts", :file_content => "not_an_md5_at_all", :search_results => "all") + assigns[:files].to_a.should =~ @matching_report.resource_statuses + + @other_matching_report.resource_statuses + + @unmatching_content_report.resource_statuses + flash[:notices].should include "not_an_md5_at_all is not a valid md5 checksum" end + end - describe "by title and negative content" do - it "should find the reports with files of this name that differ" do - get('search', :file_title => "/etc/hosts", :file_content => "ab07acbb1e496801937adfa772424bf7", :search_all_inspect_reports => true, :content_match => "negative") - assigns[:files].to_a.should =~ [ @doubly_matching_report.resource_statuses.first ] - end + describe "when only file content is specified" do + it "should not perform a search, and should add an error message" do + get('search', :file_content => "ab07acbb1e496801937adfa772424bf7", :search_results => "all") + assigns[:files].should be_empty + flash[:errors].should include "Please specify the file title to search for" + flash[:notices].should be_empty end + end - describe "by title and negative content" do - it "should find the reports that don't contain any such files" do - get('search', :file_content => "ab07acbb1e496801937adfa772424bf7", :search_all_inspect_reports => true, :content_match => "negative") - assigns[:files].to_a.should =~ @unmatching_report.resource_statuses - end + describe "when the page first loads" do + it "should not perform a search, and should not add error messages" do + get('search') + assigns[:files].should be_empty + flash[:errors].should be_empty + flash[:notices].should be_empty end end + describe "when nothing is specified" do + it "should not perform a search, and should not add any error messages" do + get('search', :search_results => "all") + assigns[:files].should be_empty + flash[:errors].should include "Please specify the file title to search for" + flash[:notices].should be_empty + end + end end end diff --git a/spec/helpers/string_helper_spec.rb b/spec/helpers/string_helper_spec.rb new file mode 100644 index 0000000..b344acd --- /dev/null +++ b/spec/helpers/string_helper_spec.rb @@ -0,0 +1,25 @@ +require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') + +describe StringHelper do + describe ".is_md5?" do + it "returns true for a valid md5" do + helper.is_md5?("abcdef0123456789abcdef0123456789").should == true + end + + it "returns false for a value that is too short" do + helper.is_md5?("abcdef0123456789").should == false + end + + it "returns false for a value that is too long" do + helper.is_md5?("abcdef012345678abcdef01234567899abcdef0123456789").should == false + end + + it "returns false for a value that contains illegal characters" do + helper.is_md5?("this is not an md5").should == false + end + + it "returns false for upper-case values" do + helper.is_md5?("ABCDEF0123456789ABCDEF0123456789").should == false + end + end +end diff --git a/spec/models/resource_status_spec.rb b/spec/models/resource_status_spec.rb index 22f17c7..9581f9e 100644 --- a/spec/models/resource_status_spec.rb +++ b/spec/models/resource_status_spec.rb @@ -54,12 +54,6 @@ describe ResourceStatus do ResourceStatus.by_file_title('/etc/sudoers').without_file_content("ab07acbb1e496801937adfa772424bf7").should == @unmatching_report.resource_statuses end end - - describe ".in_a_report_without_content" do - it "should only return statuses from reports that don't have any file with matching contents" do - ResourceStatus.by_file_title('/etc/sudoers').in_a_report_without_content("ab07acbb1e496801937adfa772424bf7").should == @unmatching_report.resource_statuses - end - end end end -- 1.7.3.5 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
