Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce excessive logs when non-admin users fetch registration ID #453

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Reduce excessive logs when non-admin users fetch registration ID #453

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f1d1256
reduce excessive access denied logs for non-sysadmin users fetching r…
zahidblackduck Mar 13, 2025
2682a20
registration id fetch allowed set based on blackduck error code
zahidblackduck Mar 17, 2025
a3b8461
unused import anc comment cleanup
zahidblackduck Mar 17, 2025
db96a52
overload method in blackduck registration service to pass boolean flag
zahidblackduck Mar 17, 2025
11d9c9e
refactor variable name, update method signature
zahidblackduck Mar 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions ...com/blackduck/integration/blackduck/service/dataservice/BlackDuckRegistrationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public class BlackDuckRegistrationService extends DataService {
private final UrlSingleResponse<RegistrationView> registrationResponse = apiDiscovery.metaRegistrationLink();
private final UrlSingleResponse<CurrentVersionView> currentVersionResponse = apiDiscovery.metaCurrentVersionLink();
private final HttpUrl blackDuckUrl;
private static final ThreadLocal<Boolean> adminOperationAttempted = ThreadLocal.withInitial(() -> false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@zahidblackduck zahidblackduck Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public BlackDuckRegistrationService(BlackDuckApiClient blackDuckApiClient, ApiDiscovery apiDiscovery, IntLogger logger, HttpUrl blackDuckUrl) {
super(blackDuckApiClient, apiDiscovery, logger);
Expand All @@ -41,15 +42,29 @@ public String getRegistrationId() throws IntegrationException {
return registrationView.getRegistrationId();
}

public BlackDuckServerData getBlackDuckServerData() throws IntegrationException {
public BlackDuckServerData getBlackDuckServerData(boolean isAdmin) throws IntegrationException {
setAdminOperationAttempted(isAdmin);
CurrentVersionView currentVersionView = blackDuckApiClient.getResponse(currentVersionResponse);
String registrationId = null;
try {
// We need to wrap this because this will most likely fail unless they are running as an admin
registrationId = getRegistrationId();
if (shouldAttemptAdminOperations()) {
registrationId = getRegistrationId();
}
} catch (IntegrationException e) {
logger.warn("Failed to fetch registration id: " + e.getMessage());
}
return new BlackDuckServerData(blackDuckUrl, currentVersionView.getVersion(), registrationId);
}

}
public BlackDuckServerData getBlackDuckServerData() throws IntegrationException {
return getBlackDuckServerData(shouldAttemptAdminOperations());
}

private boolean shouldAttemptAdminOperations() {
return adminOperationAttempted.get();
}

private void setAdminOperationAttempted(boolean isAdmin) {
adminOperationAttempted.set(isAdmin);
}
}