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

Update fence to now include Okta as an OIDC Identity Provider #890

Merged
merged 31 commits into from
Apr 20, 2021

Conversation

cginmn
Copy link
Contributor

@cginmn cginmn commented Mar 30, 2021

Added a new profile for Okta to be additional option for an OpenID Connect (OIDC) Identity Provider.

New Features
I essentially took the current Azure AD OIDC profile as a template and modified it to work with Okta as the provider.

Breaking Changes
Bug Fixes

Improvements
Now supports Okta as an OIDC provider

Dependency updates
Deployment changes

cginmn and others added 24 commits August 27, 2020 12:55
Removed some unessessary generic settings.
revert to how it is in source before okta tests.
clean up of my build file.
Adding the discovery URL for okta in test config to see if that solves issue in testing scripts.
changed the discoveryURL to have {tenant} instaed of real value for tests.
Updates to the testing scripts to return a status code for the Okta discovery URL which is not valid. Okta doesn't have a general discover URL,  only specific to tenants, so can't use one in testing framework.
removed unused modules from fence/blueprints/login/okta.py and fence/resources/openid/okta_oauth2.py
As reported by codacy, removed a trailing whitespace.
PR has fallen behind current main changes. Updated test-fence-config from main branch to get back in sync, now re-applying changes to test-fence-config.yaml for Okta configuration.
Removing extra blank lines from test_login_redirect.py
Added okta as an idp to test_audit_services.py
@coveralls
Copy link

coveralls commented Mar 30, 2021

Pull Request Test Coverage Report for Build 10767

  • 24 of 36 (66.67%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 70.44%

Changes Missing Coverage Covered Lines Changed/Added Lines %
fence/resources/openid/okta_oauth2.py 8 20 40.0%
Totals Coverage Status
Change from base Build 10765: -0.004%
Covered Lines: 5912
Relevant Lines: 8393

💛 - Coveralls

Copy link
Contributor

@vpsx vpsx left a comment

Choose a reason for hiding this comment

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

Hey @cginmn , thanks for your PR and sorry it took so long to review! This LGTM; just one thing I noticed during manual testing--see inline comment.

Other than that, we're working to get this through the integration tests, and then will approve ASAP. Thanks again!

fence/resources/openid/okta_oauth2.py Show resolved Hide resolved
@vpsx
Copy link
Contributor

vpsx commented Mar 31, 2021

*whoops--integration tests and also a little bit more manual testing!

cginmn and others added 4 commits April 15, 2021 12:30
Update the mock response for username and also updated comments for okta IDP on test-fence-config
per recommendations, ran "black ." to fix formatting.
Copy link
Contributor

@vpsx vpsx left a comment

Choose a reason for hiding this comment

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

LGTM!

Note for posterity: Needs this cloud-auto modification uc-cdis/cloud-automation#1566 for squid to allow the oauth handshake. If Fence complains about a sub claim, make sure your Okta user has a username and Okta is otherwise configured so that the sub claim is populated.

Thank you for your contribution @cginmn and for your patience! 🙏

@vpsx vpsx merged commit 1096198 into uc-cdis:master Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants