[jira] [Commented] (SOLR-13741) possible AuditLogger bugs uncovered while hardening AuditLoggerIntegrationTest

["Jan Høydahl (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SOLR-13741?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16949321#comment-16949321 ] 

Jan Høydahl commented on SOLR-13741:
------------------------------------

{quote}why did the comment for a "wrong password" claim it was going to get a 403 exception + audit log ?
{quote}
It should expect 401 when wrong password, this was probably all confused by SOLR-13835 in the initial test.
{quote}Are {{/admin/info/key}} events expect when auth is enabled ?

...is it ok for the test to explicitly ignore these events
{quote}
I can't recall dealing specially with this path. So muting during tests sounds like the right thing to do. Guess you could argue that it could be muted by default but the framework since it is a public always-open path? 
{quote}why the _actual_ audit log recieved in the "wrong password" situation is so different (and sparse) compared to other audit log events ?
// - the resource is *JUST* '/solr'
// - note that "resource" for every other expected event in this test class doesn't even
// *START* with (or include) the "/solr" portion of the URL
// - event 'resource' values are typically "/admin/etc..."
// - the requestType is 'UNKNOWN'
// - as opposed to the ADMIN that the existing test exists (and seems like should be correct){quote}
I will attach a new patch with some of this fixed:
 * Parsing "resource" from {{httpRequest.getPathInfo()}} instead of {{httpRequest.getContextPath()}} which is always /solr.
 * Detecting {{/admin/..}} as admin path in {{AuditEvent.findRequestType}} now that the resource is changed, giving requestType=ADMIN
 * However, principal is not filled since BasicAuth failed, which I believe is correct. But the HTTP headers are there for inspection... It would be nice to have the user field in AuditEvent also in this case, but that would mean that AuthPlugins would need to set it on MDC or something. It would be wrong to set principal on the request since that always means authenticated user, not?

{quote}// - this event has no solrParams at all
// - even though the httpQueryString show it's from the CREATE test2 req{quote}
This event is generated based on {{HttpServletRequest}} so we have no solrParams at this stage. In the new patch I have initialized the solrParams map from the httpRequest for a more consistent AuditEvent experience.

Hoss, this test is now so much better than what I managed to whip up the first time, thanks a ton for digging!

 

> possible AuditLogger bugs uncovered while hardening AuditLoggerIntegrationTest
> ------------------------------------------------------------------------------
>
>                 Key: SOLR-13741
>                 URL: https://issues.apache.org/jira/browse/SOLR-13741
>             Project: Solr
>          Issue Type: Bug
>      Security Level: Public(Default Security Level. Issues are Public) 
>            Reporter: Chris M. Hostetter
>            Assignee: Chris M. Hostetter
>            Priority: Major
>         Attachments: SOLR-13741.patch, SOLR-13741.patch
>
>
> A while back i saw a weird non-reproducible failure from AuditLoggerIntegrationTest.  When i started reading through that code, 2 things jumped out at me:
> # the way the 'delay' option works is brittle, and makes assumptions about CPU scheduling that aren't neccessarily going to be true (and also suffers from the problem that Thread.sleep isn't garunteed to sleep as long as you ask it too)
> # the way the existing {{waitForAuditEventCallbacks(number)}} logic works by checking the size of a (List) {{buffer}} of recieved events in a sleep/poll loop, until it contains at least N items -- but the code that adds items to that buffer in the async Callback thread async _before_ the code that updates other state variables (like the global {{count}} and the patch specific {{resourceCounts}}) meaning that a test waiting on 3 events could "see" 3 events added to the buffer, but calling {{assertEquals(3, receiver.getTotalCount())}} could subsequently fail because that variable hadn't been udpated yet.
> #2 was the source of the failures I was seeing, and while a quick fix for that specific problem would be to update all other state _before_ adding the event to the buffer, I set out to try and make more general improvements to the test:
> * eliminate the dependency on sleep loops by {{await}}-ing on concurrent data structures
> * harden the assertions made about the expected events recieved (updating some test methods that currently just assert the number of events recieved)
> * add new assertions that _only_ the expected events are recieved.
> In the process of doing this, I've found several oddities/descrepencies between things the test currently claims/asserts, and what *actually* happens under more rigerous scrutiny/assertions.
> I'll attach a patch shortly that has my (in progress) updates and inlcudes copious nocommits about things seem suspect.  the summary of these concerns is:
> * SolrException status codes that do not match what the existing test says they should (but doesn't assert)
> * extra AuditEvents occuring that the existing test does not expect
> * AuditEvents for incorrect credentials that do not at all match the expected AuditEvent in the existing test -- which the current test seems to miss in it's assertions because it's picking up some extra events from triggered by previuos requests earlier in the test that just happen to also match the asserctions.
> ...it's not clear to me if the test logic is correct and these are "code bugs" or if the test is faulty.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org