[JENKINS-74970] Log user name and repository if Bitbucket Server forbids posting a build status

[KalleOlaviNiemitalo]

If Bitbucket Server responds with HTTP status 401 (Unauthorized) or 403 (Forbidden) when this plugin posts a build status, and the HTTP response includes an "X-AUSERNAME" header field that shows Bitbucket Server authenticated the user, then add more information in the message of the BitbucketRequestException:

• The Bitbucket user name from the "X-AUSERNAME" response header field.
• The repository to which the request was sent.
• The project or user that owns the repository.

The behaviour is unchanged on Bitbucket Cloud, and on Bitbucket Server for operations other than posting a build status.

Your checklist for this pull request

• Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or in Jenkins JIRA
• Link to relevant pull requests, esp. upstream and downstream changes
• Did you provide a test-case? That demonstrates feature works or fixes the issue.

[KalleOlaviNiemitalo]

Not yet happy with:

• Would be nicer to show the permission actually required (REPO_READ, REPO_WRITE, or REPO_ADMIN).
• If the REST API operation requires some other kind of permission (SYS_ADMIN, ADMIN, PROJECT_VIEW, PROJECT_CREATE, PROJECT_READ, PROJECT_WRITE, PROJECT_ADMIN, or LICENSED_USER), then the plugin should not add repository information to the exception, because granting permissions at the repository level will not help.
• Documentation.

For the permission levels, I'm thinking the boolean requireAuthentication parameter of doRequest could be replaced with an enum, and then that would be passed to buildResponseException as well. This might require changing BitbucketCloudApiClient, too.

[nfalco79]

The username is already logged because recently I have add all headers to the error message. This helped in JENKINS-75106
I would not start to specialize client classes for each HTTP code a message because sometimes could not fall into the case we thought.
Maybe could be useful show the URL of the request that already contains all details of workspace, project and repo_slug.

For the permission levels, I'm thinking the boolean requireAuthentication parameter of doRequest could be replaced with an enum, and then that would be passed to buildResponseException as well. This might require changing BitbucketCloudApiClient, too.

I can not keep track of each permission required for each API, and move forward useless parameter to methods only for logs.
At most we can add in case of 401 and 403 a link to the specific GitHub user documention. In this way is generic for both server/cloud and authentication type.

[KalleOlaviNiemitalo]

recently I have add all headers to the error message

You mean response headers, right? Not request headers, which typically include secrets.

[KalleOlaviNiemitalo]

For the permission levels, I'm thinking the boolean requireAuthentication parameter of doRequest could be replaced with an enum

This gets tricky in the getMirroredRepository method, where the request may require authentication, but the credentials are already included in the String url parameter.

Maybe could be useful show the URL of the request that already contains all details of workspace, project and repo_slug.

According to #829 (comment), the query component of the request URL can contain an authentication token. That should not be written to a log that users can read.

At this point, I'm thinking of adding to doRequest etc. a callback parameter (functional interface) and calling that instead of buildResponseException if not null. Only postBuildStatus would provide such a callback.

[nfalco79]

You mean response headers, right? Not request headers, which typically include secrets.

Yes, request contains sensible information and username could be marked as "Treat username as secret" in credentials.
Response contains the X-AUSERNAME for server instance, not for cloud (at least when 401).
Maybe is acceptable "...username defined in credentialsId " + authenticator.getId() + " .... ensure the user is configured ..... see documentation at https://github......

[nfalco79]

Maybe could be useful show the URL of the request that already contains all details of workspace, project and repo_slug.

According to #829 (comment), the query component of the request URL can contain an authentication token. That should not be written to a log that users can read.

Enrich the URLUtils#removeAuthority to remove also the jwt queryParam so it can be used for log

[nfalco79]

At this point, I'm thinking of adding to doRequest etc. a callback parameter (functional interface) and calling that instead of buildResponseException if not null. Only postBuildStatus would provide such a callback.

You can improve the user documentation for that use case, I want share client code as much code as possible and not provide callback for which the called have to handle response and depends on http client code. See also pr #963 in case

[KalleOlaviNiemitalo]

remove also the jwt queryParam

Does Atlassian document the name of the query parameter?

[nfalco79]

Does Atlassian document the name of the query parameter?

No they speak about authenticated links

[KalleOlaviNiemitalo]

This is HttpResponse rather than CloseableHttpResponse because the method should not close the response.