Open BenEmdon opened 5 years ago
Just ran the organization_webhook_health_migration
I had been working on and got the following results:
admin:org_hook
scope: 3634We should notify classrooms that don’t have webhooks to authorize the right token.
Right now, webhooks are dependant on users OAuth tokens. This isn't great because users can revoke their tokens, or leave the organization, leaving the tokens on record as invalid.
It turns out we can use the classroom OAuth app token to create webhooks:
from the docs
If we use the classroom client OAuth token to create webhooks then we can ensure that every organization has webhooks enabled.
@BenEmdon What do you think about renaming this issue to include "Importing starter code taking a long time" or something like that so people know that it's related to this problem? That could help redirect people here as they run into issues
I'd like to keep the development track and incident reports separate so we can filter out the noise 😕 Maybe I should start a master issue for all the incidents?
That totally makes sense! I think we can open up another pinned issue just for the incidents and include some instructions on what to do if people hit this issue. Hopefully that makes it a bit easier to sort through
Pinned #1778
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.
From the docs
Interesting; hooks created by the OAuth app cannot be seen by users. And hooks created by users cannot be seen by the Oauth app.
So after investigating the API, it turns out that OAuth apps can't use their own token to register web hooks. 😔 This means that we will have to continue to use user tokens to register webhooks.
Ran the organization_webhook_health_migration
and now the number of classrooms missing webhooks is:
PRODUCTION [1] pry(main)> OrganizationWebhook.where(github_id: nil).count
=> 3674
These organizations are missing webhooks for one of two reasons:
admin:org_hook
scope (as in the user left the organization).We can't do anything about case 2, but we can let each classroom know that they are missing a user with the the admin:org_hook
scope.
This issue could be resolved if Classroom became a GitHub app #2049
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.
From running the following query on production, it is evident that there are still a significant amount of
Organizations
that are missing webhooks:This number excludes
Organizations
that have awebhook_id
but who's webhooks are deleted or inactive.The source of this problem
The problem occurred when we introduced webhooks back in https://github.com/education/classroom/pull/797, we forgot to backfill organizations created before the change with web hooks.
Why this is important
All
Organizations
require webhooks to perform starter code imports as we rely on therepository_import
event.Steps to ensure all Organizations have webhooks that are active
OrganizationWebhook
model to manage multiple classrooms (#1692 #1694)is_webhook_active
column onOrganization
to reflect the last webhook delivered as a date (move this toOrganizationWebhook
) (#1696)Organization
s to belong to aOrganizationWebhook
(#1695)GitHubOrganizationWebbhook
as aGitHubResource
(#1702)OrganizationWebhook
has a valid webhook (#1718)Organization::Creator
toGitHubOrganizationWebhook
(#1716)~Steps to ensure an org is in good health~
Organization
meets all minimum requirements (#1724)OrganizationWebhook
github_id
s that aren't active.~~Switch all webhooks to app created webhooks~
OrganizationWebhook
from user created webhooks to OAuth app generated webhooks~GitHubClassroom.github_client
to create organization webhooks instead of user clients~Do what we can with what we have
ensure_webhook_is_active!
on allOrganizationWebhooks
https://github.com/education/classroom/blob/d5d7a9a78c5b35e372abcff73936b890574c8973/app/models/organization_webhook.rb#L78-L99