[ https://issues.apache.org/jira/browse/SOLR-13741?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16932428#comment-16932428 ]
Jan Høydahl commented on SOLR-13741: ------------------------------------ Thanks for taking a look at this. I see you have some great improvements on how to assert exceptions etc. Wrt the REJECTED + UNAUTHORIZED events I see the same as you, and I believe there is a code bug, not a test bug. In HttpSolrCall#471 in the {{authorize()}} call, if authResponse == PROMPT, it will actually match both blocks and emit two audit events: [https://github.com/apache/lucene-solr/blob/26ede632e6259eb9d16861a3c0f782c9c8999762/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java#L475:L493] {code:java} if (authResponse.statusCode == AuthorizationResponse.PROMPT.statusCode) {...} if (!(authResponse.statusCode == HttpStatus.SC_ACCEPTED) && !(authResponse.statusCode == HttpStatus.SC_OK)) {...} {code} When code==401, it is also true that code!=200. Intuitively there should be both a sendErrora and return RETURN before line #484 in the first if block? The first if block was introduced back in 2005 as part of SOLR-7757. [~noble.paul] why does the if not return? It will *always* fall through to and trigger the next if block! > 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: Hoss Man > Assignee: Hoss Man > Priority: Major > Attachments: 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-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org