tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: [Bug 54729] new HttpParser.parseAuthorizationBasic method
Date Mon, 01 Apr 2013 12:09:19 GMT
On 31/03/2013 10:27, Brian Burch wrote:
> 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...

It has. At least, I don't have any immediate plans in this area at the 
moment. The most likely changes in the future would be to sync up with 
any changes to the original but that should be internal to the Base64 
implementation.

> 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.

Fair enough.

> 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.

Meh. The API uses StringReader as that is what worked best for 
MediaType. Digest then used StringReader so it could re-use some of the 
code although for raw performance MessageBytes might have been a little 
faster.

Given what I think HttpParser.parseAuthorizationBasic() is going to do I 
think MessageBytes is fine as I'm not anticipating much (any?) overlap 
between it and the existing code. Even if there is overlap having the 
code in the same place means it is easier to keep the implementations 
aligned.

> 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.

I'm less worried about that overhead. While the focus of the unit test 
is usually on a small bit of code, the process of configuring and 
starting a Tomcat instance sometimes identifies unexpected regressions.

> 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!

I'd go with a new method in HttpParser but in the end this is your itch. 
Generally, the committers have less strong opinions on unit tests than 
production code. Something like BASIC auth will get a lot of review as 
it will get called for every authenticated request. If you change any of 
the implementation, some performance tests to show that things are 
broadly as good/bad as they were before would help.

Mark


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


Mime
View raw message