Reduce excessive logs when non-admin users fetch registration ID

[zahidblackduck]

JIRA Ticket:
IDETECT-4209

Description:
This merge request addresses the issue of excessive warning logs generated when non-admin users attempt to fetch the registration ID on the endpoint /api/registration.
The solution involves using a ThreadLocal variable to manage the state of whether the getRegistrationId() method has been called before or not by a non-syadmin user. This ensures that repeated attempts to fetch the registration ID are avoided, thereby significantly reducing unnecessary warning logs.

Update:
BlackDuckRegistrationService has been refactored. Now, it follows an eager approach. getBlackDuckServerData() method has been overloaded to accept isAdmin boolean flag. The passed boolean isAdmin flag is stored in a static local variable.
Before calling getRegistrationId(), a check is done whether isAdmin is false and skip the call accordingly.
This avoids unnecessary calls to the method getRegistrationId(), thus reducing excessive logs in hub webapp container due to call to the endpoint /api/registration.

This fix is required for resolving the linked Detect issue.

[andrian-sevastyanov]

I wonder if there are other situations in which IntegrationException could be encountered here, not only permissions issues. Is there perhaps something within the exception that can be checked to confirm that that's indeed the underlying reason for the exception?

[zahidblackduck]

I see. This exception could be thrown for any other reason too. I was thinking whether we could use any utility method like isAdmin(...) that would return a boolean and based on that we could evaluate this state setting/updating instead of setting that inside the catch.
The state could also be passed from the detect to the blackduck-common cause detect has the knowledge of the user who is trying to call this method via BlackDuckRegistrationService#getBlackDuckServerData() method.

I was thinking of overloading BlackDuckRegistrationService#getBlackDuckServerData(...) method which will accept a boolean flag isAdmin or any relevant state. Instead of the calling the previous one we can call this overloaded one while checking for connectivity in BlackDuckConnectivityChecker class. That'll allow us to set the state whether the user is admin or not during the detect boot-up.

That's a great catch. Thanks!

[shantyk]

I believe if we get a 403 FORBIDDEN (which would throw IntegrationException), setting isRegistrationIdFetchAllowed to false and not trying to call that endpoint again makes sense since the user permissions are unlikely to change during the rest of the scan run. But for different responses/exceptions, maybe we keep the behaviour the same and don't update the boolean?

[zahidblackduck]

I wonder if there are other situations in which IntegrationException could be encountered here, not only permissions issues. Is there perhaps something within the exception that can be checked to confirm that that's indeed the underlying reason for the exception?

I've updated the code a bit here. Setting the isRegistrationIdFetchAllowed based on the blackDuckErrorCode. If the exception contains core.rest.unauthorized code, we set this variable false.

[andrian-sevastyanov]

Setting this to true initially could be misleading to someone looking at this code down the road and that could lead to unintended consequences.
Renaming the variable to something like attemptAdminOperations might be safer and more correct.

Also, we could consider taking out this variable completely like this:

1. change the new getBlackDuckServerData(boolean) method to do the bulk of the work
2. have the existing getBlackDuckServerData() method call the new one

And yet another option: depending on how this class is used, maybe we should have a more specific method that returns only what's required? For example, if Detect knows that the only thing it needs to look up is the BD version, we could expose a public getBlackDuckServerVersion() and call that in Detect.

[zahidblackduck]

Thanks, Andrian, for the nice suggestion. I've renamed the variable to adminOperationAttempted and refactored the getBlackDuckServerData(boolean) method to handle most of the logic, with getBlackDuckServerData() now delegating to it.

However, I believe we still need a variable to track the state of the user. This method is invoked multiple times throughout the application's execution while the connection remains active. If we were to pass the isAdmin flag explicitly for an user each time this method is called, we'd need to determine the user's admin status beforehand.

In detect, we can retrieve this information in BlackDuckConnectivityChecker based on the user's role. However, in BlackDuckPhoneHomeHelper, where getBlackDuckServerData() is also called during phone homing, user details are not directly available. To fetch them, we'd need access to UserService, which isn't currently available there.

Given this, passing the isAdmin flag when determining connectivity in BlackDuckConnectivityChecker#determineConnectivity(..) and maintaining that state throughout the runtime of Detect seems like a better approach as of now.

[andrian-sevastyanov]

Looks like you renamed the variable AND flipped the initial value.
This means that we will not be preserving the original functionality of getBlackDuckServerData().
As far as functionality goes, we should probably keep it the same as it was since we don't know all the usages.

[zahidblackduck]

I see. Let me re-evaluate this. Thanks for pointing out the concern.

[zahidblackduck]

Hi @andrian-sevastyanov, it seemed logical to me to set the initial value of the variable to false in the latest changes.
Cause, if the user is not sysadmin, why allow the user to make call to the endpoint /api/registration to fetch the registration id at all? For non-sysadmin user, the getBlackDuckServerData() method should be functional as it was before.
For reference, there's a comment just above the codeblock where registration id is fetched, probably from Kerwin dating back to 2020.

// We need to wrap this because this will most likely fail unless they are running as an admin
registrationId = getRegistrationId();

Regarding an user's sysadmin status check, it should be passed from the detect's BlackDuckConnectivityChecker#determineConnectivity(..) method, which runs during detect startup.
Here's a linked MR that I created in detect to accommodate this change.
blackducksoftware/detect#1390

[andrian-sevastyanov]

A hypothetical scenario I'm thinking about is:

• non-Detect app that uses this code that we don't control (maybe Alert?)
• the app calls getBlackDuckServerData() while in admin mode
• the app maintainers are not aware of the change we are making and so they do not pass in the needed isAdmin value

This is why we should probably preserve the original functionality of getBlackDuckServerData() when no flag has been passed in.