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.

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

SCANJLIB-230 Add warning when sonar.login (and sonar.token simultaneo… #221

Conversation

aleksandra-bozhinoska-sonarsource
Copy link
Contributor

@aleksandra-bozhinoska-sonarsource aleksandra-bozhinoska-sonarsource commented Jan 8, 2025

SCANJLIB-230

…usly) is used

I have determined the SonarQube Server version when the sonar.token property was introduced from the fix version of the following task.

How to test?
Run the scanner with both the sonar.token and sonar.login properties set, the following two warnings should be displayed in the logs:
11:28:24.316 WARN Both 'sonar.login' and 'sonar.token' (or the 'SONAR_TOKEN' env variable) are set, but only the latter will be used.
11:28:24.680 WARN Use of 'sonar.login' property has been deprecated in favor of 'sonar.token' (or the env variable alternative 'SONAR_TOKEN'). Please use the latter when passing a token.

However, to discuss:
As for this point of the task: ensure that token effectively has priority over basic auth (also check in the scanner engine of SQS and SQC)

  • Not the case for the scanner engine (both in Server and Cloud) (due to for example line 90 in ScannerWsClientProvider.java in sonarqube - if the login property is present, it is not overriden).

Copy link
Member

@henryju henryju left a comment

Choose a reason for hiding this comment

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

I have a small doubt regarding the messages mentioning properties keys. In practice, users can set the token also using the env variable SONAR_TOKEN. So mentioning they defined sonar.token might be confusing.
Either we should "remember" the source of the token (maybe not an easy task), or every message should mention both?

@aleksandra-bozhinoska-sonarsource aleksandra-bozhinoska-sonarsource force-pushed the improvement/aleksandrab/SCANJLIB-230/warn-incorrect-usage-of-sonar-login-and-token branch from b229bba to 5db6b5e Compare January 9, 2025 10:53
@aleksandra-bozhinoska-sonarsource
Copy link
Contributor Author

  • I was able to verify that when both sonar.login and sonar.token are set, the sonar.login value is used in the credentials.

  • Another observation is that we have similar sonar.login deprecation log issued from the scanner engine (both SQS, SQC): DeprecatedPropertiesWarningGenerator.java.

@henryju
Copy link
Member

henryju commented Jan 9, 2025

So to sum up, we have a different behavior in the scanner bootstrapper than in the current scanner engine, right?

  • scanner bootstrapper favors sonar.token
  • scanner engine favors sonar.login

I don't think this is a big deal, since this has been deprecated for a long time, and we are even dropping username/password in the next SQS release.

@aleksandra-bozhinoska-sonarsource
Copy link
Contributor Author

So to sum up, we have a different behavior in the scanner bootstrapper than in the current scanner engine, right?

  • scanner bootstrapper favors sonar.token
  • scanner engine favors sonar.login

I don't think this is a big deal, since this has been deprecated for a long time, and we are even dropping username/password in the next SQS release.

Yes, that's right.

@aleksandra-bozhinoska-sonarsource aleksandra-bozhinoska-sonarsource merged commit 8af5636 into master Jan 9, 2025
9 checks passed
@aleksandra-bozhinoska-sonarsource aleksandra-bozhinoska-sonarsource deleted the improvement/aleksandrab/SCANJLIB-230/warn-incorrect-usage-of-sonar-login-and-token branch January 9, 2025 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants