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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
