feat(bitbucketdatacenter): allow service accounts to not require user setup#2726
feat(bitbucketdatacenter): allow service accounts to not require user setup#2726Ru13en wants to merge 1 commit into
Conversation
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Bitbucket Data Center provider to allow authentication without an explicitly defined user by falling back to a direct repository URL request for token validation. Feedback from the review highlights a critical need for a nil check on the repository object to prevent runtime panics. Furthermore, the current error handling logic needs refinement to avoid malformed error strings when wrapping nil errors and to provide more accurate messaging when the user field is empty.
| var resp *scm.Response | ||
| var err error |
There was a problem hiding this comment.
The repo pointer is used later in this function (at line 324) and indirectly via v.Client() (which records metrics using v.repo). If repo is nil, the application will panic when accessing repo.Spec.URL. A nil check should be added before proceeding with validation to ensure defensive programming and avoid runtime panics.
if repo == nil {
return fmt.Errorf("repository object is nil")
}
var resp *scm.Response
var err error| if resp != nil && resp.Status == http.StatusUnauthorized { | ||
| return fmt.Errorf("cannot get user %s with token: %w", event.Provider.User, err) | ||
| return fmt.Errorf("failed validation of user %s with provided token: %w", event.Provider.User, err) | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf("cannot get user %s: %w", event.Provider.User, err) | ||
| return fmt.Errorf("user validation failed with: %w", err) | ||
| } |
There was a problem hiding this comment.
This error handling block has two issues:
- If
resp.Statusishttp.StatusUnauthorizedbuterrisnil(which occurs with rawv.Client().Docalls as they do not parse error bodies), using%wwith a nil error produces the string%!w(<nil>). - If
event.Provider.Useris empty (as allowed by this PR), the error messages "failed validation of user " and "user validation failed" are misleading; they should refer to token validation instead.
| if resp != nil && resp.Status == http.StatusUnauthorized { | |
| return fmt.Errorf("cannot get user %s with token: %w", event.Provider.User, err) | |
| return fmt.Errorf("failed validation of user %s with provided token: %w", event.Provider.User, err) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("cannot get user %s: %w", event.Provider.User, err) | |
| return fmt.Errorf("user validation failed with: %w", err) | |
| } | |
| if resp != nil && resp.Status == http.StatusUnauthorized { | |
| msg := "failed validation of provided token" | |
| if event.Provider.User != "" { | |
| msg = fmt.Sprintf("failed validation of user %s with provided token", event.Provider.User) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("%s: %w", msg, err) | |
| } | |
| return fmt.Errorf("%s", msg) | |
| } | |
| if err != nil { | |
| msg := "token validation failed" | |
| if event.Provider.User != "" { | |
| msg = "user validation failed" | |
| } | |
| return fmt.Errorf("%s with: %w", msg, err) | |
| } |
| w.WriteHeader(http.StatusUnauthorized) | ||
| }, | ||
| apiURL: "https://foo.bar", | ||
| wantErrSubstr: "failed validation of user with provided token: %!w(<nil>)", |
There was a problem hiding this comment.
The test expectation includes %!w(<nil>), which confirms that the implementation's error formatting is currently broken (it's trying to wrap a nil error using %w). This expectation should be updated to a meaningful message once the implementation is fixed to handle nil errors correctly.
| wantErrSubstr: "failed validation of user with provided token: %!w(<nil>)", | |
| wantErrSubstr: "failed validation of provided token", |
| } | ||
| if resp != nil && resp.Status == http.StatusUnauthorized { | ||
| return fmt.Errorf("cannot get user %s with token: %w", event.Provider.User, err) | ||
| return fmt.Errorf("failed validation of user %s with provided token: %w", event.Provider.User, err) |
There was a problem hiding this comment.
small issue with error handling here — when event.Provider.User is empty (which is now allowed by this PR), and the HTTP request returns 401 but err == nil (which happens when the server responds with unauthorized but the request itself succeeded), this produces malformed error output like %!w(<nil>).
the test case at line 372 actually documents this bug:
wantErrSubstr: "failed validation of user with provided token: %!w(<nil>)"worth checking if err is nil before using %w, or restructuring the error messages to distinguish between:
- "failed validation of user X with provided token" (when user is set)
- "failed validation of token" (when only token is provided)
something like:
if resp != nil && resp.Status == http.StatusUnauthorized {
if event.Provider.User != "" {
return fmt.Errorf("failed validation of user %s with provided token: %w", event.Provider.User, err)
}
return fmt.Errorf("token validation failed: unauthorized")
}There was a problem hiding this comment.
Error handling improved. Commit amended. Thanks for the suggestions
Ru13en
force-pushed
the
allow-project-repo-http-tokens
branch
from
339d742 to
645ee48
Compare
… in the set client
Ru13en
force-pushed
the
allow-project-repo-http-tokens
branch
from
645ee48 to
afb412f
Compare
Ru13en
changed the title
mathur07
left a comment
mathur07
left a comment
There was a problem hiding this comment.
/lgtm
cc: @zakisk @theakshaypant
|
/ok-to-test |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2726 +/- ##
==========================================
+ Coverage 59.25% 59.30% +0.04%
==========================================
Files 208 208
Lines 20573 20604 +31
==========================================
+ Hits 12191 12219 +28
- Misses 7610 7612 +2
- Partials 772 773 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5 participants
📝 Description of the Change
Previously, using project or repository HTTP scoped tokens required configuring an associated user, even when the token already provided the necessary access context.
This PR removes the requirement to configure a user when using HTTP tokens from project and repository scopes.
It updates authentication flow to rely directly on the scoped token context, when only token is provided.
Related validation and tests were adjusted accordingly
🔗 Linked GitHub Issue
Fixes #
#2685
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.