Skip to content

Commit 46c7b5e

Browse files
jaymodejaymode
committed
Revert "Security: don't call prepare index for reads (elastic#34246)"
This reverts commit 0b4e8db as some issues have been identified with the changed handling of a primary shard of the security index not being available.
1 parent 18aa1c1 commit 46c7b5e

File tree

12 files changed

+207
-346
lines changed

12 files changed

+207
-346
lines changed

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

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

121-
if (securityIndex.isAvailable() == false) {
121+
if (securityIndex.indexExists() == false) {
122+
// TODO remove this short circuiting and fix tests that fail without this!
122123
listener.onResponse(Collections.emptyList());
123124
} else if (userNames.length == 1) { // optimization for single user lookup
124125
final String username = userNames[0];
125126
getUserAndPassword(username, ActionListener.wrap(
126127
(uap) -> listener.onResponse(uap == null ? Collections.emptyList() : Collections.singletonList(uap.user())),
127128
handleException));
128129
} else {
129-
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () -> {
130+
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> {
130131
final QueryBuilder query;
131132
if (userNames == null || userNames.length == 0) {
132133
query = QueryBuilders.termQuery(Fields.TYPE.getPreferredName(), USER_DOC_TYPE);
@@ -154,10 +155,10 @@ public void getUsers(String[] userNames, final ActionListener<Collection<User>>
154155
}
155156

156157
void getUserCount(final ActionListener<Long> listener) {
157-
if (securityIndex.isAvailable() == false) {
158+
if (securityIndex.indexExists() == false) {
158159
listener.onResponse(0L);
159160
} else {
160-
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () ->
161+
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
161162
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
162163
client.prepareSearch(SECURITY_INDEX_NAME)
163164
.setQuery(QueryBuilders.termQuery(Fields.TYPE.getPreferredName(), USER_DOC_TYPE))
@@ -181,10 +182,11 @@ public void onFailure(Exception e) {
181182
* Async method to retrieve a user and their password
182183
*/
183184
private void getUserAndPassword(final String user, final ActionListener<UserAndPassword> listener) {
184-
if (securityIndex.isAvailable() == false) {
185+
if (securityIndex.indexExists() == false) {
186+
// TODO remove this short circuiting and fix tests that fail without this!
185187
listener.onResponse(null);
186188
} else {
187-
securityIndex.checkIndexVersionThenExecute(listener::onFailure, () ->
189+
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
188190
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
189191
client.prepareGet(SECURITY_INDEX_NAME,
190192
INDEX_TYPE, getIdForUser(USER_DOC_TYPE, user)).request(),
@@ -457,28 +459,24 @@ public void onFailure(Exception e) {
457459
}
458460

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

475474
@Override
476475
public void onFailure(Exception e) {
477476
listener.onFailure(e);
478477
}
479478
}, client::delete);
480-
});
481-
}
479+
});
482480
}
483481

484482
/**
@@ -500,10 +498,11 @@ void verifyPassword(String username, final SecureString password, ActionListener
500498
}
501499

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

544543
void getAllReservedUserInfo(ActionListener<Map<String, ReservedUserInfo>> listener) {
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)
544+
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
545+
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
546+
client.prepareSearch(SECURITY_INDEX_NAME)
551547
.setQuery(QueryBuilders.termQuery(Fields.TYPE.getPreferredName(), RESERVED_USER_TYPE))
552548
.setFetchSource(true).request(),
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 :
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 :
558554
"there are more than 10 reserved users we need to change this to retrieve them all!";
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) :
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) :
565561
"id [" + id + "] does not start with reserved-user prefix";
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-
}
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));
576571
}
577-
listener.onResponse(userInfos);
578572
}
573+
listener.onResponse(userInfos);
574+
}
579575

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-
}
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);
589584
}
590-
}, client::search));
591-
}
585+
}
586+
}, client::search));
592587
}
593588

594589
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) {
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()))
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()))
230232
.setRefreshPolicy(request.getRefreshPolicy())
231233
.request(),
232-
new ActionListener<DeleteResponse>() {
234+
new ActionListener<DeleteResponse>() {
233235

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

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);
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);
244246

245-
}
246-
}, client::delete);
247-
});
248-
}
247+
}
248+
}, client::delete);
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.isAvailable() == false) {
304+
if (securityIndex.indexExists() == false) {
305305
reportStats(listener, Collections.emptyList());
306306
} else {
307307
getMappings(ActionListener.wrap(mappings -> reportStats(listener, mappings), listener::onFailure));

0 commit comments

Comments
 (0)