Move XContent generation to HasPrivilegesResponse #35616
Conversation
The RestHasPrivilegesAction previously handled its own XContent generation. This change moves that into HasPrivilegesResponse and makes the response implement ToXContent. This allows HasPrivilegesResponseTests to be used to test compatibility between HLRC and X-Pack internals. A serialization bug (cluster privs) was also fixed here.
tvernum
added
v7.0.0
>refactoring
:Security/Authorization
|
Pinging @elastic/es-security |
😿 @elasticmachine Please run gradle build tests |
...e/src/main/java/org/elasticsearch/xpack/core/security/action/user/HasPrivilegesResponse.java
Outdated
Show resolved
Hide resolved
| this.completeMatch = completeMatch; | ||
| this.cluster = new HashMap<>(cluster); | ||
| this.index = new ArrayList<>(index); | ||
| this.index = sorted(new ArrayList<>(index)); |
There was a problem hiding this comment.
I would be using a Set in this circumstances.
There was a problem hiding this comment.
If I was starting from scratch, then so would I, but that would require changing the return types on getIndexPrivileges and getApplicationPrivileges which seemed more invasive than was justified by this PR.
I'm happy to revist that choice if you want - I went back-and-forth many times while making the change.
There was a problem hiding this comment.
It's good like it is , it's more civilized, otherwise it would indeed indeed violate the scope of this PR. I think I see the privilege code as new code where creative destruction is more loosely permitted.
But I still think it would be nice to do this change in a follow-up. I am happy to pick it up if you wish.
| out.writeBoolean(completeMatch); | ||
| if (out.getVersion().onOrAfter(Version.V_7_0_0)) { | ||
| out.writeMap(cluster, StreamOutput::writeString, StreamOutput::writeBoolean); | ||
| } |
There was a problem hiding this comment.
I understand this is the oversight you've mentioned
There was a problem hiding this comment.
Yes - I actually noticed it while working on application privileges, but forgot to come back and fix it.
|
Left one note and one suggestion. I think that's all there is here, but I'll scrutinize it again when you ping me again. |
@elasticmachine run sample packaging tests |
The RestHasPrivilegesAction previously handled its own XContent generation. This change moves that into HasPrivilegesResponse and makes the response implement ToXContent. This allows HasPrivilegesResponseTests to be used to test compatibility between HLRC and X-Pack internals. A serialization bug (cluster privs) was also fixed here.
The RestHasPrivilegesAction previously handled its own XContent generation. This change moves that into HasPrivilegesResponse and makes the response implement ToXContent. This allows HasPrivilegesResponseTests to be used to test compatibility between HLRC and X-Pack internals. A serialization bug (cluster privs) was also fixed here.
4 participants
The RestHasPrivilegesAction previously handled its own XContent
generation. This change moves that into HasPrivilegesResponse and
makes the response implement ToXContent.
This allows HasPrivilegesResponseTests to be used to test
compatibility between HLRC and X-Pack internals.
A serialization bug (cluster privs) was also fixed here.
Relates: #35479