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.

Sign up for GitHub

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

Jump to bottom

🎉 advanced auth impl #8345

Merged
merged 12 commits into from
Dec 3, 2021
Merged

🎉 advanced auth impl #8345

merged 12 commits into from
Dec 3, 2021

Conversation

jamakase
Copy link
Contributor

@jamakase jamakase commented Nov 30, 2021 •
edited
Loading

What

Support for new AdvancedAuth flow. This PR changes behaviour only and keeps the same UI for now.

The main change here is that we now try to base our current selected values in oneOf and whether we should display button or not on current selected values instead of WidgetState. This allows us to change UI the way we want later and makes it more flexible.

Closes #7915
Also to make new advanced auth work, it was required to fix 1 more issue ( Partially fixes #7986 )

How

  • Add new isAuthFlowSelected flag that is based on current values and whether oauthFlow is enabled or not to serviceFormContext
  • Move different sections to separate files into ServiceForm/components/Sections directory
  • Mark old auth flow as LegacyAuthFlow
  • Move FeatureSet to config directory.

Recommended reading order

  1. ServiceForm/../FormRoot.tsx
  2. ServiceForm/../useOauthFlowAdapter.ts
  3. services/useConnectorAuth.ts

@github-actions github-actions bot added area/frontend area/platform issues related to the platform labels Nov 30, 2021
@jamakase jamakase self-assigned this Nov 30, 2021
@jamakase jamakase temporarily deployed to more-secrets December 1, 2021 22:12 Inactive
@jamakase jamakase temporarily deployed to more-secrets December 1, 2021 22:47 Inactive
@jamakase jamakase temporarily deployed to more-secrets December 2, 2021 18:07 Inactive
@jamakase jamakase temporarily deployed to more-secrets December 2, 2021 20:32 Inactive
@jamakase jamakase temporarily deployed to more-secrets December 2, 2021 21:51 Inactive
@jamakase jamakase temporarily deployed to more-secrets December 2, 2021 22:52 Inactive
@jamakase jamakase changed the title Jamakase/advanced auth impl 🎉 advanced auth impl Dec 2, 2021
@jamakase jamakase added the type/enhancement New feature or request label Dec 2, 2021
ChristopheDuong
ChristopheDuong reviewed Dec 3, 2021
View reviewed changes
Copy link
Contributor

@ChristopheDuong ChristopheDuong left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Once I merged master branch and this PR #8482 into your branch, I was able to use the new OAuth flow and create a connection/make a sync with source-harvest:0.1.7

@jamakase jamakase temporarily deployed to more-secrets December 3, 2021 22:28 Inactive
@jrhizor jrhizor merged commit 8ebd3bd into master Dec 3, 2021
@jrhizor jrhizor deleted the jamakase/advanced-auth-impl branch December 3, 2021 23:34
@jrhizor jrhizor mentioned this pull request Dec 4, 2021
Merged
@ChristopheDuong
Copy link
Contributor

If I am not wrong, this PR handles the new advanced_auth object in the spec.json, but it doesn't do anything about the new field: oauth_user_input_from_connector_config_specification yet, right @jamakase?

Some values from the connector form need to be forwarded to the OAuth API endpoints (as specified in the protocol by that new field)

When calling the consent request API endpoint:

airbyte/airbyte-api/src/main/openapi/config.yaml

Line 3390 in 65ef929

oAuthInputConfiguration:

When calling the complete OAuth API endpoint:

airbyte/airbyte-api/src/main/openapi/config.yaml

Line 3432 in 65ef929

oAuthInputConfiguration:

@jamakase
Copy link
Contributor Author

jamakase commented Dec 9, 2021 •
edited
Loading

If I am not wrong, this PR handles the new advanced_auth object in the spec.json, but it doesn't do anything about the new field: oauth_user_input_from_connector_config_specification yet, right @jamakase?

Ammm Actually it should add any additional params ( you could check airbyte-webapp/src/views/Connector/ServiceForm/components/Sections/auth/useOauthFlowAdapter.tsx and airbyte-webapp/src/hooks/services/useConnectorAuth.tsx)

Do you see some problems with implementation?

schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
@jamakase @schlattk
🎉 advanced auth impl (airbytehq#8345)
2ce7c5d 
* Add advanced auth impl

* Refactor ServiceForm for advanced auth flow

* Add auth field filter

* Add input params to customSchema

* Fix casing and values sent to backend

* Fix onDone dependency list

* Fix feature set

* Refactor feature service to be part of config

* Minor codestyle fix

* Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristopheDuong ChristopheDuong ChristopheDuong approved these changes

@jrhizor jrhizor jrhizor approved these changes

@sherifnada sherifnada Awaiting requested review from sherifnada

Assignees

@jamakase jamakase

Labels
area/platform issues related to the platform type/enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dropdowns are not correctly treated as required Change UI to use new OAuth API/Protocol
3 participants
@jamakase @ChristopheDuong @jrhizor