Coerce blank fields to null in ApiKey requests #66240
Conversation
API Key request objects (`InvalidateApiKeyRequest` and `GetApiKeyRequest`) support multiple key-selection parameters such as realm-name, user-name, key-id and key-name. The specified behaviour is that if any of these are _blank_ then they act as a wildcard and do not restrict the search criteria. This change moves the "is blank" logic into the constructor for these requests so that there is a single consistent way to determine blank (`Strings.hasText(arg) == false`) and all usage of these fields can rely on the getter returning either `null` or a _real value_, and never a non-null blank. Relates: elastic#62916
tvernum
added
>refactoring
:Security/Authentication
|
Pinging @elastic/es-security (Team:Security) |
| if (Strings.hasText(id)) { | ||
| this.ids = new String[]{id}; | ||
| } else { | ||
| this.ids = ids; | ||
| } |
There was a problem hiding this comment.
Before I touched this class in 7.10, there is no check for id in either constructor. Last time I added
ids = Strings.hasText(id) == false ? null : new String[] { id };
in the constructor with StreamInput. Looking back now, this introduced a tiny inconsistency of how empty id is handled in different constructors. Your change here fixed this inconsistency. Thanks!
Now I have a new question: since a single empty string is now handled and excluded from the constructor, should we also apply the empty check for the string array of ids, i.e., should we move the validation logic of ids into here as well? On one hand, relocating the check for ids is good for consistency in that validate() is more about interactions between multiple fields. On the other hand, it complicates the constructors and also could be considered as a slight behaviour change. Overall, I think I prefer to not touch it.
There was a problem hiding this comment.
I took the cautious approach and decided to make this as close as possible to having zero visible changes.
Since the deserialisation code has a hasText check (as you note) I decided that this fix made sense, but chose not to try and filter empty values in the ids array. That seemed like a behavioural change that was outside the scope of this PR.
I'm not necessarily opposed to someone making that change I just wanted this PR to be quick and simple.
API Key request objects (`InvalidateApiKeyRequest` and `GetApiKeyRequest`) support multiple key-selection parameters such as realm-name, user-name, key-id and key-name. The specified behaviour is that if any of these are _blank_ then they act as a wildcard and do not restrict the search criteria. This change moves the "is blank" logic into the constructor for these requests so that there is a single consistent way to determine blank (`Strings.hasText(arg) == false`) and all usage of these fields can rely on the getter returning either `null` or a _real value_, and never a non-null blank. Relates: elastic#62916 Backport of: elastic#66240
API Key request objects (`InvalidateApiKeyRequest` and `GetApiKeyRequest`) support multiple key-selection parameters such as realm-name, user-name, key-id and key-name. The specified behaviour is that if any of these are _blank_ then they act as a wildcard and do not restrict the search criteria. This change moves the "is blank" logic into the constructor for these requests so that there is a single consistent way to determine blank (`Strings.hasText(arg) == false`) and all usage of these fields can rely on the getter returning either `null` or a _real value_, and never a non-null blank. Backport of: #66240
API Key request objects (
InvalidateApiKeyRequestandGetApiKeyRequest) support multiple key-selection parameters such asrealm-name, user-name, key-id and key-name.
The specified behaviour is that if any of these are blank then they
act as a wildcard and do not restrict the search criteria.
This change moves the "is blank" logic into the constructor for these
requests so that there is a single consistent way to determine blank
(
Strings.hasText(arg) == false) and all usage of these fields canrely on the getter returning either
nullor a real value, andnever a non-null blank.
Relates: #62916