launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/private-team-403-928870 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #928870 in Launchpad itself: "private team shows 403 instead of explaining the page is not shared"
  https://bugs.launchpad.net/launchpad/+bug/928870

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/private-team-403-928870/+merge/92459

== Implementation ==

Two simple fixes to SearchQuestionsView and HasSpecificationsView to put a check_permission(launchpad.View) guard around some init code that attempted to access attributes not available with only limitedView permission.

The above is sufficient to fix the problem so it is not worth doing any extra work in this particular branch to improve the 403 page as suggested in the bug comments.

== Tests ==

Enhance the existing TestTeamIndexView.test_user_without_launchpad_view test to specifically load the default team views for all the lp rootsites.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/answers/browser/questiontarget.py
  lib/lp/blueprints/browser/specificationtarget.py
  lib/lp/registry/browser/tests/test_team.py

-- 
https://code.launchpad.net/~wallyworld/launchpad/private-team-403-928870/+merge/92459
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/private-team-403-928870 into lp:launchpad.

=== modified file 'lib/lp/answers/browser/questiontarget.py'
--- lib/lp/answers/browser/questiontarget.py	2012-01-06 11:08:30 +0000
+++ lib/lp/answers/browser/questiontarget.py	2012-02-10 12:28:45 +0000
@@ -430,6 +430,8 @@
         and that language is among the user's languages, we do not render
         the language control because there are no choices to be made.
         """
+        if not check_permission('launchpad.View', self.context):
+            return False
         languages = list(self.context_question_languages)
         if len(languages) == 0:
             return False

=== modified file 'lib/lp/blueprints/browser/specificationtarget.py'
--- lib/lp/blueprints/browser/specificationtarget.py	2012-01-01 02:58:52 +0000
+++ lib/lp/blueprints/browser/specificationtarget.py	2012-02-10 12:28:45 +0000
@@ -342,6 +342,8 @@
 
     @property
     def specs(self):
+        if not check_permission('launchpad.View', self.context):
+            return []
         filter = self.spec_filter
         return self.context.specifications(filter=filter)
 

=== modified file 'lib/lp/registry/browser/tests/test_team.py'
--- lib/lp/registry/browser/tests/test_team.py	2012-02-03 06:36:31 +0000
+++ lib/lp/registry/browser/tests/test_team.py	2012-02-10 12:28:45 +0000
@@ -493,7 +493,7 @@
             personset, name=self.view_name, principal=admin)
         self.assertIn(
             'visibility', [field.__name__ for field in view.form_fields])
-        
+
     def test_random_does_not_see_visibility_field_with_flag(self):
         personset = getUtility(IPersonSet)
         person = self.factory.makePerson()
@@ -519,7 +519,6 @@
                     'visibility',
                     [field.__name__ for field in view.form_fields])
 
-
     def test_person_with_expired_cs_does_not_see_visibility(self):
         personset = getUtility(IPersonSet)
         team = self.factory.makeTeam(
@@ -778,15 +777,20 @@
             archive = self.factory.makeArchive(private=True, owner=team)
             archive.newSubscription(user, registrant=owner)
         with person_logged_in(user):
-            view = create_initialized_view(
-                team, name="+index",  server_url=canonical_url(team),
-                path_info='', principal=user)
-            document = find_tag_by_id(view(), 'document')
-        self.assertIsNone(document.find(True, id='side-portlets'))
-        self.assertIsNone(document.find(True, id='registration'))
-        self.assertEndsWith(
-            extract_text(document.find(True, id='maincontent')),
-            'The information in this page is not shared with you.')
+            for rootsite, view_name in [
+                (None, '+index'), ('code', '+branches'), ('bugs', '+bugs'),
+                ('blueprints', '+specs'), ('answers', '+questions'),
+                ('translations', '+translations')]:
+                view = create_initialized_view(
+                    team, name=view_name, path_info='', principal=user,
+                    server_url=canonical_url(team, rootsite=rootsite),
+                    rootsite=rootsite)
+                document = find_tag_by_id(view(), 'document')
+                self.assertIsNone(document.find(True, id='side-portlets'))
+                self.assertIsNone(document.find(True, id='registration'))
+                self.assertEndsWith(
+                    extract_text(document.find(True, id='maincontent')),
+                    'The information in this page is not shared with you.')
 
 
 class TestPersonIndexVisibilityView(TestCaseWithFactory):