From ac0aad67629cfaad66c92179e5d1cce26bdaf3c6 Mon Sep 17 00:00:00 2001 From: jensens Date: Wed, 26 Feb 2020 23:04:48 +0100 Subject: [PATCH] [fc] Repository: plone.app.redirector Branch: refs/heads/master Date: 2020-02-14T18:57:04+01:00 Author: ale-rt (ale-rt) Commit: https://github.com/plone/plone.app.redirector/commit/22399a60679349a5dda17e567c9a55c53eef0e6c Fix http status of the response The http status of the response is changed from 301 (Moved Permanently) to 302 (Found) for GET requests and to 307 (Temporary Redirect) for other request methods because nothing prevents the URL to be reused in the future. This was inspired by https://github.com/plone/plone.rest/pull/76 Refs #8 Files changed: A news/8.feature M plone/app/redirector/browser.py M plone/app/redirector/tests/test_view.py Repository: plone.app.redirector Branch: refs/heads/master Date: 2020-02-26T23:04:48+01:00 Author: Jens W. Klein (jensens) Commit: https://github.com/plone/plone.app.redirector/commit/2f6da2972385650800cd7a3b2c4db9bc224a81c9 Merge pull request #22 from plone/8-http-status Fix http status of the response Files changed: A news/8.feature M plone/app/redirector/browser.py M plone/app/redirector/tests/test_view.py --- last_commit.txt | 51 ++++++++++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/last_commit.txt b/last_commit.txt index a87a3cd5bc..d222692bb4 100644 --- a/last_commit.txt +++ b/last_commit.txt @@ -1,38 +1,45 @@ -Repository: Products.CMFPlone +Repository: plone.app.redirector -Branch: refs/heads/5.2.x -Date: 2020-02-25T14:54:29+01:00 -Author: Maurits van Rees (mauritsvanrees) -Commit: https://github.com/plone/Products.CMFPlone/commit/2f71b68ec99bf06e68a54249c027ded8999de282 +Branch: refs/heads/master +Date: 2020-02-14T18:57:04+01:00 +Author: ale-rt (ale-rt) +Commit: https://github.com/plone/plone.app.redirector/commit/22399a60679349a5dda17e567c9a55c53eef0e6c -Fixed TypeError when adding both a group and a user to a group. +Fix http status of the response -Fixes https://github.com/plone/Products.CMFPlone/issues/3048 +The http status of the response is changed from 301 (Moved Permanently) +to 302 (Found) for GET requests and to 307 (Temporary Redirect) for +other request methods because nothing prevents the URL to be reused in +the future. + +This was inspired by https://github.com/plone/plone.rest/pull/76 + +Refs #8 Files changed: -A news/3084.bugfix -M Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py -M Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py +A news/8.feature +M plone/app/redirector/browser.py +M plone/app/redirector/tests/test_view.py -b'diff --git a/Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py b/Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py\nindex d41793362a..5720cbd01c 100644\n--- a/Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py\n+++ b/Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py\n@@ -83,16 +83,21 @@ def isGroup(self, itemName):\n def getMembers(self):\n searchResults = self.gtool.getGroupMembers(self.groupname)\n \n- groupResults = [self.gtool.getGroupById(m) for m in searchResults]\n- groupResults.sort(key=lambda x: x is not None and normalizeString(\n- x.getGroupTitleOrName()))\n-\n- userResults = [self.mtool.getMemberById(m) for m in searchResults]\n- userResults.sort(key=lambda x: x is not None and x.getProperty(\n- \'fullname\') is not None and normalizeString(x.getProperty(\'fullname\')) or \'\')\n-\n- mergedResults = groupResults + userResults\n- return [i for i in mergedResults if i]\n+ groupResults = []\n+ userResults = []\n+ for principal_id in searchResults:\n+ principal = self.gtool.getGroupById(principal_id)\n+ if principal is not None:\n+ groupResults.append(principal)\n+ continue\n+ principal = self.mtool.getMemberById(principal_id)\n+ if principal is not None:\n+ userResults.append(principal)\n+\n+ groupResults.sort(key=lambda x: normalizeString(x.getGroupTitleOrName()))\n+ userResults.sort(key=lambda x: normalizeString(x.getProperty(\'fullname\') or \'\'))\n+\n+ return groupResults + userResults\n \n def getPotentialMembers(self, searchString):\n ignoredUsersGroups = [\ndiff --git a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py\nindex 6376fadd8f..b8af1888bf 100644\n--- a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py\n+++ b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py\n@@ -370,6 +370,22 @@ def test_group_add_group(self):\n \'Add selected groups and users to this group\').click()\n self.assertIn(\'Group 2\', self.browser.contents)\n \n+ # Check that you can still add a user too. This failed at some point:\n+ # https://github.com/plone/Products.CMFPlone/issues/3048\n+ # Add user (Autumn Brooks) to selected group (Group 1)\n+ self.browser.getControl(name=\'searchstring\').value = \'TWrMCLIo\'\n+ self.browser.getControl(name=\'form.button.Search\').click()\n+ self.browser.getControl(name=\'add:list\').getControl(\n+ value=\'TWrMCLIo\').selected = True\n+ self.browser.getControl(\n+ \'Add selected groups and users to this group\').click()\n+\n+ # Check that both group and user are now part of the group\n+ self.browser.open(self.groups_url)\n+ self.browser.getLink(\'Group 1 (group1)\').click()\n+ self.assertIn(\'Autumn Brooks\', self.browser.contents)\n+ self.assertIn(\'Group 2\', self.browser.contents)\n+\n def test_usergroups_settings_many_users(self):\n self.browser.open("%s/@@usergroup-controlpanel" % self.portal_url)\n self.browser.getControl(\ndiff --git a/news/3084.bugfix b/news/3084.bugfix\nnew file mode 100644\nindex 0000000000..a223513a84\n--- /dev/null\n+++ b/news/3084.bugfix\n@@ -0,0 +1,2 @@\n+Fixed TypeError when adding both a group and a user to a group.\n+[maurits]\n' +b'diff --git a/news/8.feature b/news/8.feature\nnew file mode 100644\nindex 0000000..014a205\n--- /dev/null\n+++ b/news/8.feature\n@@ -0,0 +1 @@\n+The http status of the response is changed from 301 (Moved Permanently) to 302 (Found) for GET requests and to 307 (Temporary Redirect) for other request methods because nothing prevents the URL to be reused in the future. [ale-rt]\ndiff --git a/plone/app/redirector/browser.py b/plone/app/redirector/browser.py\nindex 97af8c3..bf782ec 100644\n--- a/plone/app/redirector/browser.py\n+++ b/plone/app/redirector/browser.py\n@@ -84,7 +84,17 @@ def attempt_redirect(self):\n # some analytics programs might use this info to track\n if query_string:\n url += "?" + query_string\n- self.request.response.redirect(url, status=301, lock=1)\n+\n+ # Answer GET requests with 302 (Found). Every other method will be answered\n+ # with 307 (Temporary Redirect), which instructs the client to NOT\n+ # switch the method (if the original request was a POST, it should\n+ # re-POST to the new URL from the Location header).\n+ if self.request.method.upper() == "GET":\n+ status = 302\n+ else:\n+ status = 307\n+\n+ self.request.response.redirect(url, status=status, lock=1)\n return True\n \n def find_redirect_if_view(self, old_path_elements, storage):\ndiff --git a/plone/app/redirector/tests/test_view.py b/plone/app/redirector/tests/test_view.py\nindex e6326c0..295c8be 100644\n--- a/plone/app/redirector/tests/test_view.py\n+++ b/plone/app/redirector/tests/test_view.py\n@@ -38,7 +38,7 @@ def test_attempt_redirect_with_known_url(self):\n self.storage.add(fp + \'/foo\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar\',\n self.request.response.getHeader(\'location\'))\n \n@@ -48,7 +48,7 @@ def test_attempt_redirect_with_known_url_and_template(self):\n self.storage.add(fp + \'/foo\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo/view\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar/view\',\n self.request.response.getHeader(\'location\'))\n \n@@ -58,7 +58,7 @@ def test_attempt_redirect_with_known_url_and_view_with_part(self):\n self.storage.add(fp + \'/foo\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo/@@view/part\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar/@@view/part\',\n self.request.response.getHeader(\'location\'))\n \n@@ -66,13 +66,13 @@ def test_attempt_redirect_with_unknown_url(self):\n fu = self.folder.absolute_url()\n view = self.view(self.portal, fu + \'/foo\')\n self.assertEqual(False, view.attempt_redirect())\n- self.assertNotEqual(301, self.request.response.getStatus())\n+ self.assertNotEqual(302, self.request.response.getStatus())\n \n def test_attempt_redirect_with_unknown_url_with_illegal_characters(self):\n fu = self.folder.absolute_url()\n view = self.view(self.portal, fu + \'+L\xc3\x83\xc2\xa4nder\')\n self.assertEqual(False, view.attempt_redirect())\n- self.assertNotEqual(301, self.request.response.getStatus())\n+ self.assertNotEqual(302, self.request.response.getStatus())\n \n def test_attempt_redirect_with_quoted_url(self):\n fp = \'/\'.join(self.folder.getPhysicalPath())\n@@ -80,7 +80,7 @@ def test_attempt_redirect_with_quoted_url(self):\n self.storage.add(fp + \'/foo\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo/baz%20quux\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar/baz%20quux\',\n self.request.response.getHeader(\'location\'))\n \n@@ -90,7 +90,7 @@ def test_attempt_redirect_with_query_string(self):\n self.storage.add(fp + \'/foo?blah=blah\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo\', \'blah=blah\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar\',\n self.request.response.getHeader(\'location\'))\n \n@@ -100,7 +100,7 @@ def test_attempt_redirect_appending_query_string(self):\n self.storage.add(fp + \'/foo\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo\', \'blah=blah\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar?blah=blah\',\n self.request.response.getHeader(\'location\'))\n \n@@ -111,7 +111,7 @@ def test_attempt_redirect_with_external_url(self):\n \'http://otherhost\' + fp + \'/bar%20qux corge\')\n view = self.view(self.portal, fu + \'/foo\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(\'http://otherhost\' + fp + \'/bar%20qux%20corge\',\n self.request.response.getHeader(\'location\'))\n \n' -Repository: Products.CMFPlone +Repository: plone.app.redirector -Branch: refs/heads/5.2.x -Date: 2020-02-26T16:54:26+01:00 -Author: Maurits van Rees (mauritsvanrees) -Commit: https://github.com/plone/Products.CMFPlone/commit/8ac3b1f11a67d03e9b4d6981bf7c48361a703e1b +Branch: refs/heads/master +Date: 2020-02-26T23:04:48+01:00 +Author: Jens W. Klein (jensens) +Commit: https://github.com/plone/plone.app.redirector/commit/2f6da2972385650800cd7a3b2c4db9bc224a81c9 -Merge pull request #3054 from plone/maurits/issue-3048-add-user-and-group-52 +Merge pull request #22 from plone/8-http-status -Fixed TypeError when adding both a group and a user to a group. [5.2] +Fix http status of the response Files changed: -A news/3084.bugfix -M Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py -M Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py +A news/8.feature +M plone/app/redirector/browser.py +M plone/app/redirector/tests/test_view.py -b'diff --git a/Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py b/Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py\nindex d41793362a..5720cbd01c 100644\n--- a/Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py\n+++ b/Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py\n@@ -83,16 +83,21 @@ def isGroup(self, itemName):\n def getMembers(self):\n searchResults = self.gtool.getGroupMembers(self.groupname)\n \n- groupResults = [self.gtool.getGroupById(m) for m in searchResults]\n- groupResults.sort(key=lambda x: x is not None and normalizeString(\n- x.getGroupTitleOrName()))\n-\n- userResults = [self.mtool.getMemberById(m) for m in searchResults]\n- userResults.sort(key=lambda x: x is not None and x.getProperty(\n- \'fullname\') is not None and normalizeString(x.getProperty(\'fullname\')) or \'\')\n-\n- mergedResults = groupResults + userResults\n- return [i for i in mergedResults if i]\n+ groupResults = []\n+ userResults = []\n+ for principal_id in searchResults:\n+ principal = self.gtool.getGroupById(principal_id)\n+ if principal is not None:\n+ groupResults.append(principal)\n+ continue\n+ principal = self.mtool.getMemberById(principal_id)\n+ if principal is not None:\n+ userResults.append(principal)\n+\n+ groupResults.sort(key=lambda x: normalizeString(x.getGroupTitleOrName()))\n+ userResults.sort(key=lambda x: normalizeString(x.getProperty(\'fullname\') or \'\'))\n+\n+ return groupResults + userResults\n \n def getPotentialMembers(self, searchString):\n ignoredUsersGroups = [\ndiff --git a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py\nindex 6376fadd8f..b8af1888bf 100644\n--- a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py\n+++ b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_usergroups.py\n@@ -370,6 +370,22 @@ def test_group_add_group(self):\n \'Add selected groups and users to this group\').click()\n self.assertIn(\'Group 2\', self.browser.contents)\n \n+ # Check that you can still add a user too. This failed at some point:\n+ # https://github.com/plone/Products.CMFPlone/issues/3048\n+ # Add user (Autumn Brooks) to selected group (Group 1)\n+ self.browser.getControl(name=\'searchstring\').value = \'TWrMCLIo\'\n+ self.browser.getControl(name=\'form.button.Search\').click()\n+ self.browser.getControl(name=\'add:list\').getControl(\n+ value=\'TWrMCLIo\').selected = True\n+ self.browser.getControl(\n+ \'Add selected groups and users to this group\').click()\n+\n+ # Check that both group and user are now part of the group\n+ self.browser.open(self.groups_url)\n+ self.browser.getLink(\'Group 1 (group1)\').click()\n+ self.assertIn(\'Autumn Brooks\', self.browser.contents)\n+ self.assertIn(\'Group 2\', self.browser.contents)\n+\n def test_usergroups_settings_many_users(self):\n self.browser.open("%s/@@usergroup-controlpanel" % self.portal_url)\n self.browser.getControl(\ndiff --git a/news/3084.bugfix b/news/3084.bugfix\nnew file mode 100644\nindex 0000000000..a223513a84\n--- /dev/null\n+++ b/news/3084.bugfix\n@@ -0,0 +1,2 @@\n+Fixed TypeError when adding both a group and a user to a group.\n+[maurits]\n' +b'diff --git a/news/8.feature b/news/8.feature\nnew file mode 100644\nindex 0000000..014a205\n--- /dev/null\n+++ b/news/8.feature\n@@ -0,0 +1 @@\n+The http status of the response is changed from 301 (Moved Permanently) to 302 (Found) for GET requests and to 307 (Temporary Redirect) for other request methods because nothing prevents the URL to be reused in the future. [ale-rt]\ndiff --git a/plone/app/redirector/browser.py b/plone/app/redirector/browser.py\nindex 97af8c3..bf782ec 100644\n--- a/plone/app/redirector/browser.py\n+++ b/plone/app/redirector/browser.py\n@@ -84,7 +84,17 @@ def attempt_redirect(self):\n # some analytics programs might use this info to track\n if query_string:\n url += "?" + query_string\n- self.request.response.redirect(url, status=301, lock=1)\n+\n+ # Answer GET requests with 302 (Found). Every other method will be answered\n+ # with 307 (Temporary Redirect), which instructs the client to NOT\n+ # switch the method (if the original request was a POST, it should\n+ # re-POST to the new URL from the Location header).\n+ if self.request.method.upper() == "GET":\n+ status = 302\n+ else:\n+ status = 307\n+\n+ self.request.response.redirect(url, status=status, lock=1)\n return True\n \n def find_redirect_if_view(self, old_path_elements, storage):\ndiff --git a/plone/app/redirector/tests/test_view.py b/plone/app/redirector/tests/test_view.py\nindex e6326c0..295c8be 100644\n--- a/plone/app/redirector/tests/test_view.py\n+++ b/plone/app/redirector/tests/test_view.py\n@@ -38,7 +38,7 @@ def test_attempt_redirect_with_known_url(self):\n self.storage.add(fp + \'/foo\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar\',\n self.request.response.getHeader(\'location\'))\n \n@@ -48,7 +48,7 @@ def test_attempt_redirect_with_known_url_and_template(self):\n self.storage.add(fp + \'/foo\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo/view\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar/view\',\n self.request.response.getHeader(\'location\'))\n \n@@ -58,7 +58,7 @@ def test_attempt_redirect_with_known_url_and_view_with_part(self):\n self.storage.add(fp + \'/foo\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo/@@view/part\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar/@@view/part\',\n self.request.response.getHeader(\'location\'))\n \n@@ -66,13 +66,13 @@ def test_attempt_redirect_with_unknown_url(self):\n fu = self.folder.absolute_url()\n view = self.view(self.portal, fu + \'/foo\')\n self.assertEqual(False, view.attempt_redirect())\n- self.assertNotEqual(301, self.request.response.getStatus())\n+ self.assertNotEqual(302, self.request.response.getStatus())\n \n def test_attempt_redirect_with_unknown_url_with_illegal_characters(self):\n fu = self.folder.absolute_url()\n view = self.view(self.portal, fu + \'+L\xc3\x83\xc2\xa4nder\')\n self.assertEqual(False, view.attempt_redirect())\n- self.assertNotEqual(301, self.request.response.getStatus())\n+ self.assertNotEqual(302, self.request.response.getStatus())\n \n def test_attempt_redirect_with_quoted_url(self):\n fp = \'/\'.join(self.folder.getPhysicalPath())\n@@ -80,7 +80,7 @@ def test_attempt_redirect_with_quoted_url(self):\n self.storage.add(fp + \'/foo\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo/baz%20quux\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar/baz%20quux\',\n self.request.response.getHeader(\'location\'))\n \n@@ -90,7 +90,7 @@ def test_attempt_redirect_with_query_string(self):\n self.storage.add(fp + \'/foo?blah=blah\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo\', \'blah=blah\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar\',\n self.request.response.getHeader(\'location\'))\n \n@@ -100,7 +100,7 @@ def test_attempt_redirect_appending_query_string(self):\n self.storage.add(fp + \'/foo\', fp + \'/bar\')\n view = self.view(self.portal, fu + \'/foo\', \'blah=blah\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(fu + \'/bar?blah=blah\',\n self.request.response.getHeader(\'location\'))\n \n@@ -111,7 +111,7 @@ def test_attempt_redirect_with_external_url(self):\n \'http://otherhost\' + fp + \'/bar%20qux corge\')\n view = self.view(self.portal, fu + \'/foo\')\n self.assertEqual(True, view.attempt_redirect())\n- self.assertEqual(301, self.request.response.getStatus())\n+ self.assertEqual(302, self.request.response.getStatus())\n self.assertEqual(\'http://otherhost\' + fp + \'/bar%20qux%20corge\',\n self.request.response.getHeader(\'location\'))\n \n'