plone / Products.PlonePAS

User and group implementation for the Plone CMS and Zope PluggableAuthService
6 stars 21 forks source link

Notify CredentialsUpdated on password reset switches user #57

Closed thomasmassmann closed 4 years ago

thomasmassmann commented 4 years ago

Discovered in https://community.plone.org/t/after-failed-password-transaction-the-logged-in-user-is-swapped-to-another-user/12216, the notification of CredentialsUpdated at the end triggers a _setCookie() for the changed user (who's password was reset), but is applied to the current request. This will switch the current user.

mliebischer commented 4 years ago

Discovered in https://community.plone.org/t/after-failed-password-transaction-the-logged-in-user-is-swapped-to-another-user, the notification of CredentialsUpdated at the end triggers a _setCookie() for the changed user (who's password was reset), but is applied to the current request. This will switch the current user.

The link is broken. I think it should be is one: https://community.plone.org/t/after-failed-password-transaction-the-logged-in-user-is-swapped-to-another-user/12216

--

We have the same problem in one of our customer projects, in which an special function for resetting the passwords was programmed based on userSetPassword() from Products.PlonePAS. Internally, doChangeUser() is called which triggers updateCredentials() and this seems the reason for switching the user after resetting a users password, as @thomasmassmann already explained here. This issue seems to be related to merge https://github.com/plone/Products.PlonePAS/pull/54.

Same information to reproduce:

The following is a test that should make it easier to reproduce/isolate the problem:

    def test_reset_password_should_not_switch_current_user(self):
        # setup
        pas = api.portal.get_tool(name='acl_users')

        # setup: create new user
        testuser = api.user.create(username='abraham', password='pass-w', email='a@b.com')

        # do it: reset password
        pas.userSetPassword(testuser.id, 'foobar')

        # postcondition: user should stay the same as before resetting
        self.assertNotIn('__ac', self.request.response.cookies)

or a more complex test by using PAS credentials extractors plugins

    def test_reset_password_should_not_switch_current_user(self):
        def fake_publish(request, status=200):
            cookies = request.response.cookies.copy()
            new_cookies = {}
            for key in list(cookies.keys()):
                new_cookies[key] = cookies[key]['value']
            request.cookies = new_cookies
            request.response.cookies = {}
            request.response.setStatus(status)

        # setup
        pas = api.portal.get_tool(name='acl_users')

        # setup: create new user
        testuser = api.user.create(username='abraham', password='pass-w', email='a@b.com')

        # do it: reset password
        pas.userSetPassword(testuser.id, 'foobar')

        # copy cookies from response to request
        fake_publish(self.request)

        # postcondition: user should stay the same as before resetting
        user_ids = pas._extractUserIds(self.request, pas._getOb('plugins'))
        self.assertEqual(user_ids, [], "Should not switch user")

Our dirty workaround: Do not send any (wrong) auth cookie to the browser after resetting the users password.

 pas.userSetPassword(testuser.id, 'foobar')
 if '__ac' in self.request.response.cookies:
     del self.request.response.cookies['__ac']
mauritsvanrees commented 4 years ago

The cookie is set in plone.session. I will try a PR.

mauritsvanrees commented 4 years ago

Should be fixed by https://github.com/plone/plone.session/pull/21