Merge lp:~edwin-grubbs/launchpad/bug-297833-invite-private-team into lp:launchpad/db-devel

Summary
-------

This branch makes the "Constraint not satisfied" error message
clearer when someone tries to add a private team as a member
of another team.

I also noticed that the $team/+add-my-teams would OOPS if you
tried to add a private team as a member. I filtered out private
teams from the list of checkboxes, so that the error could not
occur on this page.

Tests
-----

./bin/test -vv -t team-views.txt

Demo and Q/A
------------

* Open http://launchpad.dev/~guadamen/+addmember
  * Enter "myteam" and click "Add member".
  * The error message should be:
    "A private team cannot become a member of another team."

* Open https://launchpad.dev/people/+newteam
  * Add a new team with the visibility set to private.
  * Open https://launchpad.dev/~guadamen/+add-my-teams
  * The newly created team should not appear in this list.

Putting "<p>Private teams cannot be added as a member of another team.</p>" on ever +add-my-teams page seems a bit crass. I also can't reproduce this problem running from trunk :/

The fix for +addmember is good. It's a bit of a shame the fix has to be at this level, but I don't think there's a choice about this (AFAIK no way for a vocabulary to say "no this thing isn't in this vocabulary *and here's why*).

Abstaining overall, sorry.

Hi Edwin.

Thanks for providing a fix for this situation. Brad and I discussed the issue of allowing true private teams to be members today. It is still a security nightmare. There is no easy fix, and the Launchpad team is not investing in fixing this in the next 6 months. I think your approach is correct, but I have a concern about the test on line 37--If I am not a member of the private team, I am not allowed to know it exists. This helpful warning will allow me to guess the existence of team names by trying to make them members.

Launchpad shows a 404 for teams that you cannot know. In this situation, the correct error is to say the team/person does not exist. If I am a member of the private team, then I may know that the relationship is forbidden.

Hi Curtis,

Thanks for the review.

> Hi Edwin.
>
> Thanks for providing a fix for this situation. Brad and I discussed the issue
> of allowing true private teams to be members today. It is still a security
> nightmare. There is no easy fix, and the Launchpad team is not investing in
> fixing this in the next 6 months. I think your approach is correct, but I have
> a concern about the test on line 37--If I am not a member of the private team,
> I am not allowed to know it exists. This helpful warning will allow me to
> guess the existence of team names by trying to make them members.
>
> Launchpad shows a 404 for teams that you cannot know. In this situation, the
> correct error is to say the team/person does not exist. If I am a member of
> the private team, then I may know that the relationship is forbidden.

I moved the check from the view class into the PublicPersonChoice and ParticipatingPersonChoice classes. This solves the leak, since the Choice classes constraint() is never called if the widget can't look up the team in the vocabulary, and the vocabulary filters out all private-membership teams and any private teams that the user is not a member of. Therefore, the possible error messages are:

 * "Invalid value" when the user can't know about a private team. Private membership teams will also cause this error, since they are not in the vocab. There is some redundancy here with respect to the error message in ParticipatingPersonChoice, but I didn't worry about that since you said private-membership teams are going away.

 * "A private team isnot allowed" when the user enters a private team that he belongs to. By allowing private teams in the vocabulary, we avoid the user being confused when they can't find a team in the picker that they know exists. beuno had mentioned that it would be nice if the picker showed such invalid options, but didn't let the user actually select them. I avoided the extremely verbose but correct error message "Private & private-membership teams are not allowed."

 * "A private-membership team is not allowed" when the user enters a private-membership team in a field that allows completely private teams, such as the project series' driver.

Here is the incremental diff:
{{{
=== modified file 'lib/canonical/launchpad/fields/__init__.py'
--- lib/canonical/launchpad/fields/__init__.py 2009-11-23 03:43:05 +0000
+++ lib/canonical/launchpad/fields/__init__.py 2009-12-01 22:08:45 +0000
@@ -37,6 +37,8 @@
     'PasswordField',
     'PillarAliases',
     'PillarNameField',
+ 'PrivateMembershipTeamNotAllowed',
+ 'PrivateTeamNotAllowed',
     'ProductBugTracker',
     'ProductNameField',
     'PublicPersonChoice',
@@ -758,7 +760,7 @@
     """Return True if the person is public."""
     from canonical.launchpad.interfaces import IPerson, PersonVisibility
     if not IPerson.providedBy(person):
- raise ConstraintNotSatisfied("Expected a person.")
+ return False
     if person.visibility == PersonVisibility.PUBLIC:
         return True
     else:
@@ -770,7 +772,7 @@
     """True if the person/team has private membership visibility."""
     from canonical.launchpad.interfaces import IPerson, PersonVisibili...

Thanks Edwin.

This s a great re-implementation. This is good to land when PQM opens.