plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
245 stars 188 forks source link

Auto login after password reset cannot really be switched off #3835

Closed mauritsvanrees closed 1 year ago

mauritsvanrees commented 1 year ago

I have a Plone 6.0.5 site. In the security control panel, auto login after password reset is switched off. At first glance this would seem to work okay: after a password reset, you are still anonymous, and the passwordreset page says: "Your password has been set successfully. You may now log in with your new password." When you click on the "log in" link, you get a login form. All fine.

But: if you simply navigate to Home, you are also logged in!

The problem is:

Annoying. But the result in the case of this client is worse:

How to solve this?

I thought of adding an else around line 175:

        if registry.get("plone.autologin_after_password_reset", False):
            return self._auto_login(userid, password)
        else:
            pas = getToolByName(self.context, "acl_users")
            pas.resetCredentials(self.request, self.request.response)

Unfortunately this does not work: resetCredentials only does something when the user is authenticated. And in the current request the user is still anonymous.

Perhaps updateCredentials should also only do something when the user is already authenticated. But that seems possibly dangerous to change, with a large chance of unwanted side effects. Also, this method is in Products.PluggableAuthService, so it is used outside of Plone.

In the place of the new else statement we could do:

if "__ac" in response.cookies:
    del response.cookies["__ac"]

That works.

Alternatively, do something similar to _auto_login: get the user, get a new temporary security manager, then call pas.resetCredentials. So, shortened for clarity:

member = get_member_by_login_name(self.context, userid, False)
user = member.getUser()
orig_sm = getSecurityManager()
try:
    newSecurityManager(self.request, user)
    pas = getToolByName(self.context, "acl_users")
    pas.resetCredentials(self.request, self.request.response)
finally:
    setSecurityManager(orig_sm)

This works as well.

Opinions? Better ideas?

davisagli commented 1 year ago

I don't think it's safe to change updateCredentials in PAS to only call the plugins if the user is logged in. There might be a plugin which synchronizes a password change with an external system, whether or not it is the currently logged in user.

But perhaps our cookie auth plugin should be updated to skip setting the cookie if the user is not authenticated. https://github.com/plone/Products.PlonePAS/blob/7778d47c905dc226a37ea87240c0ebdbc2e2e7db/src/Products/PlonePAS/plugins/cookie_handler.py#L64

mauritsvanrees commented 1 year ago

Thanks for the hint.

In this case the plugin is actually in plone.session. Argh, I see already fixed a similar problem (as admin reset the password of Joe, and you get the auth cookie of Joe) three years ago, in https://github.com/plone/plone.session/pull/21. I did not touch the anonymous case there. Doing that, seems to fix it. I am making a PR.

With my changes, if "auto login after password reset" is on, it still works.