SOLR-18048 Move authentication into a Servlet Filter#4120
SOLR-18048 Move authentication into a Servlet Filter#4120gus-asf merged 6 commits intoapache:mainfrom
Conversation
solr/core/src/java/org/apache/solr/servlet/AuthenticationFilter.java
Outdated
Show resolved
Hide resolved
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java
Show resolved
Hide resolved
| * | ||
| * @param eventType the audit event | ||
| */ | ||
| public boolean shouldAudit(AuditEvent.EventType eventType) { |
There was a problem hiding this comment.
I don't see callers to what you added. We should be conservative in adding yet more methods to CoreContainer.
There was a problem hiding this comment.
Yeah I generally agree. There are more places it can be used, but I shied away from fully applying these methods since it seemed like scope creep. I wound up adding them because , but there was just a ton of duplicated logic always checking null and then checking the type and it all seems a bit like a Law of Demeter code smell with core container handing out it's object rather than just answering the question too, so I added these. However you are probably right that I need to finish the intention here or punt it. Otherwise it just becomes another half done thing hanging out in the code base, so next update will put this stuff to full use.... actually, broader use seems to lead to fewer methods anyway...
|
I'm looking forward to this :-) |
|
Yeah will get back to it soon, other distractions at the moment :) |
…voked, also fixed some minor filter name nonsense in web.xml
| import static org.apache.solr.common.params.CollectionParams.CollectionAction.CREATE; | ||
| import static org.apache.solr.common.params.CollectionParams.CollectionAction.DELETE; | ||
| import static org.apache.solr.common.params.CollectionParams.CollectionAction.RELOAD; | ||
| import static org.apache.solr.servlet.HttpSolrCall.shouldAudit; |
There was a problem hiding this comment.
good riddance to this import!
| /* Private ctor prevents instantiation */ | ||
| } | ||
|
|
||
| @SuppressWarnings("ClassCanBeRecord") |
There was a problem hiding this comment.
why suppress this warning? It doesn't fail the build; does it? This seems like a suitable record.
There was a problem hiding this comment.
Once the check-mark in the IDE is yellow all manner of stuff gets ignored... converting it is off topic for this pr however.
There was a problem hiding this comment.
I understand the conversion is out of scope. But I don't think we should suppress a valid recommendation that we simply haven't prioritized addressing. I recommend simply accepting that your IDE/tools are going to recommend things you don't want to do now, if ever.
| * A servlet filter to handle authentication. Anything that needs to be served without | ||
| * authentication (such as UI) must be resolved and returned by a filter preceding this one, | ||
| * typically by forwarding to the default servlet. Also, any tracing, auditing and ratelimiting |
There was a problem hiding this comment.
Anything that needs to be served without authentication (such as UI) must be resolved and returned by a filter preceding this one, typically by forwarding to the default servlet.
This statement is false; it presumes that the only way to do this is the way it's working now. I will show how to do so in a follow-on PR but have been waiting for this PR and others to land.
There was a problem hiding this comment.
At present this wraps everything. Obviously if something is not wrapped it will be unaffected (which would be better for stuff we never mean to authenticate in the first place). I'll qualify the statement.
|
|
||
| // we want to prevent any attempts to close our request or response prematurely | ||
| chain.doFilter(closeShield(req), closeShield(res)); | ||
| } catch (SolrException e) { |
There was a problem hiding this comment.
Turns out instantiating an AuditEvent parses parameters and that can throw a SolrException, which I don't like, but stopped myself from fixing. Luckily we have a test that sends bad data in the parameters and checks that it results in 400 not 500 :)
|
Plan to merge/backport next weekend, unless encouraged by 2 or more reviewers to merge sooner. |
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Please merge at your convenience
# Conflicts: # solr/core/src/java/org/apache/solr/security/AuthorizationUtils.java
2 participants
https://issues.apache.org/jira/browse/SOLR-18048