Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

authz: Fix panic when auth provider is missing#62759

Merged
eseliger merged 1 commit into
mainfrom
es/05-17-authzfixpanicwhenauthproviderismissing
May 17, 2024
Merged

authz: Fix panic when auth provider is missing#62759
eseliger merged 1 commit into
mainfrom
es/05-17-authzfixpanicwhenauthproviderismissing

Conversation

@eseliger

Copy link
Copy Markdown
Member

When authz is enabled for a GitHub code host connection, but there is no corresponding auth.providers entry, this code currently panics, because GetOAuthContext returns nil, and then the RefreshFunc in the OAuthBearerToken tries to use that context when it refreshes.

This is a misconfiguration, and cannot work (we need both the authz setting, and the auth provider), but the worker pod should not just panic because of that, so we return a nicer error here now.

Test plan:

Added a unit test.

@cla-bot cla-bot Bot added the cla-signed label May 17, 2024
@eseliger eseliger marked this pull request as ready for review May 17, 2024 12:40
@eseliger eseliger requested a review from a team May 17, 2024 12:40

eseliger commented May 17, 2024

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @eseliger and the rest of your teammates on Graphite Graphite

@github-actions github-actions Bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels May 17, 2024
@eseliger eseliger force-pushed the es/05-17-authzfixpanicwhenauthproviderismissing branch from 5b338ba to c6c5ed3 Compare May 17, 2024 13:43
When authz is enabled for a GitHub code host connection, but there is no corresponding auth.providers entry, this code currently panics, because `GetOAuthContext` returns nil, and then the `RefreshFunc` in the OAuthBearerToken tries to use that context when it refreshes.

This is a misconfiguration, and cannot work (we need both the authz setting, and the auth provider), but the worker pod should not just panic because of that, so we return a nicer error here now.

Test plan:

Added a unit test.
@eseliger eseliger force-pushed the es/05-17-authzfixpanicwhenauthproviderismissing branch from c6c5ed3 to 51275bd Compare May 17, 2024 13:56
@eseliger eseliger merged commit 25faead into main May 17, 2024
@eseliger eseliger deleted the es/05-17-authzfixpanicwhenauthproviderismissing branch May 17, 2024 14:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants