From 901b668cfbfe93f393a61e2f02d673804a4f2fe6 Mon Sep 17 00:00:00 2001 From: davisagli Date: Wed, 6 Sep 2023 21:44:09 -0700 Subject: [PATCH] [fc] Repository: plone.session Branch: refs/heads/master Date: 2023-09-04T12:22:31+02:00 Author: Maurits van Rees (mauritsvanrees) Commit: https://github.com/plone/plone.session/commit/a093d48b1ff81e715e6836170c1f87a3f8da284a Do not set an auth cookie after password reset, unless the user is authenticated. Otherwise anonymous users will be logged in immediately, even when autologin after password reset is false. Fixes https://github.com/plone/Products.CMFPlone/issues/3835. Files changed: A news/3835.bugfix M plone/session/plugins/session.py M plone/session/tests/testPAS.py Repository: plone.session Branch: refs/heads/master Date: 2023-09-06T16:25:43+02:00 Author: Maurits van Rees (mauritsvanrees) Commit: https://github.com/plone/plone.session/commit/2c9213c69edc3b5f08e33c167458e7283d071276 testRefresh: no need to logout first. Also, fix/clarify a few comments in the tests. Also, check that calling updateCredentials for ourselves *does* set a cookie. This seems the most obvious test to do, but we seem to have missed this. Files changed: M plone/session/tests/testPAS.py Repository: plone.session Branch: refs/heads/master Date: 2023-09-06T21:44:09-07:00 Author: David Glick (davisagli) Commit: https://github.com/plone/plone.session/commit/991f77bf5d4e33f12016234570b6ff6f6da56641 Merge pull request #44 from plone/maurits-do-not-update-credentials-for-anonymous-issue-3835 Do not set auth cookie after password reset, unless user is authenticated Files changed: A news/3835.bugfix M plone/session/plugins/session.py M plone/session/tests/testPAS.py --- last_commit.txt | 59 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/last_commit.txt b/last_commit.txt index 21146272f2..5c400eff78 100644 --- a/last_commit.txt +++ b/last_commit.txt @@ -1,38 +1,59 @@ -Repository: plone.app.event +Repository: plone.session Branch: refs/heads/master -Date: 2023-08-01T00:35:32Z -Author: pre-commit-ci[bot] (pre-commit-ci[bot]) <66853113+pre-commit-ci[bot]@users.noreply.github.com> -Commit: https://github.com/plone/plone.app.event/commit/e87ede2f0fa3a55aa20ae595f8c2eada675e8dec +Date: 2023-09-04T12:22:31+02:00 +Author: Maurits van Rees (mauritsvanrees) +Commit: https://github.com/plone/plone.session/commit/a093d48b1ff81e715e6836170c1f87a3f8da284a -[pre-commit.ci] pre-commit autoupdate +Do not set an auth cookie after password reset, unless the user is authenticated. -updates: -- [github.com/asottile/pyupgrade: v3.8.0 → v3.10.1](https://github.com/asottile/pyupgrade/compare/v3.8.0...v3.10.1) -- [github.com/psf/black: 23.3.0 → 23.7.0](https://github.com/psf/black/compare/23.3.0...23.7.0) -- [github.com/PyCQA/flake8: 6.0.0 → 6.1.0](https://github.com/PyCQA/flake8/compare/6.0.0...6.1.0) -- [github.com/collective/i18ndude: 6.0.0 → 6.1.0](https://github.com/collective/i18ndude/compare/6.0.0...6.1.0) +Otherwise anonymous users will be logged in immediately, even when autologin after password reset is false. +Fixes https://github.com/plone/Products.CMFPlone/issues/3835. Files changed: -M .pre-commit-config.yaml +A news/3835.bugfix +M plone/session/plugins/session.py +M plone/session/tests/testPAS.py -b'diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml\nindex 0d259b7f..f6f04834 100644\n--- a/.pre-commit-config.yaml\n+++ b/.pre-commit-config.yaml\n@@ -7,7 +7,7 @@ ci:\n \n repos:\n - repo: https://github.com/asottile/pyupgrade\n- rev: v3.8.0\n+ rev: v3.10.1\n hooks:\n - id: pyupgrade\n args: [--py38-plus]\n@@ -16,7 +16,7 @@ repos:\n hooks:\n - id: isort\n - repo: https://github.com/psf/black\n- rev: 23.3.0\n+ rev: 23.7.0\n hooks:\n - id: black\n - repo: https://github.com/collective/zpretty\n@@ -32,7 +32,7 @@ repos:\n # """\n ##\n - repo: https://github.com/PyCQA/flake8\n- rev: 6.0.0\n+ rev: 6.1.0\n hooks:\n - id: flake8\n - repo: https://github.com/codespell-project/codespell\n@@ -63,7 +63,7 @@ repos:\n - id: check-python-versions\n args: [\'--only\', \'setup.py,pyproject.toml\']\n - repo: https://github.com/collective/i18ndude\n- rev: "6.0.0"\n+ rev: "6.1.0"\n hooks:\n - id: i18ndude\n \n' +b'diff --git a/news/3835.bugfix b/news/3835.bugfix\nnew file mode 100644\nindex 0000000..28a7ddc\n--- /dev/null\n+++ b/news/3835.bugfix\n@@ -0,0 +1,4 @@\n+Do not set an auth cookie after password reset, unless the user is authenticated.\n+Otherwise anonymous users will be logged in immediately, even when autologin after password reset is false.\n+Fixes `issue 3835 `_.\n+[maurits]\ndiff --git a/plone/session/plugins/session.py b/plone/session/plugins/session.py\nindex 803087c..bd49f83 100644\n--- a/plone/session/plugins/session.py\n+++ b/plone/session/plugins/session.py\n@@ -281,8 +281,14 @@ def updateCredentials(self, request, response, login, new_password):\n authenticated_user = getSecurityManager().getUser()\n if authenticated_user is not None:\n authenticated_id = authenticated_user.getId()\n- # For anonymous, the id is empty\n- if authenticated_id and authenticated_id != user_id:\n+ # For anonymous, the id is empty.\n+ if not authenticated_id:\n+ # We should not update credentials when there are none currently.\n+ # Otherwise for example you are logged in after a password reset,\n+ # even when autologin after password reset is false.\n+ # See https://github.com/plone/Products.CMFPlone/issues/3835\n+ return\n+ if authenticated_id != user_id:\n return\n self._setupSession(user_id, response)\n \ndiff --git a/plone/session/tests/testPAS.py b/plone/session/tests/testPAS.py\nindex 8872042..9e9b2d1 100644\n--- a/plone/session/tests/testPAS.py\n+++ b/plone/session/tests/testPAS.py\n@@ -117,7 +117,10 @@ def testCredentialsUpdateAnonymous(self):\n session = self.folder.pas.session\n request = self.makeRequest("test string")\n session.updateCredentials(request, request.response, "our_user", "password")\n- self.assertIsNotNone(\n+ # The anonymous user should not get a cookie: resetCredentials should\n+ # not do anything when there are no current credentials.\n+ # See https://github.com/plone/Products.CMFPlone/issues/3835\n+ self.assertIsNone(\n request.response.getCookie(session.cookie_name),\n )\n \n@@ -133,7 +136,7 @@ def testRefresh(self):\n logout()\n session = self.folder.pas.session\n request = self.makeRequest("test string")\n- session.updateCredentials(request, request.response, "our_user", "password")\n+ session._setupSession(self.userid, request.response)\n cookie = request.response.getCookie(session.cookie_name)["value"]\n request2 = self.makeRequest(cookie)\n request2.form["type"] = "gif"\n' -Repository: plone.app.event +Repository: plone.session Branch: refs/heads/master -Date: 2023-09-04T09:56:55+02:00 +Date: 2023-09-06T16:25:43+02:00 Author: Maurits van Rees (mauritsvanrees) -Commit: https://github.com/plone/plone.app.event/commit/cd63cfff52f803951bb022133c2905357bfc196a +Commit: https://github.com/plone/plone.session/commit/2c9213c69edc3b5f08e33c167458e7283d071276 + +testRefresh: no need to logout first. + +Also, fix/clarify a few comments in the tests. + +Also, check that calling updateCredentials for ourselves *does* set a cookie. +This seems the most obvious test to do, but we seem to have missed this. + +Files changed: +M plone/session/tests/testPAS.py + +b'diff --git a/plone/session/tests/testPAS.py b/plone/session/tests/testPAS.py\nindex 9e9b2d1..c3cf6ec 100644\n--- a/plone/session/tests/testPAS.py\n+++ b/plone/session/tests/testPAS.py\n@@ -102,7 +102,9 @@ def testExtraction(self):\n self.assertEqual(creds, {})\n \n def testCredentialsUpdateUnknownUser(self):\n- # We are logged in as test user, which we do not want.\n+ # Check that calling updateCredentials for an *unknown* user does not set a\n+ # cookie if there is no cookie with credentials yet (you are anonymous).\n+ # So first logout.\n logout()\n session = self.folder.pas.session\n request = self.makeRequest("test string")\n@@ -112,12 +114,14 @@ def testCredentialsUpdateUnknownUser(self):\n self.assertIsNone(request.response.getCookie(session.cookie_name))\n \n def testCredentialsUpdateAnonymous(self):\n- # We are logged in as test user, which we do not want.\n+ # Check that calling updateCredentials for a *known* user does not set a\n+ # cookie if there is no cookie with credentials yet (you are anonymous).\n+ # So first logout.\n logout()\n session = self.folder.pas.session\n request = self.makeRequest("test string")\n session.updateCredentials(request, request.response, "our_user", "password")\n- # The anonymous user should not get a cookie: resetCredentials should\n+ # The anonymous user should not get a cookie: updateCredentials should\n # not do anything when there are no current credentials.\n # See https://github.com/plone/Products.CMFPlone/issues/3835\n self.assertIsNone(\n@@ -125,15 +129,21 @@ def testCredentialsUpdateAnonymous(self):\n )\n \n def testCredentialsUpdateOtherUser(self):\n- # We are logged in as test user, which we DO want in this test.\n- # The session should not be updated then.\n+ # Check that calling updateCredentials for someone other than the logged in\n+ # user does not set a cookie.\n session = self.folder.pas.session\n request = self.makeRequest("test string")\n session.updateCredentials(request, request.response, "our_user", "password")\n self.assertIsNone(request.response.getCookie(session.cookie_name))\n \n+ def testCredentialsUpdateSameUser(self):\n+ # Check that calling updateCredentials for ourselves *does* set a cookie.\n+ session = self.folder.pas.session\n+ request = self.makeRequest("test string")\n+ session.updateCredentials(request, request.response, self.userid, "password")\n+ self.assertIsNone(request.response.getCookie(session.cookie_name))\n+\n def testRefresh(self):\n- logout()\n session = self.folder.pas.session\n request = self.makeRequest("test string")\n session._setupSession(self.userid, request.response)\n@@ -146,14 +156,14 @@ def testRefresh(self):\n def testUnicodeUserid(self):\n response = MockResponse()\n session = self.folder.pas.session\n- # This step would fail.\n+ # The main thing we test, is that the next call does not give a traceback:\n session._setupSession(self.userid, response)\n \n def testSpecialCharUserid(self):\n unicode_userid = "\xc3\xa3bcd\xc3\xa9fghijk"\n response = MockResponse()\n session = self.folder.pas.session\n- # This step would fail.\n+ # The main thing we test, is that the next call does not give a traceback:\n session._setupSession(unicode_userid, response)\n \n def testCookieInvalidAfterLogout(self):\n' + +Repository: plone.session + + +Branch: refs/heads/master +Date: 2023-09-06T21:44:09-07:00 +Author: David Glick (davisagli) +Commit: https://github.com/plone/plone.session/commit/991f77bf5d4e33f12016234570b6ff6f6da56641 -Merge pull request #382 from plone/pre-commit-ci-update-config +Merge pull request #44 from plone/maurits-do-not-update-credentials-for-anonymous-issue-3835 -[pre-commit.ci] pre-commit autoupdate +Do not set auth cookie after password reset, unless user is authenticated Files changed: -M .pre-commit-config.yaml +A news/3835.bugfix +M plone/session/plugins/session.py +M plone/session/tests/testPAS.py -b'diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml\nindex 0d259b7f..f6f04834 100644\n--- a/.pre-commit-config.yaml\n+++ b/.pre-commit-config.yaml\n@@ -7,7 +7,7 @@ ci:\n \n repos:\n - repo: https://github.com/asottile/pyupgrade\n- rev: v3.8.0\n+ rev: v3.10.1\n hooks:\n - id: pyupgrade\n args: [--py38-plus]\n@@ -16,7 +16,7 @@ repos:\n hooks:\n - id: isort\n - repo: https://github.com/psf/black\n- rev: 23.3.0\n+ rev: 23.7.0\n hooks:\n - id: black\n - repo: https://github.com/collective/zpretty\n@@ -32,7 +32,7 @@ repos:\n # """\n ##\n - repo: https://github.com/PyCQA/flake8\n- rev: 6.0.0\n+ rev: 6.1.0\n hooks:\n - id: flake8\n - repo: https://github.com/codespell-project/codespell\n@@ -63,7 +63,7 @@ repos:\n - id: check-python-versions\n args: [\'--only\', \'setup.py,pyproject.toml\']\n - repo: https://github.com/collective/i18ndude\n- rev: "6.0.0"\n+ rev: "6.1.0"\n hooks:\n - id: i18ndude\n \n' +b'diff --git a/news/3835.bugfix b/news/3835.bugfix\nnew file mode 100644\nindex 0000000..28a7ddc\n--- /dev/null\n+++ b/news/3835.bugfix\n@@ -0,0 +1,4 @@\n+Do not set an auth cookie after password reset, unless the user is authenticated.\n+Otherwise anonymous users will be logged in immediately, even when autologin after password reset is false.\n+Fixes `issue 3835 `_.\n+[maurits]\ndiff --git a/plone/session/plugins/session.py b/plone/session/plugins/session.py\nindex 803087c..bd49f83 100644\n--- a/plone/session/plugins/session.py\n+++ b/plone/session/plugins/session.py\n@@ -281,8 +281,14 @@ def updateCredentials(self, request, response, login, new_password):\n authenticated_user = getSecurityManager().getUser()\n if authenticated_user is not None:\n authenticated_id = authenticated_user.getId()\n- # For anonymous, the id is empty\n- if authenticated_id and authenticated_id != user_id:\n+ # For anonymous, the id is empty.\n+ if not authenticated_id:\n+ # We should not update credentials when there are none currently.\n+ # Otherwise for example you are logged in after a password reset,\n+ # even when autologin after password reset is false.\n+ # See https://github.com/plone/Products.CMFPlone/issues/3835\n+ return\n+ if authenticated_id != user_id:\n return\n self._setupSession(user_id, response)\n \ndiff --git a/plone/session/tests/testPAS.py b/plone/session/tests/testPAS.py\nindex 8872042..c3cf6ec 100644\n--- a/plone/session/tests/testPAS.py\n+++ b/plone/session/tests/testPAS.py\n@@ -102,7 +102,9 @@ def testExtraction(self):\n self.assertEqual(creds, {})\n \n def testCredentialsUpdateUnknownUser(self):\n- # We are logged in as test user, which we do not want.\n+ # Check that calling updateCredentials for an *unknown* user does not set a\n+ # cookie if there is no cookie with credentials yet (you are anonymous).\n+ # So first logout.\n logout()\n session = self.folder.pas.session\n request = self.makeRequest("test string")\n@@ -112,28 +114,39 @@ def testCredentialsUpdateUnknownUser(self):\n self.assertIsNone(request.response.getCookie(session.cookie_name))\n \n def testCredentialsUpdateAnonymous(self):\n- # We are logged in as test user, which we do not want.\n+ # Check that calling updateCredentials for a *known* user does not set a\n+ # cookie if there is no cookie with credentials yet (you are anonymous).\n+ # So first logout.\n logout()\n session = self.folder.pas.session\n request = self.makeRequest("test string")\n session.updateCredentials(request, request.response, "our_user", "password")\n- self.assertIsNotNone(\n+ # The anonymous user should not get a cookie: updateCredentials should\n+ # not do anything when there are no current credentials.\n+ # See https://github.com/plone/Products.CMFPlone/issues/3835\n+ self.assertIsNone(\n request.response.getCookie(session.cookie_name),\n )\n \n def testCredentialsUpdateOtherUser(self):\n- # We are logged in as test user, which we DO want in this test.\n- # The session should not be updated then.\n+ # Check that calling updateCredentials for someone other than the logged in\n+ # user does not set a cookie.\n session = self.folder.pas.session\n request = self.makeRequest("test string")\n session.updateCredentials(request, request.response, "our_user", "password")\n self.assertIsNone(request.response.getCookie(session.cookie_name))\n \n+ def testCredentialsUpdateSameUser(self):\n+ # Check that calling updateCredentials for ourselves *does* set a cookie.\n+ session = self.folder.pas.session\n+ request = self.makeRequest("test string")\n+ session.updateCredentials(request, request.response, self.userid, "password")\n+ self.assertIsNone(request.response.getCookie(session.cookie_name))\n+\n def testRefresh(self):\n- logout()\n session = self.folder.pas.session\n request = self.makeRequest("test string")\n- session.updateCredentials(request, request.response, "our_user", "password")\n+ session._setupSession(self.userid, request.response)\n cookie = request.response.getCookie(session.cookie_name)["value"]\n request2 = self.makeRequest(cookie)\n request2.form["type"] = "gif"\n@@ -143,14 +156,14 @@ def testRefresh(self):\n def testUnicodeUserid(self):\n response = MockResponse()\n session = self.folder.pas.session\n- # This step would fail.\n+ # The main thing we test, is that the next call does not give a traceback:\n session._setupSession(self.userid, response)\n \n def testSpecialCharUserid(self):\n unicode_userid = "\xc3\xa3bcd\xc3\xa9fghijk"\n response = MockResponse()\n session = self.folder.pas.session\n- # This step would fail.\n+ # The main thing we test, is that the next call does not give a traceback:\n session._setupSession(unicode_userid, response)\n \n def testCookieInvalidAfterLogout(self):\n'