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

CDK: Authenticator doesn't support auth with query params #3468

Closed
keu opened this issue May 18, 2021 · 3 comments · Fixed by #5731
Closed

CDK: Authenticator doesn't support auth with query params #3468

keu opened this issue May 18, 2021 · 3 comments · Fixed by #5731
Assignees
Labels
CDK Connector Development Kit CDKv2 type/enhancement New feature or request

Comments

@keu
Copy link
Contributor

keu commented May 18, 2021

Tell us about the problem you're trying to solve

The current implementation naively assumes that all authentication works through request headers, unfortunately, that is not the case, API keys can be sent via:

  • query params
  • cookies
  • body post

Describe the solution you’d like

The authenticator should support only one method:

def authenticate(self, session: Session):
    pass

Describe the alternative you’ve considered or used

Set Authenticator to NoAuth and implement authentication in request_params method.

Additional context

Add any other context or screenshots about the feature request here.

Are you willing to submit a PR?

Your answer

┆Issue is synchronized with this Asana task by Unito

@keu keu added type/enhancement New feature or request zazmic labels May 18, 2021
@sherifnada sherifnada added the CDK Connector Development Kit label May 25, 2021
@sherifnada sherifnada added CDKv2 and removed zazmic labels Aug 6, 2021
@htrueman htrueman self-assigned this Aug 17, 2021
@htrueman
Copy link
Contributor

Scoping report:

May be linked to #5246
During the research I've discovered that the best way to implement base universal authenticator class is to use it as auth argument to requests.Request class.

See docs: https://docs.python-requests.org/en/master/user/authentication/

Here is the suggested auth implementation:

  • Create a base auth class (e.g. BaseAuthenticator), inherited from requests.auth.AuthBase;
  • In this class we may add some base methods, so we could handle multiple authentication flows;
  • All of the AuthBase authentication logics needs to be put inside the __call__ method (there is good article with examples http://xion.io/post/code/requests-query-string-auth.html)
  • Create the child classes, which are going to implement some of our main authentication methods (currently NoAuth, Oauth2Authenticator, TokenAuthenticator)
  • Update the rest of the codebase to support newly created authenticators (I think it should be done in the follow up issue, and before that it's better to keep both versions of the authenticator classes).

@VasylLazebnyk VasylLazebnyk added this to the Connectors August 20th milestone Aug 17, 2021
@keu
Copy link
Contributor Author

keu commented Aug 19, 2021

we don't need NoAuth, as it is simply None

@keu
Copy link
Contributor Author

keu commented Aug 19, 2021

also, it is better to have session auth instead of Request(auth=

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

Successfully merging a pull request may close this issue.

5 participants