[Okta Login] Part 2: Add Okta Login API for Organizations/Account

michelletran-codecov commented 1 month ago

Purpose/Motivation

This adds the login endpoint for uses to sign into Okta for specific orgs/account. Depends on Part 1: https://github.com/codecov/codecov-api/pull/691

Links to relevant tickets

https://github.com/codecov/engineering-team/issues/1988

What does this PR do?

Note that this is not to authenticate the user. We check that the user has already authenticated via another mechanism (i.e. Github). This adds extra permissions for organizations that want extra level of security to see private repos.

The authentication flow looks something like this:

sequenceDiagram
    User->>+Codecov: /login/okta/{service}/{org}
    Codecov->>+Okta: if not signed in, then redirect them to okta.com/oauth2/v1/authorized
    Okta->>+User: shows the user the Okta sign-in UI
    User->>+Okta: enters in credentials and signs in
    Okta->>+Codecov: sends back credentials info to Codecov via /login/okta/callback
    Codecov->>+Okta: queries auth token via okta.com/oauth2/v1/token
    Codecov->>+Okta: decrypts auth token with keys okta.com/oauth2/v1/keys
    Codecov->>+User: if auth is successful, stores the authenticated account in user session

This PR adds the 2 endpoints:

/login/okta/{service}/{org} to send the user to Okta
/login/okta/callback to finish authenticating the user on Codecov

Notes to Reviewer

I'm not sure about the naming of OktaCloudView. Feel free to suggest another name.

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

codecov-staging[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:white_check_mark: All tests successful. No failed tests found.

:loudspeaker: Thoughts on this report? Let us know!

codecov-qa[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.70%. Comparing base (4fde36b) to head (d5737e6). Report is 3 commits behind head on main.

:white_check_mark: All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main     #685      +/-   ##
==========================================
+ Coverage   91.63%   91.70%   +0.06%     
==========================================
  Files         632      633       +1     
  Lines       16872    17005     +133     
==========================================
+ Hits        15461    15594     +133     
  Misses       1411     1411

Flag	Coverage Δ
unit	`91.70% <100.00%> (+0.06%)`	:arrow_up:
unit-latest-uploader	`91.70% <100.00%> (+0.06%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
codecov_auth/urls.py	`100.00% <100.00%> (ø)`
codecov_auth/views/okta_cloud.py	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

codecov-public-qa[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.70%. Comparing base (4fde36b) to head (d5737e6). Report is 3 commits behind head on main.

:white_check_mark: All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main     #685      +/-   ##
==========================================
+ Coverage   91.63%   91.70%   +0.06%     
==========================================
  Files         632      633       +1     
  Lines       16872    17005     +133     
==========================================
+ Hits        15461    15594     +133     
  Misses       1411     1411

Flag	Coverage Δ
unit	`91.70% <100.00%> (+0.06%)`	:arrow_up:
unit-latest-uploader	`91.70% <100.00%> (+0.06%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
codecov_auth/urls.py	`100.00% <100.00%> (ø)`
codecov_auth/views/okta_cloud.py	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.04%. Comparing base (4fde36b) to head (d5737e6). Report is 3 commits behind head on main.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

:white_check_mark: All tests successful. No failed tests found.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #685 +/- ## ================================================ + Coverage 95.98000 96.04000 +0.06000 ================================================ Files 812 813 +1 Lines 18209 18499 +290 ================================================ + Hits 17477 17767 +290 Misses 732 732 ``` | [Flag](https://app.codecov.io/gh/codecov/codecov-api/pull/685/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=codecov) | Coverage Δ | | |---|---|---| | [unit](https://app.codecov.io/gh/codecov/codecov-api/pull/685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=codecov) | `91.70% <100.00%> (+0.06%)` | :arrow_up: | | [unit-latest-uploader](https://app.codecov.io/gh/codecov/codecov-api/pull/685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=codecov) | `91.70% <100.00%> (+0.06%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=codecov#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

michelletran-codecov commented 1 month ago

We're not actually redirecting the app to an untrusted source. It is redirecting to the Codecov dashboard with some user provided values, which are validated against orgs. We return a 404 if the organization doesn't exist.

michelletran-codecov commented 1 month ago

I’m by far no expert on django internals, but isn’t it possible to have some kind of automatic decorators that resolve url parameters automatically, so you don’t have to query / check for a valid org manually?

I believe that there probably is a way to do this via Django Mixins.. I didn't go down this route because of time (and not sure about how useful it would be generally).

You use logging and assert log messages for certain outcomes. IMO it would make sense to just put those things into the response and assert that instead.

See comment about security-usability trade-offs.

Is it even possible and/or desirable to test against "real world" okta? pro: it creates more confidence than a mocked response (which itself could be broken), but also con: now your CI depends on an external service which might be slow and flaky

I tested this locally and on staging with a real Okta Preview account, but it's very manual. I don't want to automate this though because of the potential for flakiness. We can consider using smoke testing or synthetics if this feature becomes very impactful.

Thanks for all the questions and the thorough review! :)

codecov / codecov-api