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

[JENKINS-73038] Adapt plugin to allow user with Overall/Manage to configure global settings #846

Merged
merged 5 commits into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>4.80</version>
<version>4.81</version>

Choose a reason for hiding this comment

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

Isn't it risking conflict with the automatic dependency PR #844 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if so i will resolve it

<relativePath />
</parent>

Expand All @@ -30,6 +30,7 @@
<gitHubRepo>jenkinsci/bitbucket-branch-source-plugin</gitHubRepo>
<jenkins.version>2.401.3</jenkins.version>
<hpi.compatibleSinceVersion>2.0</hpi.compatibleSinceVersion>
<useBeta>true</useBeta> <!-- Jenkins.MANAGE -->

Choose a reason for hiding this comment

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

Is it supposed to stay or was it to test ?

Copy link
Contributor Author

@mikecirioli mikecirioli Apr 29, 2024

Choose a reason for hiding this comment

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

It is required until an agreement can be had that its safe to take the Overall/Manage permission feature out of beta

Choose a reason for hiding this comment

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

OK, I read badly this conf, I thought it was forcing the plugin in beta mode -_-
My bad, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no worries, i've also started JENKINS-73089 to address moving the Overall/Manage permission out of beta status

</properties>

<developers>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
if (repoOwner == null) {
return new ListBoxModel();
}
if (context == null && !Jenkins.get().hasPermission(Jenkins.ADMINISTER) ||
if (context == null && !Jenkins.get().hasPermission(Jenkins.MANAGE) ||

Check warning on line 38 in src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketApiUtils.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 38 is not covered by tests
context != null && !context.hasPermission(Item.EXTENDED_READ)) {
return new ListBoxModel(); // not supposed to be seeing this form
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@
@QueryParameter String serverUrl,
@QueryParameter String credentialsId) {
if (context == null
? !Jenkins.get().hasPermission(Jenkins.ADMINISTER)
? !Jenkins.get().hasPermission(Jenkins.MANAGE)
: !context.hasPermission(Item.EXTENDED_READ)) {
return new StandardListBoxModel().includeCurrentValue(credentialsId);
}
Expand Down Expand Up @@ -202,7 +202,7 @@
@QueryParameter String serverUrl,
@QueryParameter String value) {
if (context == null
? !Jenkins.get().hasPermission(Jenkins.ADMINISTER)
? !Jenkins.get().hasPermission(Jenkins.MANAGE)

Check warning on line 205 in src/main/java/com/cloudbees/jenkins/plugins/bitbucket/SSHCheckoutTrait.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 205 is not covered by tests
: !context.hasPermission(Item.EXTENDED_READ)) {
return FormValidation.ok();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public abstract class AbstractBitbucketEndpointDescriptor extends Descriptor<Abs
@SuppressWarnings("unused")
public ListBoxModel doFillCredentialsIdItems(@QueryParameter String serverUrl) {
Jenkins jenkins = Jenkins.get();
jenkins.checkPermission(Jenkins.ADMINISTER);
jenkins.checkPermission(Jenkins.MANAGE);
StandardListBoxModel result = new StandardListBoxModel();
result.includeMatchingAs(
ACL.SYSTEM,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import hudson.ExtensionList;
import hudson.Util;
import hudson.security.ACL;
import hudson.security.Permission;
import hudson.util.ListBoxModel;
import java.net.URI;
import java.net.URISyntaxException;
Expand Down Expand Up @@ -79,6 +80,11 @@ public static BitbucketEndpointConfiguration get() {
return ExtensionList.lookup(GlobalConfiguration.class).get(BitbucketEndpointConfiguration.class);
}

@NonNull
@Override
public Permission getRequiredGlobalConfigPagePermission() {
return Jenkins.MANAGE;
}
/**
* Called from a {@code readResolve()} method only to convert the old {@code bitbucketServerUrl} field into the new
* {@code serverUrl} field. When called from {@link ACL#SYSTEM} this will update the configuration with the
Expand Down Expand Up @@ -155,7 +161,7 @@ public synchronized List<AbstractBitbucketEndpoint> getEndpoints() {
* @param endpoints the list of endpoints.
*/
public synchronized void setEndpoints(@CheckForNull List<? extends AbstractBitbucketEndpoint> endpoints) {
Jenkins.get().checkPermission(Jenkins.ADMINISTER);
Jenkins.get().checkPermission(Jenkins.MANAGE);
List<AbstractBitbucketEndpoint> eps = new ArrayList<>(Util.fixNull(endpoints));
// remove duplicates and empty urls
Set<String> serverUrls = new HashSet<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl;
import com.google.common.io.Resources;
import hudson.XmlFile;
import hudson.model.User;
import hudson.security.ACL;
import hudson.security.ACLContext;
import hudson.security.AuthorizationStrategy;
Expand All @@ -49,7 +50,9 @@
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
import org.junit.jupiter.api.Assertions;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.MockAuthorizationStrategy;

import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.hasSize;
Expand Down Expand Up @@ -129,6 +132,29 @@
}

@Test
public void given__newInstance__when__configuredAsManage__then__OK() {
BitbucketEndpointConfiguration instance = new BitbucketEndpointConfiguration();
try {
j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
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
should be avoided because it has been deprecated.
instance.setEndpoints(Arrays.<AbstractBitbucketEndpoint>asList(
new BitbucketCloudEndpoint(true, "first"),
new BitbucketCloudEndpoint(true, "second"),
new BitbucketCloudEndpoint(true, "third")));
assertThat(instance.getEndpoints(), contains(instanceOf(BitbucketCloudEndpoint.class)));
assertThat(instance.getEndpoints().get(0).getCredentialsId(), is("first"));
} catch (RuntimeException x) {
Assertions.fail("No exception should be thrown");
}
} finally {
j.jenkins.setAuthorizationStrategy(AuthorizationStrategy.UNSECURED);
}
}

@Test
public void given__newInstance__when__configuredWithServerUsingCloudUrl__then__convertedToCloud() {
BitbucketEndpointConfiguration instance = new BitbucketEndpointConfiguration();
assumeThat(instance.getEndpoints().get(0).getCredentialsId(), not(is("dummy")));
Expand Down