[FEATURE] Remove the ThreadContext dependency on the User from Common Utils for creating detector

[owaiskazi19]

Is your feature request related to a problem?

Since we are removing the dependency of Common Utils for detector. Auth User from the codebase required to create detectors can be removed.

What solution would you like?

A clear and concise description of what you want to happen.

What alternatives have you considered?

A clear and concise description of any alternative solutions or features you've considered.

Do you have any additional context?

Add any other context or screenshots about the feature request here.

[dbwiddis]

I'm a bit concerned with the language remove.

The concept of an authenticated user is integral to identity (see #14) and the existing implementation is depended on by the current authentication implementation (see #19).

I certainly agree that User needs to be moved elsewhere and/or possibly incorporated in a different identity/authentication model, but this particular issue/feature is going to be blocked by initial answers to the above two issues.

[owaiskazi19]

Hey @dbwiddis! The intention of this issue is only for the scope of creating a detector for AD plugin without security as our first feature of extensibility. We definitely want to add security and keep the other features of AD plugin in place moving forward. To get the detector piece out for now we are not focusing on scheduling the detector and checking for the User.

[dbwiddis]

Ah, got it. Probably need to change the title to give context of the detector. May apply to the other sub-issues from that meta issue as well.

[dbwiddis]

Initial commit removed the dependency on Common Utils. However, the X in the XY problem here is that we want to stop storing and retrieving the user from the ThreadContext. That's easily done by commenting out code and disabling tests, but I think more important is to leave placeholder comments outlining the functionality that will replace them under the new security model. I'll update the PR tonight with that added context (pun intended.)

[dbwiddis]

It turns out that someone decided that a null user was the super-admin, so this is easily accomplished by making the user null. For the record, I think this was a horrible design choice!

[dbwiddis]

Having connections always pass is good, except that the failures which never happen mean the test coverage is lower and is failing jacoco tests.

Options are to lower the jacoco threshold or comment out all the code for what to do when it fails (which we would have to put back once we get a new auth/permissions scheme in).

I favor lowering the jacoco threshold. Thoughts, @saratvemulapalli and @owaiskazi19 ?

Execution failed for task ':jacocoTestCoverageVerification'.
> Rule violated for class org.opensearch.ad.auth.UserIdentity: branches covered ratio is 0.58, but expected minimum is 0.60
  Rule violated for class org.opensearch.ad.auth.UserIdentity: lines covered ratio is 0.73, but expected minimum is 0.75
  Rule violated for class org.opensearch.ad.transport.IndexAnomalyDetectorTransportAction: branches covered ratio is 0.58, but expected minimum is 0.60
  Rule violated for class org.opensearch.ad.transport.handler.ADSearchHandler: branches covered ratio is 0.25, but expected minimum is 0.60

[owaiskazi19]

I favor lowering the jacoco threshold. Thoughts, @saratvemulapalli and @owaiskazi19 ?

Since we are working on our own feature branch for AD. I'm fine with lowering the threshold for now. We can change it back to the original value once we finish #20 which will eventually comment out the rest of the code of AD except create detector. Let's track this change somewhere may be on #20.

[saratvemulapalli]

Yeah +1. We can ignore them for now. But when we are ready for production use, this should match what we had.
May be lets track this as an issue overall for AD as an extension?

[dbwiddis]

In that case the PR is ready for review with the only needed change figuring out where to lower that threshold.

[dbwiddis]

Since we are working on our own feature branch for AD. I'm fine with lowering the threshold for now.

Looks like the thresholds apply to the whole project, but classes can be excluded, so I just excluded the problematic classes with a TODO to add them back when we've re-implemented security in the new model.

[dbwiddis]

Completed in opensearch-project/anomaly-detection#617