make cookie based login more robust

[jugmac00]

The login did not work when came_from request parameter was missing.

This is now handled gracefully, as the user, who provided correct credentials, now gets logged in and gets notified about the next steps.

This fixes #65

[jugmac00]

I'd much prefer to redirect the user to the login page itself and sending along an error message instead of returning static HTML. That way the user might see site navigation elements and can go somewhere useful.

This was my first thought, too, but then the login form would need some kind of message area and all users of PAS would need to update their login forms to read a request param and print the message.

Also, I use a custom login form, which I pasted into Cookie Auth Helper -> Contents -> login_form

Maybe I miss some builtin Zope mechanism to render error messages to users such as Flask has. https://flask.palletsprojects.com/en/1.1.x/patterns/flashing/

[dataflake]

The important thing isn't really the error message. It's about leaving the user in a place that may offer some obvious "way out" if the login page shows normal site navigation elements. With static HTML the user is stuck and completely helpless.

[jugmac00]

The important thing isn't really the error message. It's about leaving the user in a place that may offer some obvious "way out" if the login page shows normal site navigation elements. With static HTML the user is stuck and completely helpless.

e.g. my custom login form has no menu - so staying on the page does not help the user either.

Yesterday, I experimented with just staying on the page, but then the browser only received a status 204, and nothing changed after the user sent the form, so this was also confusing.

There does not seem to be a very easy solution.

[dataflake]

If the browser just received a 204 status then you did not do an explicit redirect. You can look at the request to find out where the user came from, like a REFERER value, and then do an explicit redirect.

[d-maurer]

Jens Vagelpohl wrote at 2021-4-25 02:47 -0700: @.*** requested changes on this pull request.

I'd much prefer to redirect the user to the login page itself and sending along an error message instead of returning static HTML. That way the user might see site navigation elements and can go somewhere useful.

Alternatively, we could redirect to the parent of acl_users, i.e. to the root of the subhierarchy dominated by the user folder.

[dataflake]

Alternatively, we could redirect to the parent of acl_users, i.e. to the root of the subhierarchy dominated by the user folder.

That's reasonable as well.

[jugmac00]

Thanks for the feedback.

Actually, I never did much traversing, and I never used aq_parent before, so is this what you think I should do?

from Acquisition import aq_parent

if came_from is None:
    return response.redirect(aq_parent(aq_parent(self)).absolute_url())

... where self would be the CookieAuthHelperinstance.

[dataflake]

More like this, using the base class _getPAS method to grab the user folder:

from Acquisition import aq_inner
from Acquisition import aq_parent
...
if came_from is None:
    pas_root = aq_parent(aq_inner(self._getPAS()))
    return response.redirect(pas_root.absolute_url())

[jugmac00]

Attention

I applied the suggested changes, but I have not written a test case yet, as I cannot use the other test cases in test_CookieAuthHelper.py as a blueprint, as then self._getPas is None as we have not instantiated a PAS.

I'd appreciate any hint for how to setup such a test case (I will have a look myself another time, too).

[dataflake]

Grepping inside the plugin tests folder for _getPAS reveals that the test_ZODBUserManager.py file fakes that method and makes it return a dummy object for testing. You can try that or create a test class that derives from Products.PluggableAuthService.tests.pastc.PASTestCase, which creates a full PAS instance. You'd need to add the plugin and activate it in your own test code.

[d-maurer]

Jürgen Gmach wrote at 2021-5-8 06:56 -0700:

... I'd appreciate any hint for how to setup such a test case (I will have a look myself another time, too).

Search the definition for _getPas. It will tell you how the plugin finds the corresponding PAS.

I assume that _getPas is defined as: aq_parent(aq_inner(self)). In this case, create a PAS and put the plugin as an attribute on the PAS.

[jugmac00]

Ok, thanks to your guidance I was able to add a test case. Let me know if there is anything else I could do, or whether the PR could be merged (I'd need to squash the commits then).

[jugmac00]

Thanks for the review and the support!