[Feature/extensions] Remove CommonUtils ThreadContext User from AD user checks

[dbwiddis]

Signed-off-by: Daniel Widdis [email protected]

Description

This copies the User class over from the Common Utils into a UserIdentity class. Other than the class rename and addition of javadocs, there are no changes to the class. This is done primarily to remove the external dependency but also preserve the intention (user and roles) as a placeholder until a replacement security authentication mechanism is implemented, and retains existing functionality.

In places where the User is retrieved from the thread context, a null user is used, as the original designers of this plugin determined that null = super user. This will not be the path going forward but it is at least very convenient for us now.

Final changes were made to tests to recognize the lack of failed access, and coverage which is reduced by never executing code to respond to failures.

Issues Resolved

Fixes opensearch-project/opensearch-sdk-java#23

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.

[dbwiddis]

Submitted for review. Can't get tests to run due to 403 on snapshot repo. I believe the below test should fail and will comment it out if so.

anomaly-detection/src/test/java/org/opensearch/ad/transport/handler/ADSearchHandlerTests.java

Lines 85 to 92 in 4baf75c

	public void testFilterEnabled() {
	settings = Settings.builder().put(FILTER_BY_BACKEND_ROLES.getKey(), true).build();
	clusterService = new ClusterService(settings, clusterSettings, null);
	searchHandler = new ADSearchHandler(settings, clusterService, client);
	searchHandler.search(matchAllRequest(), listener);
	verify(client, times(1)).search(any(), any());
	}

[dbwiddis]

@owaiskazi19 @saratvemulapalli This is ready for review. Summary of commits:

• 66de852 Copied User class from Common Utils to a UserIdentity class here. Intention is:
  • Remove external dependency without changing other code (yet). This minimizes unnecessary code churn.
  • Keep a class as a placeholder where we will be using it to pass access tokens under the new security model.
• 4473415 Deprecated the code which fetched the user from the Thread Context. Created a new temporary "null User" method that simply returns null (granting super-admin privileges).
  • Could have modified the other code to substitute null instead of calling a "null User" method but this approach makes it easy to find those methods again in the future by tracing call hierarchy, instead of looking for uses of "null" in the codebase.
  • Eventually there will be a new method here to query the security plugin/core API/etc. to get the user access token (split token model) which will replace the temporary "null user" code.
• bd83a39 Updated tests which were intended to fail the user access checks, which now always pass thanks to the null (superadmin) user, so all test pass
• 633c18d The classes impacted by lack of failures have reduced code coverage. Excluded them from JaCoCo coverage temporarily. When the new security user code is in, we'll remove the exclusions.

[saratvemulapalli]

Thanks @dbwiddis for the changes.
Also thanks for putting in placeholders.

@peternied could you take a look as well.

[peternied]

In places where the User is retrieved from the thread context, a null user is used, as the original designers of this plugin determined that null = super user. This will not be the path going forward but it is at least very convenient for us now.

I would rather those checks for a null user became checks for UserIdentity.Admin that was returned from getUser when security was disabled.

This makes the more dangerous kind of actions much clearer in appearance as oppose to 'input validation' where we are checking for nulls.

if (getUser() == UserIdentity.Admin) {
   // DO EVERYTHING!
   ...
}

How does this align to what you were thinking?

[dbwiddis]

I would rather those checks for a null user became checks for UserIdentity.Admin that was returned from getUser when security was disabled.

I completely agree with this preference and a future direction, but:

• We haven't finished defining the UserIdenity class. Right now it's a carbon copy of User from Common Utils but we probably don't need any of the guts inside it. They are there more as a reference for what a user used to be.
• The fact is that the current code accepts null as a superuser, and hiding that flaw makes it less likely to fix it later! I'd love to simultaneously switch from null user to an admin user along with disabling the superpower of null. This is like putting a big orange cone on (or in) a pothole. Doesn't fix the hole but at least you know driving down the street requires care and you can remove the cone when you fix the road.
• There will be even more work with integrating identity in the future that we can integrate into the UserIdentity class, and the less I require now the more flexibility we have in defining exactly what we need later.

But I hope my snarky comments in the getNullUser() class indicate my feelings about that not being permanent. :)

[dbwiddis]

This makes the more dangerous kind of actions much clearer

Another thought, there should actually be no need under a new RBAC model for the extension to even know anything about whether the user is a superadmin or an anonymous user. All we need is the access token that will be provided from the security features being moved from the plugin. We won't know what access it gives, we'll just pass it along with our API requests and the security features will identify the access roles at that point. So for the purposes of an extension, all users are the same.

[owaiskazi19]

LGTM. 🚢