Merge lp://staging/~bac/launchpad/bug-410416 into lp://staging/launchpad

Proposed by Brad Crittenden
Status: Merged
Merged at revision: not available
Proposed branch: lp://staging/~bac/launchpad/bug-410416
Merge into: lp://staging/launchpad
Diff against target: None lines
To merge this branch: bzr merge lp://staging/~bac/launchpad/bug-410416
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+9932@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

= Summary =

An oversight was discovered in that private teams can not play the role of project
owner or driver, which was filed as Bug 410416.

In the process of making the fix it was discovered that creating a project also makes
the trunk productseries with the same owner. So productseries owner and driver were
also changed to allow private teams.

== Proposed fix ==

Change the type and validators for those properties to allow private teams.

== Pre-implementation notes ==

None.

== Implementation details ==

As above.

== Tests ==

bin/test -vvm lp.registry -t private-team-roles.txt

== Demo and Q/A ==

QA would be to create a private team on staging and ensure can become a project owner
and driver. Same for a product series.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/model/productseries.py
  lib/lp/registry/doc/private-team-roles.txt
  lib/lp/registry/interfaces/product.py
  lib/lp/registry/interfaces/productseries.py
  lib/lp/registry/model/product.py

== Pyflakes notices ==

lib/lp/registry/model/productseries.py
    37: 'validate_public_person' imported but unused

== Pylint notices ==

lib/lp/registry/model/productseries.py
    37: [W0611] Unused import validate_public_person

This import will be removed before landing.

Revision history for this message
Curtis Hovey (sinzui) wrote :
Download full text (3.4 KiB)

Hi Brad. Thank you for this excellent test. It does a lot to clarify what private teams
can do. I have some trivial suggesting to improve this branch.

I have a question about the very part of the test. Since you are just documenting
existing behaviour in this case, I do not think we need to change anything at this time.

> === modified file 'lib/lp/registry/doc/private-team-roles.txt'
> --- lib/lp/registry/doc/private-team-roles.txt 2009-08-03 01:41:07 +0000
> +++ lib/lp/registry/doc/private-team-roles.txt 2009-08-10 17:08:27 +0000

...

Use two leading blank lines to separate a heading from the previous content.

> +Maintainer/Owner
> +----------------
> +
> +A public team and a private team can be a project owner but a private membership team cannot.

Wrap the narrative at 78 characters.

> +
> + >>> # The registrant must be specified or it will default to the owner.
> + >>> product = factory.makeProduct(registrant=admin_user, owner=public_team)
> + >>> product = factory.makeProduct(registrant=admin_user, owner=priv_team)
> +
> + >>> product = factory.makeProduct(registrant=admin_user, owner=pm_team)
> + Traceback (most recent call last):
> + ...
> + PrivatePersonLinkageError: Cannot link person
> + (name=private-membership-team, visibility=PRIVATE_MEMBERSHIP) to
> + <Product at...
> +
> +
> +Driver
> +------
> +
> +A public team and a private team can be a project driver but a private membership team cannot.

Wrap the narrative at 78 characters.

...

> + <Product at...
> +
> +Product Series Roles
> +====================

Use two leading blank lines to separate a heading from the previous content.

> +Owner
> +-----
> +
> +A public team and a private team can be a product series owner but a
> +private membership team cannot.
> +
> + >>> product = factory.makeProduct(registrant=admin_user, owner=public_team)

Is this line 79 characters?

...

> +Driver
> +------
> +
> +A public team and a private team can be a product series driver but a
> +private membership team cannot.
> +
> + >>> product = factory.makeProduct(registrant=admin_user, owner=public_team)

Is this line 79 characters?

...

> +Team Membership
> +===============
> +
> +Mixing public and private teams can create interesting situations.
> +Not every type of team can become a member of other types.
> +
> + >>> from lp.registry.interfaces.person import PrivatePersonLinkageError
> + >>> reviewer = factory.makePerson()
> + >>> def join_team(joined_type, joiner_type):
> + ... joined = factory.makeTeam(owner=team_owner, visibility=joined_type)
> + ... joiner = factory.makeTeam(owner=team_owner, visibility=joiner_type)

Wrap the code at 78 characters.

...

> + Public <- Public: Allowed
> + Public <- Private Membership: Not Allowed
> + Public <- Private: Not Allowed
> + ---
> + Private Membership <- Public: Allowed
> + Private Membership <- Private Membership: Not Allowed
> + Private Membership <- Private: Not Allowed
> + ---
> + Private <- Public: Allowed

Really? Is my team really private if the owner of the public team adds every
team he can find on launchpad? Is there some other guard in p...

Read more...

review: Needs Fixing (code)
Revision history for this message
Brad Crittenden (bac) wrote :

> Hi Brad. Thank you for this excellent test. It does a lot to clarify what
> private teams
> can do. I have some trivial suggesting to improve this branch.
>
> I have a question about the very part of the test. Since you are just
> documenting
> existing behaviour in this case, I do not think we need to change anything at
> this time.
>
> > === modified file 'lib/lp/registry/doc/private-team-roles.txt'
> > --- lib/lp/registry/doc/private-team-roles.txt 2009-08-03 01:41:07
> +0000
> > +++ lib/lp/registry/doc/private-team-roles.txt 2009-08-10 17:08:27
> +0000
>
> ...
>
> Use two leading blank lines to separate a heading from the previous content.

Thanks for finding the formatting mistakes. They have all been corrected.

> ...
>
> > + Public <- Public: Allowed
> > + Public <- Private Membership: Not Allowed
> > + Public <- Private: Not Allowed
> > + ---
> > + Private Membership <- Public: Allowed
> > + Private Membership <- Private Membership: Not Allowed
> > + Private Membership <- Private: Not Allowed
> > + ---
> > + Private <- Public: Allowed
>
> Really? Is my team really private if the owner of the public team adds every
> team he can find on launchpad? Is there some other guard in place to prevent
> a the public team from exposing the private? A waring to the private team
> that is is risking exposure by adding a public team that it does not control?

As you said, this branch is about expanding the roles of private teams and documenting what they can and cannot do, including the current rules for team membership.

I was also surprised to see that public teams can join private teams. Or, more precisely, private teams can add a public team as a member.

If the owner of a private team willingly adds a public team to it then there are no additional safeguards. The members of the public sub-team can see into the private team's membership. The public team does not become a vector for non-members to see into the private team, however.

Adding a public team with restricted membership may have some valid use cases. Adding a public team with open membership is a disaster in waiting.

These issues are outside the scope of this branch but need to be addressed soon.

>
> > + Private <- Private Membership: Not Allowed
> > + Private <- Private: Not Allowed
> > + ---

Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Brad.

Thank you for the explanation of the private <- public team relationship. We can discuss issue later. This branch is fine to land.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/doc/private-team-roles.txt'
2--- lib/lp/registry/doc/private-team-roles.txt 2009-08-03 01:41:07 +0000
3+++ lib/lp/registry/doc/private-team-roles.txt 2009-08-10 17:08:27 +0000
4@@ -292,3 +292,162 @@
5 PrivatePersonLinkageError: Cannot link person
6 (name=private-membership-team, visibility=PRIVATE_MEMBERSHIP) to
7 <...StructuralSubscription at...
8+
9+
10+Project Roles
11+=============
12+
13+Registrant
14+----------
15+
16+Only a person can register a project, not a team, so no team, public
17+or private, can be the project registrant.
18+
19+ >>> login_person(admin_user)
20+ >>> public_team = factory.makeTeam(name='public-team',
21+ ... owner=team_owner,
22+ ... visibility=PersonVisibility.PUBLIC)
23+ >>> product = factory.makeProduct(registrant=team_owner)
24+ >>> product = factory.makeProduct(registrant=public_team)
25+ >>> product = factory.makeProduct(registrant=priv_team)
26+ Traceback (most recent call last):
27+ ...
28+ PrivatePersonLinkageError: Cannot link person
29+ (name=private-team, visibility=PRIVATE) to
30+ <Product at...
31+
32+ >>> product = factory.makeProduct(registrant=pm_team)
33+ Traceback (most recent call last):
34+ ...
35+ PrivatePersonLinkageError: Cannot link person
36+ (name=private-membership-team, visibility=PRIVATE_MEMBERSHIP) to
37+ <Product at...
38+
39+Maintainer/Owner
40+----------------
41+
42+A public team and a private team can be a project owner but a private membership team cannot.
43+
44+ >>> # The registrant must be specified or it will default to the owner.
45+ >>> product = factory.makeProduct(registrant=admin_user, owner=public_team)
46+ >>> product = factory.makeProduct(registrant=admin_user, owner=priv_team)
47+
48+ >>> product = factory.makeProduct(registrant=admin_user, owner=pm_team)
49+ Traceback (most recent call last):
50+ ...
51+ PrivatePersonLinkageError: Cannot link person
52+ (name=private-membership-team, visibility=PRIVATE_MEMBERSHIP) to
53+ <Product at...
54+
55+
56+Driver
57+------
58+
59+A public team and a private team can be a project driver but a private membership team cannot.
60+
61+ >>> product = factory.makeProduct()
62+ >>> product.driver = priv_team
63+
64+ >>> product.driver = pm_team
65+ Traceback (most recent call last):
66+ ...
67+ PrivatePersonLinkageError: Cannot link person
68+ (name=private-membership-team, visibility=PRIVATE_MEMBERSHIP) to
69+ <Product at...
70+
71+
72+Bug Supervisor
73+--------------
74+
75+A public team and a private team can be a project bug supervisor but a
76+private membership team cannot.
77+
78+ >>> product = factory.makeProduct()
79+ >>> product.setBugSupervisor(public_team, admin_user)
80+ >>> product.setBugSupervisor(priv_team, admin_user)
81+ >>> product.setBugSupervisor(pm_team, admin_user)
82+ Traceback (most recent call last):
83+ ...
84+ PrivatePersonLinkageError: Cannot link person
85+ (name=private-membership-team, visibility=PRIVATE_MEMBERSHIP) to
86+ <Product at...
87+
88+Product Series Roles
89+====================
90+
91+Owner
92+-----
93+
94+A public team and a private team can be a product series owner but a
95+private membership team cannot.
96+
97+ >>> product = factory.makeProduct(registrant=admin_user, owner=public_team)
98+ >>> product_series = factory.makeProductSeries(product, owner=public_team)
99+ >>> product_series = factory.makeProductSeries(product, owner=priv_team)
100+
101+ >>> product_series = factory.makeProductSeries(product, owner=pm_team)
102+ Traceback (most recent call last):
103+ ...
104+ PrivatePersonLinkageError: Cannot link person
105+ (name=private-membership-team, visibility=PRIVATE_MEMBERSHIP) to
106+ <ProductSeries at...
107+
108+
109+Driver
110+------
111+
112+A public team and a private team can be a product series driver but a
113+private membership team cannot.
114+
115+ >>> product = factory.makeProduct(registrant=admin_user, owner=public_team)
116+ >>> product_series = factory.makeProductSeries(product, owner=public_team)
117+ >>> product_series.driver = public_team
118+ >>> product_series.driver = priv_team
119+ >>> product_series.driver = pm_team
120+ Traceback (most recent call last):
121+ ...
122+ PrivatePersonLinkageError: Cannot link person
123+ (name=private-membership-team, visibility=PRIVATE_MEMBERSHIP) to
124+ <ProductSeries at...
125+
126+
127+Team Membership
128+===============
129+
130+Mixing public and private teams can create interesting situations.
131+Not every type of team can become a member of other types.
132+
133+ >>> from lp.registry.interfaces.person import PrivatePersonLinkageError
134+ >>> reviewer = factory.makePerson()
135+ >>> def join_team(joined_type, joiner_type):
136+ ... joined = factory.makeTeam(owner=team_owner, visibility=joined_type)
137+ ... joiner = factory.makeTeam(owner=team_owner, visibility=joiner_type)
138+ ... print "%s <- %s: " % (joined_type, joiner_type),
139+ ... try:
140+ ... joined.addMember(joiner, reviewer=reviewer)
141+ ... except PrivatePersonLinkageError:
142+ ... print "Not Allowed"
143+ ... else:
144+ ... print "Allowed"
145+
146+ >>> public = PersonVisibility.PUBLIC
147+ >>> private = PersonVisibility.PRIVATE
148+ >>> private_membership = PersonVisibility.PRIVATE_MEMBERSHIP
149+
150+ >>> visibility_list = [public, private, private_membership]
151+ >>> for joined in PersonVisibility.items:
152+ ... for joiner in PersonVisibility.items:
153+ ... join_team(joined, joiner)
154+ ... print "---"
155+ Public <- Public: Allowed
156+ Public <- Private Membership: Not Allowed
157+ Public <- Private: Not Allowed
158+ ---
159+ Private Membership <- Public: Allowed
160+ Private Membership <- Private Membership: Not Allowed
161+ Private Membership <- Private: Not Allowed
162+ ---
163+ Private <- Public: Allowed
164+ Private <- Private Membership: Not Allowed
165+ Private <- Private: Not Allowed
166+ ---
167
168=== modified file 'lib/lp/registry/interfaces/product.py'
169--- lib/lp/registry/interfaces/product.py 2009-07-17 00:26:05 +0000
170+++ lib/lp/registry/interfaces/product.py 2009-08-10 17:08:27 +0000
171@@ -37,8 +37,8 @@
172 from canonical.launchpad import _
173 from canonical.launchpad.fields import (
174 Description, IconImageUpload, LogoImageUpload, MugshotImageUpload,
175- ProductBugTracker, ProductNameField, PublicPersonChoice,
176- Summary, Title, URIField)
177+ ParticipatingPersonChoice, ProductBugTracker, ProductNameField,
178+ PublicPersonChoice, Summary, Title, URIField)
179 from lp.code.interfaces.branchvisibilitypolicy import (
180 IHasBranchVisibilityPolicy)
181 from lp.code.interfaces.hasbranches import IHasBranches, IHasMergeProposals
182@@ -353,7 +353,7 @@
183 exported_as='project_group')
184
185 owner = exported(
186- PublicPersonChoice(
187+ ParticipatingPersonChoice(
188 title=_('Maintainer'),
189 required=True,
190 vocabulary='ValidOwner',
191@@ -370,7 +370,7 @@
192 "Launchpad.")))
193
194 driver = exported(
195- PublicPersonChoice(
196+ ParticipatingPersonChoice(
197 title=_("Driver"),
198 description=_(
199 "This person or team will be able to set feature goals for "
200
201=== modified file 'lib/lp/registry/interfaces/productseries.py'
202--- lib/lp/registry/interfaces/productseries.py 2009-07-19 04:41:14 +0000
203+++ lib/lp/registry/interfaces/productseries.py 2009-08-10 17:08:27 +0000
204@@ -19,7 +19,8 @@
205 from zope.interface import Interface, Attribute
206
207 from canonical.launchpad.fields import (
208- ContentNameField, NoneableDescription, PublicPersonChoice, Title)
209+ ContentNameField, NoneableDescription, ParticipatingPersonChoice,
210+ PublicPersonChoice, Title)
211 from lp.code.interfaces.branch import IBranch
212 from lp.bugs.interfaces.bugtarget import IBugTarget
213 from lp.registry.interfaces.distroseries import DistroSeriesStatus
214@@ -125,12 +126,12 @@
215 exported_as='date_created')
216
217 owner = exported(
218- PublicPersonChoice(
219+ ParticipatingPersonChoice(
220 title=_('Owner'), required=True, vocabulary='ValidOwner',
221 description=_('Project owner, either a valid Person or Team')))
222
223 driver = exported(
224- PublicPersonChoice(
225+ ParticipatingPersonChoice(
226 title=_("Release manager"),
227 description=_(
228 "The person or team responsible for decisions about features "
229
230=== modified file 'lib/lp/registry/model/product.py'
231--- lib/lp/registry/model/product.py 2009-07-19 04:41:14 +0000
232+++ lib/lp/registry/model/product.py 2009-08-10 17:08:27 +0000
233@@ -183,11 +183,12 @@
234 project = ForeignKey(
235 foreignKey="Project", dbName="project", notNull=False, default=None)
236 owner = ForeignKey(
237- foreignKey="Person",
238- storm_validator=validate_public_person, dbName="owner", notNull=True)
239+ dbName="owner", foreignKey="Person",
240+ storm_validator=validate_person_not_private_membership,
241+ notNull=True)
242 registrant = ForeignKey(
243- foreignKey="Person",
244- storm_validator=validate_public_person, dbName="registrant",
245+ dbName="registrant", foreignKey="Person",
246+ storm_validator=validate_public_person,
247 notNull=True)
248 bug_supervisor = ForeignKey(
249 dbName='bug_supervisor', foreignKey='Person',
250@@ -200,7 +201,8 @@
251 default=None)
252 driver = ForeignKey(
253 dbName="driver", foreignKey="Person",
254- storm_validator=validate_public_person, notNull=False, default=None)
255+ storm_validator=validate_person_not_private_membership,
256+ notNull=False, default=None)
257 name = StringCol(
258 dbName='name', notNull=True, alternateID=True, unique=True)
259 displayname = StringCol(dbName='displayname', notNull=True)
260
261=== modified file 'lib/lp/registry/model/productseries.py'
262--- lib/lp/registry/model/productseries.py 2009-07-17 00:26:05 +0000
263+++ lib/lp/registry/model/productseries.py 2009-08-10 17:08:27 +0000
264@@ -34,7 +34,8 @@
265 from lp.registry.model.milestone import (
266 HasMilestonesMixin, Milestone)
267 from canonical.launchpad.database.packaging import Packaging
268-from lp.registry.interfaces.person import validate_public_person
269+from lp.registry.interfaces.person import (
270+ validate_person_not_private_membership, validate_public_person)
271 from lp.translations.model.pofile import POFile
272 from lp.translations.model.potemplate import (
273 HasTranslationTemplatesMixin,
274@@ -101,10 +102,12 @@
275 datecreated = UtcDateTimeCol(notNull=True, default=UTC_NOW)
276 owner = ForeignKey(
277 dbName="owner", foreignKey="Person",
278- storm_validator=validate_public_person, notNull=True)
279+ storm_validator=validate_person_not_private_membership,
280+ notNull=True)
281 driver = ForeignKey(
282 dbName="driver", foreignKey="Person",
283- storm_validator=validate_public_person, notNull=False, default=None)
284+ storm_validator=validate_person_not_private_membership,
285+ notNull=False, default=None)
286 branch = ForeignKey(foreignKey='Branch', dbName='branch',
287 default=None)
288 translations_autoimport_mode = EnumCol(