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, PersonVisibility if not IPerson.providedBy(person): - raise ConstraintNotSatisfied("Expected a person.") + return False if person.visibility == PersonVisibility.PRIVATE_MEMBERSHIP: # PRIVATE_MEMBERSHIP. return True @@ -779,16 +781,36 @@ return False -class PublicPersonChoice(Choice): +class PrivateTeamNotAllowed(ConstraintNotSatisfied): + __doc__ = _("A private team is not allowed.") + + +class PrivateMembershipTeamNotAllowed(ConstraintNotSatisfied): + __doc__ = _("A private-membership team is not allowed.") + + +class PersonChoice(Choice): + """A person or team. + + This is useful as a superclass and provides a clearer error message than + "Constraint not satisfied". + """ + implements(IReferenceChoice) + schema = IObject # Will be set to IPerson once IPerson is defined. + + +class PublicPersonChoice(PersonChoice): """A person or team who is public.""" - implements(IReferenceChoice) - schema = IObject # Will be set to IPerson once IPerson is defined. def constraint(self, value): - return is_valid_public_person(value) - - -class ParticipatingPersonChoice(Choice): + if is_valid_public_person(value): + return True + else: + # The vocabulary prevents the revealing of private team names. + raise PrivateTeamNotAllowed(value) + + +class ParticipatingPersonChoice(PersonChoice): """A person or team who is not a private membership team. A person can participate in all contexts. A PRIVATE team can participate @@ -796,8 +818,10 @@ user. A PRIVATE MEMBERSHIP team is severely limited in the roles in which it can participate. """ - implements(IReferenceChoice) - schema = IObject # Will be set to IPerson once IPerson is defined. def constraint(self, value): - return not is_private_membership(value) + if not is_private_membership(value): + return True + else: + # The vocabulary prevents the revealing of private team names. + raise PrivateMembershipTeamNotAllowed(value) === modified file 'lib/lp/registry/browser/team.py' --- lib/lp/registry/browser/team.py 2009-11-30 21:38:08 +0000 +++ lib/lp/registry/browser/team.py 2009-12-01 21:18:15 +0000 @@ -970,24 +970,10 @@ """ newmember = data.get('newmember') error = None - if newmember is None: - # The member may not be found because it is not in the - # the vocabulary that the interface attribute defines. - # Here we provide a clearer error message when the team - # is not in the vocabulary because it is private. Changing - # the vocabulary that this form uses is not a good idea, - # since it will cause invalid members to show up in the - # VocabularyPickerWidget results. - member_name = self.request.form.get('field.newmember') - member = getUtility(IPersonSet).getByName(member_name) - if member is not None: - if member.visibility != PersonVisibility.PUBLIC: - error = _("A private team cannot become a member of " - "another team.") - else: + if newmember is not None: if newmember.isTeam() and not newmember.activemembers: - error = _("You can't add a team that doesn't have any " - "active members.") + error = _("You can't add a team that doesn't have any active" + " members.") elif newmember in self.context.activemembers: error = _("%s (%s) is already a member of %s." % ( newmember.displayname, newmember.name, === modified file 'lib/lp/registry/interfaces/person.py' --- lib/lp/registry/interfaces/person.py 2009-09-08 12:30:13 +0000 +++ lib/lp/registry/interfaces/person.py 2009-12-01 00:57:17 +0000 @@ -65,10 +65,9 @@ from canonical.database.sqlbase import block_implicit_flushes from canonical.launchpad.fields import ( - BlacklistableContentNameField, IconImageUpload, LogoImageUpload, - MugshotImageUpload, ParticipatingPersonChoice, PasswordField, - PublicPersonChoice, StrippedTextLine, is_private_membership, - is_valid_public_person) + BlacklistableContentNameField, IconImageUpload, is_private_membership, + is_valid_public_person, LogoImageUpload, MugshotImageUpload, + PasswordField, PersonChoice, PublicPersonChoice, StrippedTextLine) from canonical.launchpad.interfaces.account import AccountStatus, IAccount from canonical.launchpad.interfaces.emailaddress import IEmailAddress from lp.app.interfaces.headings import IRootContext @@ -1501,8 +1500,7 @@ # Set the schemas to the newly defined interface for classes that deferred # doing so when defined. -PublicPersonChoice.schema = IPerson -ParticipatingPersonChoice.schema = IPerson +PersonChoice.schema = IPerson class INewPersonForm(IPerson): === modified file 'lib/lp/registry/stories/teammembership/xx-private-membership.txt' --- lib/lp/registry/stories/teammembership/xx-private-membership.txt 2009-11-25 17:54:36 +0000 +++ lib/lp/registry/stories/teammembership/xx-private-membership.txt 2009-12-01 20:38:27 +0000 @@ -235,7 +235,7 @@ 'http://launchpad.dev/%7Esimple-team/+addmember' >>> get_feedback_messages(admin_browser.contents) [...There is 1 error... - ...A private team cannot become a member of another team.'] + ...A private team is not allowed.'] Anonymous users cannot even know that the private membership team even exists. @@ -306,7 +306,7 @@ >>> admin_browser.url 'http://bugs.launchpad.dev/firefox/+bug/1/+addsubscriber' >>> get_feedback_messages(admin_browser.contents) - [...Constraint not satisfied... + [...A private-membership team is not allowed... == Restrict Subscribing to Blueprints == @@ -321,7 +321,7 @@ >>> admin_browser.url 'http://blueprints.launchpad.dev/firefox/+spec/canvas/+addsubscriber' >>> get_feedback_messages(admin_browser.contents) - [...Constraint not satisfied... + [...A private team is not allowed... == Restrict Appointing a Translator == @@ -337,7 +337,7 @@ >>> admin_browser.url 'http://translations.launchpad.dev/+groups/testing-translation-team/+appoint' >>> get_feedback_messages(admin_browser.contents) - [...Constraint not satisfied... + [...A private team is not allowed... == Restrict Project Owner == @@ -366,7 +366,7 @@ >>> admin_browser.url 'http://launchpad.dev/jokosher/+edit-people' >>> get_feedback_messages(admin_browser.contents) - [u'There is 1 error.', u'Constraint not satisfied'] + [...A private-membership team is not allowed... == Restrict Product Bug Supervisor == }}}