Thorough integration tests for index API authorization

[nibix]

Description

This introduces new integration tests for authorization against index APIs.

In order to be as thorough as possible, the test suites use parameters to define several test dimensions:

• Users with different privileges
• Cluster configurations (at the moment system index privilege disabled vs enabled)
• The final dimension are different operations with different options; this is manifested by the test methods

This is mostly a preparation for #5399; it will give us a good overview over the changed behaviors and allows us to judge whether we want to change a behavior or not.

• Category: Tests
• Why these changes are required? Preparation for Introduced explicit index resolution API #5399
• What is the old behavior before changes and new behavior after changes? No behavioral changes

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.88%. Comparing base (ebed295) to head (66f19f0).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5632      +/-   ##
==========================================
- Coverage   72.96%   72.88%   -0.08%     
==========================================
  Files         414      420       +6     
  Lines       25873    26269     +396     
  Branches     3934     3974      +40     
==========================================
+ Hits        18879    19147     +268     
- Misses       5083     5214     +131     
+ Partials     1911     1908       -3     

see 46 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cwperks]

To make sure I understand correctly, legacy here is related to how DNFOF relates to system index authorization?

[nibix]

Good point.

The naming is already in anticipation of the enhanced privilege evaluation PR which introduces one further element to the enum; thus, the enum looks like this:

security/src/integrationTest/java/org/opensearch/security/privileges/int_tests/ClusterConfig.java

Lines 20 to 38 in b3e2cca

	public enum ClusterConfig {
	LEGACY_PRIVILEGES_EVALUATION(
	"legacy",
	c -> c.doNotFailOnForbidden(true).nodeSettings(Map.of("plugins.security.system_indices.enabled", true)),
	true,
	false,
	false
	),
	LEGACY_PRIVILEGES_EVALUATION_SYSTEM_INDEX_PERMISSION(
	"legacy_system_index_perm",
	c -> c.doNotFailOnForbidden(true)
	.nodeSettings(
	Map.of("plugins.security.system_indices.enabled", true, "plugins.security.system_indices.permission.enabled", true)
	),
	true,
	true,
	false
	),
	NEXT_GEN_PRIVILEGES_EVALUATION("next_gen", c -> c.privilegesEvaluationType("next_gen"), false, true, true);

Thus, currently, this is not really legacy. But maybe we can keep it this way, as it is only temporary and avoids lots of renaming?

But, generally, I am also not sure about a good way to name these things. Also I am not too happy about this "next_gen" name.

[cwperks]

Thank you @nibix ! This is quite a large PR (even if only test files). It may make sense to break this up into smaller PRs for sake of review similar to what was done to the integrationTest framework. Would there be any good opportunities for splitting this PR up into smaller ones?

[nibix]

@cwperks

Would there be any good opportunities for splitting this PR up into smaller ones?

Well, we have 5 main test classes:

• IndexAuthorizationReadOnlyIntTests
• IndexAuthorizationReadWriteIntTests
• DataStreamAuthorizationReadOnlyIntTests
• DataStreamAuthorizationReadWriteIntTests
• SnapshotAuthorizationIntTests

Thus, one could break it down into 5 PRs:

• IndexAuthorizationReadOnlyIntTests and all the framework stuff
• One PR for the remaining classes

Still, I am not sure if this will be really helpful, as this does not really change the grouping of the changes. Already now, you could review class by class.

Also, the test cases themselves are then quite repetetive; if you reviewed the first few test cases, you will likely just skim over the remaining ones. In a few cases, these document some oddities; but in most cases, these will be addressed in the enhanced index resolution PR. In the end, this PR is only a preparation for the next one.

[DarshitChanpura]

Hi @nibix Given that this is a huge PR i did my best to thoroughly review the PR and have left some comments and clarification questions.

Could this be broken down into smaller PRs? i.e. data-stream, index and snapshot tests individually?

[DarshitChanpura]

why do we need this? would defaultObjectMapper not work?

[nibix]

JSON path allows you to use expressions like "$.data_streams[*].name" to extract information from JSON documents in a very versatile manner. This is not supported by Jackson (ObjectMapper).

[DarshitChanpura]

i'm confused, how does this validate that "this" user is an adminCert user? or is the point not to validate?

[nibix]

This is evaluated here and has the effect that a admin client certificate is used to create the connection:

security/src/integrationTest/java/org/opensearch/test/framework/cluster/OpenSearchClientProvider.java

Lines 107 to 112 in 2c321d6

	default TestRestClient getRestClient(UserCredentialsHolder user, Header... headers) {
	if (user.isAdminCertUser()) {
	return getRestClient(getTestCertificates().getAdminCertificateData());
	}
	return getRestClient(user.getName(), user.getPassword(), null, headers);
	}

[DarshitChanpura]

why return true on non 200 code? is it because of empty list of indices and non-existent status code entry ?

[nibix]

This is the case that an empty set of indices is expected.

If we have a 200 response, we have a valid response and can check this in the response body. But if we do not have 200 response, we must/can just assume that we did not get any indices and stop without further checks.

[DarshitChanpura]

will unlimited user see less results than limited users?

[nibix]

yes (still, both will only see authorized indices), the limited users will just see more than they requested (the request shall request no indices).

[DarshitChanpura]

should it be forbidden for B1 user with dnfof enabled?

[nibix]

That's also a bug, to be fixed in the follow up PR

[DarshitChanpura]

both blocks are checking same 403 response. Can be clubbed together?

[nibix]

For the first branch, this is a bug, for the else branch, this is expected behavior. That's why i kept it separate with the comments explaining the behavior.

[DarshitChanpura]

should we fix this?

[nibix]

This will be fixed by #5399