Merge lp://staging/~bac/launchpad/bug-410416 into lp://staging/launchpad
- bug-410416
- Merge into devel
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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Curtis Hovey (community) | code | Approve | |
Review via email: mp+9932@code.staging.launchpad.net |
Commit message
Description of the change
Brad Crittenden (bac) wrote : | # |
Curtis Hovey (sinzui) 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/
> --- lib/lp/
> +++ lib/lp/
...
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.
> + >>> product = factory.
> +
> + >>> product = factory.
> + Traceback (most recent call last):
> + ...
> + PrivatePersonLi
> + (name=private-
> + <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.
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.
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.
> + >>> reviewer = factory.
> + >>> def join_team(
> + ... joined = factory.
> + ... joiner = factory.
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...
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/
> > --- lib/lp/
> +0000
> > +++ lib/lp/
> +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
> > + ---
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.
Preview Diff
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( |
= 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: registry/ model/productse ries.py registry/ doc/private- team-roles. txt registry/ interfaces/ product. py registry/ interfaces/ productseries. py registry/ model/product. py
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
== Pyflakes notices ==
lib/lp/ registry/ model/productse ries.py public_ person' imported but unused
37: 'validate_
== Pylint notices ==
lib/lp/ registry/ model/productse ries.py public_ person
37: [W0611] Unused import validate_
This import will be removed before landing.