Plone 5.2: improper handling of `LOGIN_TEMPLATE_IDS` in `Products.CMFPlone.browser.login.login.LoginForm.get_came_from`

[d-maurer]

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

BUG

What I did:

http://localhost:8080/Plone-IDP/acl_users/credentials_cookie_auth/require_login?came_from=http%3A//lsdm%3A8180/Plone-IDP/idp/idpsso_logged_in%3Fskey%3Drs%253A846c1bbb-5037-4f06-8342-7e587529caa9

You cannot reproduce this exactly. However, there should be no need: the important thing is that the path of came_from contains logged_in (but not a LOGIN_TEMPLATE_IDS).

What I expect to happen:

That after a successful login, there would be a redirect to came_from.

What actually happened:

came_from was ignored.

The reason: Products.CMFPlone.browser.login.login.LoginForm.get_came_from looks for LOGIN_TEMPLATE_IDS in the path of came_from (here Plone-IDP/idp/idpsso_logged_in) and wrongly believes, it contains logged_in (while this is actually idpsso_logged_in). I think, that get_came_from should only look at the last path component of came_from (here idpsso_logged_in) and check for identity; at the least it should not compare on string level but instead split the path into its segments and check whether the login template id is one of the segments.

What version of Plone/ Addons I am using:

Plone 5.2aX

dm.zope.saml2 (unreleased; I am working to get it Plone 5.2/Zope 4/Python 3 compatible).

[pbauer]

@d-maurer please tag tickets according to the right milestone (5.2 in this case) and make sure that the issue is thill the case with the newest version (5.2b1 or - even much better - coredev).

[d-maurer]

Philip Bauer wrote at 2019-2-23 02:13 -0800:

@d-maurer please tag tickets according to the right milestone (5.2 in this case)

I cannot: "Labels", ... "Milestone", etc. are all passive elements for me (likely because I have no write permissions).

and make sure that the issue is thill the case with the newest version (5.2b1 or - even much better - coredev).

In fact, I am using "coredev" -- however, I do not know which version it corresponds to. Can you give me a tip how to determine this?

I have just made a git pull in my local "Products.CMFPlone" git clone (created by the "coredev" buildout) - and it reported "up to date".

Below is my local fix for the reported problem: lsdm: git diff diff --git a/Products/CMFPlone/browser/login/login.py b/Products/CMFPlone/browser/login/login.py index 811f47705..6f03fb409 100644 --- a/Products/CMFPlone/browser/login/login.py +++ b/Products/CMFPlone/browser/login/login.py @@ -117,7 +117,7 @@ class LoginForm(form.EditForm): url_tool = getToolByName(self.context, 'portal_url') if not url_tool.isURLInPortal(came_from): return

• came_from_path = parse.urlparse(came_from)[2]
• came_from_path = parse.urlparse(came_from)[2].split("/") for login_template_id in LOGIN_TEMPLATE_IDS: if login_template_id in came_from_path: return

It splits the "came_from" path into its segments and checks whether the "login_template_id" is one of those segments. As mentioned, I do not think that this is the optimal fix (in my view checking only against the last segment would be better) but it is quite conservative.

[pbauer]

I added the milestone. You should consider signing the contributors agreement (https://docs.plone.org/develop/coredev/docs/agreement.html). Otherwise you will not be able to create a pull-request for your fix.

If you already use the coredev for 5.2 mentioning that is enough.

[pbauer]

@d-maurer any news on this?

[d-maurer]

@pbauer What news do you expect? I do not plan to become a core Plone developer - thus, I do not plan to sign a contributor agreement. I already have clearly described the problem and suggested ways to fix it.

[petschki]

Fixed in #2783