Merge lp:~adeuring/launchpad/bug-39674-lfa-editable into lp:launchpad/db-devel

[Abel Deuring]

This branch makes LibraryFileAlias records editable.

Up to now, all attributes of LibraryFileAlias were read-only. This is reasonable, because we LFAs are used for a number of different purposes, from icons for projects and users to PPAs and bug attachments. So there is nothing like a common "permission conecpt" for LFAs.

On the other hand, in bug 39674 Francis suggests that we should make the property "restricted" of LFAs changable. (This reduces the load for our Zope servers, because the content of bug attachments of public bugs will still be served via the public Librarian, while we can serve the attachment content of private bugs via the restircted Librarian.

The restricted Librarian already "copied" the permission launchpad.View of a parent object of a Librarian file, so it was straightforward too extend this concept to edit permissions for LFA records.

This branch adds an adapter for LFAs that allows to edit a Librarian file if a parent is known; users who can edit the parent's properties can also change the LFA record. (At present, the only settable attribute is "restricted", but it would be simple and useful to make other attributes also editable so that it for example becomes easier to change the content type of a bug attachment -- at present, we create a new LFA record with the same content but a changed content type field...)

test: ./bin/test -vvt test_libraryfilealias_with_parent

no lint

[Gary Poster (gary)]

Looks good, thank you!

What do you think of this instead of the try/except blocks on the test?

    self.assertRaises(setattr(self.bug_attachment.title, 'whatever'), Unauthorized)
    self.assertRaises(self.lfa_with_parent.restricted, True), Unauthorized)

It's worth noting some of the other ideas we discussed and rejected in the course of the branch.

- Set a __parent__ on the library object in the database. The problems are that library objects may have multiple parents in theory; and, more importantly, they may have multiple *types* of parents, and making a reference to an arbitrary object is not trivial in Launchpad at the moment given our table structures. This would be my preferred solution in the long run.

- Set a __parent__ on the object during traversal. The mechanism implemented here could be used for that, but would be more heavyweight than would be needed--you could just slam a transient __parent__ on the file alias instance. Problems: we didn't care for this pattern because it had hidden fragility. The current approach clearly exposes the situation for what it is. This alternative is the one that I wonder if it would have been better, but I'm happy with the call that we made.

- Make a method on the file alias to change the restricted attribute that takes a security context and then handles security checks itself. I wanted to keep security handled by the usual machinery, and I wanted to keep security concerns out of the model code.

- Use another sort of mediator, like a function. I wanted to keep security handled by the usual machinery. Using the approach we have here keeps us in the usual patterns.

review: Approve

[Robert Collins (lifeless)]

A few notes - this is a great branch.

The spike I'm doing on restricted librarian things will likely
eventually benefit from the adapter.

It would be great to do a quick check that we don't share LFA's
anywhere; there isn't any need to share them because they themselves
are the sharing mechanism for content in the librarian.

-Rob

[Abel Deuring (adeuring)]

On 28.07.2010 02:01, Robert Collins wrote:
> A few notes - this is a great branch.
>
> The spike I'm doing on restricted librarian things will likely
> eventually benefit from the adapter.
>
> It would be great to do a quick check that we don't share LFA's
> anywhere; there isn't any need to share them because they themselves
> are the sharing mechanism for content in the librarian.

I already did a quick check and I think that LFAs referenced by bug
attachments are not referenced elsewhere. But:

1. There is an obvious difference betwen "I think" and "I am absolutely
sure"
2. Even if we were absolutely sure that we don't create such references
today, some code that creates a second reference to a bug attachment's
LFA this could in the future accidentally slip into our code base.

It would be nice if we could enforce this by a constraint on all columns
which reference LFAs, but according to
file:///usr/share/doc/postgresql-doc-8.4/html/sql-createtable.html :

> Currently, CHECK expressions cannot contain subqueries nor refer to variables other than columns of the current row.

We could perhaps add triggers for INSERTs and UPDATEs of all tables that
reference LFA which ensures that something like

  SELECT lfa_reference FROM table1 WHERE lfa_reference=this_reference
  UNION ALL
    SELECT lfa_reference FROM table2 WHERE lfa_reference=this_reference
  UNION ALL...

returns exactly one row.

=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2010-07-21 08:15:57 +0000
+++ configs/development/launchpad-lazr.conf	2010-07-26 14:46:16 +0000
@@ -161,6 +161,8 @@
 restricted_download_port: 58085
 restricted_download_url: http://launchpad.dev:58085/
 use_https = False
+oops_prefix: L
+error_dir: /var/tmp/codehosting.test
 
 [librarian_server]
 root: /var/tmp/fatsam

=== modified file 'daemons/librarian.tac'
--- daemons/librarian.tac	2010-04-20 14:27:26 +0000
+++ daemons/librarian.tac	2010-07-26 14:46:16 +0000
@@ -19,6 +19,7 @@
 from canonical.librarian.libraryprotocol import FileUploadFactory
 from canonical.librarian import storage, db
 from canonical.librarian import web as fatweb
+from lp.services.twistedsupport.loggingsupport import set_up_oops_reporting
 
 # Connect to database
 dbconfig.setConfigSection('librarian')
@@ -70,6 +71,9 @@
 uploadPort = config.librarian.restricted_upload_port
 setUpListener(uploadPort, webPort, restricted=True)
 
+# Log OOPS reports
+set_up_oops_reporting('librarian', 'librarian')
+
 # Setup a signal handler to dump the process' memory upon 'kill -44'.
 def sigdumpmem_handler(signum, frame):
     scanner.dump_all_objects(DUMP_FILE)

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2010-07-24 00:07:54 +0000
+++ lib/canonical/config/schema-lazr.conf	2010-07-26 14:46:16 +0000
@@ -1225,6 +1225,13 @@
 
 use_https = True
 
+# See [error_reports].
+# Must be unique per librarian instance.
+oops_prefix: none
+# Must be set per librarian instance.
+error_dir: none
+# Must be false: librarian has no zlog.
+copy_to_zlog: False
 
 [librarian_gc]
 # The database user which will be used by this process.

=== modified file 'lib/canonical/launchpad/database/librarian.py'
--- lib/canonical/launchpad/database/librarian.py	2009-11-24 15:36:44 +0000
+++ lib/canonical/launchpad/database/librarian.py	2010-07-26 14:46:16 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'LibraryFileAlias',
+    'LibraryFileAliasWithParent',
     'LibraryFileAliasSet',
     'LibraryFileContent',
     'LibraryFileDownloadCount',
@@ -14,22 +15,23 @@
 from datetime import datetime, timedelta
 import pytz
 
-from zope.component import getUtility
-from zope.interface import implements
+from zope.component import adapts, getUtility
+from zope.interface import implements, Interface
 
 from sqlobject import StringCol, ForeignKey, IntCol, SQLRelatedJoin, BoolCol
 from storm.locals import Date, Desc, Int, Reference, Store
 
 from canonical.config import config
 from canonical.launchpad.interfaces import (
-    ILibraryFileAlias, ILibraryFileAliasSet, ILibraryFileContent,
-    ILibraryFileDownloadCount, IMasterStore)
+    ILibraryFileAlias, ILibraryFileAliasWithParent, ILibraryFileAliasSet,
+    ILibraryFileContent, ILibraryFileDownloadCount, IMasterStore)
 from canonical.librarian.interfaces import (
     DownloadFailed, ILibrarianClient, IRestrictedLibrarianClient,
     LIBRARIAN_SERVER_DEFAULT_TIMEOUT)
 from canonical.database.sqlbase import SQLBase
 from canonical.database.constants import UTC_NOW, DEFAULT
 from canonical.database.datetimecol import UtcDateTimeCol
+from lazr.delegates import delegates
 
 
 class LibraryFileContent(SQLBase):
@@ -195,6 +197,18 @@
         self.close()
 
 
+class LibraryFileAliasWithParent:
+    """A LibraryFileAlias variant that has a parent."""
+
+    adapts(ILibraryFileAlias, Interface)
+    implements(ILibraryFileAliasWithParent)
+    delegates(ILibraryFileAlias)
+
+    def __init__(self, libraryfile, parent):
+        self.context = libraryfile
+        self.__parent__ = parent
+
+
 class LibraryFileAliasSet(object):
     """Create and find LibraryFileAliases."""
 

=== modified file 'lib/canonical/launchpad/interfaces/librarian.py'
--- lib/canonical/launchpad/interfaces/librarian.py	2010-03-01 03:06:02 +0000
+++ lib/canonical/launchpad/interfaces/librarian.py	2010-07-26 14:46:16 +0000
@@ -9,6 +9,7 @@
 
 __all__ = [
     'ILibraryFileAlias',
+    'ILibraryFileAliasWithParent',
     'ILibraryFileAliasSet',
     'ILibraryFileContent',
     'ILibraryFileDownloadCount',
@@ -116,6 +117,10 @@
         """
 
 
+class ILibraryFileAliasWithParent(ILibraryFileAlias):
+    """A ILibraryFileAlias that knows about its parent."""
+
+
 class ILibraryFileContent(Interface):
     """Actual data in the Librarian.
 

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-07-21 09:35:19 +0000
+++ lib/canonical/launchpad/security.py	2010-07-26 14:46:16 +0000
@@ -13,6 +13,8 @@
 
 from canonical.launchpad.interfaces.account import IAccount
 from canonical.launchpad.interfaces.emailaddress import IEmailAddress
+from canonical.launchpad.interfaces.librarian import (
+    ILibraryFileAliasWithParent)
 from lp.registry.interfaces.announcement import IAnnouncement
 from lp.soyuz.interfaces.archive import IArchive
 from lp.soyuz.interfaces.archivepermission import (
@@ -2481,3 +2483,23 @@
     def checkAuthenticated(self, user):
         """Users must be an admin or a member of the tech board."""
         return user.in_admin or user.in_ubuntu_techboard
+
+
+class EditLibraryFileAliasWithParent(AuthorizationBase):
+    permission = 'launchpad.Edit'
+    usedfor = ILibraryFileAliasWithParent
+
+    def checkAuthenticated(self, user):
+        """Only persons which can edit an LFA's parent can edit an LFA.
+
+        By default, a LibraryFileAlias does not know about its parent.
+        Such aliases are never editable. Use an adapter to provide a
+        parent object.
+
+        If a parent is known, users which can edit the parent can also
+        edit properties of the LibraryFileAlias.
+        """
+        parent = getattr(self.obj, '__parent__', None)
+        if parent is None:
+            return False
+        return check_permission(self.permission, parent)

=== added file 'lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py'
--- lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py	2010-07-26 14:46:16 +0000
@@ -0,0 +1,53 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+__all__ = []
+
+from zope.component import getMultiAdapter
+from zope.security.interfaces import Unauthorized
+
+from canonical.launchpad.interfaces.librarian import (
+    ILibraryFileAliasWithParent)
+from canonical.testing import LaunchpadFunctionalLayer
+from lp.testing import login_person, TestCaseWithFactory
+
+class TestLibraryFileAliasForBugAttachment(TestCaseWithFactory):
+    """Tests for ILibraryFileAliasWithParent."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestLibraryFileAliasForBugAttachment, self).setUp()
+        self.bug_owner = self.factory.makePerson()
+        login_person(self.bug_owner)
+        self.bug = self.factory.makeBug(owner=self.bug_owner, private=True)
+        self.bug_attachment = self.factory.makeBugAttachment(bug=self.bug)
+        self.lfa_with_parent = getMultiAdapter(
+            (self.bug_attachment.libraryfile, self.bug_attachment),
+            ILibraryFileAliasWithParent)
+
+    def test_setRestricted_authorized_user(self):
+        # A LibraryFileAlias instance can be adapted to an editable
+        # variant, by adapting it, together with a parent object to
+        # ILibraryFileAliasWithParent.
+        #
+        # People who can edit the parent object can also edit
+        # LibraryFilasAlias instance.
+        login_person(self.bug_owner)
+        self.assertFalse(self.lfa_with_parent.restricted)
+        self.bug_attachment.title = 'foo'
+        self.lfa_with_parent.restricted = True
+        self.assertTrue(self.lfa_with_parent.restricted)
+
+    def test_setRestricted_unauthorized_user(self):
+        # If a user cannot change properties of a bug attachment...
+        other_person = self.factory.makePerson()
+        login_person(other_person)
+        self.assertRaises(
+            Unauthorized, setattr, self.bug_attachment, 'title', 'whatever')
+        # ...he also can't change the LibraryFileAlias for this bug.
+        self.assertRaises(
+            Unauthorized, setattr, self.lfa_with_parent, 'restricted', True)
+

=== modified file 'lib/canonical/launchpad/zcml/librarian.zcml'
--- lib/canonical/launchpad/zcml/librarian.zcml	2010-02-16 18:14:26 +0000
+++ lib/canonical/launchpad/zcml/librarian.zcml	2010-07-26 14:46:16 +0000
@@ -12,6 +12,13 @@
     <allow interface="canonical.launchpad.interfaces.ILibraryFileAlias" />
   </class>
 
+  <class class="canonical.launchpad.database.LibraryFileAliasWithParent">
+    <allow interface="canonical.launchpad.interfaces.ILibraryFileAliasWithParent" />
+    <require
+      permission="launchpad.Edit"
+      set_attributes="restricted" />
+  </class>
+
   <class class="canonical.launchpad.database.LibraryFileContent">
     <allow interface="canonical.launchpad.interfaces.ILibraryFileContent" />
   </class>
@@ -48,4 +55,8 @@
     <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher" />
   </class>
 
+  <adapter
+    factory="canonical.launchpad.database.librarian.LibraryFileAliasWithParent"
+    provides="canonical.launchpad.interfaces.librarian.ILibraryFileAliasWithParent"
+    trusted="true" />
 </configure>