Merge lp:~joeborg/charm-helpers/apache-config into lp:charm-helpers

[Joseph Borg]

One of the audits had the Apache directory path hard coded.

[Alex Kavanagh (ajkavanagh)]

There's no test in tests/contrib/hardening/apache/checks/test_config.py for this change. I agree that it should be made, but it'd be nice to see a test for it.

review: Needs Fixing

[Joseph Borg (joeborg)]

Hey Alex,

Any suggestions on how to test this without having to elevate the test's permissions? Looking over the other FilePermissionAudit calls, none of them have associated specific tests.

[Alex Kavanagh (ajkavanagh)]

Hey Joe

I admit that the current test coverage is not great. This isn't a detectable change at present, but the infrastructure is there in tests/contrib/hardening/apache/checks/test_config.py at line 52:

Existing function:

    @patch.object(config.utils, 'get_settings', lambda x: {
        'common': {'apache_dir': TEST_TMPDIR,
                   'traceenable': 'Off'},
        'hardening': {
            'allowed_http_methods': {'GOGETEM'},
            'modules_to_disable': {'modfoo'}
        }
    })
    @patch.object(config.subprocess, 'call', lambda *args, **kwargs: 0)
    def test_get_audits_apache_is_installed(self):
        audits = config.get_audits()
        self.assertEqual(7, len(audits))

Essentially, the 1st element of audits is being changed here, so you could check after the self.assertEqual(7, len(audits)) something like this:

    self.assertEqual(audits[1].paths, ["{}/apache.conf".format(TEST_TMPDIR)])

This would at least verify that your change is what you think it should be. I agree that it's tricky with these config/declaration type changes. Perhaps also add a comment to indicate that it's a change of behaviour from previously?

However, I'm open to the idea of not testing this change, and just accepting it as is.

[Joseph Borg (joeborg)]

Added test

[Alex Kavanagh (ajkavanagh)]

Looks good to me. Thanks for adding the test.

review: Approve

=== modified file 'charmhelpers/contrib/hardening/apache/checks/config.py'
--- charmhelpers/contrib/hardening/apache/checks/config.py	2017-03-20 10:28:01 +0000
+++ charmhelpers/contrib/hardening/apache/checks/config.py	2017-07-21 11:26:37 +0000
@@ -46,8 +46,9 @@
     context = ApacheConfContext()
     settings = utils.get_settings('apache')
     audits = [
-        FilePermissionAudit(paths='/etc/apache2/apache2.conf', user='root',
-                            group='root', mode=0o0640),
+        FilePermissionAudit(paths=os.path.join(
+                            settings['common']['apache_dir'], 'apache2.conf'),
+                            user='root', group='root', mode=0o0640),
 
         TemplatedFile(os.path.join(settings['common']['apache_dir'],
                                    'mods-available/alias.conf'),

=== modified file 'tests/contrib/hardening/apache/checks/test_config.py'
--- tests/contrib/hardening/apache/checks/test_config.py	2017-03-20 10:28:01 +0000
+++ tests/contrib/hardening/apache/checks/test_config.py	2017-07-21 11:26:37 +0000
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
 import shutil
 import tempfile
 
@@ -86,3 +87,13 @@
                 'honor_cipher_order': 'on',
                 'cipher_suite': 'ALL:+MEDIUM:+HIGH:!LOW:!MD5:!RC4:!eNULL:!aNULL:!3DES'
             })
+
+    @patch.object(config.subprocess, 'call', lambda *args, **kwargs: 0)
+    def test_file_permission_audit(self):
+        audits = config.get_audits()
+        settings = config.utils.get_settings('apache')
+        conf_file_name = 'apache2.conf'
+        conf_file_path = os.path.join(
+            settings['common']['apache_dir'], conf_file_name
+        )
+        self.assertEqual(audits[0].paths[0], conf_file_path)