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