quote username and hostname in mssql_session.rb

[dmytro-malovany]

If username contains backslash or hosthame variable contains port number like:

host: "localhost,1433"
user: "DOMAIN\sa"

inspec scan fails with error:

Sqlcmd: '1433': Unexpected argument. Enter '-?' for help.
Could not execute the sql query

This PR fixes this issue.

[adamleff]

Hi, @bratdim! Thank you for your pull request!

First of all, it looks like this might be your first PR ever on GitHub, so THAT'S AWESOME. Thank you for taking the time to contribute to open source software.

Now, let's talk about your PR. The change itself is actually totally fine, and wrapping the user and host in quotes makes sense to address your use case. Unfortunately, we have a few test failures we need to address.

Firstly, our project uses the Developer Certificate or Origin, or DCO, to ensure that it's legally OK for us to accept your contribution. You can read more about the DCO, but essentially every commit must have a sign-off. We'll need you to fix that in your commit message so our DCO bot gives us a green checkmark.

Secondly, your change has caused some unit test breakages. During unit tests, InSpec mocks the commands that would be executed on a host and returns data rather than executing the command directly. Because you changed the actual commands being run (even though you just added single-quotes, the command is different), the commands are no longer mocked and therefore the tests are failing. We need to fix that.

I pulled down your branch and I ran just the mssql tests with the command: m test/unit/resources/mssql_session_test.rb and see the following commands not being mocked:

Command not mocked:
    sqlcmd -Q "set nocount on; select getdate()" -W -w 1024 -s ',' -U 'sa' -P 'yourStrong(!)Password' -S 'localhost'
    SHA: 4b550bb227058ac5851aa0bc946be794ee46489610f17842700136cf8bb5a0e9
Could not execute the sql query
.Command not mocked:
    sqlcmd -Q "set nocount on; select getdate()" -W -w 1024 -s ',' -U 'DOMAIN\sa' -P 'yourStrong(!)Password' -S 'localhost,1533'
    SHA: 5c2bc0f0568d11451d6cf83aff02ee3d47211265b52b6c5d45f8e57290b35082
Could not execute the sql query
.Command not mocked:
    sqlcmd -Q "set nocount on; select getdate()" -W -w 1024 -s ',' -U 'sa' -P 'yourStrong(!)Password' -S 'localhost'
    SHA: 4b550bb227058ac5851aa0bc946be794ee46489610f17842700136cf8bb5a0e9
Could not execute the sql query
Command not mocked:
    sqlcmd -Q "set nocount on; SELECT SERVERPROPERTY('ProductVersion') as result" -W -w 1024 -s ',' -U 'sa' -P 'yourStrong(!)Password' -S 'localhost'
    SHA: aeb859a4ae4288df230916075c0de28781a2b215f41d64ed1ea9c3fd633140fa
Could not execute the sql query

The 1st, 3rd, and 4th are commands that were already there but are now different because of the quotes. The 2nd is a new test that you added that also needs to be mocked.

You can mock commands either with the full string, or if the string is long, we support using the shasum outputted above. The commands are mocked here: https://github.com/chef/inspec/blob/master/test/helper.rb#L184-L391 -- and then all the mock output is located in test/unit/mock/cmd

So for your change, you need to:

• change the shasum here https://github.com/chef/inspec/blob/master/test/helper.rb#L369 to 4b550bb227058ac5851aa0bc946be794ee46489610f17842700136cf8bb5a0e9
• change the shasum here https://github.com/chef/inspec/blob/master/test/helper.rb#L370 to aeb859a4ae4288df230916075c0de28781a2b215f41d64ed1ea9c3fd633140fa
• add a new entry below line 369 that looks like this: "5c2bc0f0568d11451d6cf83aff02ee3d47211265b52b6c5d45f8e57290b35082" => cmd.call('mssql-getdate'), to account for your new test

I made these changes locally and it appeared to solve the issue.

Please reach out if you have any questions. Thanks!

[dmytro-malovany]

@adamleff , thank you for detailed guideline how to do it! You are right, this is my first public PR and and exited to have this experience! I hope it's first but not last one ;)

[adamleff]

@bratdim your fixes look good and it looks like tests are now passing!

We have a new problem however... it looks like you may have called git merge master or something similar because your PR now has a whole bunch of commits on it that are completely unrelated to your change. We need to fix that up... likely be updating your master branch against the origin, and then calling git rebase -i origin/master (or similar), eliminate the commits that aren't part of your change, and squash your changes into a single commit, and then force push back to your branch.

You can check that this was successful by going back to the PR and clicking on the "Files changed" tab and you should only see the changes you made, vs. the changes that were part of other pull requests in the past.

Let me know if you have any questions. Thanks!

[dmytro-malovany]

@adamleff yeah, seems I've made a few mistakes here. Anyway, it's fixed now. Thank you!

[adamleff]

This is a great change, @bratdim! At some point in the future, we should break the building of the cmd_string to a separate method so that we can test it independently... it would be great to test that the single-quotes are being put in place correctly, etc. However, we don't have to address it in this PR.

I approve of this change, and once another maintainer approves, we'll merge it to master. It should be included in this week's InSpec release!

Thank you again for making your first-ever GitHub PR a PR for the InSpec project!

[arlimus]

Thank you for contributing @bratdim !! Kudos for the continued improvements and kudos to @adamleff for guiding and supporting 😁 👍