Security remove datemath special handling

[albertzaharovits]

This is a composite PR that removes a number of index expression resolving behaviors related to datemath evaluation.
Most of the removed cases were redundant; there was code elsewhere that handled the same cases.

There are however two esoteric behaviors that were not redundant, but which were actually bugs (see below the discussion for each change):

• 9a4d8b0 the removed behavior is redundant because the includeDataStreams==false and indexAbstraction.getType() == Type.DATA_STREAM condition is handled in RBACEngine

  elasticsearch/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java

  Line 816 in 87eb40a

  if (includeDataStreams) {

  Moreover, it is also handled by expression resolving code in Core:

  elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

  Line 1289 in a1d8b77

  if (indexAbstraction.isDataStreamRelated() && context.includeDataStreams() == false) {

• ca1ac18 the behavior is safe to remove because there are currently no system datemath datastreams. It is desirable to be removed because this shouldn't be done in Security, as Core code handles system index access.
• 1adf647 datemath expressions can't start with a ".", they must start with "<"
• fb8d8b4 the alias existence check here is redundant because the same is already handled in Core:

  elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

  Line 1286 in 1f265eb

  if (indexAbstraction.getType() == Type.ALIAS && options.ignoreAliases()) {

• f420aaa this is the first one of the bug fix changes . The special behavior for hidden datemath aliases and datastreams doesn't make any sense. The "hidden" index attribute controls wildcard expansion not datemath evaluation.
• 1939c85 this is the other bug fix change. As is the case everywhere else, exclusions should never trigger the "not found" exception for the name they are excluding.
• 6d7370e for the case where availableIndexAbstractions.contains(dateMathName) == false, i.e. index does not exist or is not authorized, we should still add the name to the resolved expression if indicesOptions.ignoreUnavailable() == false because:
  • if the name is not authorized, it will generate a 403 exception in

    elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java

    Line 384 in 56074ab

    final boolean overallGranted = isActionGranted(action, resources);

  • if the name does not exist, it will generate the 404 "not found" in Core:

    elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

    Line 1282 in 56074ab

    if (indexAbstraction == null) {

  • this is the behavior non-datemath expression items have, and so we're eliminating inconsistent behavior.

[elasticsearchmachine]

Pinging @elastic/es-security (Team:Security)

[elasticsearchmachine]

Hi @albertzaharovits, I've created a changelog YAML for you.

[jakelandis]

LGTM

Thanks for the details they helped. However there were a commits called out that only appear to be only be relevant to the this PR. For example: isDateMathVisible was introduced and removed all in this PR and for some changes I couldn't draw a line between those called out and the final changes here.

[albertzaharovits]

Thank you for the review!

However there were a commits called out that only appear to be only be relevant to the this PR. For example: isDateMathVisible was introduced and removed all in this PR and for some changes I couldn't draw a line between those called out and the final changes here.

Yes, it is true that some references in the explanations do not make sense in the final resultant codebase, as they were relevant only in the interim context that they were introduced in. I'll keep this feedback in mind, I think there was an opportunity to split this one up in two smaller PRs, and alleviate the issue you describe.