[kforge-dev] Ticket #38 (defect) created by I love you

[Rufus Pollock]

John Bywater wrote:
> Rufus Pollock wrote:
> 
>> John Bywater wrote:
>>
>>> 10: Timeline wrote:
>>>
>>>> Good site
>>>>
>>>> URL: http://project.knowledgeforge.net/kforge/trac/ticket/38
>>>>  
>>>>
>>>
>>> :-(
>>>
>>> Guess we'll need to grep through Apache logs to see what URL was used?
>>>
>>> Perhaps, Trac tickets can be created with a GET request?
>>
>>
>> No see below.
>>
>>> Perhaps, we'll need to check the KForge logs to see what access 
>>> control events occurred?
>>>
>>> Perhaps, we'll need to start always using https?
>>
>>
>> Should have written about this yesterday. I spent 1.5 hours :( 
>> tracking the source of this down. The results are as follows:
>>
>> 1. The modpython access control code in the handlers package is 
>> working correctly in the sense that the responses are what they should 
>> be (nice refactoring of the handler code btw)
> 
> 
> Thanks. ;-)
> 
>>
>> 2. Focusing on trac I confirmed that:
>>
>>   * These changes to trac are done via a post request
>>
>>   * As would be expected from 1 non-browser User agents are correctly 
>> excluded from post requests.
>>
>>   * Access from non-browser is also handled correctly as per our model 
>> (an attempt to post on a ticket either to modify or to create) leads 
>> either to access denied page or to login page. *HOWEVER the changes 
>> still go through to trac*. To repeat an attempt to submit as visitor 
>> on a trac system does lead to a redirect (so our accesscontrol works) 
>> but the post request still gets passed on to the underlying trac 
>> application and the changes made.
>>
>> (I checked this both locally and against test.kforgeproject.com)
>>
>> Looking at our code, in particular [1], I suspect that this may have 
>> something to do with our technique of dealing with denied browser 
>> requests by returning OK from the handler and setting the location 
>> field. I think what is happening is that by returning OK though we 
>> terminate the modpython access/authen chain we do not terminate 
>> processing by other handlers so the request gets passed on with the 
>> redirect only happening once the request handling has finished and the 
>> result is returned to the browser. I also note that the 'old' system 
>> (i guess that means pre r1018) did not display this behaviour (we 
>> weren't getting spammed ...).
> 
> 
> Good detective work.
> 
> Just to check, you're saying this bug occurs for:
> 
> 1. Non-cookie browser clients

No it occurs for both cookie and non-cookie clients (checked for both ...)

> 2. A POST request

Yes. (I haven't checked GET stuff)

> 3. No access granted (allegedly)

I've checked our own unit tests and when you do what I've done with the 
browser (either as a visitor or as a logged person who doesn't have 
access) you do get the redirect (i.e. sent to login or sent to 
accessDenied) BUT your action still goes through (e.g. change to ticket).

> but then the access handlers return OK, so that the request is passed to 
> the Trac handler?
> 
> Looks like we need a new way of inspiring the redirect. From memory, I 
> think the OK was needed to get the redirect working, that is we weren't 
> getting spammed but there was a different problem of some sort. Do you 
> remember what it was? Was it that DAV clients couldn't post? I can't 
> remember.

I think the problem was that occassionally instead or a redirect it 
would pop up an apache basic password prompt rather than redirecting 
immediately.

> Perhaps we need to try returning with HTTP_REDIRECT code? But I'm sure 
> that's what we were doing before, when it wasn't working. Perhaps it 
> wasn't working for some other reason that the return code?

I think we should try reverting to this and see what happens.

> The best I can say is that the refactored code should be a bit easier to 
> work with. Perhaps it will give us the chance to finally resolve this 
> rather complicated and difficult to test concern.

Yes. It was a good refactoring. The real issue this raises however is 
comprehensive (and I mean really comprehensive) functional test coverage 
of the web interface. This is something I know you've gone on about for 
a while and with which I've heartily agreed -- but you can't do 
everything at once. We've both done some work on in the past but we 
never had the time to get the coverage up to a decent level. I recently 
started on a new version that uses twill (instead of webunit) and which 
can connect directly via django's wsgi interface:

<http://project.knowledgeforge.net/kforge/trac/browser/trunk/src/kforge/test/web_test.py>

I used this to good effect for the recently implemented forgot password 
feature but there is a lot more to be done.

Regards,

Rufus