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

Proposed by Joseph Borg
Status: Merged
Approved by: Alex Kavanagh
Approved revision: 760
Merged at revision: 784
Proposed branch: lp:~joeborg/charm-helpers/apache-config
Merge into: lp:charm-helpers
Diff against target: 41 lines (+14/-2)
2 files modified
charmhelpers/contrib/hardening/apache/checks/ (+3/-2)
tests/contrib/hardening/apache/checks/ (+11/-0)
To merge this branch: bzr merge lp:~joeborg/charm-helpers/apache-config
Reviewer Review Type Date Requested Status
Alex Kavanagh Approve
Review via email:

Description of the change

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

To post a comment you must log in.
Alex Kavanagh (ajkavanagh) wrote :

There's no test in tests/contrib/hardening/apache/checks/ 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) wrote :

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) wrote :

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/ 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) wrote :

Added test

Alex Kavanagh (ajkavanagh) wrote :

Looks good to me. Thanks for adding the test.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmhelpers/contrib/hardening/apache/checks/'
2--- charmhelpers/contrib/hardening/apache/checks/ 2017-03-20 10:28:01 +0000
3+++ charmhelpers/contrib/hardening/apache/checks/ 2017-07-21 11:26:37 +0000
4@@ -46,8 +46,9 @@
5 context = ApacheConfContext()
6 settings = utils.get_settings('apache')
7 audits = [
8- FilePermissionAudit(paths='/etc/apache2/apache2.conf', user='root',
9- group='root', mode=0o0640),
10+ FilePermissionAudit(paths=os.path.join(
11+ settings['common']['apache_dir'], 'apache2.conf'),
12+ user='root', group='root', mode=0o0640),
14 TemplatedFile(os.path.join(settings['common']['apache_dir'],
15 'mods-available/alias.conf'),
17=== modified file 'tests/contrib/hardening/apache/checks/'
18--- tests/contrib/hardening/apache/checks/ 2017-03-20 10:28:01 +0000
19+++ tests/contrib/hardening/apache/checks/ 2017-07-21 11:26:37 +0000
20@@ -12,6 +12,7 @@
21 # See the License for the specific language governing permissions and
22 # limitations under the License.
24+import os
25 import shutil
26 import tempfile
28@@ -86,3 +87,13 @@
29 'honor_cipher_order': 'on',
30 'cipher_suite': 'ALL:+MEDIUM:+HIGH:!LOW:!MD5:!RC4:!eNULL:!aNULL:!3DES'
31 })
33+ @patch.object(config.subprocess, 'call', lambda *args, **kwargs: 0)
34+ def test_file_permission_audit(self):
35+ audits = config.get_audits()
36+ settings = config.utils.get_settings('apache')
37+ conf_file_name = 'apache2.conf'
38+ conf_file_path = os.path.join(
39+ settings['common']['apache_dir'], conf_file_name
40+ )
41+ self.assertEqual(audits[0].paths[0], conf_file_path)


People subscribed via source and target branches