Allow users to get status of own async search tasks #106638
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit moves code from AsyncTaskIndexService and DeleteAsyncResultsService into a new AsyncSearchSecurity so that all security code is centralised.,
tvernum
changed the title
This changs RBAC engine so users with access to submit async searches also have permission to retrive async search status. It relies on the fact that async search will check that the user owns the async search task
Manual testing results, comparing code on
|
…enforced while search is running
quux00
added
:Security/Authorization
|
Pinging @elastic/es-security (Team:Security) |
|
I will look into reviewing this tomorrow morning. |
This was referenced Mar 26, 2024
|
Hi @tvernum, I've created a changelog YAML for you. |
|
@elasticmachine update branch |
albertzaharovits
approved these changes
Mar 27, 2024
There was a problem hiding this comment.
LGTM, very neat! Thank you @tvernum! 👍
Also thank you @quux00 for the testing from #106638 (comment) ! This was very useful to me!
Re the 403 vs 404 issue. Previously, the 403 was returned because /async-search/status was simply not allowed for users without the monitor cluster privilege, for any async search id.
But the premise of this PR is that we want that to succeed (200), if the same user submitted the async search that's querying the status of. For the other, non-owned async searches, between 403 and 404, 404 is clearly favorable. Some reasons are: consistency with the async-search (result) endpoint and consistency with the /async-search/status endpoint, when Security is disabled.
@quux00 Re the separate bug that you've identified, when the user only has the monitor cluster privilege and is issuing a /async-search/status request with the keep_alive parameter, I believe a quick fix would look like:
diff --git a/x-pack/plugin/async-search/src/main/java/org/elasticsearch/xpack/search/TransportGetAsyncStatusAction.java b/x-pack/plugin/async-search/src/main/java/org/elasticsearch/xpack/search/TransportGetAsyncStatusAction.java
index cc5cd797f3f..cc27e82a693 100644
--- a/x-pack/plugin/async-search/src/main/java/org/elasticsearch/xpack/search/TransportGetAsyncStatusAction.java
+++ b/x-pack/plugin/async-search/src/main/java/org/elasticsearch/xpack/search/TransportGetAsyncStatusAction.java
@@ -35,6 +35,7 @@ import java.util.Objects;
import static org.elasticsearch.core.Strings.format;
import static org.elasticsearch.xpack.core.ClientHelper.ASYNC_SEARCH_ORIGIN;
+import static org.elasticsearch.xpack.core.async.AsyncTaskIndexService.getTask;
public class TransportGetAsyncStatusAction extends HandledTransportAction<GetAsyncStatusRequest, AsyncStatusResponse> {
private final TransportService transportService;
@@ -76,7 +77,7 @@ public class TransportGetAsyncStatusAction extends HandledTransportAction<GetAsy
if (request.getKeepAlive() != null && request.getKeepAlive().getMillis() > 0) {
long expirationTime = System.currentTimeMillis() + request.getKeepAlive().getMillis();
store.updateExpirationTime(searchId.getDocId(), expirationTime, ActionListener.wrap(p -> {
- AsyncSearchTask asyncSearchTask = store.getTaskAndCheckAuthentication(taskManager, searchId, AsyncSearchTask.class);
+ AsyncSearchTask asyncSearchTask = getTask(taskManager, searchId, AsyncSearchTask.class);
if (asyncSearchTask != null) {
asyncSearchTask.setExpirationTime(expirationTime);
}
Would you be available to handle this in a separate fix PR?
|
Thanks @albertzaharovits for the review and for the proposed fix to the existing bug. I will open a new PR with the patch you provided. |
|
I'd like to merge this, but two of the Hdfs tests under fips conditions are failing. I'm able to reproduce one locally, but it also fails on main so seems unrelated to this PR. Stack trace of failure: Issue filed here: #106845 |
this should be fixed on main (we ignore fips for hdfs). rebasing should fix the hdfs issue for you |
|
@elasticmachine update branch |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This consists of 3 changes:
AsyncSearchSecurityTransportGetAsyncStatusActionto check for ownership if the user does not have explicit access to theGetAsyncStatusAction(if they have such access it means that they can get the status of all async searches)GetAsyncStatusActionbut does have permission to submit async searches, then let them run the action, relying on point 2 above.