feat: Okta login #67
feat: Okta login #67
Conversation
Codecov Report
@@ Coverage Diff @@
## main #67 +/- ##
=======================================
+ Coverage 95.35 95.37 +0.02
=======================================
Files 702 704 +2
Lines 14965 15098 +133
=======================================
+ Hits 14269 14399 +130
- Misses 696 699 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
* main: (74 commits) Fix indentation error (#133) Add cache cleanup (#130) Feature/lint pre commit (#134) feat: trigger label analysis task after update (#131) Filter file comparisons by flags (#129) chore: Remove hard-coded dev BB redirect URL (#132) feat: Validate OIDC ID token during Sentry OAuth flow (#52) Adding beginnings of GHA CI (#127) feat: Filter flags by flags for pathContents (#128) Create checkbox in Owner form in Django admin to set uses_invoice (#109) build(deps): bump certifi from 2020.6.20 to 2023.7.22 (#32) Feature/no compile (#126) Bump django from 4.2.2 to 4.2.3 (#42) Don't compile since source is available (#106) feat: Add firstPull resolver to GraphQL pull type (#108) chore: Upgrade requests and redis dependencies (#124) Update LICENSE (#122) Attempt migration (#121) 359 adjust monthly uploads for trialled customers (#119) Add changes for monthly uploads to account for trialing customer (#101) ...
| fields=[ | ||
| ("id", models.BigAutoField(primary_key=True, serialize=False)), | ||
| ("external_id", models.UUIDField(default=uuid.uuid4, editable=False)), | ||
| ("created_at", models.DateTimeField(auto_now_add=True)), |
There was a problem hiding this comment.
Hmmm, when we use the BaseCodecovModel, we don't save this with the ...WithoutTZField, we okay with that?
There was a problem hiding this comment.
I think it's probably fine. We're extending from BaseCodecovModel in a lot of other models as well.
scott-codecov
force-pushed
the
scott/okta-login
branch
from
0dc2c13
to
dee42c3
Compare
| if request.GET.get("code"): | ||
| return self._perform_login(request) | ||
| else: | ||
| iss = settings.OKTA_ISS or request.GET.get("iss") |
There was a problem hiding this comment.
Optional: not sure if iss over issuer is understandable if you're familiar with OIDC, but otherwise I'd push for the latter for readability
There was a problem hiding this comment.
The query string param has to be iss since that's what Okta sends. I think since it's referred to as iss elsewhere (i.e. in the ID token, query param, etc.) then I'm inclined to keep it as iss so you know it's the same thing. I expect you'd need some familiarity with OIDC to fully follow this code anyway.
There was a problem hiding this comment.
Gotchaa, yeap that makes sense! We can keep it as is
| def _perform_login(self, request: HttpRequest) -> HttpResponse: | ||
| code = request.GET.get("code") | ||
| iss = settings.OKTA_ISS or request.COOKIES.get("_okta_iss") | ||
| if iss is None: |
There was a problem hiding this comment.
Could we ever hit this? Since at the very least we'd have the iss from _redirect_to_consent
There was a problem hiding this comment.
I'd say this would be unexpected. IDK - maybe somebody has cookies disabled or something.
There was a problem hiding this comment.
Yeaah that's fair, asking in case this could be dead code that adds complexity, but I think that's fair. In that scenario they wouldn't be able to use okta hmmm, unlikely, but we can keep it in here and see if we hit this
| okta_user = OktaUser.objects.filter(okta_id=okta_id).first() | ||
|
|
||
| current_user = None | ||
| if request.user is not None and not request.user.is_anonymous: |
There was a problem hiding this comment.
Say a user log in with Okta, and eventually with Github. Woul that user be connected to both? (that would make sense to me)
There was a problem hiding this comment.
Yeah, they'll have a single users record and then an okta_users record + owners record that both reference that single user.
| response = redirect(f"{settings.CODECOV_DASHBOARD_URL}/{service}") | ||
| else: | ||
| # user has not connected any owners yet | ||
| response = redirect(f"{settings.CODECOV_DASHBOARD_URL}/sync") |
There was a problem hiding this comment.
Is this a new url? I don't think I've seen this sync endpoint 👀
There was a problem hiding this comment.
Yeah a new page that Nick created to support "Login with Sentry". Once a user is authenticated but doesn't have any linked Git providers we send them to this page (where they can sync a Git provider).
There was a problem hiding this comment.
Oh it's not merged yet: codecov/gazebo#2235
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
Codecov Report
@@ Coverage Diff @@
## main #67 +/- ##
=======================================
Coverage ? 95.49%
=======================================
Files ? 590
Lines ? 14886
Branches ? 0
=======================================
Hits ? 14215
Misses ? 671
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
Purpose/Motivation
Login to Codecov using Okta
Links to relevant tickets
codecov/engineering-team#145
codecov/engineering-team#146
What does this PR do?
Adds a new OAuth flow for Okta available at
/login/oktaNotes to Reviewer
Unlike other OAuth providers we've integrated with, Okta requires an "issuer" which is a domain like
https://{orgname}.okta.com. When clicking on a tile in Okta the issuer is passed as anissquery string param which we store in a cookie for the duration of the OAuth flow.If we ever want to support some sort of "Single Sign On" button in Codecov then we would first need to ask for this issuer domain (or just
orgname).This will only be deployed in ECDN environments to start. We'll configure the
OKTA_ISSfor a specific org and setDISABLE_GIT_BASED_LOGINto true. This will force members of that org to use Okta to login and only then can they link their Git provider account.