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

feat: fetch secret from IOptionsMonitor if defined #550

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Conversation

JamieMagee
Copy link
Contributor

Currently, the secret is statically defined when registering the GitHub webhooks endpoint with MapGitHubWebhooks. But this means that the secret used to validate the webhooks signature is static for the lifetime of the application.

If, instead, we attempt to fetch the secret from IOptionsMonitor<GitHubWebhookOptions> it allows us to rotate the secret without restarting the application.

This is also implemented in a backwards compatible manner. If the secret parameter passed to MapGitHubWebhooks is null, but an IOptionsMonitor<GitHubWebhookOptions> instance has been registered, the secret will be fetched from there instead.

Closes #486


Before the change?

  • The GitHub webhook validation secret is static for the lifetime of the application

After the change?

  • The GitHub webhook

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

Copy link

github-actions bot commented Aug 7, 2024

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

Copy link

@justinmchase justinmchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add a little example in the docs of how one could update the secret?

@JamieMagee
Copy link
Contributor Author

@justinmchase I think the official documentation does a better job than I could ever do it. I could link to something like this instead?

https://learn.microsoft.com/en-us/azure/azure-app-configuration/enable-dynamic-configuration-aspnet-core

@JamieMagee JamieMagee enabled auto-merge (squash) September 24, 2024 03:29
@JamieMagee JamieMagee merged commit d08264f into main Sep 24, 2024
7 checks passed
@JamieMagee JamieMagee deleted the ioptionsmonitor branch September 24, 2024 03:31
@@ -0,0 +1,6 @@
namespace Octokit.Webhooks.AspNetCore;

public sealed record GitHubWebhookOptions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be sealed? I can imagine a use case (at least personally) where I would like to be able to also lump unrelated configuration to webhooks (like my client secret or app id) and not have to create a separate class for webhooks and GitHub app configurations.

Copy link
Contributor Author

@JamieMagee JamieMagee Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sealed it mostly because I seal all of the classes in this library. The performance benefits of the webhook object classes is probably more important than this object. Webhook objects will be constructed on each event, whereas this will be constructed once, on startup.

I can send a follow-up PR to unseal it. I'm still waiting on #568 to be merged before I cut a release.

@JamieMagee JamieMagee mentioned this pull request Sep 27, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEAT]: Please support fetching the secret asynchronously
4 participants