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

Adds support for iis_app InSpec testing #1905

Merged
merged 1 commit into from
Jun 15, 2017
Merged

Adds support for iis_app InSpec testing #1905

merged 1 commit into from
Jun 15, 2017

Conversation

EasyAsABC123
Copy link
Contributor

Signed-off-by: Justin Schuhmann jmschu02@gmail.com

adamleff
adamleff suggested changes Jun 9, 2017
View reviewed changes
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

@EasyAsABC123 well done! I'm so green when it comes to Windows knowledge, so it brings me great joy to see PRs like this come in.

I have two things I'd love to see changed before we move forward on this. Otherwise, it's a very clean implementation and I can't wait to get it into InSpec.

lib/resources/iis_app.rb Outdated
end

def has_protocol?(protocol)
(iis_app[:protocols].include? protocol)
Copy link
Contributor

Choose a reason for hiding this comment

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

This syntax is a bit strange. Can we change this to:

iis_app[:protocols].include?(protocol)

lib/resources/iis_app.rb Outdated

def iis_app
return @cache unless @cache.nil?
@cache = @app_provider.iis_app(@path, @site_name) unless @app_provider.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually introduce a new "provider" class like this whenever we have a situation where we may need to interact differently with a target depending on the OS type/version, etc. In this case, since this will only be supported on windows, we can probably get rid of AppProvider and move the contents of AppProvider#iis_app into this iis_app method. That will eliminate an unnecessary layer of abstraction.

@EasyAsABC123
Copy link
Contributor Author

@adamleff let me know if this is what you meant

@adamleff
Copy link
Contributor

@EasyAsABC123 yes, exactly!

Now you're failing tests unfortunately. It looks like indentation got messed up which has led to you missing an end. Lines 17 onward should be indented two spaces in under the class declaration, and then it becomes clearer that an end got dropped.

Ping me once you've got that fixed up and I'll give it another review.

@EasyAsABC123
Copy link
Contributor Author

@adamleff good catch, totally didn't notice. It is now fixed
...locally it didn't break because i can't have module Inspec::Resources at the top current with how i was testing.

adamleff
adamleff approved these changes Jun 12, 2017
View reviewed changes
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

@EasyAsABC123 well done! Thanks for your contribution!

chris-rock
chris-rock suggested changes Jun 15, 2017
View reviewed changes
Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

Great job @EasyAsABC123 I left some comments about minor improvements. Once they are addressed, LGTM

lib/resources/iis_app.rb Outdated
"iis_app '#{@site_name}#{@path}'"
end

def iis_app
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a private function

lib/resources/iis_app.rb Outdated
# frozen_string_literal: true
# check for web applications in IIS
# Usage:
# describe iis_app('/myapp', 'Website') do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do not need the usage comments here, since it is part of the inline examples

test/cookbooks/os_prepare/recipes/default.rb
@@ -16,7 +16,7 @@
include_recipe('os_prepare::service')
include_recipe('os_prepare::package')
include_recipe('os_prepare::registry_key')
include_recipe('os_prepare::iis_site')
include_recipe('os_prepare::iis')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good move. We should add this resource to our integration tests then too

@EasyAsABC123
Copy link
Contributor Author

@chris-rock Thanks, good recommendations. Added them, although i implemented New-WebApplication as a powershell script for testing purposes.

chris-rock
chris-rock reviewed Jun 15, 2017
View reviewed changes
lib/resources/iis_app.rb Outdated

def iis_app
return @cache unless @cache.nil?
command = "Import-Module WebAdministration; Get-WebApplication -Name '#{@path}' -Site '#{@site_name}' | Select-Object * | ConvertTo-Json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed that before: What are the preconditions for the Powershell module? Can we expect this is deployed on IIS servers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None that i'm aware of this is just a built in module for 2012r2 as far as i know, i am avoid xWebAdministration for this reason.

For Windows 7 to 10 you will need to install the feature IIS Management Scripts and tools under Internet Information Services >> Web Management Tools

So it should be done simply from the IIS install.

@chris-rock
Copy link
Contributor

Wow, that was quick :-)

@EasyAsABC123
Adds support for iis_app InSpec testing
906251f 
Signed-off-by: Justin Schuhmann <jmschu02@gmail.com>
@EasyAsABC123
Copy link
Contributor Author

yeah, can't sleep...always a bummer tomorrow is going to be a coffee fueled haze

chris-rock
chris-rock reviewed Jun 15, 2017
View reviewed changes
lib/resources/iis_app.rb

def iis_app
return @cache unless @cache.nil?
command = "Import-Module WebAdministration; Get-WebApplication -Name '#{@path}' -Site '#{@site_name}' | Select-Object * | ConvertTo-Json"
Copy link
Contributor

Choose a reason for hiding this comment

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

One last question: What is the requirement for the WebAdministration module. Will it be on all IIS servers by default or do users need to install it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chris-rock
None that i'm aware of this is just a built in module for 2012r2 as far as i know, i am avoid xWebAdministration for this reason.

For Windows 7 to 10 you will need to install the feature IIS Management Scripts and tools under Internet Information Services >> Web Management Tools

So it should be done simply from the IIS install.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chris-rock never an issue, i'll take these recommendations to the other inspec resources i've written in the iis-cookbook.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to bring more of those resources into core inspec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chris-rock of course! i'll be bringing them over one at a time to keep the reviews small.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very cool. Thank you for doing that!

@chris-rock chris-rock merged commit a69cd1e into inspec:master Jun 15, 2017
@EasyAsABC123 EasyAsABC123 deleted the enhancement-iis_app branch June 15, 2017 09:13
@adamleff adamleff added the Type: New Feature Adds new functionality label Jun 15, 2017
@chris-rock chris-rock mentioned this pull request Jun 28, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris-rock chris-rock chris-rock approved these changes

@adamleff adamleff adamleff approved these changes

@arlimus arlimus Awaiting requested review from arlimus

Assignees
No one assigned
Labels
Type: New Feature Adds new functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@EasyAsABC123 @adamleff @chris-rock