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

create exception view before handling Unauthorized #304

Merged
merged 7 commits into from Oct 2, 2018

Conversation

5 participants
@tschorr
Contributor

tschorr commented Jul 20, 2018

@tschorr tschorr changed the base branch from master to xmlrpc Jul 20, 2018

@tschorr tschorr requested a review from davisagli Jul 20, 2018

@tschorr tschorr changed the title from WIP: create exception view before handling Unauthorized to create exception view before handling Unauthorized Jul 20, 2018

@lukasgraf

This comment has been minimized.

Show comment
Hide comment
@lukasgraf

lukasgraf Jul 20, 2018

Just to clarify, I'm not necessarily proposing that the order in WSGIPublisher needs to be changed - I just noticed that this is the underlying cause of these failures in plone.rest. I'm simply not familar enough with the WSGIPublisher to say whether changing the order is the right course of action or not.

But I did notice that this comment

                 # _unauthorized modifies the response in-place. If this hook
                 # is used, an exception view for Unauthorized has to merge
                 # the state of the response and the exception instance.

basically becomes void if you switch the order of triggering the _unauthorized() hook and calling the exception view.

This comment and the surrounding logic was introduced in
#112 (Restore a _unauthorized hook on the response object) and later updated in
#121 (Restore exception views for unauthorized) by @hannosch - maybe he has some insight into whether this change here would be warranted to accomodate plone.rest?


I also tried to apply the recommendation

an exception view for Unauthorized has to merge the state of the response and the exception instance

from that comment to plone.rest's exception view:

The way I interpret it, this would mean that in plone.rest's exception view, we basically would need to undo the possible "damage" done to the response by an IChallengePlugin.

For a stock Plone, this sort of works by undoing the redirect triggered by the CookieAuthHelper: resetting the (locked) status code of 30x to that of the exception (Unauthorized, 401), and removing the Location header from the response. But this feels messy, and I can't see how this could be generalized for other possible IChallengePlugins that plone.rest can't know about, if they can just modify the response in place.

lukasgraf commented Jul 20, 2018

Just to clarify, I'm not necessarily proposing that the order in WSGIPublisher needs to be changed - I just noticed that this is the underlying cause of these failures in plone.rest. I'm simply not familar enough with the WSGIPublisher to say whether changing the order is the right course of action or not.

But I did notice that this comment

                 # _unauthorized modifies the response in-place. If this hook
                 # is used, an exception view for Unauthorized has to merge
                 # the state of the response and the exception instance.

basically becomes void if you switch the order of triggering the _unauthorized() hook and calling the exception view.

This comment and the surrounding logic was introduced in
#112 (Restore a _unauthorized hook on the response object) and later updated in
#121 (Restore exception views for unauthorized) by @hannosch - maybe he has some insight into whether this change here would be warranted to accomodate plone.rest?


I also tried to apply the recommendation

an exception view for Unauthorized has to merge the state of the response and the exception instance

from that comment to plone.rest's exception view:

The way I interpret it, this would mean that in plone.rest's exception view, we basically would need to undo the possible "damage" done to the response by an IChallengePlugin.

For a stock Plone, this sort of works by undoing the redirect triggered by the CookieAuthHelper: resetting the (locked) status code of 30x to that of the exception (Unauthorized, 401), and removing the Location header from the response. But this feels messy, and I can't see how this could be generalized for other possible IChallengePlugins that plone.rest can't know about, if they can just modify the response in place.

@lukasgraf lukasgraf referenced this pull request Jul 20, 2018

Open

Python 3 support #75

@pbauer

This comment has been minimized.

Show comment
Hide comment
@pbauer

pbauer Sep 27, 2018

Contributor

@davisagli when rebasing with master after #290 was merged I found the fix for https://bugs.python.org/issue27777 was causing the conflicts but I did not know which implementation of the fix was the right one.... Can you do the rebase?

Contributor

pbauer commented Sep 27, 2018

@davisagli when rebasing with master after #290 was merged I found the fix for https://bugs.python.org/issue27777 was causing the conflicts but I did not know which implementation of the fix was the right one.... Can you do the rebase?

@icemac icemac added this to In progress in Zope 4 final release via automation Sep 28, 2018

@davisagli davisagli changed the base branch from xmlrpc to master Oct 2, 2018

@davisagli

This comment has been minimized.

Show comment
Hide comment
@davisagli

davisagli Oct 2, 2018

Member

I rebased it @pbauer

Member

davisagli commented Oct 2, 2018

I rebased it @pbauer

@pbauer

This comment has been minimized.

Show comment
Hide comment
@pbauer

pbauer Oct 2, 2018

Contributor

Thanks!

Contributor

pbauer commented Oct 2, 2018

Thanks!

@pbauer pbauer requested a review from icemac Oct 2, 2018

@icemac icemac moved this from In progress to Needs review in Zope 4 final release Oct 2, 2018

@icemac

icemac approved these changes Oct 2, 2018

LGTM. Maybe the change should have a change log entry, too.

Zope 4 final release automation moved this from Needs review to Reviewer approved Oct 2, 2018

@icemac icemac added this to the 4.0 final milestone Oct 2, 2018

tschorr added some commits Oct 2, 2018

@tschorr tschorr merged commit 1d78cfa into master Oct 2, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 79.25%
Details

Zope 4 final release automation moved this from Reviewer approved to Done Oct 2, 2018

@tschorr tschorr deleted the py3_fix_wsgi_unauthorized branch Oct 2, 2018

icemac added a commit to zopefoundation/Products.PluggableAuthService that referenced this pull request Oct 9, 2018

Keep existing response body during HTTPBasicAuthHelper.challenge().
This allows to set the response body via an exception view in Zope >= 4.0b6.

zopefoundation/Zope#304 moved the call of the
exception view before calling `response._unauthorized()`.

@icemac icemac removed this from the 4.0 final milestone Oct 11, 2018

@icemac icemac added this to the 4.0b6 milestone Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment