launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03396
[Merge] lp:~jcsackett/launchpad/remove-duplicate-settings into lp:launchpad
j.c.sackett has proposed merging lp:~jcsackett/launchpad/remove-duplicate-settings into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #765207 in Launchpad itself: "Duplicate settings in security.cfg"
https://bugs.launchpad.net/launchpad/+bug/765207
For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/remove-duplicate-settings/+merge/58528
Summary
=======
Investigating errors occuring in some cronscripts, duplicate security settings were found in database/schema/security.cfg. This has the effect of removing permissions that are needed for some scripts, resulting in failures.
A previous branch created a utiility that identified these duplicates. This branch removes the dupes under the advisement that in each case we want to keep the setting that provides the broadest permissions for the cronscripts.
Preimplementation
=================
Spoke with Curtis Hovey about which settings to keep.
Implementation
==============
database/schema/security.cfg
----------------------------
Removed duplicate settings within each configuration block (e.g. a section beginning with [$SECTION].
In some cases, comments have also been removed. This only happened when a group of settings the comment applied to were better defined elsewhere in the configuration block, so the commented section wasn't needed.
Tests
=====
Given the number of things removed, ec2 is the only viable test. This branch has already been submitted.
QA
==
qa-untestable
Lint
====
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
database/schema/security.cfg
lib/lp/scripts/utilities/tests/test_audit_security_settings.py
utilities/audit-security-settings.py
database/schema/security.cfg
740: Line exceeds 78 characters.
741: Line exceeds 78 characters.
742: Line exceeds 78 characters.
768: Line exceeds 78 characters.
772: Line exceeds 78 characters.
827: Line exceeds 78 characters.
842: Line exceeds 78 characters.
843: Line exceeds 78 characters.
859: Line exceeds 78 characters.
860: Line exceeds 78 characters.
863: Line exceeds 78 characters.
864: Line exceeds 78 characters.
865: Line exceeds 78 characters.
917: Line exceeds 78 characters.
918: Line exceeds 78 characters.
919: Line exceeds 78 characters.
947: Line exceeds 78 characters.
1029: Line exceeds 78 characters.
1039: Line exceeds 78 characters.
1040: Line exceeds 78 characters.
Errors in security.cfg are a result of the cfg format.
--
https://code.launchpad.net/~jcsackett/launchpad/remove-duplicate-settings/+merge/58528
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/remove-duplicate-settings into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2011-04-19 12:20:36 +0000
+++ database/schema/security.cfg 2011-04-20 16:23:42 +0000
@@ -236,7 +236,6 @@
public.packagecopyrequest = SELECT, INSERT, UPDATE
public.packagebuild = DELETE
public.packagediff = SELECT, INSERT, UPDATE, DELETE
-public.packagediff = SELECT, INSERT, UPDATE, DELETE
public.packageset = SELECT, INSERT, UPDATE, DELETE
public.packagesetgroup = SELECT, INSERT, UPDATE, DELETE
public.packagesetsources = SELECT, INSERT, UPDATE, DELETE
@@ -316,7 +315,6 @@
public.translationmessage = SELECT, INSERT, UPDATE, DELETE
public.translationtemplatesbuild = SELECT, INSERT, UPDATE, DELETE
public.translator = SELECT, INSERT, UPDATE, DELETE
-public.usertouseremail = SELECT, UPDATE
public.validpersoncache = SELECT
public.validpersonorteamcache = SELECT
public.votecast = SELECT, INSERT
@@ -614,7 +612,6 @@
public.person = SELECT, INSERT, UPDATE
public.personsettings = SELECT, INSERT
public.personlanguage = SELECT
-public.personsettings = SELECT, INSERT
public.product = SELECT, UPDATE
public.productseries = SELECT
public.project = SELECT, UPDATE
@@ -853,7 +850,6 @@
public.archiveauthtoken = SELECT, UPDATE
public.archivepermission = SELECT, INSERT
public.archivesubscriber = SELECT, UPDATE
-public.binarypackagepublishinghistory = SELECT
public.distributionjob = SELECT, INSERT
public.gpgkey = SELECT, INSERT, UPDATE
public.packagecopyrequest = SELECT, INSERT, UPDATE
@@ -867,7 +863,6 @@
public.flatpackagesetinclusion = SELECT, INSERT, UPDATE, DELETE
public.binarypackagepublishinghistory = SELECT, INSERT, UPDATE, DELETE
public.sourcepackagepublishinghistory = SELECT, INSERT, UPDATE, DELETE
-public.sourcepackagepublishinghistory = SELECT
public.distributionsourcepackage = SELECT, INSERT, UPDATE
# Closing bugs for publication copies.
@@ -930,12 +925,10 @@
public.distroseries = SELECT, UPDATE
public.distroarchseries = SELECT, UPDATE
public.sourcepackagepublishinghistory = SELECT
-public.sourcepackagepublishinghistory = SELECT
public.sourcepackagerelease = SELECT
public.sourcepackagereleasefile = SELECT
public.sourcepackagename = SELECT
public.binarypackagepublishinghistory = SELECT
-public.binarypackagepublishinghistory = SELECT
public.binarypackagerelease = SELECT
public.binarypackagefile = SELECT
public.binarypackagename = SELECT
@@ -1291,7 +1284,7 @@
public.distributionjob = SELECT, INSERT
public.distroseries = SELECT, UPDATE
public.distroarchseries = SELECT
-public.sourcepackagepublishinghistory = SELECT
+public.sourcepackagepublishinghistory = SELECT, INSERT
public.distributionsourcepackage = SELECT, INSERT, UPDATE
public.sourcepackagefilepublishing = SELECT
public.binarypackagefilepublishing = SELECT
@@ -1335,9 +1328,6 @@
public.packageuploadbuild = SELECT, INSERT
public.packageuploadcustom = SELECT, INSERT
-# For premature source-only publication
-public.sourcepackagepublishinghistory = SELECT, INSERT
-
# Closing bugs for premature source-only publication
public.bug = SELECT, UPDATE
public.bugactivity = SELECT, INSERT
@@ -1360,8 +1350,6 @@
public.message = SELECT, INSERT
public.messagechunk = SELECT, INSERT
public.productseries = SELECT
-public.validpersoncache = SELECT
-public.validpersonorteamcache = SELECT
public.karmaaction = SELECT
public.karma = SELECT, INSERT
public.questionbug = SELECT
@@ -1377,7 +1365,6 @@
public.questionsubscription = SELECT
public.answercontact = SELECT
public.personlanguage = SELECT
-public.section = SELECT
public.structuralsubscription = SELECT
# Diffing against ancestry and maintenance tasks.
@@ -1394,7 +1381,6 @@
public.emailaddress = SELECT, INSERT, UPDATE
public.teamparticipation = SELECT, INSERT
public.teammembership = SELECT
-public.distrocomponentuploader = SELECT
public.gpgkey = SELECT
# The Queue
@@ -1434,7 +1420,6 @@
public.sourcepackagerecipebuild = SELECT
public.sourcepackagerecipebuildjob = SELECT, INSERT, UPDATE
public.component = SELECT
-public.section = SELECT
public.componentselection = SELECT
public.sectionselection = SELECT
public.packagediff = SELECT, UPDATE
@@ -1732,7 +1717,6 @@
public.distribution = SELECT, UPDATE
public.distributionsourcepackage = SELECT, INSERT, UPDATE
public.distrocomponentuploader = SELECT
-public.distroseries = SELECT
public.archivepermission = SELECT
public.distroseries = SELECT
public.project = SELECT, UPDATE
@@ -1759,10 +1743,10 @@
public.bugaffectsperson = SELECT, INSERT, UPDATE, DELETE
public.bugjob = SELECT, INSERT
public.bugsubscription = SELECT, INSERT
-public.bugsubscriptionfilter = SELECT, INSERT
-public.bugsubscriptionfilterstatus = SELECT, INSERT
-public.bugsubscriptionfilterimportance = SELECT, INSERT
-public.bugsubscriptionfiltertag = SELECT, INSERT
+public.bugsubscriptionfilter = SELECT, INSERT, UPDATE, DELETE
+public.bugsubscriptionfilterstatus = SELECT, INSERT, UPDATE, DELETE
+public.bugsubscriptionfilterimportance = SELECT, INSERT, UPDATE, DELETE
+public.bugsubscriptionfiltertag = SELECT, INSERT, UPDATE, DELETE
public.bugnotification = SELECT, INSERT
public.bugnotificationfilter = SELECT, INSERT
public.bugnotificationattachment = SELECT
@@ -1772,10 +1756,6 @@
public.bugtask = SELECT, INSERT, UPDATE
public.bugmessage = SELECT, INSERT
public.bugsubscription = SELECT, INSERT, UPDATE, DELETE
-public.bugsubscriptionfilter = SELECT, INSERT, UPDATE, DELETE
-public.bugsubscriptionfilterstatus = SELECT, INSERT, UPDATE, DELETE
-public.bugsubscriptionfilterimportance = SELECT, INSERT, UPDATE, DELETE
-public.bugsubscriptionfiltertag = SELECT, INSERT, UPDATE, DELETE
public.bugtracker = SELECT, INSERT
public.bugtrackeralias = SELECT, INSERT
public.bugwatch = SELECT, INSERT
@@ -1805,7 +1785,6 @@
# Specification notifications
public.specification = SELECT
public.specificationsubscription = SELECT
-public.structuralsubscription = SELECT
# Emails may have files attached.
public.bugattachment = SELECT, INSERT
@@ -2243,7 +2222,6 @@
public.codeimportevent = SELECT, DELETE
public.codeimporteventdata = SELECT, DELETE
public.codeimportresult = SELECT, DELETE
-public.emailaddress = SELECT
public.oauthnonce = SELECT, DELETE
public.openidassociation = SELECT, DELETE
public.openidconsumerassociation = SELECT, DELETE
=== added file 'lib/lp/scripts/utilities/tests/test_audit_security_settings.py'
--- lib/lp/scripts/utilities/tests/test_audit_security_settings.py 1970-01-01 00:00:00 +0000
+++ lib/lp/scripts/utilities/tests/test_audit_security_settings.py 2011-04-20 16:23:42 +0000
@@ -0,0 +1,26 @@
+
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests the security.cfg auditor."""
+
+__metaclass__ = type
+
+import os
+
+from canonical.config import config
+from canonical.testing.layers import BaseLayer
+from lp.testing import TestCase
+
+
+class TestAuditSecuitySettings(TestCase):
+
+ layer = BaseLayer
+
+ def test_duplicate_parsing(self):
+ utility = os.path.join(
+ config.root, 'utilities', 'audit-security-settings.py')
+ cmd = '%s smoketest' % utility
+ error_msg = os.popen(cmd).read()
+ expected = '[bad]\n\tDuplicate setting found: public.bar\n'
+ self.assertTrue(expected in error_msg)
=== added file 'utilities/audit-security-settings.py'
--- utilities/audit-security-settings.py 1970-01-01 00:00:00 +0000
+++ utilities/audit-security-settings.py 2011-04-20 16:23:42 +0000
@@ -0,0 +1,112 @@
+#! /usr/bin/python -S
+
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Check that everything is alright in security.cfg
+
+Usage hint:
+
+% utilities/audit-security.py
+"""
+__metatype__ = type
+
+import os
+import sys
+import re
+
+from collections import defaultdict
+
+TEST_DATA = """
+[good]
+public.foo = SELECT
+public.bar = SELECT, INSERT
+public.baz = SELECT
+
+[bad]
+public.foo = SELECT
+public.bar = SELECT, INSERT
+public.bar = SELECT
+public.baz = SELECT
+"""
+
+BRANCH_ROOT = os.path.split(
+ os.path.dirname(os.path.abspath(__file__)))[0]
+SECURITY_PATH = os.path.join(
+ BRANCH_ROOT, 'database', 'schema', 'security.cfg')
+
+
+def strip(data):
+ data = [d.strip() for d in data]
+ return [d for d in data if not (d.startswith('#') or d == '')]
+
+
+class SettingsAuditor:
+ """Reads the security.cfg file and collects errors.
+
+ We can't just use ConfigParser for this case, as we're doing our own
+ specialized parsing--not interpreting the settings, but verifying."""
+
+ section_regex = re.compile(r'\[.*\]')
+
+ def __init__(self):
+ self.errors = {}
+ self.current_section = ''
+ self.observed_settings = defaultdict(lambda: 0)
+
+ def _get_section_name(self, line):
+ if line.strip().startswith('['):
+ return self.section_regex.match(line).group()
+ else:
+ return None
+
+ def _get_setting(self, line):
+ return line.split()[0]
+
+ def start_new_section(self, new_section):
+ for k in self.observed_settings.keys():
+ if self.observed_settings[k] == 1:
+ self.observed_settings.pop(k)
+ duplicated_settings = self.observed_settings.keys()
+ if len(duplicated_settings) > 0:
+ self.errors[self.current_section] = self.observed_settings.keys()
+ self.observed_settings = defaultdict(lambda: 0)
+ self.current_section = new_section
+
+ def readline(self, line):
+ new_section = self._get_section_name(line)
+ if new_section is not None:
+ self.start_new_section(new_section)
+ else:
+ setting = self._get_setting(line)
+ self.observed_settings[setting] += 1
+
+ def print_error_data(self):
+ print "The following errors were found in security.cfg"
+ print "-----------------------------------------------"
+ for section in self.errors.keys():
+ print "In section: %s" % section
+ for setting in self.errors[section]:
+ print '\tDuplicate setting found: %s' % setting
+
+
+def main(test=False):
+ # This is a cheap hack to allow testing in the testrunner.
+ if test:
+ data = TEST_DATA.split('\n')
+ else:
+ data = file(SECURITY_PATH).readlines()
+ data = strip(data)
+ auditor = SettingsAuditor()
+ for line in data:
+ auditor.readline(line)
+ auditor.start_new_section('')
+ auditor.print_error_data()
+
+if __name__ == '__main__':
+ # smoketest check is a cheap hack to test the utility in the testrunner.
+ try:
+ test = sys.argv[1] == 'smoketest'
+ except IndexError:
+ test = False
+ main(test=test)
Follow ups
