Fix TestingBot credential resolution and Test Connection#40
Merged
Conversation
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.
📝 WalkthroughWalkthrough
ChangesCredential updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/testingbot/TestingBotCredentials.java`:
- Around line 305-318: Add regression tests for the credential verification
logic around the null-user failure and success-message branches, covering users
with an email, users without an email but with a first name, and users missing
optional identity and plan fields. Assert the expected validation status and
message for each case, including that missing fields still produce successful
verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 64ed1f3c-b4e2-499b-bf8b-6c096c8ff6e6
📒 Files selected for processing (1)
src/main/java/testingbot/TestingBotCredentials.java
Comment on lines
+305
to
+318
| 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)" : ""); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add regression coverage for the new verification branches.
Cover null-user failure, email-present success, email-absent/first-name fallback, and success when optional identity or plan fields are missing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TestingBotCredentials.java` around lines 305 - 318,
Add regression tests for the credential verification logic around the null-user
failure and success-message branches, covering users with an email, users
without an email but with a first name, and users missing optional identity and
plan fields. Assert the expected validation status and message for each case,
including that missing fields still produce successful verification.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/testingbot/TestingBotCredentials.java`:
- Around line 305-308: Update the exception handling around the TestingBot
credential validation to catch TestingbotUnauthorizedException separately and
retain the authentication-failed message only for that exception. Handle
TestingbotApiException in its own branch with a connection/API failure message,
preserving the existing exception details in both responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e536193e-d8d3-4c8e-8471-a41d592713ea
📒 Files selected for processing (2)
src/main/java/testingbot/TestingBotCredentials.javasrc/test/java/testingbot/TestingBotCredentialsVerificationTest.java
Comment on lines
305
to
308
| } 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()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Distinguish API/connectivity failures from invalid credentials.
TestingbotApiException also covers network/parse errors, but this branch labels all of them as authentication failures. Catch it separately and return a connection/API error; retain the authentication message for TestingbotUnauthorizedException.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TestingBotCredentials.java` around lines 305 - 308,
Update the exception handling around the TestingBot credential validation to
catch TestingbotUnauthorizedException separately and retain the
authentication-failed message only for that exception. Handle
TestingbotApiException in its own branch with a connection/API failure message,
preserving the existing exception details in both responses.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two correctness fixes surfaced by an end-to-end run against a real TestingBot account.
1. Credentials never resolved at build time
TestingBotCredentialsextendedBaseCredentials(implementingStandardCredentials).CredentialsProvider.lookupCredentialsInItem(TestingBotCredentials.class, …)— used by the build wrapper'sgetCredentials(),availableCredentials()and every credential dropdown — returned an empty list for it, even though the stored objects are instances of the class:StandardCredentials.classlookup returns them;UsernamePasswordCredentialsImpl(whichextends BaseStandardCredentials) is found by its concrete class.So the freestyle wrapper silently resolved null credentials —
TB_KEY/TB_SECRET/TESTINGBOT_BUILDwere never injected, and the credential dropdowns were empty.Fix: make
TestingBotCredentials extend BaseStandardCredentials, which ownsid/description(and the id-basedequals/hashCode), so concrete-type lookups resolve it. Verified old-formatcredentials.xml(scope/id/description/key/secret) still deserializes unchanged — the field names are identical.2. Test Connection failed for accounts without an email
doVerifyCredentialsrequired a non-null user email. A valid account can return HTTP 200 with anullemail (e.g. sub-accounts), producing a false 'Could not verify these credentials' for working key/secret. Now any authenticated user is treated as success, preferring the email and falling back to the account first name.Both verified live: credential now injects into the build; Test Connection returns 'Connection successful — signed in as … (… plan)' for a null-email account and still reports auth failures for bad secrets.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests