Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

redirect loops can be caused when authenicated user gets Unauthorised #36

Open
djay opened this issue Jun 4, 2018 · 7 comments
Open

Comments

@djay
Copy link
Member

djay commented Jun 4, 2018

Normally plone redirects to a login form when it gets unauthorised.
Some PAS plugins like saml2 do redirects to other services to do the login. Those services might already remember the user and redirect back. This leads to a redirect loop.

I consider this a Plone failure. It doesn't really make sense to ask the user to login when they are already logged in. Plone should instead give an unauthorised error page perhaps with a button to logout and login as a different user.
Even without the redirect loops this would be better UX as often the user doesn't know why they are given a login box and this creates confusion.

@davisagli
Copy link
Member

Can you provide more details about what is happening? Normally Plone shows an "Insufficient Privileges" message if a logged-in user encounters an Unauthorized error, not the login form.

@djay
Copy link
Member Author

djay commented Jun 12, 2018

@davisagli In my case it was the original url was to content which was private and logging in didn't give them enough access to see it. Perhaps thats a different error?

@d-maurer
Copy link

@davisagli The "Insufficient Priviledges" logic is likely in the wrong place: implemented in a specific plugin rather than in general infrastructure.

@djay
Copy link
Member Author

djay commented Jun 26, 2018

@davisagli The insufficient permissions is something that only appears due to restrictedpython issues. In the normal case PAS handles the exception by redirecting the top plugins login form. see https://zope.readthedocs.io/en/latest/zdgbook/Security.html#unauthorized-exceptions-and-through-the-web-code.

The problem with this is if a SSO system is used such as saml2, then that often results in a redirect to another site, which in terms thinks the user is already logged in so redirects back which in turn determines teh user doesn't have the required permission so raises an unauthorisedexception and the loop starts again.

My suggestion would be that Plone should not try to allow the user to login again when an already logged in user gets unauthorised. Instead it shows a screen which explains they don't have sufficient permissions and gives them a link to logout and login with another user. Or maybe contact the site administrator.

@d-maurer
Copy link

d-maurer commented Jun 26, 2018

I have the following workaround:

Products.PluggableAuthservice chooses challenges in the following way:``

    for challenger_id, challenger in challengers:
        challenger_protocol = getattr(challenger, 'protocol',
                                      challenger_id)
        if valid_protocols and challenger_protocol not in valid_protocols:
            # Skip invalid protocol for this request type.
            continue
        if protocol is None or protocol == challenger_protocol:
            if challenger.challenge(request, response):
                protocol = challenger_protocol

    if protocol is not None:
        # something fired, so it was a successful PAS challenge
        return True

``

This means that we can implement the "reject already authenticated user" logic by a special challenge plugin. We must ensure that it comes before the "normal" plugin and that it has an associated "protocol" different from that of the "normal" plugin -- this will inhibit the "normal" plugin once our special challengs has fired. If "protocol chooser" plugins are registered, one of them must allow our special plugin.

@d-maurer
Copy link

And there is a variant of the above workaround (maybe simpler): again we use a special plugin to implement our "reject already authenticated user" logic. This time, it comes after the "normal" plugin and uses the same associated protocol as that one. This ensures that its "challenge" is called after the one of the "normal" plugin. In case, the user is already authenticated, it overrides the challenge of the "normal" plugin; otherwise, it does nothing.

@djay
Copy link
Member Author

djay commented Jun 28, 2018

@d-maurer that sounds like a workable solution. It's a little fiddly to ensure the plugin remains at the top but the advantage of your way is that it makes it easy to have it optional.
How to work out if this makes sense to build into plone and have it installed by default though? @davisagli ? Community post? PLIP? Ask more people here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants