launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/bug-800123 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-800123/+merge/69264

= Bug 800123 =

We want to allow project (internally "product") maintainers and drivers control of their translation import queue.  Along the way, we sanitize how checks are done (move all of the permissions code to security.py where it needs to be, and then use permissions checking as appropriate).

There is already a comprehensive test for canSetStatus that we modify to take new permissions into account.

== Tests ==

bin/test -cvvt canSetStatus

== Demo and Q/A ==

https://translations.launchpad.dev/+imports
and
https://translations.launchpad.dev/firefox/+imports

(logged in as different users: driver, owner, no-priv etc)

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/lp/translations/model/translationimportqueue.py
  lib/lp/translations/tests/test_translationimportqueue.py
-- 
https://code.launchpad.net/~danilo/launchpad/bug-800123/+merge/69264
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-800123 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-07-13 10:35:30 +0000
+++ lib/canonical/launchpad/security.py	2011-07-26 12:07:32 +0000
@@ -600,6 +600,7 @@
         able to change translation settings for a product.
         """
         return (user.isOwner(self.obj) or
+                user.isOneOfDrivers(self.obj) or
                 user.in_rosetta_experts or
                 user.in_admin)
 
@@ -1415,7 +1416,13 @@
     usedfor = ITranslationImportQueueEntry
 
     def checkAuthenticated(self, user):
-        return self.obj.canAdmin(user)
+        if self.obj.distroseries is not None:
+            series = self.obj.distroseries
+        else:
+            series = self.obj.productseries
+        return (
+            self.forwardCheckAuthenticated(user, series,
+                                           'launchpad.TranslationsAdmin'))
 
 
 class EditTranslationImportQueueEntry(AuthorizationBase):
@@ -1426,7 +1433,9 @@
         """Anyone who can admin an entry, plus its owner or the owner of the
         product or distribution, can edit it.
         """
-        return self.obj.canEdit(user)
+        return (self.forwardCheckAuthenticated(
+                    user, self.obj, 'launchpad.Admin') or
+                user.inTeam(self.obj.importer))
 
 
 class AdminTranslationImportQueue(OnlyRosettaExpertsAndAdmins):
@@ -1902,8 +1911,8 @@
         Distribution managers can also manage IDistroSeries
         """
 
-        return (AdminDistributionTranslations(
-            self.obj.distribution).checkAuthenticated(user))
+        return (user.isOneOfDrivers(self.obj) or
+                self.forwardCheckAuthenticated(user, self.obj.distribution))
 
 
 class AdminDistributionSourcePackageTranslations(
@@ -1919,7 +1928,9 @@
     def checkAuthenticated(self, user):
         """Is the user able to manage `IProductSeries` translations."""
 
-        return OnlyRosettaExpertsAndAdmins(self.obj).checkAuthenticated(user)
+        return (user.isOwner(self.obj) or
+                user.isOneOfDrivers(self.obj) or
+                self.forwardCheckAuthenticated(user, self.obj.product))
 
 
 class BranchMergeProposalView(AuthorizationBase):

=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2011-05-27 19:53:20 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2011-07-26 12:07:32 +0000
@@ -34,7 +34,10 @@
     Int,
     Reference,
     )
-from zope.component import getUtility
+from zope.component import (
+    getUtility,
+    queryAdapter,
+    )
 from zope.interface import implements
 
 from canonical.database.constants import (
@@ -58,6 +61,7 @@
 from canonical.librarian.interfaces import ILibrarianClient
 from lp.app.errors import NotFoundError
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.app.interfaces.security import IAuthorization
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.person import (
@@ -296,35 +300,19 @@
 
     def canAdmin(self, roles):
         """See `ITranslationImportQueueEntry`."""
-        # As a special case, the Ubuntu translation group owners can
-        # manage Ubuntu uploads.
-        if self.is_targeted_to_ubuntu:
-            group = self.distroseries.distribution.translationgroup
-            if group is not None and roles.inTeam(group.owner):
-                return True
-        # Rosetta experts and admins can administer the entry.
-        return roles.in_rosetta_experts or roles.in_admin
-
-    def _canEditExcludeImporter(self, roles):
-        """All people that can edit the entry except the importer."""
-        # Admin rights include edit rights.
-        if self.canAdmin(roles):
-            return True
-        # The maintainer and the drivers can edit the entry.
-        if self.productseries is not None:
-            return (roles.isOwner(self.productseries.product) or
-                    roles.isOneOfDrivers(self.productseries))
-        if self.distroseries is not None:
-            return (roles.isOwner(self.distroseries.distribution) or
-                    roles.isOneOfDrivers(self.distroseries))
-        return False
+        next_adapter = queryAdapter(self, IAuthorization, 'launchpad.Admin')
+        if next_adapter is None:
+            return False
+        else:
+            return next_adapter.checkAuthenticated(roles)
 
     def canEdit(self, roles):
         """See `ITranslationImportQueueEntry`."""
-        # The importer can edit the entry.
-        if roles.inTeam(self.importer):
-            return True
-        return self._canEditExcludeImporter(roles)
+        next_adapter = queryAdapter(self, IAuthorization, 'launchpad.Edit')
+        if next_adapter is None:
+            return False
+        else:
+            return next_adapter.checkAuthenticated(roles)
 
     def canSetStatus(self, new_status, user):
         """See `ITranslationImportQueueEntry`."""
@@ -352,7 +340,7 @@
             return roles.in_admin or roles.in_rosetta_experts
         if new_status == RosettaImportStatus.BLOCKED:
             # Importers are not allowed to set BLOCKED
-            return self._canEditExcludeImporter(roles)
+            return self.canAdmin(roles)
         # All other statuses can be set set by all authorized persons.
         return self.canEdit(roles)
 

=== modified file 'lib/lp/translations/tests/test_translationimportqueue.py'
--- lib/lp/translations/tests/test_translationimportqueue.py	2011-05-27 21:12:25 +0000
+++ lib/lp/translations/tests/test_translationimportqueue.py	2011-07-26 12:07:32 +0000
@@ -101,43 +101,43 @@
         # The owner (maintainer) of the product gets to set Blocked as well.
         owner = self.productseries.product.owner
         self._assertCanSetStatus(owner, self.entry,
-            #  A      B     D     F      I     NI     NR
-            [False, True, True, False, False, False, True])
+            #  A     B     D     F      I     NI    NR
+            [True, True, True, False, False, True, True])
 
     def test_canSetStatus_owner_and_uploader(self):
         # Corner case: Nothing changes if the maintainer is also the uploader.
         self.productseries.product.owner = self.uploaderperson
         self._assertCanSetStatus(self.uploaderperson, self.entry,
-            #  A      B     D     F      I     NI     NR
-            [False, True, True, False, False, False, True])
+            #  A     B     D     F      I     NI    NR
+            [True, True, True, False, False, True, True])
 
     def test_canSetStatus_driver(self):
         # The driver gets the same permissions as the maintainer.
         driver = self.productseries.driver
         self._assertCanSetStatus(driver, self.entry,
-            #  A      B     D     F      I     NI     NR
-            [False, True, True, False, False, False, True])
+            #  A     B     D     F      I     NI    NR
+            [True, True, True, False, False, True, True])
 
     def test_canSetStatus_driver_and_uploader(self):
         # Corner case: Nothing changes if the driver is also the uploader.
         self.productseries.driver = self.uploaderperson
         self._assertCanSetStatus(self.uploaderperson, self.entry,
-            #  A      B     D     F      I     NI     NR
-            [False, True, True, False, False, False, True])
+            #  A     B     D     F      I     NI    NR
+            [True, True, True, False, False, True, True])
 
     def test_canSetStatus_product_driver(self):
         # The driver of the product, too.
         driver = self.productseries.product.driver
         self._assertCanSetStatus(driver, self.entry,
-            #  A      B     D     F      I     NI     NR
-            [False, True, True, False, False, False, True])
+            #  A      B     D     F     I     NI    NR
+            [True, True, True, False, False, True, True])
 
     def test_canSetStatus_product_driver_and_uploader(self):
         # Corner case: Nothing changes if the driver is also the uploader.
         self.productseries.product.driver = self.uploaderperson
         self._assertCanSetStatus(self.uploaderperson, self.entry,
-            #  A      B     D     F      I     NI     NR
-            [False, True, True, False, False, False, True])
+            #  A      B     D     F     I     NI    NR
+            [True, True, True, False, False, True, True])
 
     def _setUpUbuntu(self):
         self.ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
@@ -345,7 +345,7 @@
         # queue, the entry importer is not updated because that would
         # cause an non-unique key for the entry.
         with person_logged_in(self.new_owner):
-            new_entry = self.import_queue.addOrUpdateEntry(
+            self.import_queue.addOrUpdateEntry(
                 u'po/sr.po', 'foo', True, self.new_owner,
                 productseries=self.product.series[0])
         with person_logged_in(self.old_owner):