tgext.routes changes

[Thomas De Schampheleire]

Mads, Søren,

On Fri, Apr 8, 2016 at 11:23 PM, Alessandro Molina
<alessandro.molina at gmail.com> wrote:
> https://bitbucket.org/_amol_/kallithea-tg/src/8ada6ec6c4d3f3c518b4f67b56a20c6efb46971c/kallithea/lib/auth.py?at=default&fileviewer=file-view-default#auth.py-759:760
>
> As tgext.routes actually updates environ['REQUEST_METHOD'] I suppose that
> from that time on request.method will also return DELETE which is not
> included in that list.
>

I need to request your feedback here.
In kallithea/tests/functional/test_admin_defaults.py, there are three
similar tests:

    def test_update_browser_fakeout(self):
        response = self.app.post(url('default', id=1),
params=dict(_method='put',
_authentication_token=self.authentication_token()))

    def test_delete(self):
        # Not possible due to CSRF protection.
        response = self.app.delete(url('default', id=1), status=405)

    def test_delete_browser_fakeout(self):
        response = self.app.post(url('default', id=1),
params=dict(_method='delete',
_authentication_token=self.authentication_token()))

Both 'fakeout' tests failed under Turbogears with tgext.routes
routing, initially with 404. After a modification in tgext.routes
(https://github.com/TurboGears/tgext.routes/commit/39fde429094506ce213060a0a8ecf09f15d83365),
test_update_browser_fakeout succeeds, but the
test_delete_browser_fakeout now failed with 405 Method Not Allowed.

Alessandro identified the cause as coming from kallithea/lib/auth.py
from LoginRequired.__wrapper__ where there is an explicit test on
request.method being one of ['GET', 'HEAD', 'POST', 'PUT'], with
following explanation:

        # Only allow the following HTTP request methods. (We sometimes use POST
        # requests with a '_method' set to 'PUT' or 'DELETE'; but that is only
        # used for the route lookup, and does not affect request.method.)

If I temporarily change that code so that 'DELETE' is also part of the
accepted request methods, then test_delete fails due to 403 Forbidden
while it expects 405 Method Not Allowed, and
test_delete_browser_fakeout fails with 403 too (CSRF protection, I
guess?)


Previously, using routes, we received a real POST request with
_method==PUT or _method==DELETE, while with Alessandro's change in
tgext.routes, we receive a real PUT or DELETE request, which is not
what the current code expects.

Could you share your view about this?
Is it possible to adapt our codebase, or do we need a different fix in
tgext.routes?

Thanks,
Thomas



> On Wed, Apr 6, 2016 at 10:19 PM, Thomas De Schampheleire
> <patrickdepinguin at gmail.com> wrote:
>>
>>  Hi Alessandro,
>>
>> On Wed, Apr 6, 2016 at 1:30 AM, Alessandro Molina
>> <alessandro.molina at gmail.com> wrote:
>> > Hi Thomas,
>> >
>> > Had time to give it a quick look.
>> > Issue was caused by the fact that form was relying on ?_method parameter
>> > to
>> > override request method.
>> >
>> > It's now supported in tgext.routes 0.1.2
>> > Just upgrade tgext.routes and it should work ( please raise dependency
>> > to
>> > kallithea-tg, I cannot commit right now :( )
>>
>> Thanks, I verified that the mentioned test scenario now works and
>> pushed the bump to 0.1.2.
>>
>> When I retry the failing tests from the test suite, I see that the 404
>> is indeed gone, but one of the two failing tests still fails.
>> Originally, with tgext.routes 0.1.1, following tests failed with 404:
>>
>> FAIL
>> kallithea/tests/functional/test_admin_defaults.py::TestDefaultsController::()::test_update_browser_fakeout
>> FAIL
>> kallithea/tests/functional/test_admin_defaults.py::TestDefaultsController::()::test_delete_browser_fakeout
>>
>> With tgext.routes 0.1.2, only the delete test fails, as follows:
>>
>> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>> TestDefaultsController.test_delete_browser_fakeout
>> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>> kallithea/tests/functional/test_admin_defaults.py:70: in
>> test_delete_browser_fakeout
>>     response = self.app.post(url('default', id=1),
>> params=dict(_method='delete',
>> _authentication_token=self.authentication_token()))
>> ../venv/kallithea-tg/lib/python2.7/site-packages/webtest/app.py:371: in
>> post
>>     content_type=content_type)
>> ../venv/kallithea-tg/lib/python2.7/site-packages/webtest/app.py:736:
>> in _gen_request
>>     expect_errors=expect_errors)
>> ../venv/kallithea-tg/lib/python2.7/site-packages/webtest/app.py:632:
>> in do_request
>>     self._check_status(status, res)
>> ../venv/kallithea-tg/lib/python2.7/site-packages/webtest/app.py:664:
>> in _check_status
>>     res)
>> E   AppError: Bad response: 405 Method Not Allowed (not 200 OK or 3xx
>> redirect for http://localhost/_admin/defaults/1)
>> E   405 Method Not Allowed
>> E
>> E   The method DELETE is not allowed for this resource.
>>
>> I looked at your changes in tgext.routes but it does not look a
>> problem introduced by these changes; maybe it's a latent problem that
>> now pops up with the 404 fixed.
>> Is it tg / tgext.routes that generates the 405? How can such a problem
>> be debugged efficiently?
>>
>> Thanks,
>> Thomas
>
>