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

Source Okta: OAuth2.0 support - disabled #20877

Merged
merged 11 commits into from
Jan 6, 2023
Merged

Conversation

lazebnyi
Copy link
Collaborator

@lazebnyi lazebnyi commented Dec 23, 2022

What

OAuth 2.0 temporarily disabled until full we get public account. - https://airbytehq.slack.com/archives/C02U9R3AF37/p1671731021620059

How

Deleted part of spec implementation.

@octavia-squidington-iv octavia-squidington-iv added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/okta labels Dec 23, 2022
@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jan 4, 2023

/test connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/3837955948
❌ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/3837955948
🐛 https://gradle.com/s/2bva246hhtr4m

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestConnection::test_check[inputs0] - AssertionError: as...
FAILED test_core.py::TestConnection::test_check[inputs1] - AssertionError: as...
FAILED test_core.py::TestConnection::test_check[inputs2] - AssertionError: as...
FAILED test_core.py::TestBasicRead::test_read[inputs0] - docker.errors.Contai...
FAILED test_core.py::TestBasicRead::test_read[inputs1] - docker.errors.Contai...
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs0]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs0]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:377: The previous and actual discovered catalogs are identical.
============ 9 failed, 25 passed, 1 skipped, 31 warnings in 28.89s =============

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jan 5, 2023

/test connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/3850217641
✅ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/3850217641
Python tests coverage:

Name                           Stmts   Miss  Cover
--------------------------------------------------
source_okta/utils.py              44      0   100%
source_okta/authenticator.py      14      0   100%
source_okta/__init__.py            2      0   100%
source_okta/source.py            194      7    96%
--------------------------------------------------
TOTAL                            254      7    97%
	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       141      5    96%   87, 93, 239, 243-244
	 source_acceptance_test/conftest.py                     211     95    55%   36, 42-44, 49, 54, 77, 83, 89-91, 110, 115-117, 123-125, 131-132, 137-138, 143, 149, 158-167, 173-178, 193, 217, 248, 254, 262-267, 275-285, 293-306, 311-317, 324-335, 342-358
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              402    115    71%   53, 58, 93-104, 109-116, 120-121, 125-126, 308, 346-363, 376-387, 391-396, 402, 435-440, 478-485, 528-530, 533, 598-606, 618-621, 626, 682-683, 689, 692, 728-738, 751-776
	 source_acceptance_test/tests/test_incremental.py       160     14    91%   56-63, 68-81, 244
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1609    339    79%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:377: The previous and actual discovered catalogs are identical.
================== 32 passed, 1 skipped in 217.57s (0:03:37) ===================

@alafanechere
Copy link
Contributor

@lazebnyi could you please share more details about the motivation of this PR (I can't access the slack conversation). What will be the impact on existing cloud connections?

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jan 6, 2023

@alafanechere During Okta certification to Beta we implement OAuth, but now we has problem with getting public application. So will be decided hide Okta OAuth flow from cloud (leave only credentials manual input) to avoid issue during authentication with common OAuth flow.

I also refactored the acceptance tests to high strictness for speed up future certification.

@alafanechere
Copy link
Contributor

Thanks for the context. @lazebnyi does it mean that Okta OAuth is not working on cloud (because we don't have a public application) and its safe to disable it?

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jan 6, 2023

@alafanechere
Okta OAuth has never worked on the cloud 😅
Yes, it's safe. With this PR, the user will be authenticated with client_id, client_secret and refresh_token or with an API token.

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation and for setting high test strictness level!

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jan 6, 2023

/publish connector=connectors/source-okta

🕑 Publishing the following connectors:
connectors/source-okta
https://github.com/airbytehq/airbyte/actions/runs/3856783659


Connector Did it publish? Were definitions generated?
connectors/source-okta

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 6, 2023 16:55 — with GitHub Actions Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 6, 2023 16:55 — with GitHub Actions Inactive
Copy link
Contributor

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

Do we need to remove any oAuth secrets from Cloud? If users have previously used oAuth + Okta will the connector still try to use that method of connection from the config, even though the SEPC option is removed?

(edit) Looks like we never got the app authorized, so this is unlikely. Carry on!

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jan 6, 2023

@evantahler we remove only part what get user authenticate with common OAuth flow (enter creds to provider and approve scopes). So, if user already authenticate using OAuth flow he has refresh_token and can refresh it with connector authentication logic (but it's not possible because we never has public application for our account, so OAuth authentication with UI never work)

We never had an public account for Okta, so we no need delete any oAuth secrets from Cloud.

@lazebnyi lazebnyi temporarily deployed to more-secrets January 6, 2023 18:44 — with GitHub Actions Inactive
@lazebnyi lazebnyi temporarily deployed to more-secrets January 6, 2023 18:44 — with GitHub Actions Inactive
@lazebnyi lazebnyi temporarily deployed to more-secrets January 6, 2023 22:27 — with GitHub Actions Inactive
@lazebnyi lazebnyi temporarily deployed to more-secrets January 6, 2023 22:27 — with GitHub Actions Inactive
@lazebnyi lazebnyi enabled auto-merge (squash) January 6, 2023 22:43
@lazebnyi lazebnyi merged commit f9fc56c into master Jan 6, 2023
@lazebnyi lazebnyi deleted the lazebnyi/disable-oauth-okta branch January 6, 2023 23:01
@pedroslopez
Copy link
Contributor

pedroslopez commented Jan 6, 2023

@evantahler / @lazebnyi - I do see we had configured global OAuth parameters in cloud prod for Okta. In the past we've seen leaving the OAuth parameters in place cause issues when removing OAuth support (e.g. PayPal Transactions pulling in the same data for all users), so whenever we decide to remove OAuth support we should also make sure to remove the global OAuth parameters that may have been set.

Since this has now been released to cloud, I have removed the global OAuth parameters for Okta from the Cloud prod db.

Note: I found one active OAuth connection (for non-internal/test workspaces) for Okta, but it's set to manual mode and the workspace is called "Okta Partner Account" so presumably we know about this one and are ok with this no longer working?

jbfbell pushed a commit that referenced this pull request Jan 13, 2023
* Disabled OAuth Okta

* Updated acceptance tests

* Updated PR number

* auto-bump connector version

Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/okta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants