-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
[7.x] Add include_data_streams flag for authorization #59008
Conversation
Pinging @elastic/es-core-features (:Core/Features/Data streams) |
@elasticmachine run elasticsearch-ci/2 |
@elasticmachine run elasticsearch-ci/bwc |
@elasticmachine run elasticsearch-ci/default-distro |
@elasticmachine run elasticsearch-ci/oss-distro-docs |
@elasticmachine update branch |
@elasticmachine run elasticsearch-ci/1 |
@elasticmachine run elasticsearch-ci/2 |
…mann/elasticsearch into backport_7x_58154_ds_auth
@@ -207,7 +207,7 @@ public ClusterState execute(ClusterState currentState) { | |||
throw new ConcurrentSnapshotExecutionException(repositoryName, snapshotName, " a snapshot is already running"); | |||
} | |||
// Store newSnapshot here to be processed in clusterStateProcessed | |||
List<String> indices = Arrays.asList(indexNameExpressionResolver.concreteIndexNames(currentState, request)); | |||
indices = Arrays.asList(indexNameExpressionResolver.concreteIndexNames(currentState, request)); |
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.
😱 , but luckily assertions tripped!
assertEquals(1, indices.length); | ||
assertEquals(backingIndex.getIndex().getName(), indices[0]); | ||
} | ||
//{ |
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.
why is this commented out?
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 missed that in an earlier commit. The backport of #57900 will un-comment it.
@elasticmachine run elasticsearch-ci/2 |
1 similar comment
@elasticmachine run elasticsearch-ci/2 |
Most of the work around the addition of an
includeDataStreams
flag was done in #58381. The remainder of this PR adds the flag to the appropriate request classes, updatesIndexNameExpressionResolver
to rely on the flag rather than a separate boolean flag, and updates tests.This PR adds anincludeDataStreams
flag to theIndicesRequest
interface so that it is available whenAuthorizationEngine::loadAuthorizedIndices
is called. This is necessary to avoid different behaviors when security is enabled and disabled.The disparate behavior is described in the referenced issue. When aGET */_alias
request (or any other request that does not operate on data streams) is sent without security enabled, any data streams present are ignored by theIndexNameExpressionResolver
class and the correct HTTP 200 response is returned.When security is enabled, the star inGET */_alias
is first resolved to all authorized indices, aliases, and data streams. Because the_alias
endpoint does not understand data streams, the first data stream it encounters among the list of authorized indices is treated as an alias and an incorrect 404 for "unknown alias" is returned.This PR changesAuthorizationService
to selectively exclude (in the case of most requests) or include (and the case ofResolveIndexAction.Request
and other requests that opt in) data streams during the authorized index resolution phase.Especially interested in feedback from @elastic/es-security.Fixes #57712
Relates to #53100
Backport of #58154