feat: Add Sentry OAuth and current user endpoint #19
Conversation
scott-codecov
force-pushed
the
scott/sentry-oauth
branch
from
448d891
to
ffbd315
Compare
Codecov Report
@@ Coverage Diff @@
## main #19 +/- ##
=======================================
+ Coverage 95.22 95.25 +0.03
=======================================
Files 564 568 +4
Lines 14178 14287 +109
=======================================
+ Hits 13500 13609 +109
Misses 678 678
Flags with carried forward coverage won't be shown. Click here to find out more.
|
* main: Add api_health endpoint Move `Commit.report` to GCS (#10) fix: Set SameSite=None; Secure for sessionid cookie delete unnecessary redacted secrets from config Get rid of reduntant validation error Update shared. (#20) fix: Handle no current owner in GraphQL resolvers Return and catch validation error Addressed feedback Add mutation to start trial
| @@ -455,6 +455,8 @@ | |||
| "setup", "sentry", "jwt_shared_secret", default=None | |||
| ) | |||
| SENTRY_USER_WEBHOOK_URL = get_config("setup", "sentry", "webhook_url", default=None) | |||
| SENTRY_OAUTH_CLIENT_ID = get_config("setup", "sentry", "oauth_client_id") | |||
There was a problem hiding this comment.
Are these secrets stored already in k8s? And are there secret values to play with local too?
There was a problem hiding this comment.
Not yet - just got these last Friday - I'll open a k8s-v2 PR to add the values and link it here.
To test things locally you can create an OAuth app in Sentry under your personal account: https://codecov.sentry.io/settings/account/api/applications/
There was a problem hiding this comment.
Sounds good, yeah as long as you can test that locally yourself that sounds good to me 👌
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ("codecov_auth", "0031_user_owner_user"), |
There was a problem hiding this comment.
I think you'll have to rerun this migration cause the latest trial one is a 0032 and will have migration name violation
|
|
||
| # TEMPORARY: we're assuming a single owner for the time being since there's | ||
| # no supporting UI to select which owner you'd like to view | ||
| owner = current_user.owners.first() |
There was a problem hiding this comment.
Do you know if we eventually want to redirect to the default owner if there's one? If so might be worth leaving a comment here as well
There was a problem hiding this comment.
Not really sure. I dunno if there will be a concept of a default owner - we might just redirect to a list of possible owners and you pick one? I dunno.
There was a problem hiding this comment.
gotcha gotcha, the other login routes have a way to pick this up, but I don't expect that to be part of the 1st pass, maybe more of a mental note for the future
| # user has not connected any owners yet | ||
| return redirect(f"{settings.CODECOV_DASHBOARD_URL}/sync") | ||
|
|
||
| def _login_user(self, request: HttpRequest, user_data: dict): |
There was a problem hiding this comment.
Is there generally a way to automatically map a dictionary to a dataclass or a typedDict? Like a utility to do so? Not necessarily for this PR but I think can come in handy in the future
There was a problem hiding this comment.
It's possible to declare the types for everything in a dict using TypedDict: https://peps.python.org/pep-0589/
There was a problem hiding this comment.
Yeaah, what I'm thinking is in situations like this, or when we use Stripe for instance, we expect a dictionary from a webhook or from another server's response and we don't type those. Im wondering if there's a way to automatically pass dictionaries into some utility/middleware to transform any dictionary into its dataclass/typeddict equivalent so we always have the shape of an object you know? Not for this, but that would be something neat
There was a problem hiding this comment.
Oh, like auto generate the TypedDict definition based on some data we pass? That could be convenient but we'd still need to review the third-party docs and make sure each type is potentially nullable, etc. Our example data might not be representative of every case.
* main: conflicts merge + pr review feedback More PR feedback review Adjust code based on PR feedback Added command to add initital value for trial status Minor test+logic adjustment Modified plan + billing file adjusted tests and got rid of prints Use plan service thorughout the codebase
scott-codecov
force-pushed
the
scott/sentry-oauth
branch
from
09e4994
to
36528e2
Compare
Purpose/Motivation
Login with Sentry
Links to relevant tickets
codecov/engineering-team#87
codecov/engineering-team#88
What does this PR do?
/login/sentrycodecov_auth_sentryuserwhich stores Senty user info + access token and belongs to a singleUser/internal/userendpoint for returning the current user infoownersto which the current user is linkedTODO
Sentry folks are still working on the OIDC implementation. At the very least we should try decoding the
id_token(a JWT) using a shared secret that we were given. This way we can ensure the OAuth callback redirect is indeed being made by Sentry.