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: added SessionTokenAuthenticator #19716

Merged
merged 7 commits into from
Dec 12, 2022

Conversation

darynaishchenko
Copy link
Collaborator

This auth method will be used in source-metabase #19236

@darynaishchenko darynaishchenko requested a review from a team as a code owner November 22, 2022 15:11
@darynaishchenko darynaishchenko self-assigned this Nov 22, 2022
@octavia-squidington-iv octavia-squidington-iv added the CDK Connector Development Kit label Nov 22, 2022
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -115,3 +119,103 @@ def token(self) -> str:
auth_string = f"{self._username.eval(self.config)}:{self._password.eval(self.config)}".encode("utf8")
b64_encoded = base64.b64encode(auth_string).decode("utf8")
return f"Basic {b64_encoded}"


cacheSessionTokenAuthenticator = TTLCache(maxsize=1000, ttl=86400)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment on how those values were chosen for posterity? Is it worth parameterizing them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@girarda
Copy link
Contributor

girarda commented Nov 22, 2022

@darynaishchenko please run format on the files before merging because some files are not properly formatted

Copy link
Contributor

@brianjlai brianjlai left a comment

Choose a reason for hiding this comment

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

small adjustment on the version since its a new component feature, but otherwise looks good to me!

@@ -1,5 +1,8 @@
# Changelog

## 0.9.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that this is a net new feature, we should bump the minor version to 0.10.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@darynaishchenko
Copy link
Collaborator Author

@girarda @brianjlai could you help me with it please. I'm doing this steps from README

Setup a virtual env:

python -m venv .venv
source .venv/bin/activate
pip install -e ".[dev]" # [dev] installs development-only dependencies
Iteration
Iterate on the code locally
Run tests via pytest -s unit_tests
Perform static type checks using mypy airbyte_cdk. MyPy configuration is in .mypy.ini.

But I get type checks errors in files which I didn't change. Is this ok, or I do something wrong?

@darynaishchenko darynaishchenko force-pushed the daryna/airbyte-cdk-session-token-auth-for-low-code branch 2 times, most recently from 73f318a to 6505267 Compare November 30, 2022 13:58
@girarda
Copy link
Contributor

girarda commented Dec 7, 2022

@darynaishchenko what are the type check errors you're seeing?

@darynaishchenko
Copy link
Collaborator Author

@darynaishchenko please run format on the files before merging because some files are not properly formatted

done

@darynaishchenko
Copy link
Collaborator Author

@darynaishchenko what are the type check errors you're seeing?

It's not relevant, I fixed checks. thanks

if response.ok:
json_response = response.json()
self.logger.info(
f"Connection check for source successful for {json_response['common_name']} login at {json_response['last_login']}"
Copy link
Contributor

Choose a reason for hiding this comment

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

These two fields need to be configurable because the endpoints won't always return responses with "common_name" and "last_login" fields

Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively, it would be fine not to log this much details here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

username = self._username.eval(self.config)
password = self._password.eval(self.config)
session_token_response_key = self._session_token_response_key.eval(self.config)
api_url = f"{self._api_url.eval(self.config)}session"
Copy link
Contributor

Choose a reason for hiding this comment

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

the path will need to be configurable. This could be done by adding a field "login_url", which could be configured as
"session" in the case of metabase

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

def is_valid_session_token(self) -> bool:
try:
response = requests.get(
f"{self._api_url.eval(self.config)}user/current", headers={self.auth_header: self._session_token.eval(self.config)}
Copy link
Contributor

Choose a reason for hiding this comment

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

the path will need to be configurable. This could be done by adding a field "validate_session_url", which could be configured as
"/user/current" in the case of metabase

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@lazebnyi lazebnyi linked an issue Dec 9, 2022 that may be closed by this pull request
@darynaishchenko darynaishchenko requested a review from a team as a code owner December 12, 2022 09:51
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 12, 2022
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

It seems like the stylesheet changes are purely about whitespace, could you revert them?

@darynaishchenko darynaishchenko force-pushed the daryna/airbyte-cdk-session-token-auth-for-low-code branch from 081815e to 6713da8 Compare December 12, 2022 10:14
@octavia-squidington-iv octavia-squidington-iv removed area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 12, 2022
@darynaishchenko
Copy link
Collaborator Author

It seems like the stylesheet changes are purely about whitespace, could you revert them?

thanks, fixed

@darynaishchenko darynaishchenko force-pushed the daryna/airbyte-cdk-session-token-auth-for-low-code branch from 101302b to a4346c6 Compare December 12, 2022 10:45
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

this is great! :shipit:

@lazebnyi
Copy link
Collaborator

lazebnyi commented Dec 12, 2022

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/3679536348
https://github.com/airbytehq/airbyte/actions/runs/3679536348

@lazebnyi
Copy link
Collaborator

lazebnyi commented Dec 12, 2022

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3679590977
https://github.com/airbytehq/airbyte/actions/runs/3679590977

@lazebnyi lazebnyi merged commit 2f306ea into master Dec 12, 2022
@lazebnyi lazebnyi deleted the daryna/airbyte-cdk-session-token-auth-for-low-code branch December 12, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Low-Code: added SessionTokenAuthenticator
6 participants