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

Method List dialog #23106

Merged
merged 2 commits into from
Jul 29, 2024
Merged

Method List dialog #23106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4e86f0
Update MiqAeMethod specs to remove @ variables
kbrock Jul 26, 2024
5d2ddfe
Add MiqAeMethod scopes for name and path search
kbrock Jul 25, 2024
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
6 changes: 6 additions & 0 deletions app/models/miq_ae_method.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ class MiqAeMethod < ApplicationRecord
AVAILABLE_SCOPES = ["class", "instance"]
validates_inclusion_of :scope, :in => AVAILABLE_SCOPES

# finds by name or namespace. not domain
# @param [nil,String] search
scope :name_path_search, lambda { |search|
search.present? ? where(arel_table[:relative_path].matches("%#{search}%")) : where({})
Copy link
Member

@Fryguy Fryguy Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is a potential security issue? We try to avoid interpolation and use parameter binding instead. What would happen if the search itself had a % ?

Perhaps use sanitize_sql_like ?

Copy link
Member Author

@kbrock kbrock Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In core I see:

  • 1: use of sanitize_sql_like (actually across all our code)
  • 15: uses of the way I presented it. (grep for 'matches.*%')
  • 24: uses of "like ?", ...% (grep 'like *(*\?')
  • 44: uses of where... #{} (grep where.*#{)

Do we actually care if someone uses % in the phrase?

I'll look into making these changes but since we are adding in a wild character, not sure why it buys us.

Copy link
Member

@Fryguy Fryguy Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's more of a best practices thing? I thought like ? with bindings was typically the way to do it, but if matches with interpolation also works, then I'm fine with it.

Do we actually care if someone uses % in the phrase?

% in the phrase is really a question to you? Will name_path_search break if someone has a literal % in their namespace path? Is that even a valid character? Same goes for ?. It might be worth having a test case for that particular scenario to make sure it works and that this code can find those edge cases.

Copy link
Member Author

@kbrock kbrock Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Verified that this pattern is not susceptible to sql injection attacks. interpolation in the main sql is, but the arguments are secure
  2. If searching for a string with a %, it finds it, but treats it as a wild character
  3. Method names can not have a % in them, but I feel our conversation may be about the bigger picture, so I'm trying not to get bound down by this

I added a test case. Was not able to add a method with a % in the name, but came as close as I could.

}

def self.available_languages
AVAILABLE_LANGUAGES
end
Expand Down
1 change: 1 addition & 0 deletions spec/factories/miq_ae_method.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
sequence(:name) { |n| "miq_ae_method#{seq_padded_for_sorting(n)}" }
language { "ruby" }
location { "inline" }
scope { "instance" }

transient do
params { {} }
Expand Down
144 changes: 76 additions & 68 deletions spec/models/miq_ae_method_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,44 @@
RSpec.describe MiqAeMethod do
let(:user) { FactoryBot.create(:user_with_group) }
it "should return editable as false if the parent namespace/class is not editable" do
n1 = FactoryBot.create(:miq_ae_system_domain, :tenant => user.current_tenant)
c1 = FactoryBot.create(:miq_ae_class, :namespace_id => n1.id, :name => "foo")
f1 = FactoryBot.create(:miq_ae_method,
:class_id => c1.id,
:name => "foo_method",
:scope => "instance",
:language => "ruby",
:location => "inline")
expect(f1.editable?(user)).to be_falsey
end
let(:sys_domain) { FactoryBot.create(:miq_ae_system_domain) }
let(:domain) { FactoryBot.create(:miq_ae_domain) }
let(:sub_domain) { FactoryBot.create(:miq_ae_namespace, :parent => domain) }

it "should return editable as true if the parent namespace/class is editable" do
n1 = FactoryBot.create(:miq_ae_domain, :tenant => user.current_tenant)
c1 = FactoryBot.create(:miq_ae_class, :namespace_id => n1.id, :name => "foo")
f1 = FactoryBot.create(:miq_ae_method,
:class_id => c1.id,
:name => "foo_method",
:scope => "instance",
:language => "ruby",
:location => "inline")
expect(f1.editable?(user)).to be_truthy
let(:sys_class) { FactoryBot.create(:miq_ae_class, :namespace_id => sys_domain.id) }
let(:reg_class) { FactoryBot.create(:miq_ae_class, :namespace_id => domain.id) }
let(:sub_class) { FactoryBot.create(:miq_ae_class, :namespace_id => sub_domain.id) }

describe "#editable" do
it "should return editable as false if the parent namespace/class is not editable" do
f1 = FactoryBot.create(:miq_ae_method,
:class_id => sys_class.id,
:name => "foo_method")
expect(f1.editable?(user)).to be_falsey
end

it "should return editable as true if the parent namespace/class is editable" do
f1 = FactoryBot.create(:miq_ae_method,
:class_id => reg_class.id,
:name => "foo_method")
expect(f1.editable?(user)).to be_truthy
end
end

it "should lookup method" do
n1 = FactoryBot.create(:miq_ae_system_domain, :tenant => user.current_tenant)
c1 = FactoryBot.create(:miq_ae_class, :namespace_id => n1.id, :name => "foo")
f1 = FactoryBot.create(:miq_ae_method,
:class_id => c1.id,
:name => "foo_method",
:scope => "instance",
:language => "ruby",
:location => "inline")
expect(f1.editable?(user)).to be_falsey

expect(MiqAeMethod.lookup_by_class_id_and_name(c1.id, "foo_method")).to eq(f1)
describe ".lookup_by_class_id_and_name" do
it "should lookup method" do
f1 = FactoryBot.create(:miq_ae_method,
:class_id => sys_class.id,
:name => "foo_method")
expect(f1.editable?(user)).to be_falsey

expect(MiqAeMethod.lookup_by_class_id_and_name(sys_class.id, "foo_method")).to eq(f1)
end
end

it "doesn’t access database when unchanged model is saved" do
n1 = FactoryBot.create(:miq_ae_system_domain, :tenant => user.current_tenant)
c1 = FactoryBot.create(:miq_ae_class, :namespace_id => n1.id, :name => "foo")
f1 = FactoryBot.create(:miq_ae_method,
:class_id => c1.id,
:name => "foo_method",
:scope => "instance",
:language => "ruby",
:location => "inline")
:class_id => reg_class.id,
:name => "foo_method")
expect { f1.valid? }.not_to make_database_queries
end

Expand Down Expand Up @@ -85,15 +77,13 @@
end

context "#copy" do
let(:d2) { FactoryBot.create(:miq_ae_domain, :name => "domain2", :priority => 2) }
let(:ns1) { FactoryBot.create(:miq_ae_namespace, :name => "ns1", :parent => @d1) }
let(:m1) { FactoryBot.create(:miq_ae_method, :class_id => @cls1.id, :name => "foo_method1", :scope => "instance", :language => "ruby", :location => "inline") }
let(:m2) { FactoryBot.create(:miq_ae_method, :class_id => @cls1.id, :name => "foo_method2", :scope => "instance", :language => "ruby", :location => "inline") }
before do
@d1 = FactoryBot.create(:miq_ae_domain, :name => "domain1", :parent => nil, :priority => 1)
@cls1 = FactoryBot.create(:miq_ae_class, :name => "cls1", :namespace_id => ns1.id)
@ns2 = FactoryBot.create(:miq_ae_namespace, :name => "ns2", :parent => d2)
end
let(:d1) { FactoryBot.create(:miq_ae_domain, :priority => 1) }
let(:d2) { FactoryBot.create(:miq_ae_domain, :priority => 2) }
let(:ns1) { FactoryBot.create(:miq_ae_namespace, :parent => d1) }
let(:ns2) { FactoryBot.create(:miq_ae_namespace, :parent => d2) }
let(:m1) { FactoryBot.create(:miq_ae_method, :class_id => cls1.id, :scope => "instance") }
let(:m2) { FactoryBot.create(:miq_ae_method, :class_id => cls1.id, :scope => "instance") }
let(:cls1) { FactoryBot.create(:miq_ae_class, :name => "cls1", :namespace_id => ns1.id) }

it "copies instances under specified namespace" do
options = {
Expand All @@ -109,7 +99,7 @@

it "copy instances under same namespace raise error when class exists" do
options = {
:domain => @d1.name,
:domain => d1.name,
:namespace => ns1.fqname,
:overwrite_location => false,
:ids => [m1.id, m2.id]
Expand All @@ -120,7 +110,7 @@
it "replaces instances under same namespace when class exists" do
options = {
:domain => d2.name,
:namespace => @ns2.name,
:namespace => ns2.name,
:overwrite_location => true,
:ids => [m1.id, m2.id]
}
Expand Down Expand Up @@ -162,28 +152,16 @@
end

it "#domain" do
d1 = FactoryBot.create(:miq_ae_system_domain, :name => 'dom1', :priority => 10)
n1 = FactoryBot.create(:miq_ae_namespace, :name => 'ns1', :parent => d1)
c1 = FactoryBot.create(:miq_ae_class, :namespace_id => n1.id, :name => "foo")
m1 = FactoryBot.create(:miq_ae_method,
:class_id => c1.id,
:name => "foo_method",
:scope => "instance",
:language => "ruby",
:location => "inline")
expect(m1.domain.name).to eql('dom1')
:class_id => sub_class.id,
:name => "foo_method")
expect(m1.domain).to eql(domain)
end

it "#to_export_yaml" do
d1 = FactoryBot.create(:miq_ae_system_domain, :name => 'dom1', :priority => 10)
n1 = FactoryBot.create(:miq_ae_namespace, :name => 'ns1', :parent => d1)
c1 = FactoryBot.create(:miq_ae_class, :namespace_id => n1.id, :name => "foo")
m1 = FactoryBot.create(:miq_ae_method,
:class_id => c1.id,
:name => "foo_method",
:scope => "instance",
:language => "ruby",
:location => "inline")
:class_id => sub_class.id,
:name => "foo_method")
result = m1.to_export_yaml

expect(result['name']).to eql('foo_method')
Expand All @@ -192,4 +170,34 @@
expect(keys.exclude?('options')).to be_truthy
expect(keys.exclude?('embedded_methods')).to be_truthy
end

describe ".name_path_search" do
it "matches name" do
m1 = FactoryBot.create(:miq_ae_method, "name" => "match", :class_id => sub_class.id)
FactoryBot.create(:miq_ae_method, "name" => "nope", :class_id => sys_class.id)

expect(MiqAeMethod.name_path_search("match")).to eq([m1])
end

it "matches a name with a %" do
m1 = FactoryBot.create(:miq_ae_method, "name" => "10_is_bigger", :class_id => sub_class.id)
FactoryBot.create(:miq_ae_method, "name" => "nope", :class_id => sys_class.id)

expect(MiqAeMethod.name_path_search("10%_big")).to eq([m1])
end

it "matches path" do
m1 = FactoryBot.create(:miq_ae_method, "name" => "match", :class_id => sub_class.id)
FactoryBot.create(:miq_ae_method, "name" => "nope", :class_id => sys_class.id)

expect(MiqAeMethod.name_path_search(sub_domain.name)).to eq([m1])
end

it "searches all when blank" do
m1 = FactoryBot.create(:miq_ae_method, "name" => "match", :class_id => sub_class.id)
m2 = FactoryBot.create(:miq_ae_method, "name" => "nope", :class_id => sys_class.id)

expect(MiqAeMethod.name_path_search(nil)).to match_array([m1, m2])
end
end
end