launchpad-reviewers team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxx>]

Martin Pool has proposed merging lp:~mbp/launchpad/feature-admin-party into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #790025 in Launchpad itself: "provide LP developers full access to QAS/Staging feature flags"
  https://bugs.launchpad.net/launchpad/+bug/790025

For more details, see:
https://code.launchpad.net/~mbp/launchpad/feature-admin-party/+merge/84044

per bug 790025, this lets members of ~registry edit things on demo instances.

It also deduplicates and separates out a test fixture for demo instances.

Of course I welcome feedback on how the security is done.  I probably could have hooked this in zcml or config, but it seemed no easier to change and to me less clear.
-- 
https://code.launchpad.net/~mbp/launchpad/feature-admin-party/+merge/84044
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/feature-admin-party into lp:launchpad.

=== modified file 'lib/canonical/launchpad/pagetests/basics/demo-and-lpnet.txt'
--- lib/canonical/launchpad/pagetests/basics/demo-and-lpnet.txt	2011-09-29 10:23:29 +0000
+++ lib/canonical/launchpad/pagetests/basics/demo-and-lpnet.txt	2011-12-01 01:32:29 +0000
@@ -27,15 +27,9 @@
 information and styles.
 
     # Set config to pretend we're on a demo site:
-    >>> from textwrap import dedent
-    >>> site_message = ('This is a demo site mmk. '
-    ...                 '<a href="http://example.com";>File a bug</a>.')
-    >>> test_data = dedent("""
-    ...     [launchpad]
-    ...     is_demo: True
-    ...     site_message: %s
-    ...     """ % site_message)
-    >>> config.push('test_data', test_data)
+    >>> from lp.testing.fixture import DemoMode
+    >>> demo_mode_fixture = DemoMode()
+    >>> demo_mode_fixture.setUp()
     >>> print config.launchpad.is_demo
     True
 
@@ -61,8 +55,7 @@
 
 When you are not on a demo site, the text no longer appears.
 
-    >>> # Restore the previous config:
-    >>> config_data = config.pop('test_data')
+    >>> demo_mode_fixture.cleanUp()
     >>> print config.launchpad.is_demo
     False
 

=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2011-11-28 05:14:57 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2011-12-01 01:32:29 +0000
@@ -27,6 +27,9 @@
     person_logged_in,
     TestCaseWithFactory,
     )
+from lp.testing.fixture import (
+    DemoMode,
+    )
 from lp.testing.mail_helpers import pop_notifications
 from lp.testing.service_usage_helpers import set_service_usage
 from lp.testing.views import (
@@ -164,10 +167,7 @@
         self.assertTrue(message is not None)
 
     def test_staging_message_is_demo(self):
-        config.push('staging-test', '''
-            [launchpad]
-            is_demo: true
-            ''')
+        self.useFixture(DemoMode())
         view = create_initialized_view(self.product_set, '+new')
         message = find_tag_by_id(view.render(), 'staging-message')
         self.assertEqual(None, message)

=== modified file 'lib/lp/services/features/browser/configure.zcml'
--- lib/lp/services/features/browser/configure.zcml	2011-02-16 22:03:30 +0000
+++ lib/lp/services/features/browser/configure.zcml	2011-12-01 01:32:29 +0000
@@ -1,5 +1,7 @@
-<!-- Copyright 2010 Canonical Ltd.  This software is licensed under the
-     GNU Affero General Public License version 3 (see the file LICENSE).
+<!-- Copyright 2010, 2011 Canonical Ltd.  
+
+     This software is licensed under the GNU Affero General Public License
+     version 3 (see the file LICENSE).
 -->
 
 <configure
@@ -13,7 +15,8 @@
 
      Readonly access is guarded by launchpad.Edit on ILaunchpadRoot, which
      limits it to ~admins + ~registry, which are all trusted users.  Write
-     access is for admins only.
+     access is for admins only on production sites, and also launchpad.Edit
+     on demo sites.
     -->
     <browser:page
         for="canonical.launchpad.webapp.interfaces.ILaunchpadRoot"

=== modified file 'lib/lp/services/features/browser/edit.py'
--- lib/lp/services/features/browser/edit.py	2011-03-23 16:28:51 +0000
+++ lib/lp/services/features/browser/edit.py	2011-12-01 01:32:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """View and edit feature rules."""
@@ -14,16 +14,20 @@
 import logging
 
 from zope.app.form.browser import TextAreaWidget
+from zope.component import getUtility
 from zope.interface import Interface
 from zope.schema import Text
 
+from canonical import config
 from canonical.launchpad.webapp.authorization import check_permission
+
 from lp.app.browser.launchpadform import (
     action,
     custom_widget,
     LaunchpadFormView,
     )
 from lp.app.browser.stringformatter import FormattersAPI
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.services.features.changelog import ChangeLog
 from lp.services.features.rulesource import DuplicatePriorityError
 
@@ -69,7 +73,16 @@
             return []
 
     def canSubmit(self, action):
-        """Is the user authorized to change the rules?"""
+        """Is the user authorized to change the rules?
+
+        On development machines, developers can change the rules; on
+        production machines only admins can change them.
+        """
+        if config.config['launchpad']['is_demo']:
+            celebrities = getUtility(ILaunchpadCelebrities)
+            if self.user.inTeam(celebrities.registry_experts):
+                return True
+        # Edit permission on the root object: an admin.
         return check_permission('launchpad.Admin', self.context)
 
     @action(u"Change", name="change", condition=canSubmit)

=== modified file 'lib/lp/services/features/browser/tests/test_feature_editor.py'
--- lib/lp/services/features/browser/tests/test_feature_editor.py	2011-05-27 21:12:25 +0000
+++ lib/lp/services/features/browser/tests/test_feature_editor.py	2011-12-01 01:32:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for feature rule editor"""
@@ -7,7 +7,16 @@
 
 from textwrap import dedent
 
-from testtools.matchers import Equals
+from soupmatchers import (
+    HTMLContains,
+    Tag,
+    )
+
+from testtools.matchers import (
+    Equals,
+    MatchesAll,
+    Not,
+    )
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
 
@@ -26,6 +35,7 @@
     BrowserTestCase,
     person_logged_in,
     )
+from lp.testing.fixture import DemoMode
 from lp.testing.matchers import Contains
 
 
@@ -54,7 +64,6 @@
 
     def getUserBrowserAsAdmin(self):
         """Make a new TestBrowser logged in as an admin user."""
-        url = self.getFeatureRulesViewURL()
         admin_team = getUtility(ILaunchpadCelebrities).admin
         return self.getUserBrowserAsTeamMember([admin_team])
 
@@ -102,18 +111,20 @@
             browser.open,
             self.getFeatureRulesViewURL())
 
-    def test_feature_page_can_view(self):
+    def test_feature_page_html_is_readonly(self):
         """User that can only view the rules do not see the form."""
         browser = self.getUserBrowserAsTeamMember(
             [getUtility(ILaunchpadCelebrities).registry_experts])
         browser.open(self.getFeatureRulesViewURL())
-        content = find_main_content(browser.contents)
-        self.assertEqual(
-            None, find_tag_by_id(content, 'field.feature_rules'))
-        self.assertEqual(
-            None, find_tag_by_id(content, 'field.actions.change'))
-        self.assertTrue(
-            find_tag_by_id(content, 'feature-rules'))
+        self.assertThat(
+            browser.contents,
+            MatchesAll(
+                Not(HTMLContains(Tag(
+                    'rules textarea', 'textarea',
+                    dict(id='field.feature_rules')))),
+                Not(HTMLContains(Tag(
+                    'submit button', 'input',
+                    dict(id='field.actions.change', type='submit'))))))
 
     def test_feature_page_submit_changes(self):
         """Submitted changes show up in the db."""
@@ -207,6 +218,19 @@
         view = FeatureControlView(None, None)
         self.assertFalse(view.change_action.available())
 
+    def test_feature_view_not_for_registry_on_lpnet(self):
+        """Registry cannot change features on lpnet"""
+        with person_logged_in(self.factory.makeRegistryExpert()):
+            view = FeatureControlView(None, None)
+            self.assertFalse(view.change_action.available())
+
+    def test_feature_view_for_registry_on_demo(self):
+        """Registry cannot change features on lpnet"""
+        self.useFixture(DemoMode())
+        with person_logged_in(self.factory.makeRegistryExpert()):
+            view = FeatureControlView(None, None)
+            self.assertTrue(view.change_action.available())
+
     def test_error_for_duplicate_priority(self):
         """Duplicate priority values for a flag result in a nice error."""
         browser = self.getUserBrowserAsAdmin()

=== modified file 'lib/lp/testing/fixture.py'
--- lib/lp/testing/fixture.py	2011-11-23 07:29:09 +0000
+++ lib/lp/testing/fixture.py	2011-12-01 01:32:29 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'CaptureOops',
+    'DemoMode',
     'PGBouncerFixture',
     'Urllib2Fixture',
     'ZopeAdapterFixture',
@@ -344,3 +345,21 @@
         self.timeline = get_request_timeline(
             get_current_browser_request())
         self.addCleanup(webapp.adapter.clear_request_started)
+
+
+class DemoMode(Fixture):
+    """Run with an is_demo configuration.
+
+    This changes the page styling, feature flag permissions, and perhaps
+    other things.
+    """
+
+    def setUp(self):
+        Fixture.setUp(self)
+        config.push('demo-fixture', '''
+[launchpad]
+is_demo: true
+site_message = This is a demo site mmk. \
+<a href="http://example.com";>File a bug</a>.
+            ''')
+        self.addCleanup(lambda: config.pop('demo-fixture'))