[prev in list] [next in list] [prev in thread] [next in thread] 

List:       tomcat-dev
Subject:    Re: [Bug 54729] new HttpParser.parseAuthorizationBasic method
From:       Brian Burch <brian () PingToo ! com>
Date:       2013-03-31 9:27:04
Message-ID: 51580168.2020901 () PingToo ! com
[Download RAW message or body]

On 22/03/13 01:11, Brian Burch wrote:
<snip/>
> I'll keep this enhancement open until I've had time to think properly...
> although your new Basic "parser" has returned to BasicAuthenticator,
> there might still be some merit in moving it to HttpParser and keeping
> my proposed test suite, especially now you have written a performance test!

It looks as if the dust has settled...

I noticed the commons Base64 unit tests were not ported, so effectively 
the only tests we have currently are very high-level and so carry a lot 
of overhead, e.g. TestNonLoginAndBasicAuthenticator hauls up and tears 
down at least one tomcat instance for every test case.

I've looked at the code Mark committed for Base64 and I don't feel 
attracted to the idea of me porting the relevant sections of the commons 
test suite. On the other hand, I also don't feel comfortable simply 
retooling my proposed tests for basic auth in HttpParser to act as an 
embryo test suite for the new Base64 class - that would imply much more 
than it would deliver.

I feel most comfortable with the idea of more-or-less retaining my 
existing proposed test suite. It doesn't require a tomcat instance to 
run, and yet it makes a reasonably complete attempt at covering all 
permissible, tolerable and illegal cases of basic authentication. I know 
that approach doesn't cover FileUploader, or other base64 users, but we 
don't have any open bugs in those areas either - just a few todo's in my 
authentication tests.

I am just a contributor, with no ambition to become more. I value the 
work done by the developers and want to give something back when 
possible. Therefore, I want my proposals to feel comfortable to you guys 
and not require a lot of rework to match your style.

Assuming you agree with me so far, I would appreciate your opinions on 
how I should proceed. The main decision hinges on style, I think...

I like the idea of retaining a HttpParser.parseAuthorizationBasic 
method, but the obvious and the most useful signature would be 
(MessageBytes authorization). This would make a neat division between 
the higher-level server logic and the low-level handling of the specific 
authorization header. The drawback is the signature would have 
absolutely no similarity to 
HttpParser.parseAuthorizationDigest(StringReader). It feels a bit like 
fitting a square peg into a round hole, but at least they are on the 
same block of wood.

An alternative would be to refactor the existing rather minimal parser 
logic into a protected method such as 
BasicAuthenticator.parseAuthorization((MessageBytes authorization). This 
new method could be explicitly called by new test cases in 
TestNonLoginAndBasicAuthenticator, thus avoiding the tomcat haul-up/down 
overhead.

Alternatively, it might be simpler to pass a ByteChunk argument to the 
chosen new method.

Let me know what you think - I won't start work until I'm sure I know 
where to go!

Regards,

Brian




---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[prev in list] [next in list] [prev in thread] [next in thread]