From a6f8dc4e209032903bf2ae4ad742d3cbf6b0424f Mon Sep 17 00:00:00 2001 From: Jochen Delabie Date: Fri, 17 Jul 2026 16:13:12 +0200 Subject: [PATCH 1/2] Fix TestingBot credential resolution and Test Connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness fixes surfaced by an end-to-end run against a real account. 1. Credentials never resolved at build time. TestingBotCredentials extended BaseCredentials (implements StandardCredentials). CredentialsProvider.lookupCredentialsInItem(TestingBotCredentials.class, ...) — used by the build wrapper's getCredentials(), availableCredentials() and every credential dropdown — returned an empty list for it, even though the stored objects are instances of the class (a StandardCredentials.class lookup found them; a UsernamePasswordCredentialsImpl, which extends BaseStandardCredentials, is found by concrete class). As a result the freestyle wrapper silently resolved null credentials, so TB_KEY/TB_SECRET/TESTINGBOT_BUILD were never injected and the dropdowns were empty. Make TestingBotCredentials extend BaseStandardCredentials, which owns id and description (and the id-based equals/hashCode), so concrete-type lookups work. Old-format credentials.xml (scope/id/description/key/secret) still deserializes unchanged — field names are identical. 2. Test Connection failed for accounts without an email. doVerifyCredentials required a non-null user email; a valid account can return 200 with a null email (e.g. sub-accounts), which produced a false 'Could not verify these credentials' for working key/secret. Treat any authenticated user as success, preferring the email and falling back to the account first name. --- .../testingbot/TestingBotCredentials.java | 46 ++++++------------- 1 file changed, 13 insertions(+), 33 deletions(-) diff --git a/src/main/java/testingbot/TestingBotCredentials.java b/src/main/java/testingbot/TestingBotCredentials.java index c4755f8..944d387 100644 --- a/src/main/java/testingbot/TestingBotCredentials.java +++ b/src/main/java/testingbot/TestingBotCredentials.java @@ -7,7 +7,6 @@ import org.kohsuke.stapler.export.Exported; import org.kohsuke.stapler.verb.POST; -import com.cloudbees.plugins.credentials.BaseCredentials; import com.cloudbees.plugins.credentials.Credentials; import com.cloudbees.plugins.credentials.CredentialsDescriptor; import com.cloudbees.plugins.credentials.CredentialsMatcher; @@ -28,7 +27,6 @@ import com.testingbot.testingbotrest.TestingbotREST; import com.testingbot.testingbotrest.TestingbotUnauthorizedException; import edu.umd.cs.findbugs.annotations.CheckForNull; -import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; import hudson.Util; import hudson.model.AbstractItem; @@ -56,19 +54,15 @@ import java.util.logging.Logger; @NameWith(value = TestingBotCredentials.NameProvider.class) -public class TestingBotCredentials extends BaseCredentials implements StandardCredentials { +public class TestingBotCredentials extends BaseStandardCredentials { private static final String CREDENTIAL_DISPLAY_NAME = "TestingBot"; - private final String id; - private final String description; private final String key; private final Secret secret; @DataBoundConstructor public TestingBotCredentials(String id, String description, String key, String secret) { - super(CredentialsScope.GLOBAL); - this.id = IdCredentials.Helpers.fixEmptyId(id); - this.description = Util.fixNull(description); + super(CredentialsScope.GLOBAL, IdCredentials.Helpers.fixEmptyId(id), Util.fixNull(description)); this.key = Util.fixNull(key); this.secret = Secret.fromString(secret); } @@ -86,28 +80,6 @@ public String getDecryptedSecret() { return secret.getPlainText(); } - @NonNull - @Exported - public String getDescription() { - return description; - } - - @NonNull - @Exported - public String getId() { - return id; - } - - @Override - public final boolean equals(Object o) { - return IdCredentials.Helpers.equals(this, o); - } - - @Override - public final int hashCode() { - return IdCredentials.Helpers.hashCode(this); - } - private static List getLegacyCredentials() { DataInputStream in = null; BufferedReader br = null; @@ -330,12 +302,20 @@ public FormValidation doVerifyCredentials(@QueryParameter String key, @QueryPara String plainSecret = Secret.fromString(secret).getPlainText(); try (TestingbotREST rest = new TestingbotREST(key.trim(), plainSecret)) { TestingbotUser user = rest.getUserInfo(); - if (user == null || Util.fixEmptyAndTrim(user.getEmail()) == null) { + if (user == null) { return FormValidation.error("Could not verify these credentials with TestingBot."); } + // A valid TestingBot account may not expose an email (e.g. sub-accounts); the + // authenticated user object itself is proof the key/secret work. Prefer the email, + // fall back to the account's first name, so verification never fails on a null email. + String who = Util.fixEmptyAndTrim(user.getEmail()); + if (who == null) { + who = Util.fixEmptyAndTrim(user.getFirstName()); + } String plan = Util.fixEmptyAndTrim(user.getPlan()); - return FormValidation.ok("Connection successful — signed in as %s%s", - user.getEmail(), plan != null ? " (" + plan + " plan)" : ""); + return FormValidation.ok("Connection successful%s%s", + who != null ? " — signed in as " + who : "", + plan != null ? " (" + plan + " plan)" : ""); } catch (TestingbotUnauthorizedException | TestingbotApiException e) { // The REST client's typed failures: bad key/secret, and network/parse errors (it wraps // IOException into TestingbotApiException). Unexpected runtime exceptions propagate. From f9e5f942b0e332c972e9851c552044104e34d06d Mon Sep 17 00:00:00 2001 From: Jochen Delabie Date: Fri, 17 Jul 2026 17:39:53 +0200 Subject: [PATCH 2/2] Add regression tests for Test Connection result branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the user-facing result building of doVerifyCredentials into a pure, package-private verificationResult(TestingbotUser) helper (behavior unchanged) so it can be unit-tested without the network call or the Jenkins permission check (a JenkinsRule test is not viable here — see the harness/Jetty conflict). Cover: null user -> error; user with email -> success naming the email; user without email but with a first name -> success naming the first name; user missing email, first name and plan -> still success, with no identity or plan fragment in the message. --- .../testingbot/TestingBotCredentials.java | 36 ++++++----- ...TestingBotCredentialsVerificationTest.java | 59 +++++++++++++++++++ 2 files changed, 80 insertions(+), 15 deletions(-) create mode 100644 src/test/java/testingbot/TestingBotCredentialsVerificationTest.java diff --git a/src/main/java/testingbot/TestingBotCredentials.java b/src/main/java/testingbot/TestingBotCredentials.java index 944d387..c9829b2 100644 --- a/src/main/java/testingbot/TestingBotCredentials.java +++ b/src/main/java/testingbot/TestingBotCredentials.java @@ -301,27 +301,33 @@ public FormValidation doVerifyCredentials(@QueryParameter String key, @QueryPara // fromString transparently yields the plaintext either way. String plainSecret = Secret.fromString(secret).getPlainText(); try (TestingbotREST rest = new TestingbotREST(key.trim(), plainSecret)) { - TestingbotUser user = rest.getUserInfo(); - if (user == null) { - return FormValidation.error("Could not verify these credentials with TestingBot."); - } - // A valid TestingBot account may not expose an email (e.g. sub-accounts); the - // authenticated user object itself is proof the key/secret work. Prefer the email, - // fall back to the account's first name, so verification never fails on a null email. - String who = Util.fixEmptyAndTrim(user.getEmail()); - if (who == null) { - who = Util.fixEmptyAndTrim(user.getFirstName()); - } - String plan = Util.fixEmptyAndTrim(user.getPlan()); - return FormValidation.ok("Connection successful%s%s", - who != null ? " — signed in as " + who : "", - plan != null ? " (" + plan + " plan)" : ""); + return verificationResult(rest.getUserInfo()); } catch (TestingbotUnauthorizedException | TestingbotApiException e) { // The REST client's typed failures: bad key/secret, and network/parse errors (it wraps // IOException into TestingbotApiException). Unexpected runtime exceptions propagate. return FormValidation.error("TestingBot authentication failed: " + e.getMessage()); } } + + /** + * Builds the user-facing result for a fetched {@link TestingbotUser}. A null user means the + * credentials could not be verified; otherwise any authenticated user is a success. A valid + * account may not expose an email (e.g. sub-accounts), so prefer the email but fall back to + * the account's first name, and treat both the identity and the plan as optional. + */ + static FormValidation verificationResult(TestingbotUser user) { + if (user == null) { + return FormValidation.error("Could not verify these credentials with TestingBot."); + } + String who = Util.fixEmptyAndTrim(user.getEmail()); + if (who == null) { + who = Util.fixEmptyAndTrim(user.getFirstName()); + } + String plan = Util.fixEmptyAndTrim(user.getPlan()); + return FormValidation.ok("Connection successful%s%s", + who != null ? " — signed in as " + who : "", + plan != null ? " (" + plan + " plan)" : ""); + } } public static class NameProvider extends CredentialsNameProvider { diff --git a/src/test/java/testingbot/TestingBotCredentialsVerificationTest.java b/src/test/java/testingbot/TestingBotCredentialsVerificationTest.java new file mode 100644 index 0000000..ef37ff1 --- /dev/null +++ b/src/test/java/testingbot/TestingBotCredentialsVerificationTest.java @@ -0,0 +1,59 @@ +package testingbot; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.testingbot.models.TestingbotUser; +import hudson.util.FormValidation; +import org.junit.Test; + +/** + * Covers {@link TestingBotCredentials.DescriptorImpl#verificationResult} — the Test Connection + * result branches — without touching the network or the Jenkins permission check: a null user + * fails, and any authenticated user succeeds even when the email, first name or plan are absent. + */ +public class TestingBotCredentialsVerificationTest { + + @Test + public void nullUserFailsVerification() { + FormValidation r = TestingBotCredentials.DescriptorImpl.verificationResult(null); + assertThat(r.kind).isEqualTo(FormValidation.Kind.ERROR); + assertThat(r.getMessage()).contains("Could not verify"); + } + + @Test + public void userWithEmailSucceeds() { + TestingbotUser user = new TestingbotUser(); + user.setEmail("jane@example.com"); + user.setFirstName("Jane"); + user.setPlan("Enterprise Plan"); + FormValidation r = TestingBotCredentials.DescriptorImpl.verificationResult(user); + assertThat(r.kind).isEqualTo(FormValidation.Kind.OK); + assertThat(r.getMessage()) + .contains("Connection successful") + .contains("jane@example.com") + .contains("Enterprise Plan plan"); + } + + @Test + public void userWithoutEmailFallsBackToFirstName() { + TestingbotUser user = new TestingbotUser(); + user.setFirstName("demo"); + user.setPlan("Free"); + FormValidation r = TestingBotCredentials.DescriptorImpl.verificationResult(user); + assertThat(r.kind).isEqualTo(FormValidation.Kind.OK); + assertThat(r.getMessage()) + .contains("signed in as demo") + .contains("Free plan") + .doesNotContain("@"); + } + + @Test + public void userMissingIdentityAndPlanStillSucceeds() { + TestingbotUser user = new TestingbotUser(); // no email, first name or plan + FormValidation r = TestingBotCredentials.DescriptorImpl.verificationResult(user); + assertThat(r.kind).isEqualTo(FormValidation.Kind.OK); + assertThat(r.getMessage()).contains("Connection successful"); + assertThat(r.getMessage()).doesNotContain("signed in as"); + assertThat(r.getMessage()).doesNotContain(" plan)"); + } +}