Search API keys with a subset of Query DSL#73705
Search API keys with a subset of Query DSL#73705ywangd wants to merge 5 commits intoelastic:masterfrom
Conversation
|
Notes for reviewers This purpose of this draft PR is to demostrate the overall approach. It will be polished (or even rewritten) if the overall approach is agreed. I went back and forth with different ideas and settled down with the following two design decisions:
The main reason for first decision is simplicity. Since we have tight control of the supported query types and fields, it is sufficient to process them at the query DSL level. Processing at Lucene level is not necessary and technically very challenging because it is outside of security domain and is hard to connect them together. The second decision was made mainly to work with the Another issue is what the API endpoint should be. This PR works by add a request body to PS: This test class is a quick way to see what queries are supported (or not) for API keys. |
|
This is interesting, and I think it suggests that this is a path we can make work. I have lots of ideas about specifics (endpoint naming, existing parameters, etc) but they're details we can work through once we're happy we have a clear direction. |
Not yet. I can message Jim to ask who can help out in the search area team. |
jimczi
left a comment
There was a problem hiding this comment.
I left one comment regarding the setter for the allowed field names but the approach looks good to me.
| this.allowedFieldNames = allowedFieldNames; | ||
| } | ||
|
|
||
| public SearchExecutionContext withAllowedFieldNames(Predicate<String> allowedFieldNames) { |
There was a problem hiding this comment.
The context object has some mutable variables that we should preserve in the mutation. So instead of creating a new object we could add a setter for the allowed field names like we do for setAllowUnmappedFields ?
There was a problem hiding this comment.
I wasn't sure between using setter or constructor. Thanks for the tip. I changed it to use setter.
| } | ||
|
|
||
| static boolean isFieldNameAllowed(String fieldName) { | ||
| if (Set.of("doc_type", "name", "api_key_invalidated", "creation_time", "expiration_time").contains(fieldName)) { |
There was a problem hiding this comment.
this can be extracted in a static variable ?
There was a problem hiding this comment.
Yep it was done in a rush. Extracted now.
| private ApiKeySearchQueryBuilder() {} | ||
|
|
||
| public static BoolQueryBuilder build(GetApiKeyRequest getApiKeyRequest, Authentication authentication) { | ||
| BoolQueryBuilder finalQuery; |
There was a problem hiding this comment.
Why not building an ApiKeyBoolQueryBuilder directly ?
There was a problem hiding this comment.
Good point. No need to keep both ApiKeySearchQueryBuidler and ApiKeyBoolQueryBuilder. I consolidated everything into ApiKeyBoolQueryBuilder. Thanks!
|
Closing this draft PR in favor of #75335. |
This PR aims to provide a more feature rich search experience for API keys. It allows a subset of Query DSL to be specified for fields that are explicilty allowed. This change enables the followings: