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

Jump to bottom

[JENKINS-75073] Missing serverUrl in fillCredentialsIdItems cause some credentials missing in the initial configuration #953

Merged
merged 1 commit into from
Jan 10, 2025
Merged

[JENKINS-75073] Missing serverUrl in fillCredentialsIdItems cause some credentials missing in the initial configuration #953

merged 1 commit into from
Jan 10, 2025

Conversation

nfalco79
Copy link
Member

@nfalco79 nfalco79 commented Jan 1, 2025 •
edited
Loading

If the number of server instances is only one, when a new Jenkins project is created the server list combobox is not available. This causes, in the UI component that loads credentials, to pass the stapler parameter serverUrl as an empty string.
In this scenario, serverUrl must be taken from the first of configured endpoints so that it can properly filter the list of credentials.

Test why and the use case to normalizeServerUrl must be bitbucket cloud if server url is null

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 1, 2025
View reviewed changes
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMNavigator.java Fixed Show fixed Hide fixed
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMNavigator.java Fixed Show fixed Hide fixed
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMNavigator.java
}

@SuppressWarnings("unused") // used By stapler
public static FormValidation doCheckMirrorId(@QueryParameter String value, @QueryParameter String serverUrl) {
public static FormValidation doCheckMirrorId(@QueryParameter String value,

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing permission check Warning

Potential missing permission check in DescriptorImpl#doCheckMirrorId
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMSource.java Fixed Show fixed Hide fixed
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMSource.java
@@ -1349,10 +1353,11 @@
}

@SuppressWarnings("unused") // used By stapler
public static FormValidation doCheckMirrorId(@QueryParameter String value, @QueryParameter String serverUrl) {
public static FormValidation doCheckMirrorId(@QueryParameter String value,

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing permission check Warning

Potential missing permission check in DescriptorImpl#doCheckMirrorId
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 1, 2025
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

nfalco79
nfalco79 commented Jan 1, 2025
View reviewed changes
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketGitSCMBuilder.java
endpoint = new BitbucketServerEndpoint(null, serverURL, false, null);
}
AbstractBitbucketEndpoint endpoint = BitbucketEndpointConfiguration.get()
.findEndpoint(serverURL)
Copy link
Member Author

Choose a reason for hiding this comment

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

serverURL is never null because scmSource.getServerUrl() return a non null value (default is cloud).
normalizeServerUrl is useless here

nfalco79
nfalco79 commented Jan 1, 2025
View reviewed changes
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMSourceBuilder.java
@@ -74,7 +73,7 @@ public BitbucketSCMSourceBuilder(@CheckForNull String id, @NonNull String server
@NonNull String repoName, @CheckForNull String mirrorId) {
super(BitbucketSCMSource.class, repoName);
this.id = id;
this.serverUrl = BitbucketEndpointConfiguration.normalizeServerUrl(serverUrl);
this.serverUrl = serverUrl;
Copy link
Member Author

@nfalco79 nfalco79 Jan 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

it is declared nonnull normalizeServerUrl is useless to default empty to bitbucket cloud

nfalco79
nfalco79 commented Jan 1, 2025
View reviewed changes
...n/java/com/cloudbees/jenkins/plugins/bitbucket/endpoints/BitbucketEndpointConfiguration.java
@@ -324,7 +324,7 @@ public static String normalizeServerUrl(@CheckForNull String serverUrl) {
} catch (URISyntaxException e) {
// ignore, this was a best effort tidy-up
}
return serverUrl.replaceAll("/$", "");
return serverURL.replaceAll("/$", "");
Copy link
Member Author

@nfalco79 nfalco79 Jan 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

which server url could have /$ ?
If serverUrl contains variable than any /${VAR} will be translated to {VAR} and will not work in any bitbucket client

@nfalco79 nfalco79 force-pushed the feature/JENKINS-75073 branch 6 times, most recently from f70043e to 7ff7bc3 Compare January 2, 2025 14:45
@nfalco79
Copy link
Member Author

nfalco79 commented Jan 2, 2025

Fixes #608

@nfalco79 nfalco79 force-pushed the feature/JENKINS-75073 branch 2 times, most recently from d7235d1 to 0f1552e Compare January 9, 2025 09:04
@nfalco79 nfalco79 marked this pull request as ready for review January 9, 2025 09:04
@nfalco79 nfalco79 force-pushed the feature/JENKINS-75073 branch 4 times, most recently from 2a4408e to 4a65742 Compare January 10, 2025 00:15
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 10, 2025
View reviewed changes
...va/com/cloudbees/jenkins/plugins/bitbucket/endpoints/BitbucketEndpointConfigurationTest.java
MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
mockStrategy.grant(Jenkins.MANAGE).onRoot().to("admin");
j.jenkins.setAuthorizationStrategy(mockStrategy);
try (ACLContext context = ACL.as(User.get("admin"))) {

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
User.get
Loading
should be avoided because it has been deprecated.
@nfalco79 nfalco79 force-pushed the feature/JENKINS-75073 branch 2 times, most recently from 50d80b9 to d142bf4 Compare January 10, 2025 00:59
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 10, 2025
View reviewed changes
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/endpoints/BitbucketServerEndpoint.java Outdated
super(manageHooks, credentialsId);
this.serverUrl = BitbucketEndpointConfiguration.normalizeServerUrl(serverUrl);

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
displayName
Loading
may be null at this access because of
this
Loading
null argument.
Variable
displayName
Loading
may be null at this access because of
this
Loading
null argument.
@nfalco79 nfalco79 force-pushed the feature/JENKINS-75073 branch 4 times, most recently from a775182 to 9c6a177 Compare January 10, 2025 10:16
@nfalco79
[JENKINS-75073] Missing serverUrl in fillCredentialsIdItems cause som…
0cbd6b6 
…e credentials missing in the initial configuration

If the number of server instances is only one, when a new Jenkins project is created the server list combobox is not available. This causes, in the UI component that loads credentials, to pass the stapler parameter serverUrl as an empty string. In this scenario, serverUrl must be taken from the first of configured endpoints so that it can properly filter the list of credentials.
Move some unit test to JUnit5
@nfalco79 nfalco79 force-pushed the feature/JENKINS-75073 branch from 9c6a177 to 0cbd6b6 Compare January 10, 2025 10:16
@nfalco79 nfalco79 merged commit 6a0d987 into master Jan 10, 2025
15 of 17 checks passed
@nfalco79 nfalco79 deleted the feature/JENKINS-75073 branch January 10, 2025 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug needs-manual-testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@nfalco79