Correctly fire events when user auto login after the password has been reset #2994

ericof · 2019-12-03T11:54:42Z

…mbership_tool.

mister-roboto · 2019-12-03T11:54:55Z

@ericof thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

ericof · 2019-12-03T11:56:05Z

@jenkins-plone-org please run jobs

ericof · 2019-12-03T13:01:12Z

@jenkins-plone-org please run jobs

jensens · 2019-12-03T13:06:24Z

Products/CMFPlone/browser/login/password_reset.py

+        user = membership_tool.getMemberById(userid).getUser()
        login_time = user.getProperty('login_time', None)
        if login_time is None:
            notify(UserInitialLoginInEvent(user))


If the notify still needed or does getMemberById takes care of tit as well?

@jensens I think notify is still needed here because getMemberById -- or the updateCredentials above -- is not authenticating the user.

The original code would always, in a default Plone installation, return an Anonymous User, and that is why we use the membership tool to get a user object for that userid.

Not related to this PR, but it feels strange to me that a call to getSecurityManager().getUser() in a method called _auto_login before firing the UserLoggedInEvent does not return the proper user 🤔

@ale-rt I agree. My understanding of the whole situation is the following:

Request to the password reset view is done without a valid auth cookie

Zope/PAS authentication considers this user as anonymous

After the password is reset we come to this method, still without authentication

We call update credentials for all registered plugins (in the default Plone installation, this will send back a new cookie to the user)

Current request still has no auth cookie (only the response will set it)

getSecurityManager still believes this is an Anonymous user

User receives a redirect and the new cookie

Next request will see the user authenticated (as the new cookie is valid)

Yes, that is really plausible. I am wondering if this method should actually login the user so that the security manager is aware of that.
But maybe this has some other implications.
For me the PR is fine as a workaround (then mergeable) but the real issue is that the user at this point is not logged in.
Any other opinion?

ericof · 2019-12-03T14:31:37Z

@jenkins-plone-org please run jobs

ericof · 2019-12-03T16:13:55Z

@ale-rt, @jensens Could you take a look at the latest change? Instead of using getMemberById, I used get_member_by_login_name -- the same way PasswordResetTool uses it.

ericof · 2019-12-03T16:14:11Z

@jenkins-plone-org please run jobs

ale-rt

Cool, I was not aware of get_member_by_login_name

…n reset (#2994) * Test if events fired after auto login have the correct principal * At this point the user is not logged in yet, so we get it from the membership_tool. * Update changelog * Remove adapter registration for IUserLoggedInEvent * Fix unregistering of adapter * Deal with cases where username/email were not the userid

* Fix TTW Bundle compilation broken. Fixes #2969 * Add plone.staticresources to list of addons which are automatically upgraded if upgrade steps are available. * Fix #1318: Always install default content types on Plone site creation (#2971) * Fix #1318: Always install default content types on Plone site creation * Update changelog entry * Correctly fire events when user auto login after the password has been reset (#2994) * Test if events fired after auto login have the correct principal * At this point the user is not logged in yet, so we get it from the membership_tool. * Update changelog * Remove adapter registration for IUserLoggedInEvent * Fix unregistering of adapter * Deal with cases where username/email were not the userid * Create FUNDING.yml * Improve tests for the workflow tool method listWFStatesByTitle Refs #3032 * require Zope wsgi extra (because of Paste) depending on Python version * add changelog entry * Make setup.py reflect to be Plone 6 and Py3 only * Include changelog of 5.2.1. Removed related news snippets. There are now no differences between master and 5.2.x, except basically in setup.py. [ci skip] * Password strength checks were not always checked Include Hotfix 20200121 with tests * Merge isURLInPortal patch from Hotfix20200121. The isURLInPortal check that is done to avoid linking to an external site could be tricked into accepting malicious links. * Fix typo causes double translation text. Same message is on line 350 * enable custom format string i18n/l10n * fix/add suggestions from review * Use time.process_time always. Available in all relevant Python 3 versions. * add viewlet manager below content description * structure cleanup * fix card markup for footer * fix footer and colophon card classes * Add changelog entry * Fix TTW Bundle compilation broken. Fixes #2969 * Add plone.staticresources to list of addons which are automatically upgraded if upgrade steps are available. * Fix #1318: Always install default content types on Plone site creation (#2971) * Fix #1318: Always install default content types on Plone site creation * Update changelog entry * Correctly fire events when user auto login after the password has been reset (#2994) * Test if events fired after auto login have the correct principal * At this point the user is not logged in yet, so we get it from the membership_tool. * Update changelog * Remove adapter registration for IUserLoggedInEvent * Fix unregistering of adapter * Deal with cases where username/email were not the userid * changelog * Create FUNDING.yml * require Zope wsgi extra (because of Paste) depending on Python version * add changelog entry * Make setup.py reflect to be Plone 6 and Py3 only * Include changelog of 5.2.1. Removed related news snippets. There are now no differences between master and 5.2.x, except basically in setup.py. [ci skip] * Password strength checks were not always checked Include Hotfix 20200121 with tests * Merge isURLInPortal patch from Hotfix20200121. The isURLInPortal check that is done to avoid linking to an external site could be tricked into accepting malicious links. * Improve tests for the workflow tool method listWFStatesByTitle Refs #3032 * Fix typo causes double translation text. Same message is on line 350 * enable custom format string i18n/l10n * fix/add suggestions from review * Use time.process_time always. Available in all relevant Python 3 versions. * Add custom.css resource after diazo bundle * Add changelog entry * Add issue number to changelog entry * set GS version to 6000 * Add zcml-condition plone-60 for conditional configuration. * update HTMLFilter defaults to enable TinyMCE layout features * Removed a few news snippets that are already in latest 5.2.2 rc. * Add links to official resources * The http://plone.org/products/plone page is no longer current. * Fix rebase aftermath: Missing merge * Merge isURLInPortal patch from Hotfix20200121. The isURLInPortal check that is done to avoid linking to an external site could be tricked into accepting malicious links. * add viewlet manager below content description * structure cleanup * fix card markup for footer * fix footer and colophon card classes * simplify login form and fix login action pattern-options * Styled content and usergroups in Site Setup * fix primary button css class * fix tests to match bootstrap form markup * fix broken/incomplete merge from rebase * fix broken/incomplete merge from rebase (2) * Fix issue with @@search view when filtering by creation date (cherry picked from commit 4dbed04) * remove six - we are Python 3 only * Remove portal_utf8 and it twin utf8_portal from utils and PloneTool since its never used nowhere. * Renamed wrongly named news snippets. [ci skip] * Removed folder_publish.cpy script. Moved it to a new browser view in plone.app.content. Removed deprecated transitionObjectsByPaths. It was only used by this script, and I have now put it in the view. * No longer consider calling len(batch) as deprecated. The deprecation warning is unvoidable with current `Products.PageTemplates` code. Fixes #3176 (cherry picked from commit 52a3267) * find . -name "*.py" |xargs pyupgrade --py36-plus * Fixed news snippet extension. [ci skip] * Removed our CMFQuickInstallerTool code completely. * Removed QuickInstallerTool.py With the Products.CMFQuickInstallerTool package gone, the bare bones fallback in this file did not work correctly. You get an error like this when viewing the Plone Site in the ZMI, or even just loading the root ZMI: ``` 2020-04-24 23:29:39,113 ERROR [ZODB.Connection:809][waitress] Couldn't load state for Products.CMFPlone.Portal.PloneSite 0x9f8a Traceback (most recent call last): File "/Users/maurits/shared-eggs/cp37m/ZODB-5.5.1-py3.7.egg/ZODB/Connection.py", line 795, in setstate self._reader.setGhostState(obj, p) File "/Users/maurits/shared-eggs/cp37m/ZODB-5.5.1-py3.7.egg/ZODB/serialize.py", line 637, in setGhostState state = self.getState(pickle) File "/Users/maurits/shared-eggs/cp37m/ZODB-5.5.1-py3.7.egg/ZODB/serialize.py", line 630, in getState return unpickler.load() File "/Users/maurits/shared-eggs/cp37m/ZODB-5.5.1-py3.7.egg/ZODB/serialize.py", line 492, in _persistent_load return self.load_persistent(*reference) File "/Users/maurits/shared-eggs/cp37m/ZODB-5.5.1-py3.7.egg/ZODB/serialize.py", line 535, in load_persistent self._cache.new_ghost(oid, obj) TypeError: Cache values must be persistent objects. ``` I am creating an alias in plone.app.upgrade. * Removed outdated test for QuickInstallerTool. * Removed unused quickinstaller test zcml files. These were already copied to controlpanel/tests/ and adapted. * On Python 3 in testMigrationTool only test upgrade from Plone 5.2. You cannot upgrade a Plone 4.0 or 5.1 site to 5.2 or higher when you are on Python 3. You must first migrate to 5.2 with Python 2.7. Also, this avoids a test failure on 6.0 because the 5.0alpha upgrade calls qi.isProductInstalled, which has been removed. * Remove Python 2 code from testMigrationTool.py. * Merge isURLInPortal patch from Hotfix20200121. The isURLInPortal check that is done to avoid linking to an external site could be tricked into accepting malicious links. * add viewlet manager below content description * structure cleanup * fix card markup for footer * fix footer and colophon card classes * simplify login form and fix login action pattern-options * Styled content and usergroups in Site Setup * fix primary button css class * fix tests to match bootstrap form markup * fix broken/incomplete merge from rebase * fix broken/incomplete merge from rebase (2) * add test rendering as view, add basic example * bootstrap markup update for login * add icon retriever as view * add changelog entry * changed due to feedback * re-add init method, add fallback for icon lookup * update registration, refactor icons.py, add tests * shorten icon prefix * use iconresolver as name * update markup * sync ajax_main_template defines with main_template * fix outdated pull classes * lookup fallback trial for mimetypes * simplify debugging * better error handling with non-existig icons * catch specific Exception * bs5 form updates * overhaul the admin-ui templates to have a modern bootstrap 5 look * forgotten test condition removed * ensure installation of "default_extension_profiles" * update markup for bootstrap * updated viewlets for barceloneta-lts * remove deprecated old controlpanel * get rid of kss traces * better var name * use plone icon names * move image handling controlpanel to default profile * use icons from icon resolver * use icons from resolver * fallback for icon_expr with urls * add icons from icon resolver * cleanup * update button classes for bootstrap * update classes for bootstrap * add SVG modifier * liberated prefs_main_temaplte from skins * turn configlet portlets into menu * manage spaces in add site * update icon fallback for explicit icon names * update controlpanel icons to explicit icon names * fix ajax load: Compilation failed b/c of typo * controlpanel icon tweaks * bs5-b1 update - prefixed data attributes * Cool URLs do not change * bootstrap b1 changes * bootstrap b1 changes * fix alert margin * Display the path to have a better hint of were the site is * Fix problem to remove username and password if there was already one set (cherry picked from commit 0499b63) * fixes in usergroups controlpanel * fixes in group member administration * again some polishing in user/groups admin * and again some polishing in user/groups admin * another alert statusmessage * always allow to set timezone * fix action controlpanel robot tests * use different text to check, since breadcrumbs have changed * fix robot test: no documentFirstheading in this place * fix checkboxes in usergroups * register test rendering page as test_rendering and test-rendering * fix selector for warning alert * Start fixing search page * fix search.js -> new selectors * ajax pagination and sane result numbering * fix cite tag * fix searchterm in title * convert tal:attributes expressions * reset batch start when filtering * one more batch fix * fix controlpanel tests * remove outdated test - portal_skins doesn't exist anymore * fix resource url * fix authenticator test * fix selector * add Sleep 0.5s * fix usergroups robot tests * refactor usergroups settings description * fix selector in security_controlpanel test * Link modal has no input field "title" * fix overlay tests * Fix create new action in action control panel * update icon_expr for actions Co-authored-by: Johannes Raggam <thetetet@gmail.com> Co-authored-by: Érico Andrei <ericof@gmail.com> Co-authored-by: Thomas Massmann <thomas.massmann@it-spir.it> Co-authored-by: Jens W. Klein <jk@kleinundpartner.at> Co-authored-by: T. Kim Nguyen <tkimnguyen@users.noreply.github.com> Co-authored-by: ale-rt <alessandro.pisa@gmail.com> Co-authored-by: tschorr <t_schorr@gmx.de> Co-authored-by: Maurits van Rees <maurits@vanrees.org> Co-authored-by: Maurits van Rees <m.van.rees@zestsoftware.nl> Co-authored-by: Fred van Dijk <fredvd@gmail.com> Co-authored-by: agitator <agitator@users.noreply.github.com> Co-authored-by: Stefan Antonelli <stefan.antonelli@operun.de> Co-authored-by: Peter Holzer <peter.holzer@agitator.com> Co-authored-by: MrTango <md@derico.de> Co-authored-by: Kristin Kuche <kuche@hrz.uni-marburg.de> Co-authored-by: Mikel Larreategi <mlarreategi@codesyntax.com> Co-authored-by: Fulvio Casali <fulviocasali@gmail.com> Co-authored-by: Nils Hofer <niho2001@gmail.com> Co-authored-by: Franco Pellegrini <frapell@gmail.com> Co-authored-by: Robert Kuzma <robert@balavec.com>

ericof added 3 commits December 3, 2019 12:48

Test if events fired after auto login have the correct principal

3322010

At this point the user is not logged in yet, so we get it from the me…

3894e94

…mbership_tool.

Update changelog

cf12cb7

ericof added 01 type: bug 32 needs: review labels Dec 3, 2019

ericof requested a review from jensens December 3, 2019 11:54

Remove adapter registration for IUserLoggedInEvent

310ceca

jensens reviewed Dec 3, 2019

View reviewed changes

Fix unregistering of adapter

de929c4

ale-rt approved these changes Dec 3, 2019

View reviewed changes

Deal with cases where username/email where not the userid

34b83b2

jensens approved these changes Dec 3, 2019

View reviewed changes

ale-rt approved these changes Dec 3, 2019

View reviewed changes

ericof merged commit ba1c7cb into 5.2.x Dec 3, 2019

ericof deleted the fix-2993-UserLoggedInEvent-with-anonymous-user branch December 3, 2019 17:23

ericof mentioned this pull request Dec 3, 2019

Login user after password reset notify events with Anonymous User #2993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly fire events when user auto login after the password has been reset #2994

Correctly fire events when user auto login after the password has been reset #2994

ericof commented Dec 3, 2019

mister-roboto commented Dec 3, 2019

ericof commented Dec 3, 2019

ericof commented Dec 3, 2019

jensens Dec 3, 2019

ericof Dec 3, 2019

ale-rt Dec 3, 2019

ericof Dec 3, 2019

ale-rt Dec 3, 2019

ericof commented Dec 3, 2019

ericof commented Dec 3, 2019

ericof commented Dec 3, 2019

ale-rt left a comment

Correctly fire events when user auto login after the password has been reset #2994

Correctly fire events when user auto login after the password has been reset #2994

Conversation

ericof commented Dec 3, 2019

mister-roboto commented Dec 3, 2019

ericof commented Dec 3, 2019

ericof commented Dec 3, 2019

jensens Dec 3, 2019

Choose a reason for hiding this comment

ericof Dec 3, 2019

Choose a reason for hiding this comment

ale-rt Dec 3, 2019

Choose a reason for hiding this comment

ericof Dec 3, 2019

Choose a reason for hiding this comment

ale-rt Dec 3, 2019

Choose a reason for hiding this comment

ericof commented Dec 3, 2019

ericof commented Dec 3, 2019

ericof commented Dec 3, 2019

ale-rt left a comment

Choose a reason for hiding this comment