Skip to content

Commit 37501e7

Browse files
jaymodejaymode
committed
Revert "Revert "Security: don't call prepare index for reads (elastic#34246)""
This reverts commit 46c7b5e.
1 parent 46c7b5e commit 37501e7

File tree

12 files changed

+346
-207
lines changed

12 files changed

+346
-207
lines changed

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java

Lines changed: 92 additions & 78 deletions
Large diffs are not rendered by default.

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java

Lines changed: 57 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -118,16 +118,15 @@ public void getUsers(String[] userNames, final ActionListener<Collection<User>>
118118
}
119119
};
120120

121-
if (securityIndex.indexExists() == false) {
122-
// TODO remove this short circuiting and fix tests that fail without this!
121+
if (securityIndex.isAvailable() == false) {
123122
listener.onResponse(Collections.emptyList());
124123
} else if (userNames.length == 1) { // optimization for single user lookup
125124
final String username = userNames[0];
126125
getUserAndPassword(username, ActionListener.wrap(
127126
(uap) -> listener.onResponse(uap == null ? Collections.emptyList() : Collections.singletonList(uap.user())),
128127
handleException));
129128
} else {
130-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> {
129+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> {
131130
final QueryBuilder query;
132131
if (userNames == null || userNames.length == 0) {
133132
query = QueryBuilders.termQuery(Fields.TYPE.getPreferredName(), USER_DOC_TYPE);
@@ -155,10 +154,10 @@ public void getUsers(String[] userNames, final ActionListener<Collection<User>>
155154
}
156155

157156
void getUserCount(final ActionListener<Long> listener) {
158-
if (securityIndex.indexExists() == false) {
157+
if (securityIndex.isAvailable() == false) {
159158
listener.onResponse(0L);
160159
} else {
161-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
160+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () ->
162161
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
163162
client.prepareSearch(SECURITY_INDEX_NAME)
164163
.setQuery(QueryBuilders.termQuery(Fields.TYPE.getPreferredName(), USER_DOC_TYPE))
@@ -182,11 +181,10 @@ public void onFailure(Exception e) {
182181
* Async method to retrieve a user and their password
183182
*/
184183
private void getUserAndPassword(final String user, final ActionListener<UserAndPassword> listener) {
185-
if (securityIndex.indexExists() == false) {
186-
// TODO remove this short circuiting and fix tests that fail without this!
184+
if (securityIndex.isAvailable() == false) {
187185
listener.onResponse(null);
188186
} else {
189-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
187+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () ->
190188
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
191189
client.prepareGet(SECURITY_INDEX_NAME,
192190
INDEX_TYPE, getIdForUser(USER_DOC_TYPE, user)).request(),
@@ -459,24 +457,28 @@ public void onFailure(Exception e) {
459457
}
460458

461459
public void deleteUser(final DeleteUserRequest deleteUserRequest, final ActionListener<Boolean> listener) {
462-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> {
463-
DeleteRequest request = client.prepareDelete(SECURITY_INDEX_NAME,
460+
if (securityIndex.isAvailable() == false) {
461+
listener.onResponse(false);
462+
} else {
463+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> {
464+
DeleteRequest request = client.prepareDelete(SECURITY_INDEX_NAME,
464465
INDEX_TYPE, getIdForUser(USER_DOC_TYPE, deleteUserRequest.username())).request();
465-
request.setRefreshPolicy(deleteUserRequest.getRefreshPolicy());
466-
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, request,
466+
request.setRefreshPolicy(deleteUserRequest.getRefreshPolicy());
467+
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, request,
467468
new ActionListener<DeleteResponse>() {
468469
@Override
469470
public void onResponse(DeleteResponse deleteResponse) {
470471
clearRealmCache(deleteUserRequest.username(), listener,
471-
deleteResponse.getResult() == DocWriteResponse.Result.DELETED);
472+
deleteResponse.getResult() == DocWriteResponse.Result.DELETED);
472473
}
473474

474475
@Override
475476
public void onFailure(Exception e) {
476477
listener.onFailure(e);
477478
}
478479
}, client::delete);
479-
});
480+
});
481+
}
480482
}
481483

482484
/**
@@ -498,11 +500,10 @@ void verifyPassword(String username, final SecureString password, ActionListener
498500
}
499501

500502
void getReservedUserInfo(String username, ActionListener<ReservedUserInfo> listener) {
501-
if (securityIndex.indexExists() == false) {
502-
// TODO remove this short circuiting and fix tests that fail without this!
503+
if (securityIndex.isAvailable() == false) {
503504
listener.onResponse(null);
504505
} else {
505-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
506+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () ->
506507
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
507508
client.prepareGet(SECURITY_INDEX_NAME, INDEX_TYPE,
508509
getIdForUser(RESERVED_USER_TYPE, username)).request(),
@@ -541,49 +542,53 @@ public void onFailure(Exception e) {
541542
}
542543

543544
void getAllReservedUserInfo(ActionListener<Map<String, ReservedUserInfo>> listener) {
544-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
545-
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
546-
client.prepareSearch(SECURITY_INDEX_NAME)
545+
if (securityIndex.isAvailable() == false) {
546+
listener.onResponse(Collections.emptyMap());
547+
} else {
548+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () ->
549+
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
550+
client.prepareSearch(SECURITY_INDEX_NAME)
547551
.setQuery(QueryBuilders.termQuery(Fields.TYPE.getPreferredName(), RESERVED_USER_TYPE))
548552
.setFetchSource(true).request(),
549-
new ActionListener<SearchResponse>() {
550-
@Override
551-
public void onResponse(SearchResponse searchResponse) {
552-
Map<String, ReservedUserInfo> userInfos = new HashMap<>();
553-
assert searchResponse.getHits().getTotalHits() <= 10 :
553+
new ActionListener<SearchResponse>() {
554+
@Override
555+
public void onResponse(SearchResponse searchResponse) {
556+
Map<String, ReservedUserInfo> userInfos = new HashMap<>();
557+
assert searchResponse.getHits().getTotalHits() <= 10 :
554558
"there are more than 10 reserved users we need to change this to retrieve them all!";
555-
for (SearchHit searchHit : searchResponse.getHits().getHits()) {
556-
Map<String, Object> sourceMap = searchHit.getSourceAsMap();
557-
String password = (String) sourceMap.get(Fields.PASSWORD.getPreferredName());
558-
Boolean enabled = (Boolean) sourceMap.get(Fields.ENABLED.getPreferredName());
559-
final String id = searchHit.getId();
560-
assert id != null && id.startsWith(RESERVED_USER_TYPE) :
559+
for (SearchHit searchHit : searchResponse.getHits().getHits()) {
560+
Map<String, Object> sourceMap = searchHit.getSourceAsMap();
561+
String password = (String) sourceMap.get(Fields.PASSWORD.getPreferredName());
562+
Boolean enabled = (Boolean) sourceMap.get(Fields.ENABLED.getPreferredName());
563+
final String id = searchHit.getId();
564+
assert id != null && id.startsWith(RESERVED_USER_TYPE) :
561565
"id [" + id + "] does not start with reserved-user prefix";
562-
final String username = id.substring(RESERVED_USER_TYPE.length() + 1);
563-
if (password == null) {
564-
listener.onFailure(new IllegalStateException("password hash must not be null!"));
565-
return;
566-
} else if (enabled == null) {
567-
listener.onFailure(new IllegalStateException("enabled must not be null!"));
568-
return;
569-
} else {
570-
userInfos.put(username, new ReservedUserInfo(password.toCharArray(), enabled, false));
566+
final String username = id.substring(RESERVED_USER_TYPE.length() + 1);
567+
if (password == null) {
568+
listener.onFailure(new IllegalStateException("password hash must not be null!"));
569+
return;
570+
} else if (enabled == null) {
571+
listener.onFailure(new IllegalStateException("enabled must not be null!"));
572+
return;
573+
} else {
574+
userInfos.put(username, new ReservedUserInfo(password.toCharArray(), enabled, false));
575+
}
571576
}
577+
listener.onResponse(userInfos);
572578
}
573-
listener.onResponse(userInfos);
574-
}
575579

576-
@Override
577-
public void onFailure(Exception e) {
578-
if (e instanceof IndexNotFoundException) {
579-
logger.trace("could not retrieve built in users since security index does not exist", e);
580-
listener.onResponse(Collections.emptyMap());
581-
} else {
582-
logger.error("failed to retrieve built in users", e);
583-
listener.onFailure(e);
580+
@Override
581+
public void onFailure(Exception e) {
582+
if (e instanceof IndexNotFoundException) {
583+
logger.trace("could not retrieve built in users since security index does not exist", e);
584+
listener.onResponse(Collections.emptyMap());
585+
} else {
586+
logger.error("failed to retrieve built in users", e);
587+
listener.onFailure(e);
588+
}
584589
}
585-
}
586-
}, client::search));
590+
}, client::search));
591+
}
587592
}
588593

589594
private <Response> void clearRealmCache(String username, ActionListener<Response> listener, Response response) {

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/mapper/NativeRoleMappingStore.java

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -220,32 +220,32 @@ public void onFailure(Exception e) {
220220
});
221221
}
222222

223-
private void innerDeleteMapping(DeleteRoleMappingRequest request, ActionListener<Boolean> listener) throws IOException {
224-
if (securityIndex.isIndexUpToDate() == false) {
225-
listener.onFailure(new IllegalStateException(
226-
"Security index is not on the current version - the native realm will not be operational until " +
227-
"the upgrade API is run on the security index"));
228-
return;
229-
}
230-
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
231-
client.prepareDelete(SECURITY_INDEX_NAME, SECURITY_GENERIC_TYPE, getIdForName(request.getName()))
223+
private void innerDeleteMapping(DeleteRoleMappingRequest request, ActionListener<Boolean> listener) {
224+
if (securityIndex.isAvailable() == false) {
225+
listener.onResponse(false);
226+
} else {
227+
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> {
228+
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
229+
client.prepareDelete(SECURITY_INDEX_NAME, SECURITY_GENERIC_TYPE, getIdForName(request.getName()))
232230
.setRefreshPolicy(request.getRefreshPolicy())
233231
.request(),
234-
new ActionListener<DeleteResponse>() {
232+
new ActionListener<DeleteResponse>() {
235233

236-
@Override
237-
public void onResponse(DeleteResponse deleteResponse) {
238-
boolean deleted = deleteResponse.getResult() == DELETED;
239-
listener.onResponse(deleted);
240-
}
234+
@Override
235+
public void onResponse(DeleteResponse deleteResponse) {
236+
boolean deleted = deleteResponse.getResult() == DELETED;
237+
listener.onResponse(deleted);
238+
}
241239

242-
@Override
243-
public void onFailure(Exception e) {
244-
logger.error(new ParameterizedMessage("failed to delete role-mapping [{}]", request.getName()), e);
245-
listener.onFailure(e);
240+
@Override
241+
public void onFailure(Exception e) {
242+
logger.error(new ParameterizedMessage("failed to delete role-mapping [{}]", request.getName()), e);
243+
listener.onFailure(e);
246244

247-
}
248-
}, client::delete);
245+
}
246+
}, client::delete);
247+
});
248+
}
249249
}
250250

251251
/**
@@ -301,7 +301,7 @@ private void getMappings(ActionListener<List<ExpressionRoleMapping>> listener) {
301301
* </ul>
302302
*/
303303
public void usageStats(ActionListener<Map<String, Object>> listener) {
304-
if (securityIndex.indexExists() == false) {
304+
if (securityIndex.isAvailable() == false) {
305305
reportStats(listener, Collections.emptyList());
306306
} else {
307307
getMappings(ActionListener.wrap(mappings -> reportStats(listener, mappings), listener::onFailure));

0 commit comments

Comments
 (0)