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
1=== modified file 'configs/development/launchpad-lazr.conf'
2--- configs/development/launchpad-lazr.conf 2010-07-21 08:15:57 +0000
3+++ configs/development/launchpad-lazr.conf 2010-07-26 14:46:16 +0000
4@@ -161,6 +161,8 @@
5 restricted_download_port: 58085
6 restricted_download_url: http://launchpad.dev:58085/
7 use_https = False
8+oops_prefix: L
9+error_dir: /var/tmp/codehosting.test
10
11 [librarian_server]
12 root: /var/tmp/fatsam
13
14=== modified file 'daemons/librarian.tac'
15--- daemons/librarian.tac 2010-04-20 14:27:26 +0000
16+++ daemons/librarian.tac 2010-07-26 14:46:16 +0000
17@@ -19,6 +19,7 @@
18 from canonical.librarian.libraryprotocol import FileUploadFactory
19 from canonical.librarian import storage, db
20 from canonical.librarian import web as fatweb
21+from lp.services.twistedsupport.loggingsupport import set_up_oops_reporting
22
23 # Connect to database
24 dbconfig.setConfigSection('librarian')
25@@ -70,6 +71,9 @@
26 uploadPort = config.librarian.restricted_upload_port
27 setUpListener(uploadPort, webPort, restricted=True)
28
29+# Log OOPS reports
30+set_up_oops_reporting('librarian', 'librarian')
31+
32 # Setup a signal handler to dump the process' memory upon 'kill -44'.
33 def sigdumpmem_handler(signum, frame):
34 scanner.dump_all_objects(DUMP_FILE)
35
36=== modified file 'lib/canonical/config/schema-lazr.conf'
37--- lib/canonical/config/schema-lazr.conf 2010-07-24 00:07:54 +0000
38+++ lib/canonical/config/schema-lazr.conf 2010-07-26 14:46:16 +0000
39@@ -1225,6 +1225,13 @@
40
41 use_https = True
42
43+# See [error_reports].
44+# Must be unique per librarian instance.
45+oops_prefix: none
46+# Must be set per librarian instance.
47+error_dir: none
48+# Must be false: librarian has no zlog.
49+copy_to_zlog: False
50
51 [librarian_gc]
52 # The database user which will be used by this process.
53
54=== modified file 'lib/canonical/launchpad/database/librarian.py'
55--- lib/canonical/launchpad/database/librarian.py 2009-11-24 15:36:44 +0000
56+++ lib/canonical/launchpad/database/librarian.py 2010-07-26 14:46:16 +0000
57@@ -6,6 +6,7 @@
58 __metaclass__ = type
59 __all__ = [
60 'LibraryFileAlias',
61+ 'LibraryFileAliasWithParent',
62 'LibraryFileAliasSet',
63 'LibraryFileContent',
64 'LibraryFileDownloadCount',
65@@ -14,22 +15,23 @@
66 from datetime import datetime, timedelta
67 import pytz
68
69-from zope.component import getUtility
70-from zope.interface import implements
71+from zope.component import adapts, getUtility
72+from zope.interface import implements, Interface
73
74 from sqlobject import StringCol, ForeignKey, IntCol, SQLRelatedJoin, BoolCol
75 from storm.locals import Date, Desc, Int, Reference, Store
76
77 from canonical.config import config
78 from canonical.launchpad.interfaces import (
79- ILibraryFileAlias, ILibraryFileAliasSet, ILibraryFileContent,
80- ILibraryFileDownloadCount, IMasterStore)
81+ ILibraryFileAlias, ILibraryFileAliasWithParent, ILibraryFileAliasSet,
82+ ILibraryFileContent, ILibraryFileDownloadCount, IMasterStore)
83 from canonical.librarian.interfaces import (
84 DownloadFailed, ILibrarianClient, IRestrictedLibrarianClient,
85 LIBRARIAN_SERVER_DEFAULT_TIMEOUT)
86 from canonical.database.sqlbase import SQLBase
87 from canonical.database.constants import UTC_NOW, DEFAULT
88 from canonical.database.datetimecol import UtcDateTimeCol
89+from lazr.delegates import delegates
90
91
92 class LibraryFileContent(SQLBase):
93@@ -195,6 +197,18 @@
94 self.close()
95
96
97+class LibraryFileAliasWithParent:
98+ """A LibraryFileAlias variant that has a parent."""
99+
100+ adapts(ILibraryFileAlias, Interface)
101+ implements(ILibraryFileAliasWithParent)
102+ delegates(ILibraryFileAlias)
103+
104+ def __init__(self, libraryfile, parent):
105+ self.context = libraryfile
106+ self.__parent__ = parent
107+
108+
109 class LibraryFileAliasSet(object):
110 """Create and find LibraryFileAliases."""
111
112
113=== modified file 'lib/canonical/launchpad/interfaces/librarian.py'
114--- lib/canonical/launchpad/interfaces/librarian.py 2010-03-01 03:06:02 +0000
115+++ lib/canonical/launchpad/interfaces/librarian.py 2010-07-26 14:46:16 +0000
116@@ -9,6 +9,7 @@
117
118 __all__ = [
119 'ILibraryFileAlias',
120+ 'ILibraryFileAliasWithParent',
121 'ILibraryFileAliasSet',
122 'ILibraryFileContent',
123 'ILibraryFileDownloadCount',
124@@ -116,6 +117,10 @@
125 """
126
127
128+class ILibraryFileAliasWithParent(ILibraryFileAlias):
129+ """A ILibraryFileAlias that knows about its parent."""
130+
131+
132 class ILibraryFileContent(Interface):
133 """Actual data in the Librarian.
134
135
136=== modified file 'lib/canonical/launchpad/security.py'
137--- lib/canonical/launchpad/security.py 2010-07-21 09:35:19 +0000
138+++ lib/canonical/launchpad/security.py 2010-07-26 14:46:16 +0000
139@@ -13,6 +13,8 @@
140
141 from canonical.launchpad.interfaces.account import IAccount
142 from canonical.launchpad.interfaces.emailaddress import IEmailAddress
143+from canonical.launchpad.interfaces.librarian import (
144+ ILibraryFileAliasWithParent)
145 from lp.registry.interfaces.announcement import IAnnouncement
146 from lp.soyuz.interfaces.archive import IArchive
147 from lp.soyuz.interfaces.archivepermission import (
148@@ -2481,3 +2483,23 @@
149 def checkAuthenticated(self, user):
150 """Users must be an admin or a member of the tech board."""
151 return user.in_admin or user.in_ubuntu_techboard
152+
153+
154+class EditLibraryFileAliasWithParent(AuthorizationBase):
155+ permission = 'launchpad.Edit'
156+ usedfor = ILibraryFileAliasWithParent
157+
158+ def checkAuthenticated(self, user):
159+ """Only persons which can edit an LFA's parent can edit an LFA.
160+
161+ By default, a LibraryFileAlias does not know about its parent.
162+ Such aliases are never editable. Use an adapter to provide a
163+ parent object.
164+
165+ If a parent is known, users which can edit the parent can also
166+ edit properties of the LibraryFileAlias.
167+ """
168+ parent = getattr(self.obj, '__parent__', None)
169+ if parent is None:
170+ return False
171+ return check_permission(self.permission, parent)
172
173=== added file 'lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py'
174--- lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py 1970-01-01 00:00:00 +0000
175+++ lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py 2010-07-26 14:46:16 +0000
176@@ -0,0 +1,53 @@
177+# Copyright 2010 Canonical Ltd. This software is licensed under the
178+# GNU Affero General Public License version 3 (see the file LICENSE).
179+
180+__metaclass__ = type
181+
182+__all__ = []
183+
184+from zope.component import getMultiAdapter
185+from zope.security.interfaces import Unauthorized
186+
187+from canonical.launchpad.interfaces.librarian import (
188+ ILibraryFileAliasWithParent)
189+from canonical.testing import LaunchpadFunctionalLayer
190+from lp.testing import login_person, TestCaseWithFactory
191+
192+class TestLibraryFileAliasForBugAttachment(TestCaseWithFactory):
193+ """Tests for ILibraryFileAliasWithParent."""
194+
195+ layer = LaunchpadFunctionalLayer
196+
197+ def setUp(self):
198+ super(TestLibraryFileAliasForBugAttachment, self).setUp()
199+ self.bug_owner = self.factory.makePerson()
200+ login_person(self.bug_owner)
201+ self.bug = self.factory.makeBug(owner=self.bug_owner, private=True)
202+ self.bug_attachment = self.factory.makeBugAttachment(bug=self.bug)
203+ self.lfa_with_parent = getMultiAdapter(
204+ (self.bug_attachment.libraryfile, self.bug_attachment),
205+ ILibraryFileAliasWithParent)
206+
207+ def test_setRestricted_authorized_user(self):
208+ # A LibraryFileAlias instance can be adapted to an editable
209+ # variant, by adapting it, together with a parent object to
210+ # ILibraryFileAliasWithParent.
211+ #
212+ # People who can edit the parent object can also edit
213+ # LibraryFilasAlias instance.
214+ login_person(self.bug_owner)
215+ self.assertFalse(self.lfa_with_parent.restricted)
216+ self.bug_attachment.title = 'foo'
217+ self.lfa_with_parent.restricted = True
218+ self.assertTrue(self.lfa_with_parent.restricted)
219+
220+ def test_setRestricted_unauthorized_user(self):
221+ # If a user cannot change properties of a bug attachment...
222+ other_person = self.factory.makePerson()
223+ login_person(other_person)
224+ self.assertRaises(
225+ Unauthorized, setattr, self.bug_attachment, 'title', 'whatever')
226+ # ...he also can't change the LibraryFileAlias for this bug.
227+ self.assertRaises(
228+ Unauthorized, setattr, self.lfa_with_parent, 'restricted', True)
229+
230
231=== modified file 'lib/canonical/launchpad/zcml/librarian.zcml'
232--- lib/canonical/launchpad/zcml/librarian.zcml 2010-02-16 18:14:26 +0000
233+++ lib/canonical/launchpad/zcml/librarian.zcml 2010-07-26 14:46:16 +0000
234@@ -12,6 +12,13 @@
235 <allow interface="canonical.launchpad.interfaces.ILibraryFileAlias" />
236 </class>
237
238+ <class class="canonical.launchpad.database.LibraryFileAliasWithParent">
239+ <allow interface="canonical.launchpad.interfaces.ILibraryFileAliasWithParent" />
240+ <require
241+ permission="launchpad.Edit"
242+ set_attributes="restricted" />
243+ </class>
244+
245 <class class="canonical.launchpad.database.LibraryFileContent">
246 <allow interface="canonical.launchpad.interfaces.ILibraryFileContent" />
247 </class>
248@@ -48,4 +55,8 @@
249 <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher" />
250 </class>
251
252+ <adapter
253+ factory="canonical.launchpad.database.librarian.LibraryFileAliasWithParent"
254+ provides="canonical.launchpad.interfaces.librarian.ILibraryFileAliasWithParent"
255+ trusted="true" />
256 </configure>

Subscribers

People subscribed via source and target branches

to status/vote changes: