Merge lp:~bac/launchpad/bug-498179 into lp:launchpad

[Brad Crittenden]

Allow admins of a public team invited to join a private team to see the information for the private team.

[Brad Crittenden (bac)]

= Summary =

Private and Private Membership teams can invite public teams to become members. When
that happens an admin of the public team must go to a page to accept the membership.
 Since the admin is likely not a member of the private team she is barred from
viewing anything related to the private team, resulting in an OOPS as reported in bug
498179.

== Proposed fix ==

Change the security rules to allow the admins of invited teams to see the private parts.

== Pre-implementation notes ==

None.

== Implementation details ==

As above.

== Tests ==

bin/test -vvt private-team-visibility.txt

== Demo and Q/A ==

Create a private team.
Create a public team.
As an admin on the private team invite the private team to be a member.
As an admin of the public team attempt to accept the invitation.

= Launchpad lint =

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

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/lp/registry/doc/private-team-roles.txt
  lib/lp/registry/doc/private-team-visibility.txt

[Edwin Grubbs (edwin-grubbs)]

Hi Brad,

This is a nice improvement. Besides what we discussed on IRC, I have a couple of minor comments below.

merge-conditional

-Edwin

IRC Log
-------

[3:00 PM] <EdwinGrubbs> bac: What is the purpose of adding the pub_member to the pub_team? It appears to have no effect on the test.
[3:00 PM] <bac> EdwinGrubbs: likely a mistake
[3:01 PM] <bac> EdwinGrubbs: oh, i was going to show that even though pub_owner could see private parts the mere pub_member could not
[3:01 PM] <bac> EdwinGrubbs: i think that's still a good thing to do, i just forgot to do it
[3:05 PM] <bac> EdwinGrubbs: this is what i intended: http://pastebin.ubuntu.com/351429/
{{{
=== modified file 'lib/lp/registry/doc/private-team-visibility.txt'
--- lib/lp/registry/doc/private-team-visibility.txt 2010-01-04 19:33:33 +0000
+++ lib/lp/registry/doc/private-team-visibility.txt 2010-01-04 21:04:34 +0000
@@ -75,9 +75,18 @@
     >>> pub_owner in priv_team.activemembers
     False

-But the public team's owner can see the priv-team's bits since his team
+The public team's owner can now see the priv-team's bits since his team
 has been invited to join.

     >>> login_person(pub_owner)
     >>> print priv_team.name
     priv-team
+
+But a non-admin member of the public team still cannot see anything
+about the team.
+
+ >>> login_person(pub_member)
+ >>> print priv_team.name
+ Traceback (most recent call last):
+ ...
+ Unauthorized: (<Person at ... priv-team (Priv Team)>, 'name', 'launchpad.View')
}}}

[3:06 PM] <EdwinGrubbs> that looks good
[3:30 PM] <EdwinGrubbs> bac: I was thinking about the test to see if the user is in getDirectAdministrators(), and it seems like it would cause a problem for teams that are owned by another team. Therefore, you should probably use can_edit_team(invited_team, user)
[3:32 PM] <bac> EdwinGrubbs: good idea, i think

Comments on Diff
----------------

>=== modified file 'lib/lp/registry/doc/private-team-roles.txt'
>--- lib/lp/registry/doc/private-team-roles.txt 2009-10-26 20:08:10 +0000
>+++ lib/lp/registry/doc/private-team-roles.txt 2010-01-04 20:51:08 +0000
>@@ -243,6 +243,7 @@
> >>> import transaction
> >>> transaction.abort()
>
>+
> Structural Subscriptions
> ========================
>
>
>=== added file 'lib/lp/registry/doc/private-team-visibility.txt'
>--- lib/lp/registry/doc/private-team-visibility.txt 1970-01-01 00:00:00 +0000
>+++ lib/lp/registry/doc/private-team-visibility.txt 2010-01-04 20:51:08 +0000
>@@ -0,0 +1,83 @@
>+=========================
>+ Private team visibility
>+=========================
>+
>+Private and private membership team restrict the visibility of their

s/team/teams/

>+attributes to select sets of users in order to prevent leaking
>+confidential data.
>+
>+Private teams restrict the viewing of the membership list
>+to administrators and other members of the team.

You might want to say "Launchpad administrators", since
the team admins are also members, so it can sound redundant even
though it isn't.

>+ >>> from lp.registry.interfaces.person import PersonVisibility
>+ >>> priv_owner = factory.makePerson(name="priv-owner")
>+ >>> priv_member = factory.makePerson(name="...

review: Approve (code)

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-01-04 22:50:37 +0000
+++ lib/canonical/launchpad/security.py	2010-01-04 23:19:15 +0000
@@ -698,6 +698,11 @@
         admins = getUtility(ILaunchpadCelebrities).admin
         if user.inTeam(admins) or user.inTeam(self.obj):
             return True
+        # We also grant visibility of the private team to administrators of
+        # other teams that have been invited to join the private team.
+        for invitee in self.obj.invited_members:
+            if invitee.is_team and invitee in user.getAdministratedTeams():
+                return True
         return False
 
 

=== modified file 'lib/lp/registry/doc/private-team-roles.txt'
--- lib/lp/registry/doc/private-team-roles.txt	2009-10-26 20:08:10 +0000
+++ lib/lp/registry/doc/private-team-roles.txt	2010-01-04 23:19:15 +0000
@@ -243,6 +243,7 @@
     >>> import transaction
     >>> transaction.abort()
 
+
 Structural Subscriptions
 ========================
 

=== added file 'lib/lp/registry/doc/private-team-visibility.txt'
--- lib/lp/registry/doc/private-team-visibility.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/doc/private-team-visibility.txt	2010-01-04 23:19:15 +0000
@@ -0,0 +1,93 @@
+=========================
+ Private team visibility
+=========================
+
+Private and private membership teams restrict the visibility of their
+attributes to select sets of users in order to prevent leaking
+confidential data.
+
+Private teams restrict the viewing of the membership list
+to team administrators, other members of the team, and Launchpad
+administrators .
+
+    >>> from lp.registry.interfaces.person import PersonVisibility
+    >>> priv_owner = factory.makePerson(name="priv-owner")
+    >>> priv_member = factory.makePerson(name="priv-member")
+    >>> login('[e-mail]')
+    >>> priv_team = factory.makeTeam(owner=priv_owner, name="priv-team",
+    ...                              visibility=PersonVisibility.PRIVATE)
+    >>> login_person(priv_owner)
+    >>> priv_team.addMember(priv_member, reviewer=priv_owner)
+
+The team owner can see the membership.
+
+    >>> members = priv_team.activemembers
+    >>> for member in members:
+    ...     print member.name
+    priv-member
+    priv-owner
+
+A team member can access the membership.
+
+    >>> login_person(priv_member)
+    >>> members = priv_team.activemembers
+
+A person who is not in the team cannot see the membership and cannot
+see other details of the team, such as the name.
+
+    >>> login('[e-mail]')
+    >>> members = priv_team.activemembers
+    Traceback (most recent call last):
+    ...
+    Unauthorized: (<Person at ... priv-team (Priv Team)>, 'activemembers', 'launchpad.View')
+
+    >>> print priv_team.name
+    Traceback (most recent call last):
+    ...
+    Unauthorized: (<Person at ... priv-team (Priv Team)>, 'name', 'launchpad.View')
+
+Public teams can join private teams.  When adding one team to another
+the team is invited to join and that invitation must be accepted by
+one of the invited team's admins.  Normally the admin of the invited
+team is not a member of the private team and therefore cannot even see
+the page to accept the invitation!  To resolve that situation the
+rules for viewing a private team include admins of invited teams.
+
+    >>> pub_owner = factory.makePerson(name="pub-owner")
+    >>> pub_member = factory.makePerson(name="pub-member")
+    >>> pub_team = factory.makeTeam(owner=pub_owner, name="pubteam")
+    >>> login_person(pub_owner)
+    >>> pub_team.addMember(pub_member, reviewer=pub_owner)
+
+At this point the public team owner cannot see the priv-team's bits.
+
+    >>> print priv_team.name
+    Traceback (most recent call last):
+    ...
+    Unauthorized: (<Person at ... priv-team (Priv Team)>, 'name', 'launchpad.View')
+
+    >>> login_person(priv_owner)
+    >>> priv_team.addMember(pub_team, reviewer=priv_owner)
+
+The public team is not yet a member of the priv-team.
+
+    >>> pub_team in priv_team.activemembers
+    False
+    >>> pub_owner in priv_team.activemembers
+    False
+
+The public team's owner can now see the priv-team's bits since his team
+has been invited to join.
+
+    >>> login_person(pub_owner)
+    >>> print priv_team.name
+    priv-team
+
+But a non-admin member of the public team still cannot see anything
+about the team.
+
+    >>> login_person(pub_member)
+    >>> print priv_team.name
+    Traceback (most recent call last):
+    ...
+    Unauthorized: (<Person at ... priv-team (Priv Team)>, 'name', 'launchpad.View')