launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #16330
[Merge] lp:~wgrant/launchpad/relax-team-privacy-transitions into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/relax-team-privacy-transitions into lp:launchpad.
Commit message:
Permit private teams to become public, and permit common safe relationships in privacy transitions.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/relax-team-privacy-transitions/+merge/200777
We permit teams to transition from public -> private, but not private -> public. Nowadays we permit pretty much every private artifact to become public later, and it's increasingly problematic that teams must be recreated as part of revealing projects. So let's remove the senseless restriction against private -> public transitions, and update the list of exceptions to remove some common pain points.
--
https://code.launchpad.net/~wgrant/launchpad/relax-team-privacy-transitions/+merge/200777
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/relax-team-privacy-transitions into lp:launchpad.
=== modified file 'lib/lp/registry/browser/tests/private-team-creation-views.txt'
--- lib/lp/registry/browser/tests/private-team-creation-views.txt 2013-05-10 06:44:11 +0000
+++ lib/lp/registry/browser/tests/private-team-creation-views.txt 2014-01-08 07:49:09 +0000
@@ -223,46 +223,9 @@
----------------------
A team can only change visibility if the new state will not leak any
-data or put the team into an inconsistent state. The rules are:
-
-Private -> Public: Fail
-Public -> Private : OK, if the team only has the permitted artifacts
-Public -> Private : Fail, if the team has a restricted artifact
-
-Private teams cannot be made public.
-
- >>> super_secret2 = personset.getByName('super-secret2')
- >>> form = {
- ... 'field.name': 'super-secret-two',
- ... 'field.displayname': 'New Display Name',
- ... 'field.defaultmembershipperiod': '365',
- ... 'field.defaultrenewalperiod': '',
- ... 'field.membership_policy': 'OPEN',
- ... 'field.renewal_policy': 'NONE',
- ... 'field.visibility': 'PUBLIC',
- ... 'field.actions.save': 'Save',
- ... }
- >>> view = create_initialized_view(
- ... super_secret2, '+edit',
- ... form=form, principal=foo_bar)
-
- >>> for notification in view.request.notifications:
- ... print notification.message
- A private team cannot change visibility.
-
- >>> print len(view.errors)
- 0
-
-All changes are aborted when a data validation error occurs. The
-displayname for the team is the old value.
-
- >>> transaction.commit()
- >>> super_secret2 = personset.getByName('super-secret2')
- >>> print super_secret2.name
- super-secret2
-
- >>> print super_secret2.displayname
- Shhhh
+data or put the team into an inconsistent state. Public teams can become
+private and vice-versa, as long as they only participate in a set list
+of known-OK relationships.
Public teams can be made private if the only artifacts they have are
those permitted by private teams.
@@ -357,6 +320,18 @@
This team cannot be converted to Private since it is referenced by a
bugtracker.
+All changes are aborted when a data validation error occurs. The
+displayname for the team is the old value.
+
+ >>> transaction.commit()
+ >>> super_secret2 = personset.getByName('super-secret2')
+ >>> print super_secret2.name
+ super-secret2
+
+ >>> print super_secret2.displayname
+ Shhhh
+
+
Use of 'private-' prefix
------------------------
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2013-08-08 04:11:59 +0000
+++ lib/lp/registry/interfaces/person.py 2014-01-08 07:49:09 +0000
@@ -582,8 +582,7 @@
Choice(title=_("Visibility"),
description=_(
"Anyone can see a public team's data. Only team members "
- "and Launchpad admins can see private team data. "
- "Private teams cannot become public."),
+ "can see private team data."),
required=True, vocabulary=PersonVisibility,
default=PersonVisibility.PUBLIC, readonly=True))
=== modified file 'lib/lp/registry/javascript/team.js'
--- lib/lp/registry/javascript/team.js 2013-03-20 22:32:47 +0000
+++ lib/lp/registry/javascript/team.js 2014-01-08 07:49:09 +0000
@@ -215,41 +215,12 @@
/**
- * Show the extra help about private teams.
- * @param visibility
- */
-module.visibility_changed_visibility = function(visibility) {
- var widget_label = Y.one("[for='field.visibility']");
- if (!Y.Lang.isValue(widget_label)) {
- return;
- }
- var extra_help_node = Y.one('#visibility-extra-help');
- if (!extra_help_node) {
- // Create the needed extra help node once.
- extra_help_node = Y.Node.create(
- '<div id="visibility-extra-help"></div>')
- .addClass('sprite')
- .addClass('info')
- .addClass('hidden')
- .set('text', 'Private teams cannot become public later.');
- widget_label.ancestor('div').one('.formHelp').insert(
- extra_help_node, 'before');
- }
- if (visibility === 'PRIVATE') {
- extra_help_node.removeClass('hidden');
- } else {
- extra_help_node.addClass('hidden');
- }
-};
-
-/**
* The team's visibility has changed so we need to update the form and
* explain the consequences.
* @param visibility
*/
module.visibility_changed = function(visibility) {
module.visibility_changed_subscription(visibility);
- module.visibility_changed_visibility(visibility);
};
/**
=== modified file 'lib/lp/registry/javascript/tests/test_team.js'
--- lib/lp/registry/javascript/tests/test_team.js 2013-03-20 03:41:40 +0000
+++ lib/lp/registry/javascript/tests/test_team.js 2014-01-08 07:49:09 +0000
@@ -83,11 +83,6 @@
Y.Assert.areEqual(
'Private teams must have a restricted membership policy.',
extra_help.get('text'));
- var extra_visibility_help = Y.one('#visibility-extra-help');
- Y.Assert.isFalse(extra_visibility_help.hasClass('hidden'));
- Y.Assert.areEqual(
- 'Private teams cannot become public later.',
- extra_visibility_help.get('text'));
},
// When the visibility field becomes public, all subscription policies
@@ -116,8 +111,6 @@
extra_help = Y.one('[for="field.membership_policy"]')
.ancestor('div').one('.info');
Y.Assert.isNull(extra_help);
- var extra_visibility_help = Y.one('#visibility-extra-help');
- Y.Assert.isTrue(extra_visibility_help.hasClass('hidden'));
},
// When the membership policy changes to private and back to public,
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2013-08-19 06:43:04 +0000
+++ lib/lp/registry/model/person.py 2014-01-08 07:49:09 +0000
@@ -364,23 +364,12 @@
def validate_person_visibility(person, attr, value):
"""Validate changes in visibility.
- * Prevent teams with inconsistent connections from being made private.
- * Prevent private teams from any transition.
+ Prevent teams with links to other objects from transitioning,
+ ignoring known-OK links.
"""
-
- # Prohibit any visibility changes for private teams. This rule is
- # recognized to be Draconian and may be relaxed in the future.
- if person.visibility == PersonVisibility.PRIVATE:
- raise ImmutableVisibilityError(
- 'A private team cannot change visibility.')
-
- # If transitioning to a non-public visibility, check for existing
- # relationships that could leak data.
- if value != PersonVisibility.PUBLIC:
- warning = person.visibilityConsistencyWarning(value)
- if warning is not None:
- raise ImmutableVisibilityError(warning)
-
+ warning = person.visibilityConsistencyWarning(value)
+ if warning is not None:
+ raise ImmutableVisibilityError(warning)
return value
@@ -523,8 +512,8 @@
_sortingColumnsForSetOperations = SQL(
"person_sort_key(displayname, name)")
_defaultOrder = sortingColumns
- _visibility_warning_marker = object()
- _visibility_warning_cache = _visibility_warning_marker
+ _visibility_warning_cache_key = None
+ _visibility_warning_cache = None
account = ForeignKey(dbName='account', foreignKey='Account', default=None)
@@ -2306,17 +2295,27 @@
A private-membership team cannot be connected to other
objects, since it may be possible to infer the membership.
"""
- if self._visibility_warning_cache != self._visibility_warning_marker:
+ if self._visibility_warning_cache_key == new_value:
return self._visibility_warning_cache
cur = cursor()
references = list(
postgresql.listReferences(cur, 'person', 'id', indirect=False))
# These tables will be skipped since they do not risk leaking
- # team membership information, except StructuralSubscription
- # which will be checked further down to provide a clearer warning.
+ # team membership information.
# Note all of the table names and columns must be all lowercase.
skip = set([
+ ('accessartifactgrant', 'grantee'),
+ ('accessartifactgrant', 'grantor'),
+ ('accesspolicy', 'person'),
+ ('accesspolicygrant', 'grantee'),
+ ('accesspolicygrant', 'grantor'),
+ ('archive', 'owner'),
+ ('archivesubscriber', 'subscriber'),
+ ('branch', 'owner'),
+ ('branchsubscription', 'person'),
+ ('bugsubscription', 'person'),
+ ('bugtask', 'assignee'),
('emailaddress', 'person'),
('gpgkey', 'owner'),
('ircid', 'person'),
@@ -2330,6 +2329,12 @@
('personsettings', 'person'),
('persontransferjob', 'minor_person'),
('persontransferjob', 'major_person'),
+ ('product', 'bug_supervisor'),
+ ('product', 'driver'),
+ ('product', 'owner'),
+ ('productseries', 'driver'),
+ ('productseries', 'owner'),
+ ('sharingjob', 'grantee'),
('signedcodeofconduct', 'owner'),
('sshkey', 'person'),
('structuralsubscription', 'subscriber'),
@@ -2345,18 +2350,6 @@
('latestpersonsourcepackagereleasecache', 'maintainer'),
])
- # The following relationships are allowable for Private teams and
- # thus should be skipped.
- if new_value == PersonVisibility.PRIVATE:
- skip.update([('bugsubscription', 'person'),
- ('bugtask', 'assignee'),
- ('branch', 'owner'),
- ('branchsubscription', 'person'),
- ('branchvisibilitypolicy', 'team'),
- ('archive', 'owner'),
- ('archivesubscriber', 'subscriber'),
- ])
-
warnings = set()
ref_query = []
for src_tab, src_col, ref_tab, ref_col, updact, delact in references:
@@ -2377,36 +2370,6 @@
article = 'a'
warnings.add('%s %s' % (article, table_name))
- # Private teams may have structural subscription, so the following
- # test is not applied to them.
- if new_value != PersonVisibility.PRIVATE:
- # Add warnings for subscriptions in StructuralSubscription table
- # describing which kind of object is being subscribed to.
- cur.execute("""
- SELECT
- count(product) AS product_count,
- count(productseries) AS productseries_count,
- count(project) AS project_count,
- count(milestone) AS milestone_count,
- count(distribution) AS distribution_count,
- count(distroseries) AS distroseries_count,
- count(sourcepackagename) AS sourcepackagename_count
- FROM StructuralSubscription
- WHERE subscriber=%d LIMIT 1
- """ % self.id)
-
- row = cur.fetchone()
- for count, warning in zip(row, [
- 'a project subscriber',
- 'a project series subscriber',
- 'a project subscriber',
- 'a milestone subscriber',
- 'a distribution subscriber',
- 'a distroseries subscriber',
- 'a source package subscriber']):
- if count > 0:
- warnings.add(warning)
-
# Non-purged mailing list check for transitioning to or from PUBLIC.
if PersonVisibility.PUBLIC in [self.visibility, new_value]:
mailing_list = getUtility(IMailingListSet).get(self.name)
@@ -2419,6 +2382,7 @@
if len(warnings) == 0:
self._visibility_warning_cache = None
+ self._visibility_warning_cache_key = new_value
else:
if len(warnings) == 1:
message = warnings[0]
@@ -2429,6 +2393,7 @@
self._visibility_warning_cache = (
'This team cannot be converted to %s since it is '
'referenced by %s.' % (new_value, message))
+ self._visibility_warning_cache_key = new_value
return self._visibility_warning_cache
@property
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2013-05-01 21:23:16 +0000
+++ lib/lp/registry/tests/test_person.py 2014-01-08 07:49:09 +0000
@@ -900,6 +900,7 @@
# needlessly run.
fake_warning = 'Warning! Warning!'
naked_team = removeSecurityProxy(self.otherteam)
+ naked_team._visibility_warning_cache_key = PersonVisibility.PRIVATE
naked_team._visibility_warning_cache = fake_warning
warning = self.otherteam.visibilityConsistencyWarning(
PersonVisibility.PRIVATE)
@@ -913,18 +914,33 @@
self.otherteam.visibility = PersonVisibility.PRIVATE
def test_visibility_validator_team_private_to_public(self):
- # A PRIVATE team cannot convert to PUBLIC.
- self.otherteam.visibility = PersonVisibility.PRIVATE
+ # A PRIVATE team can transition to PUBLIC.
+ self.otherteam.visibility = PersonVisibility.PRIVATE
+ self.otherteam.visibility = PersonVisibility.PUBLIC
+
+ def test_visibility_validator_team_private_to_public_with_forbidden(self):
+ # A PRIVATE team that owns a distribution can't become PUBLIC.
+ # XXX wgrant 2014-01-08: This probably isn't a sane constraint,
+ # but it'll do to test the forbidden case until we decide to
+ # relax it.
+ self.otherteam.visibility = PersonVisibility.PRIVATE
+ self.factory.makeDistribution(bug_supervisor=self.otherteam)
+ Store.of(self.otherteam).flush()
try:
self.otherteam.visibility = PersonVisibility.PUBLIC
except ImmutableVisibilityError as exc:
self.assertEqual(
str(exc),
- 'A private team cannot change visibility.')
+ 'This team cannot be converted to Public since it is '
+ 'referenced by a distribution.')
+ else:
+ raise AssertionError("Expected exception.")
def test_visibility_validator_team_private_to_public_view(self):
# A PRIVATE team cannot convert to PUBLIC.
self.otherteam.visibility = PersonVisibility.PRIVATE
+ self.factory.makeDistribution(bug_supervisor=self.otherteam)
+ Store.of(self.otherteam).flush()
view = create_initialized_view(self.otherteam, '+edit', {
'field.name': 'otherteam',
'field.displayname': 'Other Team',
@@ -935,8 +951,10 @@
})
self.assertEqual(len(view.errors), 0)
self.assertEqual(len(view.request.notifications), 1)
- self.assertEqual(view.request.notifications[0].message,
- 'A private team cannot change visibility.')
+ self.assertEqual(
+ view.request.notifications[0].message,
+ 'This team cannot be converted to Public since it is '
+ 'referenced by a distribution.')
def test_person_snapshot(self):
omitted = (
Follow ups
