Launchpad itself

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

  1. bug-39674-lfa-editable
  2. Merge into db-devel
Proposed by Abel Deuring
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: 9578
Proposed branch: lp:~adeuring/launchpad/bug-39674-lfa-editable
Merge into: lp:launchpad/db-devel
Diff against target: 256 lines (+122/-4)
8 files modified
configs/development/launchpad-lazr.conf (+2/-0)
daemons/librarian.tac (+4/-0)
lib/canonical/config/schema-lazr.conf (+7/-0)
lib/canonical/launchpad/database/librarian.py (+18/-4)
lib/canonical/launchpad/interfaces/librarian.py (+5/-0)
lib/canonical/launchpad/security.py (+22/-0)
lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py (+53/-0)
lib/canonical/launchpad/zcml/librarian.zcml (+11/-0)
To merge this branch: bzr merge lp:~adeuring/launchpad/bug-39674-lfa-editable
Reviewer Review Type Date Requested Status
Gary Poster (community) Approve
Review via email: mp+29314@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :

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
Revision history for this message
Robert Collins (lifeless) 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.

-Rob

Revision history for this message
Abel Deuring (adeuring) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
161restricted_download_port: 58085161restricted_download_port: 58085
162restricted_download_url: http://launchpad.dev:58085/162restricted_download_url: http://launchpad.dev:58085/
163use_https = False163use_https = False
164oops_prefix: L
165error_dir: /var/tmp/codehosting.test
164166
165[librarian_server]167[librarian_server]
166root: /var/tmp/fatsam168root: /var/tmp/fatsam
167169
=== 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 @@
19from canonical.librarian.libraryprotocol import FileUploadFactory19from canonical.librarian.libraryprotocol import FileUploadFactory
20from canonical.librarian import storage, db20from canonical.librarian import storage, db
21from canonical.librarian import web as fatweb21from canonical.librarian import web as fatweb
22from lp.services.twistedsupport.loggingsupport import set_up_oops_reporting
2223
23# Connect to database24# Connect to database
24dbconfig.setConfigSection('librarian')25dbconfig.setConfigSection('librarian')
@@ -70,6 +71,9 @@
70uploadPort = config.librarian.restricted_upload_port71uploadPort = config.librarian.restricted_upload_port
71setUpListener(uploadPort, webPort, restricted=True)72setUpListener(uploadPort, webPort, restricted=True)
7273
74# Log OOPS reports
75set_up_oops_reporting('librarian', 'librarian')
76
73# Setup a signal handler to dump the process' memory upon 'kill -44'.77# Setup a signal handler to dump the process' memory upon 'kill -44'.
74def sigdumpmem_handler(signum, frame):78def sigdumpmem_handler(signum, frame):
75 scanner.dump_all_objects(DUMP_FILE)79 scanner.dump_all_objects(DUMP_FILE)
7680
=== 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 @@
12251225
1226use_https = True1226use_https = True
12271227
1228# See [error_reports].
1229# Must be unique per librarian instance.
1230oops_prefix: none
1231# Must be set per librarian instance.
1232error_dir: none
1233# Must be false: librarian has no zlog.
1234copy_to_zlog: False
12281235
1229[librarian_gc]1236[librarian_gc]
1230# The database user which will be used by this process.1237# The database user which will be used by this process.
12311238
=== 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 @@
6__metaclass__ = type6__metaclass__ = type
7__all__ = [7__all__ = [
8 'LibraryFileAlias',8 'LibraryFileAlias',
9 'LibraryFileAliasWithParent',
9 'LibraryFileAliasSet',10 'LibraryFileAliasSet',
10 'LibraryFileContent',11 'LibraryFileContent',
11 'LibraryFileDownloadCount',12 'LibraryFileDownloadCount',
@@ -14,22 +15,23 @@
14from datetime import datetime, timedelta15from datetime import datetime, timedelta
15import pytz16import pytz
1617
17from zope.component import getUtility18from zope.component import adapts, getUtility
18from zope.interface import implements19from zope.interface import implements, Interface
1920
20from sqlobject import StringCol, ForeignKey, IntCol, SQLRelatedJoin, BoolCol21from sqlobject import StringCol, ForeignKey, IntCol, SQLRelatedJoin, BoolCol
21from storm.locals import Date, Desc, Int, Reference, Store22from storm.locals import Date, Desc, Int, Reference, Store
2223
23from canonical.config import config24from canonical.config import config
24from canonical.launchpad.interfaces import (25from canonical.launchpad.interfaces import (
25 ILibraryFileAlias, ILibraryFileAliasSet, ILibraryFileContent,26 ILibraryFileAlias, ILibraryFileAliasWithParent, ILibraryFileAliasSet,
26 ILibraryFileDownloadCount, IMasterStore)27 ILibraryFileContent, ILibraryFileDownloadCount, IMasterStore)
27from canonical.librarian.interfaces import (28from canonical.librarian.interfaces import (
28 DownloadFailed, ILibrarianClient, IRestrictedLibrarianClient,29 DownloadFailed, ILibrarianClient, IRestrictedLibrarianClient,
29 LIBRARIAN_SERVER_DEFAULT_TIMEOUT)30 LIBRARIAN_SERVER_DEFAULT_TIMEOUT)
30from canonical.database.sqlbase import SQLBase31from canonical.database.sqlbase import SQLBase
31from canonical.database.constants import UTC_NOW, DEFAULT32from canonical.database.constants import UTC_NOW, DEFAULT
32from canonical.database.datetimecol import UtcDateTimeCol33from canonical.database.datetimecol import UtcDateTimeCol
34from lazr.delegates import delegates
3335
3436
35class LibraryFileContent(SQLBase):37class LibraryFileContent(SQLBase):
@@ -195,6 +197,18 @@
195 self.close()197 self.close()
196198
197199
200class LibraryFileAliasWithParent:
201 """A LibraryFileAlias variant that has a parent."""
202
203 adapts(ILibraryFileAlias, Interface)
204 implements(ILibraryFileAliasWithParent)
205 delegates(ILibraryFileAlias)
206
207 def __init__(self, libraryfile, parent):
208 self.context = libraryfile
209 self.__parent__ = parent
210
211
198class LibraryFileAliasSet(object):212class LibraryFileAliasSet(object):
199 """Create and find LibraryFileAliases."""213 """Create and find LibraryFileAliases."""
200214
201215
=== 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 @@
99
10__all__ = [10__all__ = [
11 'ILibraryFileAlias',11 'ILibraryFileAlias',
12 'ILibraryFileAliasWithParent',
12 'ILibraryFileAliasSet',13 'ILibraryFileAliasSet',
13 'ILibraryFileContent',14 'ILibraryFileContent',
14 'ILibraryFileDownloadCount',15 'ILibraryFileDownloadCount',
@@ -116,6 +117,10 @@
116 """117 """
117118
118119
120class ILibraryFileAliasWithParent(ILibraryFileAlias):
121 """A ILibraryFileAlias that knows about its parent."""
122
123
119class ILibraryFileContent(Interface):124class ILibraryFileContent(Interface):
120 """Actual data in the Librarian.125 """Actual data in the Librarian.
121126
122127
=== 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 @@
1313
14from canonical.launchpad.interfaces.account import IAccount14from canonical.launchpad.interfaces.account import IAccount
15from canonical.launchpad.interfaces.emailaddress import IEmailAddress15from canonical.launchpad.interfaces.emailaddress import IEmailAddress
16from canonical.launchpad.interfaces.librarian import (
17 ILibraryFileAliasWithParent)
16from lp.registry.interfaces.announcement import IAnnouncement18from lp.registry.interfaces.announcement import IAnnouncement
17from lp.soyuz.interfaces.archive import IArchive19from lp.soyuz.interfaces.archive import IArchive
18from lp.soyuz.interfaces.archivepermission import (20from lp.soyuz.interfaces.archivepermission import (
@@ -2481,3 +2483,23 @@
2481 def checkAuthenticated(self, user):2483 def checkAuthenticated(self, user):
2482 """Users must be an admin or a member of the tech board."""2484 """Users must be an admin or a member of the tech board."""
2483 return user.in_admin or user.in_ubuntu_techboard2485 return user.in_admin or user.in_ubuntu_techboard
2486
2487
2488class EditLibraryFileAliasWithParent(AuthorizationBase):
2489 permission = 'launchpad.Edit'
2490 usedfor = ILibraryFileAliasWithParent
2491
2492 def checkAuthenticated(self, user):
2493 """Only persons which can edit an LFA's parent can edit an LFA.
2494
2495 By default, a LibraryFileAlias does not know about its parent.
2496 Such aliases are never editable. Use an adapter to provide a
2497 parent object.
2498
2499 If a parent is known, users which can edit the parent can also
2500 edit properties of the LibraryFileAlias.
2501 """
2502 parent = getattr(self.obj, '__parent__', None)
2503 if parent is None:
2504 return False
2505 return check_permission(self.permission, parent)
24842506
=== 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 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4__metaclass__ = type
5
6__all__ = []
7
8from zope.component import getMultiAdapter
9from zope.security.interfaces import Unauthorized
10
11from canonical.launchpad.interfaces.librarian import (
12 ILibraryFileAliasWithParent)
13from canonical.testing import LaunchpadFunctionalLayer
14from lp.testing import login_person, TestCaseWithFactory
15
16class TestLibraryFileAliasForBugAttachment(TestCaseWithFactory):
17 """Tests for ILibraryFileAliasWithParent."""
18
19 layer = LaunchpadFunctionalLayer
20
21 def setUp(self):
22 super(TestLibraryFileAliasForBugAttachment, self).setUp()
23 self.bug_owner = self.factory.makePerson()
24 login_person(self.bug_owner)
25 self.bug = self.factory.makeBug(owner=self.bug_owner, private=True)
26 self.bug_attachment = self.factory.makeBugAttachment(bug=self.bug)
27 self.lfa_with_parent = getMultiAdapter(
28 (self.bug_attachment.libraryfile, self.bug_attachment),
29 ILibraryFileAliasWithParent)
30
31 def test_setRestricted_authorized_user(self):
32 # A LibraryFileAlias instance can be adapted to an editable
33 # variant, by adapting it, together with a parent object to
34 # ILibraryFileAliasWithParent.
35 #
36 # People who can edit the parent object can also edit
37 # LibraryFilasAlias instance.
38 login_person(self.bug_owner)
39 self.assertFalse(self.lfa_with_parent.restricted)
40 self.bug_attachment.title = 'foo'
41 self.lfa_with_parent.restricted = True
42 self.assertTrue(self.lfa_with_parent.restricted)
43
44 def test_setRestricted_unauthorized_user(self):
45 # If a user cannot change properties of a bug attachment...
46 other_person = self.factory.makePerson()
47 login_person(other_person)
48 self.assertRaises(
49 Unauthorized, setattr, self.bug_attachment, 'title', 'whatever')
50 # ...he also can't change the LibraryFileAlias for this bug.
51 self.assertRaises(
52 Unauthorized, setattr, self.lfa_with_parent, 'restricted', True)
53
054
=== 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 @@
12 <allow interface="canonical.launchpad.interfaces.ILibraryFileAlias" />12 <allow interface="canonical.launchpad.interfaces.ILibraryFileAlias" />
13 </class>13 </class>
1414
15 <class class="canonical.launchpad.database.LibraryFileAliasWithParent">
16 <allow interface="canonical.launchpad.interfaces.ILibraryFileAliasWithParent" />
17 <require
18 permission="launchpad.Edit"
19 set_attributes="restricted" />
20 </class>
21
15 <class class="canonical.launchpad.database.LibraryFileContent">22 <class class="canonical.launchpad.database.LibraryFileContent">
16 <allow interface="canonical.launchpad.interfaces.ILibraryFileContent" />23 <allow interface="canonical.launchpad.interfaces.ILibraryFileContent" />
17 </class>24 </class>
@@ -48,4 +55,8 @@
48 <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher" />55 <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher" />
49 </class>56 </class>
5057
58 <adapter
59 factory="canonical.launchpad.database.librarian.LibraryFileAliasWithParent"
60 provides="canonical.launchpad.interfaces.librarian.ILibraryFileAliasWithParent"
61 trusted="true" />
51</configure>62</configure>

Subscribers

People subscribed via source and target branches

to status/vote changes: