launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04332
[Merge] lp:~gary/launchpad/bug812335 into lp:launchpad
Gary Poster has proposed merging lp:~gary/launchpad/bug812335 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~gary/launchpad/bug812335/+merge/68762
This branch adds security checks for branches, so that we do not leak knowledge of private branches.
As discussed in the bug comments, it does not take stacked branches into account for security. It does take subscriptions, ownership, and team membership into account, as shown in the tests.
Lint is happy.
The biggest thing to talk about with this branch that I know about is performance. In order to evaluate performance of the SQL, I logged on to the staging database and played around with queries.
For Ubuntu, the original security-naive query took about 3.7 seconds on staging, and the best query that I found takes about 4.2. I experimented with a lot of different SQL approaches to try and reduce the query time. This was the fastest, compared to the worst case of around 6 seconds. You are welcome to look at those and see if you have any suggestions for further experiments.
http://pastebin.ubuntu.com/649548/
This approach assumes that public branches will far outweigh private. That's certainly the case now: we have no private branches.
I am concerned about this timing, even though I have added relatively minimally to it. The simplest solution to me is to move the getBranchTips from IDistribution to IDistroSerues. This is what Jono suggested would make more sense for Ubuntu anyway. For Prinipia, Ensemble, or other clients that might actually the full distribution, it would be trivial to iterate of a distro's .series and call getBranchTips on each. This change would be fairly easy to make, and I'd like to do so. It brings the Ubuntu calls without and with security from 3.7 and 4.2 seconds respectively for the distribution down to 0.66 and 0.69 respectively for the Oneiric distro series. Anybody else in favor? :-)
--
https://code.launchpad.net/~gary/launchpad/bug812335/+merge/68762
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug812335 into lp:launchpad.
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py 2011-07-14 14:08:04 +0000
+++ lib/lp/registry/model/distribution.py 2011-07-22 00:53:30 +0000
@@ -69,6 +69,7 @@
IHasMugshot,
)
from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.launchpad.webapp.interfaces import ILaunchBag
from canonical.launchpad.webapp.url import urlparse
from lp.answers.enums import QUESTION_STATUS_DEFAULT_SEARCH
from lp.answers.interfaces.faqtarget import IFAQTarget
@@ -657,24 +658,89 @@
def getBranchTips(self, since=None):
"""See `IDistribution`."""
- query = """
- SELECT unique_name, last_scanned_id, SPBDS.name FROM Branch
- JOIN DistroSeries
- ON Branch.distroseries = DistroSeries.id
- LEFT OUTER JOIN SeriesSourcePackageBranch
- ON Branch.id = SeriesSourcePackageBranch.branch
- LEFT OUTER JOIN DistroSeries SPBDS
- -- (SPDBS stands for Source Package Branch Distro Series)
- ON SeriesSourcePackageBranch.distroseries = SPBDS.id
- WHERE DistroSeries.distribution = %s""" % sqlvalues(self.id)
-
+ person = getUtility(ILaunchBag).user
+ # This, ignoring privacy issues, is what we want.
+ base_query = """
+ SELECT Branch.unique_name,
+ Branch.last_scanned_id,
+ SPBDS.name AS distro_series_name,
+ Branch.id,
+ Branch.private,
+ Branch.owner
+ FROM Branch
+ JOIN DistroSeries
+ ON Branch.distroseries = DistroSeries.id
+ LEFT OUTER JOIN SeriesSourcePackageBranch
+ ON Branch.id = SeriesSourcePackageBranch.branch
+ LEFT OUTER JOIN DistroSeries SPBDS
+ -- (SPDBS stands for Source Package Branch Distro Series)
+ ON SeriesSourcePackageBranch.distroseries = SPBDS.id
+ WHERE DistroSeries.distribution = %s
+ """ % sqlvalues(self.id)
if since is not None:
- query += (
- ' AND branch.last_scanned > %s' % sqlvalues(since))
-
- query += ' ORDER BY unique_name, last_scanned_id;'
-
- data = Store.of(self).execute(query)
+ # If "since" was provided, take into account.
+ base_query += (
+ ' AND branch.last_scanned > %s\n' % sqlvalues(since))
+ if person is None:
+ # Now we see just a touch of privacy concerns.
+ # If the current user is anonymous, they cannot see any private
+ # branches.
+ base_query += (' AND NOT Branch.private\n')
+ # We want to order the results, in part for easier grouping at the
+ # end.
+ base_query += 'ORDER BY unique_name, last_scanned_id'
+ if (person is None or
+ person.inTeam(getUtility(ILaunchpadCelebrities).admin)):
+ # Anonymous is already handled above; admins can see everything.
+ # In both cases, we can just use the query as it already stands.
+ query = base_query
+ else:
+ # Otherwise (an authenticated, non-admin user), we need to do some
+ # more sophisticated privacy dances. Note that the one thing we
+ # are ignoring here is stacking. See the discussion in comment 1
+ # of https://bugs.launchpad.net/launchpad/+bug/812335 . Often, we
+ # use unions for this kind of work. The WITH statement can give
+ # us a similar approach with more flexibility. In both cases,
+ # we're essentially declaring that we have a better idea of a good
+ # high-level query plan than Postgres will.
+ query = """
+ WITH principals AS (
+ SELECT team AS id
+ FROM TeamParticipation
+ WHERE TeamParticipation.person = %s
+ UNION
+ SELECT %s
+ ), all_branches AS (
+ %%s
+ ), private_branches AS (
+ SELECT unique_name,
+ last_scanned_id,
+ distro_series_name,
+ id,
+ owner
+ FROM all_branches
+ WHERE private
+ ), owned_branch_ids AS (
+ SELECT private_branches.id
+ FROM private_branches
+ JOIN principals ON private_branches.owner = principals.id
+ ), subscribed_branch_ids AS (
+ SELECT private_branches.id
+ FROM private_branches
+ JOIN BranchSubscription
+ ON BranchSubscription.branch = private_branches.id
+ JOIN principals
+ ON BranchSubscription.person = principals.id
+ )
+ SELECT unique_name, last_scanned_id, distro_series_name
+ FROM all_branches
+ WHERE NOT private OR
+ id IN (SELECT id FROM owned_branch_ids) OR
+ id IN (SELECT id FROM subscribed_branch_ids)
+ """ % sqlvalues(person.id, person.id)
+ query = query % base_query
+
+ data = Store.of(self).execute(query + ';')
result = []
# Group on location (unique_name) and revision (last_scanned_id).
@@ -682,7 +748,7 @@
result.append(list(key))
# Pull out all the official series names and append them as a list
# to the end of the current record, removing Nones from the list.
- result[-1].append(filter(None, map(itemgetter(-1), group)))
+ result[-1].append(filter(None, map(itemgetter(2), group)))
return result
def getMirrorByName(self, name):
=== modified file 'lib/lp/registry/tests/test_distro_webservice.py'
--- lib/lp/registry/tests/test_distro_webservice.py 2011-07-13 20:29:17 +0000
+++ lib/lp/registry/tests/test_distro_webservice.py 2011-07-22 00:53:30 +0000
@@ -8,12 +8,17 @@
import pytz
from launchpadlib.errors import Unauthorized
-from zope.security.management import (
- endInteraction,
- newInteraction,
- )
+from zope.component import getUtility
+from zope.security.management import endInteraction
+from zope.security.proxy import removeSecurityProxy
from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.code.enums import (
+ BranchSubscriptionDiffSize,
+ BranchSubscriptionNotificationLevel,
+ CodeReviewNotificationLevel,
+ )
from lp.code.model.seriessourcepackagebranch import (
SeriesSourcePackageBranchSet,
)
@@ -21,6 +26,7 @@
from lp.testing import (
api_url,
launchpadlib_for,
+ person_logged_in,
TestCaseWithFactory,
)
@@ -51,7 +57,8 @@
series_2 = self.series_2 = self.factory.makeDistroRelease(self.distro)
source_package = self.factory.makeSourcePackage(distroseries=series_1)
branch = self.factory.makeBranch(sourcepackage=source_package)
- unofficial_branch = self.factory.makeBranch(sourcepackage=source_package)
+ unofficial_branch = self.factory.makeBranch(
+ sourcepackage=source_package)
registrant = self.factory.makePerson()
now = datetime.now(pytz.UTC)
sourcepackagename = self.factory.makeSourcePackageName()
@@ -115,3 +122,98 @@
self.assertEqual(tips[0], self.unofficial_branch_name)
self.assertEqual(tips[1], None)
self.assertEqual(tips[2], [])
+
+
+class TestGetBranchTipsSecurity(TestCaseWithFactory):
+ """Test the getBranchTips method and it's exposure to the web service."""
+
+ layer = DatabaseFunctionalLayer
+
+ # Security tests are inspired by TestGenericBranchCollectionVisibleFilter
+ # in lp.code.model.tests.test_branchcollection, and TestAccessBranch in
+ # lp.code.tests.test_branch. Ideally we'd have one code base and one
+ # set of tests to handle them all. We don't. :-/ As a way to try and
+ # partially compensate, we verify here that branch.visibleByUser
+ # agrees with our results.
+
+ # These tests (and the application code that allows them to pass)
+ # consciously ignores the stacked aspect of the branch visibility rules.
+ # See https://bugs.launchpad.net/launchpad/+bug/812335/comments/1 .
+
+ # Similarly, we do not support the LAUNCHPAD_SERVICES user because this
+ # is a special-cased string in the codehosting xmlrpc machinery and
+ # does not correspond to an actual LP Person.
+
+ def makeBranch(self, **kwargs):
+ distro = self.factory.makeDistribution()
+ series = self.factory.makeDistroRelease(distro)
+ source_package = self.factory.makeSourcePackage(distroseries=series)
+ branch = self.factory.makeBranch(
+ sourcepackage=source_package, private=True, **kwargs)
+ return branch, distro
+
+ def test_private_branch_hidden(self):
+ # A private branch should not be included for anonymous users or for
+ # authenticated users who do not have the necessary privileges.
+ branch, distro = self.makeBranch()
+ self.assertFalse( # Double-checking.
+ removeSecurityProxy(branch).visibleByUser(None))
+ self.assertEqual([], distro.getBranchTips())
+ person = self.factory.makePerson()
+ self.assertFalse( # Double-checking.
+ removeSecurityProxy(branch).visibleByUser(person))
+ with person_logged_in(person):
+ self.assertEqual([], distro.getBranchTips())
+
+ def assertVisible(self, distro, branch, person):
+ self.assertTrue( # Double-checking.
+ removeSecurityProxy(branch).visibleByUser(person))
+ with person_logged_in(person):
+ self.assertEqual(1, len(distro.getBranchTips()))
+
+ def test_owned_visible(self):
+ # If user owns the branch, it is visible.
+ person = self.factory.makePerson()
+ branch, distro = self.makeBranch(owner=person)
+ self.assertVisible(distro, branch, person)
+
+ def test_owner_member_visible(self):
+ # If user is a member of the team that owns the branch, it is visible.
+ person = self.factory.makePerson()
+ team = self.factory.makeTeam(members=[person])
+ branch, distro = self.makeBranch(owner=team)
+ self.assertVisible(distro, branch, person)
+
+ def test_subscriber_visible(self):
+ # If user is a subscriber to the branch, it is visible.
+ branch, distro = self.makeBranch()
+ person = self.factory.makePerson()
+ removeSecurityProxy(branch).subscribe(
+ person, BranchSubscriptionNotificationLevel.NOEMAIL,
+ BranchSubscriptionDiffSize.NODIFF,
+ CodeReviewNotificationLevel.NOEMAIL,
+ person)
+ self.assertVisible(distro, branch, person)
+
+ def test_subscriber_member_visible(self):
+ # If user is a member of a team that is a subscriber to the branch,
+ # it is visible.
+ branch, distro = self.makeBranch()
+ person = self.factory.makePerson()
+ team = self.factory.makeTeam(members=[person])
+ removeSecurityProxy(branch).subscribe(
+ team, BranchSubscriptionNotificationLevel.NOEMAIL,
+ BranchSubscriptionDiffSize.NODIFF,
+ CodeReviewNotificationLevel.NOEMAIL,
+ team)
+ self.assertVisible(distro, branch, person)
+
+ def test_admin_visible(self):
+ # All private branches are visible to members of the Launchpad
+ # admin team.
+ person = self.factory.makePerson()
+ admin_team = removeSecurityProxy(
+ getUtility(ILaunchpadCelebrities).admin)
+ admin_team.addMember(person, admin_team.teamowner)
+ branch, distro = self.makeBranch()
+ self.assertVisible(distro, branch, person)
Follow ups
