launchpad-reviewers team mailing list archive

[Colin Watson <mp+414200@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:revision-status-permissions into launchpad:master with ~cjwatson/launchpad:testing-webservice-access-tokens as a prerequisite.

Commit message:
Fix various revision status report/artifact permission bugs

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/414200

If the containing repository is public, revision status reports and artifacts attached to it should be public too.  (Without this, the authorization logic falls back to `ViewByLoggedInUser`, which doesn't allow unauthenticated views.)

`RevisionStatusArtifact` wasn't marked as implementing `IRevisionStatusArtifact`, so its security adapter wasn't used.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:revision-status-permissions into launchpad:master.

diff --git a/lib/lp/code/model/revisionstatus.py b/lib/lp/code/model/revisionstatus.py
index 26eb4f5..7d1f911 100644
--- a/lib/lp/code/model/revisionstatus.py
+++ b/lib/lp/code/model/revisionstatus.py
@@ -23,6 +23,7 @@ from lp.code.enums import (
     RevisionStatusResult,
     )
 from lp.code.interfaces.revisionstatus import (
+    IRevisionStatusArtifact,
     IRevisionStatusArtifactSet,
     IRevisionStatusReport,
     IRevisionStatusReportSet,
@@ -160,6 +161,7 @@ class RevisionStatusReportSet:
         self.findByRepository(repository).remove()
 
 
+@implementer(IRevisionStatusArtifact)
 class RevisionStatusArtifact(StormBase):
     __storm_table__ = 'RevisionStatusArtifact'
 
diff --git a/lib/lp/code/model/tests/test_revisionstatus.py b/lib/lp/code/model/tests/test_revisionstatus.py
index 202c73a..ac30636 100644
--- a/lib/lp/code/model/tests/test_revisionstatus.py
+++ b/lib/lp/code/model/tests/test_revisionstatus.py
@@ -14,21 +14,110 @@ from testtools.matchers import (
     )
 from zope.component import getUtility
 
+from lp.app.enums import InformationType
 from lp.code.enums import (
     RevisionStatusArtifactType,
     RevisionStatusResult,
     )
 from lp.code.interfaces.revisionstatus import IRevisionStatusArtifactSet
 from lp.services.auth.enums import AccessTokenScope
+from lp.services.webapp.authorization import check_permission
 from lp.testing import (
+    anonymous_logged_in,
     api_url,
     person_logged_in,
     TestCaseWithFactory,
     )
-from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.testing.pages import webservice_for_person
 
 
+class TestRevisionStatusReport(TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def makeRevisionStatusArtifact(self, report):
+        # We don't need to upload files to the librarian in this test suite.
+        lfa = self.factory.makeLibraryFileAlias(db_only=True)
+        return self.factory.makeRevisionStatusArtifact(lfa=lfa, report=report)
+
+    def test_owner_public(self):
+        # The owner of a public repository can view and edit its reports and
+        # artifacts.
+        report = self.factory.makeRevisionStatusReport()
+        artifact = self.makeRevisionStatusArtifact(report=report)
+        with person_logged_in(report.git_repository.owner):
+            self.assertTrue(check_permission("launchpad.View", report))
+            self.assertTrue(check_permission("launchpad.View", artifact))
+            self.assertTrue(check_permission("launchpad.Edit", report))
+            self.assertTrue(check_permission("launchpad.Edit", artifact))
+
+    def test_owner_private(self):
+        # The owner of a private repository can view and edit its reports
+        # and artifacts.
+        with person_logged_in(self.factory.makePerson()) as owner:
+            report = self.factory.makeRevisionStatusReport(
+                git_repository=self.factory.makeGitRepository(
+                    owner=owner, information_type=InformationType.USERDATA))
+            artifact = self.makeRevisionStatusArtifact(report=report)
+            self.assertTrue(check_permission("launchpad.View", report))
+            self.assertTrue(check_permission("launchpad.View", artifact))
+            self.assertTrue(check_permission("launchpad.Edit", report))
+            self.assertTrue(check_permission("launchpad.Edit", artifact))
+
+    def test_random_public(self):
+        # An unrelated user can view but not edit reports and artifacts in
+        # public repositories.
+        report = self.factory.makeRevisionStatusReport()
+        artifact = self.makeRevisionStatusArtifact(report=report)
+        with person_logged_in(self.factory.makePerson()):
+            self.assertTrue(check_permission("launchpad.View", report))
+            self.assertTrue(check_permission("launchpad.View", artifact))
+            self.assertFalse(check_permission("launchpad.Edit", report))
+            self.assertFalse(check_permission("launchpad.Edit", artifact))
+
+    def test_random_private(self):
+        # An unrelated user can neither view nor edit reports and artifacts
+        # in private repositories.
+        with person_logged_in(self.factory.makePerson()) as owner:
+            report = self.factory.makeRevisionStatusReport(
+                git_repository=self.factory.makeGitRepository(
+                    owner=owner, information_type=InformationType.USERDATA))
+            artifact = self.makeRevisionStatusArtifact(report=report)
+        with person_logged_in(self.factory.makePerson()):
+            self.assertFalse(check_permission("launchpad.View", report))
+            self.assertFalse(check_permission("launchpad.View", artifact))
+            self.assertFalse(check_permission("launchpad.Edit", report))
+            self.assertFalse(check_permission("launchpad.Edit", artifact))
+
+    def test_anonymous_public(self):
+        # Anonymous users can view but not edit reports and artifacts in
+        # public repositories.
+        report = self.factory.makeRevisionStatusReport()
+        artifact = self.makeRevisionStatusArtifact(report=report)
+        with anonymous_logged_in():
+            self.assertTrue(check_permission("launchpad.View", report))
+            self.assertTrue(check_permission("launchpad.View", artifact))
+            self.assertFalse(check_permission("launchpad.Edit", report))
+            self.assertFalse(check_permission("launchpad.Edit", artifact))
+
+    def test_anonymous_private(self):
+        # Anonymous users can neither view nor edit reports and artifacts in
+        # private repositories.
+        with person_logged_in(self.factory.makePerson()) as owner:
+            report = self.factory.makeRevisionStatusReport(
+                git_repository=self.factory.makeGitRepository(
+                    owner=owner, information_type=InformationType.USERDATA))
+            artifact = self.makeRevisionStatusArtifact(report=report)
+        with anonymous_logged_in():
+            self.assertFalse(check_permission("launchpad.View", report))
+            self.assertFalse(check_permission("launchpad.View", artifact))
+            self.assertFalse(check_permission("launchpad.Edit", report))
+            self.assertFalse(check_permission("launchpad.Edit", artifact))
+
+
 class TestRevisionStatusReportWebservice(TestCaseWithFactory):
     layer = LaunchpadFunctionalLayer
 
diff --git a/lib/lp/security.py b/lib/lp/security.py
index 5e74fd5..044e197 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -687,6 +687,15 @@ class EditSpecificationByRelatedPeople(AuthorizationBase):
                     self.obj, ['owner', 'drafter', 'assignee', 'approver']))
 
 
+class ViewRevisionStatusReport(DelegatedAuthorization):
+    """Anyone who can see a Git repository can see its status reports."""
+    permission = 'launchpad.View'
+    usedfor = IRevisionStatusReport
+
+    def __init__(self, obj):
+        super().__init__(obj, obj.git_repository, 'launchpad.View')
+
+
 class EditRevisionStatusReport(AuthorizationBase):
     """The owner of a Git repository can edit its status reports."""
     permission = 'launchpad.Edit'
@@ -696,6 +705,14 @@ class EditRevisionStatusReport(AuthorizationBase):
         return user.isOwner(self.obj.git_repository)
 
 
+class ViewRevisionStatusArtifact(DelegatedAuthorization):
+    permission = 'launchpad.View'
+    usedfor = IRevisionStatusArtifact
+
+    def __init__(self, obj):
+        super().__init__(obj, obj.report, 'launchpad.View')
+
+
 class EditRevisionStatusArtifact(DelegatedAuthorization):
     permission = 'launchpad.Edit'
     usedfor = IRevisionStatusArtifact