Skip to content

HLRC: Add delete user action #35294

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

Merged
merged 24 commits into from
Nov 29, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
396e65c
HLRC: Add delete user action
iverase Nov 6, 2018
876f4a0
Fix precommit
iverase Nov 6, 2018
ae9059f
Fix precommit
iverase Nov 6, 2018
2d4aa98
Merge remote-tracking branch 'origin/master' into deleteUser
iverase Nov 6, 2018
321935a
address some of the review comments
iverase Nov 7, 2018
e4a0904
fist bug serializing response
iverase Nov 7, 2018
058f6c4
Implement DeleteUserResponse by extending AcknowledgedResponse
iverase Nov 8, 2018
86786e2
Merge remote-tracking branch 'origin/master' into deleteUser
iverase Nov 8, 2018
ff6f055
Acknowledge response is in core now
iverase Nov 8, 2018
041aeb6
javadocs
iverase Nov 8, 2018
ee6d4c6
simplify docs
iverase Nov 9, 2018
a2f2d52
sync with head and implement response as Optional
iverase Nov 12, 2018
7465462
fix checkStyle
iverase Nov 12, 2018
b83312d
remove otioonal and use AcknowledgeResponse
iverase Nov 19, 2018
e89ea8b
Merge remote-tracking branch 'origin/master' into deleteUser
iverase Nov 19, 2018
7e90b83
Implement delete response
iverase Nov 19, 2018
f3e76a5
remove unused imports
iverase Nov 19, 2018
0351a26
add license
iverase Nov 19, 2018
5f10520
formatting
iverase Nov 19, 2018
1f7fb31
add license
iverase Nov 19, 2018
c697fc0
fix checkStyle
iverase Nov 19, 2018
a55981a
Merge remote-tracking branch 'origin/master' into deleteUser
iverase Nov 19, 2018
4368f2e
Merge remote-tracking branch 'origin/master' into deleteUser
iverase Nov 23, 2018
d4d66af
Merge remote-tracking branch 'origin/master' into deleteUser
iverase Nov 28, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import org.elasticsearch.client.security.DeleteRoleMappingResponse;
import org.elasticsearch.client.security.DeleteRoleRequest;
import org.elasticsearch.client.security.DeleteRoleResponse;
import org.elasticsearch.client.security.DeleteUserRequest;
import org.elasticsearch.client.security.DeleteUserResponse;
import org.elasticsearch.client.security.DisableUserRequest;
import org.elasticsearch.client.security.EmptyResponse;
import org.elasticsearch.client.security.EnableUserRequest;
Expand Down Expand Up @@ -102,6 +104,33 @@ public void putUserAsync(PutUserRequest request, RequestOptions options, ActionL
PutUserResponse::fromXContent, listener, emptySet());
}

/**
* Removes user from the native realm synchronously.
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-delete-user.html">
* the docs</a> for more.
* @param request the request with the user to delete
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @return the response from the delete user call
* @throws IOException in case there is a problem sending the request or parsing back the response
*/
public DeleteUserResponse deleteUser(DeleteUserRequest request, RequestOptions options) throws IOException {
return restHighLevelClient.performRequestAndParseEntity(request, SecurityRequestConverters::deleteUser, options,
DeleteUserResponse::fromXContent, singleton(404));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey I saw some updates on this PR and I just wanted to throw a reminder out that we are not going to do a singleton(404) here, because we want a delete that is not found to throw an exception. Also, last time we spoke you were going to change this to AcknowledgedResponse. <3

Copy link
Contributor Author

@iverase iverase Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried those approaches but they seem not to work. If I use AcknowledgedResponse, then you get the following error:

   > Throwable #1: java.io.IOException: Unable to parse response body for Response{requestLine=DELETE /_xpack/security/user/testUser?refresh=true HTTP/1.1, host=http://[::1]:58075, response=HTTP/1.1 200 OK}
   >    at __randomizedtesting.SeedInfo.seed([311ED8EAD4721921:2905907290C29FEC]:0)
   >    at org.elasticsearch.client.RestHighLevelClient.internalPerformRequest(RestHighLevelClient.java:1417)
   >    at org.elasticsearch.client.RestHighLevelClient.performRequest(RestHighLevelClient.java:1383)
   >    at org.elasticsearch.client.RestHighLevelClient.performRequestAndParseEntity(RestHighLevelClient.java:1350)
   >    at org.elasticsearch.client.SecurityClient.deleteUser(SecurityClient.java:114)
   >    at org.elasticsearch.client.documentation.SecurityDocumentationIT.testDeleteUser(SecurityDocumentationIT.java:158)
   >    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   >    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
   >    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   >    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
   >    at java.base/java.lang.Thread.run(Thread.java:834)
   > Caused by: java.lang.IllegalArgumentException: Required [acknowledged

I think I need to override the class to be able to parse the response. Then I tried not to catch the exception and then I got the following error:

   > Throwable #1: ElasticsearchStatusException[Unable to parse response body]; nested: ResponseException[method [DELETE], host [http://[::1]:57316], URI [/_xpack/security/user/testUser?refresh=true], status line [HTTP/1.1 404 Not Found]
   > {"found":false}]; nested: ResponseException[method [DELETE], host [http://[::1]:57316], URI [/_xpack/security/user/testUser?refresh=true], status line [HTTP/1.1 404 Not Found]
   > {"found":false}];
   >    at __randomizedtesting.SeedInfo.seed([1E08456835E2EAC7:6130DF071526C0A]:0)
   >    at org.elasticsearch.client.RestHighLevelClient.parseResponseException(RestHighLevelClient.java:1650)
   >    at org.elasticsearch.client.RestHighLevelClient.internalPerformRequest(RestHighLevelClient.java:1411)
   >    at org.elasticsearch.client.RestHighLevelClient.performRequest(RestHighLevelClient.java:1383)
   >    at org.elasticsearch.client.RestHighLevelClient.performRequestAndParseEntity(RestHighLevelClient.java:1350)
   >    at org.elasticsearch.client.SecurityClient.deleteUser(SecurityClient.java:113)
   >    at org.elasticsearch.client.documentation.SecurityDocumentationIT.testDeleteUser(SecurityDocumentationIT.java:166)
   >    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   >    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
   >    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   >    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
   >    at java.base/java.lang.Thread.run(Thread.java:834)
   >    Suppressed: ParsingException[Failed to parse object: expecting field with name [error] but found [found]]

Not sure if I can implement it in any other way without changing the underlaying implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also depend on which implementation of AcknowledgedResponse you are using, since we have two, yay duplication!!! You should have a look at StartRollupJobResponse, which is an example of changing the word from acknowledged to started. this should get you on the right track.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DeleteUserResponse is a subclass of AcknowledgedResponse. I am using the same pattern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh, duh... Sorry, ive been on vacation and full of turkey since last week.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the only thing here now is to remove singleton(404), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, happy you enjoyed your time off!

Yes, the issue we have is the 404 and as I showed before, the problem is that if I do not capture the error there is a parsing error because the framework is expecting a different type of message:

Suppressed: ParsingException[Failed to parse object: expecting field with name [error] but found [found]]

Not sure if we can avoid without changing the current implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, I misread the comment before. Since server side is not throwing exceptions, lets keep the singleton(404), so we dont have to mod the parsing code. This is something we should definitely fix server side.

}

/**
* Asynchronously deletes a user in the native realm.
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-delete-user.html">
* the docs</a> for more.
* @param request the request with the user to delete
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param listener the listener to be notified upon request completion
*/
public void deleteUserAsync(DeleteUserRequest request, RequestOptions options, ActionListener<DeleteUserResponse> listener) {
restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::deleteUser, options,
DeleteUserResponse::fromXContent, listener, singleton(404));
}

/**
* Create/Update a role mapping.
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-put-role-mapping.html">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.elasticsearch.client.security.GetPrivilegesRequest;
import org.elasticsearch.client.security.DeleteRoleMappingRequest;
import org.elasticsearch.client.security.DeleteRoleRequest;
import org.elasticsearch.client.security.DeleteUserRequest;
import org.elasticsearch.client.security.InvalidateTokenRequest;
import org.elasticsearch.client.security.GetRolesRequest;
import org.elasticsearch.client.security.PutRoleMappingRequest;
Expand Down Expand Up @@ -76,6 +77,17 @@ static Request putUser(PutUserRequest putUserRequest) throws IOException {
return request;
}

static Request deleteUser(DeleteUserRequest deleteUserRequest) {
String endpoint = new RequestConverters.EndpointBuilder()
.addPathPartAsIs("_xpack","security", "user")
.addPathPart(deleteUserRequest.getName())
.build();
Request request = new Request(HttpDelete.METHOD_NAME, endpoint);
RequestConverters.Params params = new RequestConverters.Params(request);
params.withRefreshPolicy(deleteUserRequest.getRefreshPolicy());
return request;
}

static Request putRoleMapping(final PutRoleMappingRequest putRoleMappingRequest) throws IOException {
final String endpoint = new RequestConverters.EndpointBuilder()
.addPathPartAsIs("_xpack/security/role_mapping")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.client.security;

import org.elasticsearch.client.Validatable;

import java.util.Objects;

/**
* A request to delete a user from the native realm.
*/
public final class DeleteUserRequest implements Validatable {

private final String name;
private final RefreshPolicy refreshPolicy;

public DeleteUserRequest(String name) {
this(name, RefreshPolicy.IMMEDIATE);
}

public DeleteUserRequest(String name, RefreshPolicy refreshPolicy) {
this.name = Objects.requireNonNull(name, "user name is required");
this.refreshPolicy = Objects.requireNonNull(refreshPolicy, "refresh policy is required");
}

public String getName() {
return name;
}

public RefreshPolicy getRefreshPolicy() {
return refreshPolicy;
}

@Override
public int hashCode() {
return Objects.hash(name, refreshPolicy);
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (obj == null) {
return false;
}
if (getClass() != obj.getClass()) {
return false;
}
final DeleteUserRequest other = (DeleteUserRequest) obj;

return (refreshPolicy == other.refreshPolicy) && Objects.equals(name, other.name);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.client.security;

import org.elasticsearch.client.core.AcknowledgedResponse;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;

/**
* Response for a user being deleted from the native realm
*/
public final class DeleteUserResponse extends AcknowledgedResponse {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can gut this altogether since optional has a "isPresent()" field. See #35470 and leave your thoughts there or here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im still on the fence for whether we even need a found true/false in the code since we return an optional. WDYT?


private static final String PARSE_FIELD_NAME = "found";

private static final ConstructingObjectParser<DeleteUserResponse, Void> PARSER = AcknowledgedResponse
.generateParser("delete_user_response", DeleteUserResponse::new, PARSE_FIELD_NAME);

public DeleteUserResponse(boolean acknowledged) {
super(acknowledged);
}

public static DeleteUserResponse fromXContent(final XContentParser parser) throws IOException {
return PARSER.parse(parser, null);
}

@Override
protected String getFieldName() {
return PARSE_FIELD_NAME;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import org.apache.http.client.methods.HttpDelete;
import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.client.security.AuthenticateResponse;
import org.elasticsearch.client.security.DeleteUserRequest;
import org.elasticsearch.client.security.DeleteUserResponse;
import org.elasticsearch.client.security.PutUserRequest;
import org.elasticsearch.client.security.PutUserResponse;
import org.elasticsearch.client.security.RefreshPolicy;
Expand Down Expand Up @@ -74,14 +76,22 @@ public void testAuthenticate() throws Exception {
assertThat(authenticateResponse.enabled(), is(true));

// delete user
final Request deleteUserRequest = new Request(HttpDelete.METHOD_NAME,
"/_xpack/security/user/" + putUserRequest.getUser().getUsername());
highLevelClient().getLowLevelClient().performRequest(deleteUserRequest);
final DeleteUserRequest deleteUserRequest =
new DeleteUserRequest(putUserRequest.getUser().getUsername(), putUserRequest.getRefreshPolicy());

final DeleteUserResponse deleteUserResponse =
execute(deleteUserRequest, securityClient::deleteUser, securityClient::deleteUserAsync);
assertThat(deleteUserResponse.isAcknowledged(), is(true));

// authentication no longer works
ElasticsearchStatusException e = expectThrows(ElasticsearchStatusException.class, () -> execute(securityClient::authenticate,
securityClient::authenticateAsync, authorizationRequestOptions(basicAuthHeader)));
assertThat(e.getMessage(), containsString("unable to authenticate user [" + putUserRequest.getUser().getUsername() + "]"));

// delete non-existing user
final DeleteUserResponse deleteUserResponse2 =
execute(deleteUserRequest, securityClient::deleteUser, securityClient::deleteUserAsync);
assertThat(deleteUserResponse2.isAcknowledged(), is(false));
}

private static User randomUser() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.elasticsearch.client.security.DeletePrivilegesRequest;
import org.elasticsearch.client.security.DeleteRoleMappingRequest;
import org.elasticsearch.client.security.DeleteRoleRequest;
import org.elasticsearch.client.security.DeleteUserRequest;
import org.elasticsearch.client.security.DisableUserRequest;
import org.elasticsearch.client.security.EnableUserRequest;
import org.elasticsearch.client.security.GetPrivilegesRequest;
Expand Down Expand Up @@ -80,6 +81,18 @@ public void testPutUser() throws IOException {
assertToXContentBody(putUserRequest, request.getEntity());
}

public void testDeleteUser() {
final String name = randomAlphaOfLengthBetween(4, 12);
final RefreshPolicy refreshPolicy = randomFrom(RefreshPolicy.values());
final Map<String, String> expectedParams = getExpectedParamsFromRefreshPolicy(refreshPolicy);
DeleteUserRequest deleteUserRequest = new DeleteUserRequest(name, refreshPolicy);
Request request = SecurityRequestConverters.deleteUser(deleteUserRequest);
assertEquals(HttpDelete.METHOD_NAME, request.getMethod());
assertEquals("/_xpack/security/user/" + name, request.getEndpoint());
assertEquals(expectedParams, request.getParameters());
assertNull(request.getEntity());
}

public void testPutRoleMapping() throws IOException {
final String username = randomAlphaOfLengthBetween(4, 7);
final String rolename = randomAlphaOfLengthBetween(4, 7);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
import org.elasticsearch.client.security.DeleteRoleMappingResponse;
import org.elasticsearch.client.security.DeleteRoleRequest;
import org.elasticsearch.client.security.DeleteRoleResponse;
import org.elasticsearch.client.security.DeleteUserRequest;
import org.elasticsearch.client.security.DeleteUserResponse;
import org.elasticsearch.client.security.DisableUserRequest;
import org.elasticsearch.client.security.EmptyResponse;
import org.elasticsearch.client.security.EnableUserRequest;
Expand Down Expand Up @@ -185,6 +187,67 @@ public void onFailure(Exception e) {
}
}

public void testDeleteUser() throws Exception {
RestHighLevelClient client = highLevelClient();
addUser(client, "testUser", "testPassword");

{
// tag::delete-user-request
DeleteUserRequest deleteUserRequest = new DeleteUserRequest(
"testUser"); // <1>
// end::delete-user-request

// tag::delete-user-execute
DeleteUserResponse deleteUserResponse = client.security().deleteUser(deleteUserRequest, RequestOptions.DEFAULT);
// end::delete-user-execute

// tag::delete-user-response
boolean found = deleteUserResponse.isAcknowledged(); // <1>
// end::delete-user-response
assertTrue(found);

// check if deleting the already deleted user again will give us a different response
deleteUserResponse = client.security().deleteUser(deleteUserRequest, RequestOptions.DEFAULT);
assertFalse(deleteUserResponse.isAcknowledged());
}

{
DeleteUserRequest deleteUserRequest = new DeleteUserRequest("testUser", RefreshPolicy.IMMEDIATE);

ActionListener<DeleteUserResponse> listener;
//tag::delete-user-execute-listener
listener = new ActionListener<DeleteUserResponse>() {
@Override
public void onResponse(DeleteUserResponse deleteUserResponse) {
// <1>
}

@Override
public void onFailure(Exception e) {
// <2>
}
};
//end::delete-user-execute-listener

// Replace the empty listener by a blocking listener in test
final CountDownLatch latch = new CountDownLatch(1);
listener = new LatchedActionListener<>(listener, latch);

//tag::delete-user-execute-async
client.security().deleteUserAsync(deleteUserRequest, RequestOptions.DEFAULT, listener); // <1>
//end::delete-user-execute-async

assertTrue(latch.await(30L, TimeUnit.SECONDS));
}
}

private void addUser(RestHighLevelClient client, String userName, String password) throws IOException {
User user = new User(userName, Collections.singletonList(userName));
PutUserRequest request = new PutUserRequest(user, password.toCharArray(), true, RefreshPolicy.NONE);
PutUserResponse response = client.security().putUser(request, RequestOptions.DEFAULT);
assertTrue(response.isCreated());
}

public void testPutRoleMapping() throws Exception {
final RestHighLevelClient client = highLevelClient();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.client.security;

import org.elasticsearch.common.bytes.BytesReference;
Expand Down
Loading