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

Low code CDK: Conditional authenticator #28351

Closed
flash1293 opened this issue Jul 17, 2023 · 5 comments · Fixed by #33526 · May be fixed by #33536
Closed

Low code CDK: Conditional authenticator #28351

flash1293 opened this issue Jul 17, 2023 · 5 comments · Fixed by #33526 · May be fixed by #33536
Assignees
Labels
CDK Connector Development Kit needs-triage team/extensibility type/enhancement New feature or request

Comments

@flash1293
Copy link
Contributor

What area the feature impact?

None

Revelant Information

It's a common scenario to allow the user of a connector to choose from multiple possible authentication methods. Currently this is not reflected in the low code CDK.

By introducing a new component ConditionalAuthenticator, this can be made a properly supported feature:

type: ConditionalAuthenticator
authenticators:
  - condition: "{{ config.auth.type is "username_password" }}"
    authenticator:
      # ... 
  - condition: "{{ config.auth.type is "token" }}"
    authenticator:
      type: ApiKeyAuthenticator
      header: "X-Metabase-Session"
      api_token: "{{ config.auth.session_token }}"

Once this is supported, the existing LegacySessionTokenAuthenticator can be removed as it will be possible to re-create it using the conditional authenticator.

@flash1293 flash1293 added type/enhancement New feature or request CDK Connector Development Kit needs-triage team/extensibility labels Jul 17, 2023
@maxi297
Copy link
Contributor

maxi297 commented Aug 15, 2023

+1 on Slack by Markos from Hackathon

@girarda
Copy link
Contributor

girarda commented Nov 17, 2023

Alternative interface that uses a selector instead of string interpolation:

type: OneOfAuthenticator
config_path:
 - auth
 - type
authenticators:
  token:
    type: ApiKeyAuthenticator
    header: "X-Metabase-Session"
    api_token: "{{ config.auth.session_token }}"
  username_password:
   type:
   # ...

I don't feel strongly about the use of a mapping vs array of authenticators, but avoiding string interpolation makes the interface simpler

Acceptance criteria

  • low-code connector developers can create connectors that support multiple types of authentication mechanism without custom components
  • source-retently does not use a custom authenticator
  • Fine to swap with facebook-pages, zoom, or square if source-retently is not in a healthy state when this issue is implemented
  • A follow up issue is created to add this functionality to the connector builder

@girarda
Copy link
Contributor

girarda commented Nov 28, 2023

grooming notes:

  • if there are no authenticators that meet the condition: the source should raise an exception
  • config_path is not an ideal field name. we should decide on something more descriptive. Idea: authenticator_selection_path

@keu
Copy link
Contributor

keu commented Dec 15, 2023

the PR for retently #33536

@keu
Copy link
Contributor

keu commented Dec 19, 2023

@girarda I've just realized that for affected connectors this is a backward incompatible change.
I.e. user has to specify new field in the connector config - auth.type
What is our strategy in such cases?

Migrate it one by one with boosting major version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit needs-triage team/extensibility type/enhancement New feature or request
Projects
None yet
4 participants