Utilize branch information from webhook events instead of querying them from the API

tsalzinger commented 3 months ago

What feature do you want to see added?

Similar to how PRs are discovered on a webhook events like pullrequest:updated or pullrequest:created also the branch information could be extracted from these events as well as others like repo:push.

The benefit of these changes is mostly a cleaner log, especially in repos with many branches, although also a few API reqeusts can be skipped to retreive the branch list if the data is alerady used directly from within the event payload.

Upstream changes

No response

Are you interested in contributing this feature?

yes, I already have a working prototype for this in our fork and we had this deployed for a few days arleady without seeing any issues, so if this is something that is generally wanted I'm happy to open a PR to discuss the actual implementation details too.

KalleOlaviNiemitalo commented 3 months ago

Getting the branch name from the webhook seems OK but the corresponding commit ID should still be queried from the server unless you add webhook authentication.

tsalzinger commented 3 months ago

Why? What would be the security implication / attack vector here? If someone just randomly creates events for branches / commit hashes the actual checkout in the end will fail.

But even if you could somehow argue for a potential security implication I'd be inclined to leave the decision up to the actual users if they want to trust there webhook events or not. For example we ensured on infrastructure level that the webhook endpoints cannot just be randomly accessed. So adding a configuration option for this might be an option.

I'm looking forward to a response from you on this, as I might really just be missing something here (but to be honest it's also not the most critical thing anyway, so dropping this request is also fine by me).

That said adding an authentication possibility to the webhooks still seems like a nice idea, but I guess that should be a separate issue / discussion. I couldn't find anything related to this in the open issues, onyl 2 TODOs which seem kind of related to your comment, but in a place that I didn't look at before to be honest:

KalleOlaviNiemitalo commented 3 months ago

If someone just randomly creates events for branches / commit hashes the actual checkout in the end will fail.

The checkout could succeed if there is such a commit ID in a fork of the repository.

KalleOlaviNiemitalo commented 3 months ago

Or if the commit ID exists in the history of the ref. The Jenkins pipeline could then deploy the results somewhere, replacing a newer version.

tsalzinger commented 3 months ago

Both great reasons I didn't think about - with that in mind I don't think the gain is worth the change at the moment. If trusted events at some point are available this could be picked up again.

Thanks @KalleOlaviNiemitalo

jenkinsci / bitbucket-branch-source-plugin