-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HLRest: add xpack put user API #32332
Conversation
This commit adds a security client to the high level rest client, which includes an implementation for the put user api. As part of these changes, the request and response classes were moved to the x-pack protocol project and licensed under the Apache 2 license. The PutUserRequest previously performed some validation of the username that really is more fitting for the server validation. This validation has been removed to reduce the amount of logic on the client side and also reduce the number of classes that would need to be moved to the protocol project. The removed validation now happens in the transport action. See elastic#29827
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started a review and then realized that this contains logic that is outside of my comfort zone, especially moving the logic for user validation to transport. Please move those into a separate PR and have someone else w/ more expertise review them.
@@ -1140,6 +1141,15 @@ static Request xpackUsage(XPackUsageRequest usageRequest) { | |||
return request; | |||
} | |||
|
|||
static Request putUser(PutUserRequest putUserRequest) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other request converters have been prepending some xpack
stuff to these methods. While im not sure im a big fan of it, its prolly best to stay consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the recent decision to move away from xpack naming for new things, I am leaving this as is.
* @return the {@link PutUserResponse} parsed | ||
* @throws IOException in case there is a problem parsing the response | ||
*/ | ||
private static PutUserResponse parsePutUserResponse(XContentParser parser) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yuck. Is it possible to break this in the API for 7.0? Do we want to keep that custom wrapping in RestPutUserAction
? Is it necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR I have added a solution that prevents API breakage in 6.x by keeping the wrapping AND having the value unwrapped. For 7.0 we can remove the wrapped value. Does that work for you?
@@ -1,9 +1,22 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im super weary of moving things that rest depends on to server. The hope is that we will depend even less on server, or not at all. Id rather see this moved into client if it needs to be, or if its just using one or two of those helpers, to just move those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until we can remove the need for serialization, I think this should stay as is. Once we can split our internal serialization from the request/response objects then this becomes a non-issue. Currently we already depend on StreamInput and StreamOutput, which pulls in this dependency and lucene too.
This change moves the validation for values of usernames and passwords from the request to the transport action. This is done to prevent the need to move more classes into protocol once we add this API to the high level rest client. Additionally, this resolves an issue where validation depends on settings and we always pass empty settings instead of the actual settings. Relates elastic#32332
I have just committed #32596 which changes the location of the xpack clients. Its very likely your tests wont compile now that the xpack portion of |
This change cleans up some methods in the CharArrays class from x-pack, which includes the unification of char[] to utf8 and utf8 to char[] conversions that intentionally do not use strings. There was previously an implementation in x-pack and in the reloading of secure settings. The method from the reloading of secure settings was adopted as it handled more scenarios related to the backing byte and char buffers that were used to perform the conversions. The cleaned up class is moved into libs/core to allow it to be used by requests that will be migrated to the high level rest client. Relates elastic#32332
This change moves the validation for values of usernames and passwords from the request to the transport action. This is done to prevent the need to move more classes into protocol once we add this API to the high level rest client. Additionally, this resolves an issue where validation depends on settings and we always pass empty settings instead of the actual settings. Relates #32332
This change moves the validation for values of usernames and passwords from the request to the transport action. This is done to prevent the need to move more classes into protocol once we add this API to the high level rest client. Additionally, this resolves an issue where validation depends on settings and we always pass empty settings instead of the actual settings. Relates #32332
This change cleans up some methods in the CharArrays class from x-pack, which includes the unification of char[] to utf8 and utf8 to char[] conversions that intentionally do not use strings. There was previously an implementation in x-pack and in the reloading of secure settings. The method from the reloading of secure settings was adopted as it handled more scenarios related to the backing byte and char buffers that were used to perform the conversions. The cleaned up class is moved into libs/core to allow it to be used by requests that will be migrated to the high level rest client. Relates #32332
This change cleans up some methods in the CharArrays class from x-pack, which includes the unification of char[] to utf8 and utf8 to char[] conversions that intentionally do not use strings. There was previously an implementation in x-pack and in the reloading of secure settings. The method from the reloading of secure settings was adopted as it handled more scenarios related to the backing byte and char buffers that were used to perform the conversions. The cleaned up class is moved into libs/core to allow it to be used by requests that will be migrated to the high level rest client. Relates #32332
@hub-cap this is ready for review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutUserRequest.java
Show resolved
Hide resolved
|
||
["source","java",subs="attributes,callouts,macros"] | ||
-------------------------------------------------- | ||
include-tagged::{doc-tests}/WatcherDocumentationIT.java[x-pack-put-user-execute-async] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/WatcherDocumentation/SecurityDocumentation/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good eye. we should be testing these running build_docs.pl
from the docs repo
expectedParams = Collections.emptyMap(); | ||
} | ||
|
||
PutUserRequest putUserRequest =new PutUserRequest(username, password, roles, fullName, email, enabled, metadata, refreshPolicy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/=new
/= new
/
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.client.security; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe organize under package org.elasticsearch.client.security.user
? As we will keep adding a lot of such classes in the same package for different APIs like role mapping etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think its too much to put them all in the same package, I wont enforce keeping them in one package. I certainly do not think the number of classes that currently exist in all of the action.* classes is too many for a single package here. But I dont think we need to enforce it one way or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need more packages. I think our goal should be to keep the number of packages to a minimum and at this point I do not see justification for it.
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.client.security; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as PutUserResponse class in package org.elasticsearch.client.security.user
@@ -136,4 +140,15 @@ protected static void clusterUpdateSettings(Settings persistentSettings, | |||
request.transientSettings(transientSettings); | |||
assertOK(client().performRequest(RequestConverters.clusterPutSettings(request))); | |||
} | |||
|
|||
@Override | |||
protected Settings restClientSettings() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, I think of our integration tests as examples of API usage using HLRC and I propose the use of HttpClientConfigCallback
to set credentials which can be used by HttpClient when security is enabled. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is definitely how the docs read for configuring security on the client. I would be ++ for showing examples of that in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to leave this as is. It is out of scope for this PR and if we choose to go this way, we should not use ESRestTestCase
and instead show all configuration of a HLRC in its own base class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Minor comments / nits.
client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutUserRequest.java
Show resolved
Hide resolved
@Override | ||
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
builder.startObject(); | ||
if (password != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this ever get returned to the user here? im not familiar w security over rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The password is never returned to the user. We are simply transferring it to the server for hashing and storage.
@@ -136,4 +140,15 @@ protected static void clusterUpdateSettings(Settings persistentSettings, | |||
request.transientSettings(transientSettings); | |||
assertOK(client().performRequest(RequestConverters.clusterPutSettings(request))); | |||
} | |||
|
|||
@Override | |||
protected Settings restClientSettings() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is definitely how the docs read for configuring security on the client. I would be ++ for showing examples of that in the tests.
public void testPutUser() throws IOException { | ||
final String username = randomAlphaOfLengthBetween(4, 12); | ||
final char[] password = randomBoolean() ? randomAlphaOfLengthBetween(8, 12).toCharArray() : null; | ||
final List<String> roles = Arrays.asList(generateRandomStringArray(randomIntBetween(2, 8), randomIntBetween(8, 16), false, true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the case of an empty roles list get tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does. The last parameter is whether or not to allow empty arrays and we set it to true
final Map<String, Object> metadata; | ||
if (randomBoolean()) { | ||
metadata = new HashMap<>(); | ||
for (int i = 0; i < randomIntBetween(1, 10); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to have it 0, 10
to produce an empty hashmap here or is this invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ I switched to 0, 10 to test a empty map
|
||
["source","java",subs="attributes,callouts,macros"] | ||
-------------------------------------------------- | ||
include-tagged::{doc-tests}/WatcherDocumentationIT.java[x-pack-put-user-execute-async] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good eye. we should be testing these running build_docs.pl
from the docs repo
@@ -49,4 +45,22 @@ public void readFrom(StreamInput in) throws IOException { | |||
super.readFrom(in); | |||
this.created = in.readBoolean(); | |||
} | |||
|
|||
@Override | |||
public boolean equals(Object o) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.client.security; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think its too much to put them all in the same package, I wont enforce keeping them in one package. I certainly do not think the number of classes that currently exist in all of the action.* classes is too many for a single package here. But I dont think we need to enforce it one way or the other.
//end::x-pack-put-user-execute | ||
|
||
//tag::x-pack-put-user-response | ||
boolean isCreated = response.isCreated(); // <1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert for verification whether it is created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thank you.
This commit adds a security client to the high level rest client, which includes an implementation for the put user api. As part of these changes, a new request and response class have been added that are specific to the high level rest client. One change here is that the response was previously wrapped inside a user object. The plan is to remove this wrapping and this PR adds an unwrapped response outside of the user object so we can remove the user object later on. See #29827
...urity/src/main/java/org/elasticsearch/xpack/security/rest/action/user/RestPutUserAction.java
Show resolved
Hide resolved
This change removes the wrapping of the created field in the put user response. The created field was added as a top level field in elastic#32332, while also still being wrapped within the `user` object of the response. Since the value is available in both formats in 6.x, we can remove the wrapped version for 7.0.
This change removes the wrapping of the created field in the put user response. The created field was added as a top level field in #32332, while also still being wrapped within the `user` object of the response. Since the value is available in both formats in 6.x, we can remove the wrapped version for 7.0.
} | ||
if (metadata != null) { | ||
builder.field("metadata", metadata); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaymode
enabled
is missing, so this HL REST API will only be putting enabled=true users.
I have actually pulled out enabled
out of the User
object on the client side (which I am arduously peddling #33552 (comment)) but the PutUserRequest
, containing the User
object, has to have an enabled
flag that it puts in the XContent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Adding a test in SecurityIT
introduced by #33552 is one way to test for this)
This commit adds a security client to the high level rest client, which
includes an implementation for the put user api. As part of these
changes, a new request and response class have been added that are
specific to the high level rest client. One change here is that the response
was previously wrapped inside a
user
object. The plan is to remove thiswrapping and this PR adds an unwrapped response outside of the
user
object so we can remove the
user
object later on.See #29827