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

chore(integrations): oauth2 identity pipeline metrics #79325

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cathteng
Copy link
Member

@cathteng cathteng commented Oct 17, 2024

register(SlackIdentityProvider) # NOQA
register(GitHubIdentityProvider) # NOQA
register(GitHubEnterpriseIdentityProvider) # NOQA
register(VSTSNewIdentityProvider) # NOQA
register(VSTSIdentityProvider) # NOQA
register(VstsExtensionIdentityProvider) # NOQA
register(VercelIdentityProvider) # NOQA
register(BitbucketIdentityProvider) # NOQA
register(GitlabIdentityProvider) # NOQA
register(GoogleIdentityProvider) # NOQA
register(DiscordIdentityProvider) # NOQA

These are used in the installation flows for:

  • GitHub Enterprise
  • VSTSNewIdentityProvider
  • Slack
  • Vercel
  • Gitlab
  • GitHub (via a redirect, the pipeline is not directly used)

Missing:

  • VSTSOAuth2CallbackView overwrites the exchange_token function (VSTSIdentityProvider, VSTSExtensionIdentityProvider)

Not applicable

@cathteng cathteng requested review from a team as code owners October 17, 2024 21:50
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 17, 2024
Comment on lines 302 to +304
def exchange_token(self, request: Request, pipeline, code):
# TODO: this needs the auth yet
data = self.get_token_params(code=code, redirect_uri=absolute_uri(pipeline.redirect_url()))
verify_ssl = pipeline.config.get("verify_ssl", True)
try:
req = safe_urlopen(self.access_token_url, data=data, verify_ssl=verify_ssl)
body = safe_urlread(req)
if req.headers.get("Content-Type", "").startswith("application/x-www-form-urlencoded"):
return dict(parse_qsl(body))
return orjson.loads(body)
except SSLError:
logger.info(
"identity.oauth2.ssl-error",
extra={"url": self.access_token_url, "verify_ssl": verify_ssl},
with record_event(
IntegrationPipelineViewType.TOKEN_EXCHANGE, pipeline.provider.key
Copy link
Member Author

Choose a reason for hiding this comment

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

even though exchange_token is used inside a function that already has a context manager, i decided to emit another lifecycle metric because the errors are more granular inside exchange_token

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant