launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #29360
[Merge] ~andrey-fedoseev/launchpad:edit-feature-rules into launchpad:master
Andrey Fedoseev has proposed merging ~andrey-fedoseev/launchpad:edit-feature-rules into launchpad:master.
Commit message:
Allow members of `launchpad` team to edit feature rules
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~andrey-fedoseev/launchpad/+git/launchpad/+merge/432249
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~andrey-fedoseev/launchpad:edit-feature-rules into launchpad:master.
diff --git a/lib/lp/permissions.zcml b/lib/lp/permissions.zcml
index dd5a177..0e45a78 100644
--- a/lib/lp/permissions.zcml
+++ b/lib/lp/permissions.zcml
@@ -110,4 +110,14 @@
title="Special permission to allow only scripts to write."
access_level="write" />
+ <!-- Permissions for feature rules, see `lp.services.features` -->
+ <permission
+ id="launchpad.ViewFeatureRules"
+ title="View feature rules."
+ access_level="read" />
+ <permission
+ id="launchpad.EditFeatureRules"
+ title="Edit feature rules."
+ access_level="write" />
+
</configure>
diff --git a/lib/lp/services/features/browser/configure.zcml b/lib/lp/services/features/browser/configure.zcml
index d858fca..0511992 100644
--- a/lib/lp/services/features/browser/configure.zcml
+++ b/lib/lp/services/features/browser/configure.zcml
@@ -9,36 +9,27 @@
xmlns:xmlrpc="http://namespaces.zope.org/xmlrpc";
i18n_domain="launchpad">
- <!-- View or edit all feature rules.
-
- 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.
- -->
+ <!-- View or edit all feature rules. -->
<browser:page
for="lp.services.webapp.interfaces.ILaunchpadRoot"
class="lp.services.features.browser.edit.FeatureControlView"
name="+feature-rules"
- permission="launchpad.Edit"
+ permission="launchpad.ViewFeatureRules"
template="../templates/feature-rules.pt"/>
- <!-- View documentary info about the available feature flags.
-
- Access is guarded by launchpad.Edit on ILaunchpadRoot, just like
- +feature-rules.
- -->
+ <!-- View documentary info about the available feature flags. -->
<browser:page
for="lp.services.webapp.interfaces.ILaunchpadRoot"
class="lp.services.features.browser.info.FeatureInfoView"
name="+feature-info"
- permission="launchpad.Edit"
+ permission="launchpad.ViewFeatureRules"
template="../templates/feature-info.pt"/>
<browser:page
for="lp.services.webapp.interfaces.ILaunchpadRoot"
class="lp.services.features.browser.changelog.FeatureChangeLogView"
name="+feature-changelog"
- permission="launchpad.Edit"
+ permission="launchpad.ViewFeatureRules"
template="../templates/feature-changelog.pt"/>
</configure>
diff --git a/lib/lp/services/features/browser/edit.py b/lib/lp/services/features/browser/edit.py
index 3acc671..36ea7a3 100644
--- a/lib/lp/services/features/browser/edit.py
+++ b/lib/lp/services/features/browser/edit.py
@@ -69,7 +69,7 @@ class FeatureControlView(LaunchpadFormView):
def canSubmit(self, action):
"""Is the user authorized to change the rules?"""
- return check_permission("launchpad.Admin", self.context)
+ return check_permission("launchpad.EditFeatureRules", self.context)
@action("Change", name="change", condition=canSubmit)
def change_action(self, action, data):
diff --git a/lib/lp/services/features/browser/tests/test_feature_editor.py b/lib/lp/services/features/browser/tests/test_feature_editor.py
index a6fb812..98262d5 100644
--- a/lib/lp/services/features/browser/tests/test_feature_editor.py
+++ b/lib/lp/services/features/browser/tests/test_feature_editor.py
@@ -30,6 +30,27 @@ class TestFeatureControlPage(BrowserTestCase):
def setUp(self):
super().setUp()
self.useFixture(FakeLogger())
+ self.celebrities = getUtility(ILaunchpadCelebrities)
+
+ @property
+ def admin_browser(self):
+ return self.getUserBrowserAsTeamMember([self.celebrities.admin])
+
+ @property
+ def launchpad_developer_browser(self):
+ return self.getUserBrowserAsTeamMember(
+ [self.celebrities.launchpad_developers]
+ )
+
+ @property
+ def registry_expert_browser(self):
+ return self.getUserBrowserAsTeamMember(
+ [self.celebrities.registry_experts]
+ )
+
+ @property
+ def unprivileged_browser(self):
+ return self.getUserBrowserAsTeamMember([])
def getUserBrowserAsTeamMember(self, teams):
"""Make a TestBrowser authenticated as a team member.
@@ -42,11 +63,6 @@ class TestFeatureControlPage(BrowserTestCase):
team.addMember(self.user, reviewer=team.teamowner)
return self.getUserBrowser(url=None, user=self.user)
- def getUserBrowserAsAdmin(self):
- """Make a new TestBrowser logged in as an admin user."""
- admin_team = getUtility(ILaunchpadCelebrities).admin
- return self.getUserBrowserAsTeamMember([admin_team])
-
def getFeatureRulesViewURL(self):
root = getUtility(ILaunchpadRoot)
return canonical_url(root, view_name="+feature-rules")
@@ -57,7 +73,7 @@ class TestFeatureControlPage(BrowserTestCase):
def test_feature_page_default_value(self):
"""No rules in the sampledata gives no content in the page"""
- browser = self.getUserBrowserAsAdmin()
+ browser = self.admin_browser
browser.open(self.getFeatureRulesViewURL())
textarea = browser.getControl(name="field.feature_rules")
# and by default, since there are no rules in the sample data, it's
@@ -71,7 +87,7 @@ class TestFeatureControlPage(BrowserTestCase):
("ui.icing", "beta_user", 300, "4.0"),
]
)
- browser = self.getUserBrowserAsAdmin()
+ browser = self.admin_browser
browser.open(self.getFeatureRulesViewURL())
textarea = browser.getControl(name="field.feature_rules")
self.assertThat(
@@ -87,27 +103,32 @@ class TestFeatureControlPage(BrowserTestCase):
Unauthorized, browser.open, self.getFeatureRulesViewURL()
)
- def test_feature_rules_plebian_unauthorized(self):
+ def test_feature_rules_unprivileged_unauthorized(self):
"""Logged in, but not a member of any interesting teams."""
- browser = self.getUserBrowserAsTeamMember([])
+ browser = self.unprivileged_browser
self.assertRaises(
Unauthorized, browser.open, self.getFeatureRulesViewURL()
)
def test_feature_page_can_view(self):
"""User that can only view the rules do not see the form."""
- browser = self.getUserBrowserAsTeamMember(
- [getUtility(ILaunchpadCelebrities).registry_experts]
- )
+ browser = self.registry_expert_browser
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"))
- def test_feature_page_submit_changes(self):
+ def test_feature_page_can_submit_changes_admin(self):
+ browser = self.admin_browser
+ self._test_feature_page_submit_changes(browser)
+
+ def test_feature_page_can_submit_changes_lp_developer(self):
+ browser = self.launchpad_developer_browser
+ self._test_feature_page_submit_changes(browser)
+
+ def _test_feature_page_submit_changes(self, browser):
"""Submitted changes show up in the db."""
- browser = self.getUserBrowserAsAdmin()
browser.open(self.getFeatureRulesEditURL())
new_value = "beta_user some_key 10 some value with spaces"
textarea = browser.getControl(name="field.feature_rules")
@@ -132,7 +153,7 @@ class TestFeatureControlPage(BrowserTestCase):
def test_change_message(self):
"""Submitting shows a message that the changes have been applied."""
- browser = self.getUserBrowserAsAdmin()
+ browser = self.admin_browser
browser.open(self.getFeatureRulesEditURL())
textarea = browser.getControl(name="field.feature_rules")
textarea.value = "beta_user some_key 10 some value with spaces"
@@ -144,7 +165,7 @@ class TestFeatureControlPage(BrowserTestCase):
def test_change_diff(self):
"""Submitting shows a diff of the changes."""
- browser = self.getUserBrowserAsAdmin()
+ browser = self.admin_browser
browser.open(self.getFeatureRulesEditURL())
browser.getControl(
name="field.feature_rules"
@@ -173,7 +194,7 @@ class TestFeatureControlPage(BrowserTestCase):
def test_change_logging_note(self):
"""When submitting changes the name of the logger is shown."""
- browser = self.getUserBrowserAsAdmin()
+ browser = self.admin_browser
browser.open(self.getFeatureRulesEditURL())
browser.getControl(
name="field.feature_rules"
@@ -189,7 +210,7 @@ class TestFeatureControlPage(BrowserTestCase):
"""Correctly handle submitting an empty value."""
# Zope has the quirk of conflating empty with absent; make sure we
# handle it properly.
- browser = self.getUserBrowserAsAdmin()
+ browser = self.admin_browser
browser.open(self.getFeatureRulesEditURL())
new_value = ""
textarea = browser.getControl(name="field.feature_rules")
@@ -208,7 +229,7 @@ class TestFeatureControlPage(BrowserTestCase):
def test_error_for_duplicate_priority(self):
"""Duplicate priority values for a flag result in a nice error."""
- browser = self.getUserBrowserAsAdmin()
+ browser = self.admin_browser
browser.open(self.getFeatureRulesEditURL())
textarea = browser.getControl(name="field.feature_rules")
textarea.value = dedent(
diff --git a/lib/lp/services/features/configure.zcml b/lib/lp/services/features/configure.zcml
index 20ba273..57937ec 100644
--- a/lib/lp/services/features/configure.zcml
+++ b/lib/lp/services/features/configure.zcml
@@ -6,6 +6,8 @@
xmlns="http://namespaces.zope.org/zope";
xmlns:browser="http://namespaces.zope.org/browser";>
+ <authorizations module=".security" />
+
<include
package=".browser"/>
diff --git a/lib/lp/services/features/security.py b/lib/lp/services/features/security.py
new file mode 100644
index 0000000..b0c5458
--- /dev/null
+++ b/lib/lp/services/features/security.py
@@ -0,0 +1,42 @@
+# Copyright 2022 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+from zope.component import getUtility
+
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.app.security import AuthorizationBase
+from lp.services.webapp.interfaces import ILaunchpadRoot
+
+
+class ViewFeatureRules(AuthorizationBase):
+ """
+ A person with permission to edit `ILaunchpadRoot`
+ (members of ~admin or ~registry), or members of `~launchpad`
+ can view the feature rules
+ """
+
+ permission = "launchpad.ViewFeatureRules"
+ usedfor = ILaunchpadRoot
+
+ def checkAuthenticated(self, user):
+ celebrities = getUtility(ILaunchpadCelebrities)
+ return self.forwardCheckAuthenticated(
+ user, permission="launchpad.Edit"
+ ) or user.inTeam(celebrities.launchpad_developers)
+
+
+class EditFeatureRules(AuthorizationBase):
+ """
+ Members of ~admin and ~launchpad can edit the feature rules (they must
+ also have permission to view the feature rules)
+ """
+
+ permission = "launchpad.EditFeatureRules"
+ usedfor = ILaunchpadRoot
+
+ def checkAuthenticated(self, user):
+ celebrities = getUtility(ILaunchpadCelebrities)
+ return self.forwardCheckAuthenticated(
+ user, permission="launchpad.ViewFeatureRules"
+ ) and user.inAnyTeam(
+ (celebrities.admin, celebrities.launchpad_developers)
+ )
Follow ups
