resolve indices for prefixed _all expressions #137330
Conversation
richard-dennehy
added
>non-issue
:Security/Security
| ); | ||
| } | ||
|
|
||
| public void testResolveIndexWithRemotePrefix() { |
There was a problem hiding this comment.
I noticed that I could just set all cross project requests to go down the "all indices" path and all the tests would pass, even though this is obviously wrong; I've added this test which fails under that scenario
There was a problem hiding this comment.
all the tests would pass
Do you mean all tests in this test class? I am surprised unless all existing tests are effectively resolving to all accessible indices. Is that the case? Also, I assume tests in other places, e.g. the serverless REST test, should fail?
There was a problem hiding this comment.
I think it's because the majority of the other tests in this file have CPS mode disabled; I'll see if the REST test guards against this, but I don't want it to be the only test that would fail - it makes the feedback loop too long
There was a problem hiding this comment.
Confirmed that the REST test fails when this functionality is implemented incorrectly
|
Pinging @elastic/es-security (Team:Security) |
| isAllIndices = crossProjectModeDecider.resolvesCrossProject(replaceable) | ||
| ? IndexNameExpressionResolver.isAllIndices( | ||
| indicesList(indicesRequest.indices()), | ||
| (expr) -> CrossProjectIndexExpressionsRewriter.rewriteIndexExpression( | ||
| expr, | ||
| authorizedProjects.originProjectAlias(), | ||
| authorizedProjects.allProjectAliases() | ||
| ).localExpression() | ||
| ) | ||
| : IndexNameExpressionResolver.isAllIndices( | ||
| indicesList(indicesRequest.indices()), | ||
| (expr) -> IndexNameExpressionResolver.splitSelectorExpression(expr).v1() | ||
| ); |
There was a problem hiding this comment.
This matches what I was thinking, i.e. let cases like *:_all go through the all indices handling. I suggest we use RemoteClusterAware#splitIndexName instead of rewriteIndexExpression. It is lighter and we don't care about resolving projects at this point. It also needs to handle syntax like selector. I'd prefer we don't error out at isAllIndices check. The unsupported syntax will still be caught later when we call rewriteIndexExpression. So overall I am suggeting something like
IndexNameExpressionResolver.isAllIndices(
indicesList(indicesRequest.indices()),
expr -> {
IndexNameExpressionResolver.splitSelectorExpression(RemoteClusterAware.splitIndexName(expr)[1]).v1()
}
)There was a problem hiding this comment.
It appears this isn't enough, as this means any expression targeting _all will resolve all indices on the local project, even if that expression is e.g. some_remote_project:_all.
We need to check if the expression is _all, and that the prefix includes the local project. It looks to me like rewriteIndexExpression handles all the edge cases here, even if it does more than what's necessary here.
| ); | ||
| } | ||
|
|
||
| public void testResolveIndexWithRemotePrefix() { |
There was a problem hiding this comment.
all the tests would pass
Do you mean all tests in this test class? I am surprised unless all existing tests are effectively resolving to all accessible indices. Is that the case? Also, I assume tests in other places, e.g. the serverless REST test, should fail?
elasticsearchmachine
added
the
serverless-linked
3 participants
Follow up to #135425
As identified, expressions such as
*:_allcurrently don't resolve correctly (it appears that it currently tries to authorize the user against the literal index "_all").This PR fixes the resolution of prefixed
_allexpressions.Relates: ES-13213