Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle github repository transfer events. #4130

Merged
merged 2 commits into from
Aug 14, 2024
Merged

Handle github repository transfer events. #4130

merged 2 commits into from
Aug 14, 2024

Conversation

blkt
Copy link
Contributor

@blkt blkt commented Aug 13, 2024

Summary

Github webhook events having type repository and action transferred are handled by producing an internal event that triggers an evaluation. Despite being safe, it cannot lead to any useful action since the github app installation lost permissions to operate on the repository.

This change makes it so that repository.transferred events are handled as deletions. In order for such deletion to be successful, 403 Forbidden errors on webhook deletions must be handled gracefully, so that repository deletion can terminate without errors.

This is safe since (b) there's nothing Minder can do about this, having lost the necessary permissions, and (b) repository registration removes stale webhooks. The latter point is relevant to fix repository transfers when auto registration is enabled: in these cases, repository ownership is migrated before the event is delivered to Minder, making it impossible to cleanup webhooks, which leads to an SQL deletion failure, which eventually makes it impossibile to re-register the repository under the destination project.

Fixes #3274

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Unit and manually tested.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@blkt blkt self-assigned this Aug 13, 2024
jhrozek
jhrozek previously approved these changes Aug 13, 2024
@blkt blkt marked this pull request as ready for review August 13, 2024 17:19
@blkt blkt requested a review from a team as a code owner August 13, 2024 17:19
dmjb
dmjb previously approved these changes Aug 14, 2024
@coveralls
Copy link

Coverage Status

coverage: 53.972% (+0.02%) from 53.952%
when pulling 5a6cfd4 on issue-3274
into ffddb6a on main.

Github webhook events having type `repository` and action
`transferred` are handled by producing an internal event that triggers
an evaluation. Despite being safe, it cannot lead to any useful action
since the github app installation lost permissions to operate on the
repository.

This change makes it so that `repository.transferred` events are
handled as deletions. In order for such deletion to be successful, 403
Forbidden errors on webhook deletions must be handled gracefully, so
that repository deletion can terminate without errors.

This is safe since (b) there's nothing Minder can do about this,
having lost the necessary permissions, and (b) repository registration
removes stale webhooks. The latter point is relevant to fix repository
transfers when auto registration is enabled: in these cases,
repository ownership is migrated before the event is delivered to
Minder, making it impossible to cleanup webhooks, which leads to an
SQL deletion failure, which eventually makes it impossibile to
re-register the repository under the destination project.

Fixes #3274
@blkt blkt dismissed stale reviews from dmjb and jhrozek via 2996ed6 August 14, 2024 09:17
@blkt blkt merged commit ca9c8f1 into main Aug 14, 2024
21 checks passed
@blkt blkt deleted the issue-3274 branch August 14, 2024 14:13
psekar pushed a commit to tinytrail/minder that referenced this pull request Aug 15, 2024
Github webhook events having type `repository` and action
`transferred` are handled by producing an internal event that triggers
an evaluation. Despite being safe, it cannot lead to any useful action
since the github app installation lost permissions to operate on the
repository.

This change makes it so that `repository.transferred` events are
handled as deletions. In order for such deletion to be successful, 403
Forbidden errors on webhook deletions must be handled gracefully, so
that repository deletion can terminate without errors.

This is safe since (b) there's nothing Minder can do about this,
having lost the necessary permissions, and (b) repository registration
removes stale webhooks. The latter point is relevant to fix repository
transfers when auto registration is enabled: in these cases,
repository ownership is migrated before the event is delivered to
Minder, making it impossible to cleanup webhooks, which leads to an
SQL deletion failure, which eventually makes it impossibile to
re-register the repository under the destination project.

Fixes mindersec#3274

Co-authored-by: Don Browne <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When auto-registration of repos is enabled, test moving repositories in and out of organization
4 participants