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

Add support for modifying the asset linking global preference #202

Merged
merged 3 commits into from
Dec 10, 2015
Merged

Add support for modifying the asset linking global preference #202

merged 3 commits into from
Dec 10, 2015

Conversation

erran-r7
Copy link
Contributor

Create the new #parse_asset_linking_from_xml and #add_asset_linking_to_xml
methods to Nexpose::GlobalSettings.

Example usage:

connection  = Nexpose::Connection.new("<YOUR_CONSOLE>", "<YOUR USER>", "<YOUR PASSWORD>")

global_settings = Nexpose::GlobalSettings.load(connection)

global_settings.asset_linking = true

global_settings.save(connection)

recommended reviewers: @gschneider-r7 @lsanchez-r7 @mpodber-r7

@erran
Add support for modifying the asset correlation
ed45e19 
Create the new #parse_asset_correlation_from_xml and #add_asset_correlation_to_xml
methods to Nexpose::GlobalSettings.

Example usage:

```ruby
global_settings = Nexpose::GlobalSettings.load(connection)

global_settings.asset_correlation = true

global_settings.save(connection)
```
@erran-r7 erran-r7 changed the title Add support for modifying the asset correlation Add support for modifying the asset correlation global preference Dec 10, 2015
jhart-r7
jhart-r7 reviewed Dec 10, 2015
View reviewed changes
lib/nexpose/global_settings.rb
@@ -6,6 +6,10 @@ class GlobalSettings
# all sites.
attr_accessor :asset_exclusions

# Whether asset linking in enabled. A feature tied to ControlsInsight
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the ControlsInsight comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really the right description ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy pasta 🍝

@kprzerwa-r7
Copy link
Contributor

Awesome! This is exactly what I need to entirely script the scenario I am trying to reproduce :)

@erran
Rename GlobalSettings#asset_correlation to GlobalSettings#asset_linking
b790d2c 
The Nexpose UI references correlation as cross site asset linking.
erran-r7
erran-r7 reviewed Dec 10, 2015
View reviewed changes
lib/nexpose/global_settings.rb

# Internal method for parsing XML for whether asset linking in enabled.
def parse_asset_linking_from_xml(xml)
enabled = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kprzerwa-r7 @gschneider-r7 @sgreen-r7 @jhart-r7 I can't recall what the default value in Nexpose is, maybe it is actually true.

Old versions of Nexpose didn't support it which is why I left this as false.

Copy link
Contributor

Choose a reason for hiding this comment

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

For new installs, the default is true; for upgrades, false. AFAIK.

Regarding adding the element if it doesn't exist, yeah, I think leaving it out is probably safer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is enabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kprzerwa-r7 @jhart-r7 Pushing a change to make this true. I'll leave out adding the element if it doesn't exist.

@erran-r7
Copy link
Contributor Author

@jhart-r7 looks like your comment auto-collapsed since I pushed code that affected a line near what you'd mentioned:

At least in the code for managing scan templates, which I admit is a different beast than global settings, the way it works there is if the corresponding enclosing element is not found, it is added and then the corresponding attributes/elements are added on/below it. May or may not be a good thing here to do that.

@gschneider-r7 @kprzerwa-r7 @sgreen-r7 Opinions? I think my approach is sufficient. It also ensures we don't submit an invalid element to versions of Nexpose that didn't support asset linking/correlation.

@erran
Update #parse_asset_linking_from_xml's default return value
21a2894 
By default Nexpose customers have this value enabled. Customers who have
set it false will have the element in their global settings and
therefore have it correctly mutated to false in the lines that follow
this default value.
@gschneider-r7
Copy link
Contributor

It also ensures we don't submit an invalid element to versions of Nexpose that didn't support asset linking/correlation.

I am pretty sure that any consoles that don't support asset linking can't really be used very well with the 1.0 or later versions of the gem anyway, so I'm not really concerned about that.

The changes look good to me. 👍

@gschneider-r7 gschneider-r7 changed the title Add support for modifying the asset correlation global preference Add support for modifying the asset linking global preference Dec 10, 2015
gschneider-r7 added a commit that referenced this pull request Dec 10, 2015
@gschneider-r7
Merge pull request #202 from rapid7/support-asset-linking
c57e9e2 
Add support for modifying the asset linking global preference
@gschneider-r7 gschneider-r7 merged commit c57e9e2 into master Dec 10, 2015
@erran-r7 erran-r7 deleted the support-asset-linking branch December 10, 2015 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erran-r7 @kprzerwa-r7 @gschneider-r7 @jhart-r7 @erran