perf: Implement mechanism for caching token on login #146
Conversation
…lt-python-sdk into FIR-11540-reuse-token
src/firebolt/common/token_storage.py
Outdated
| self._data_dir = user_data_dir(appname="firebolt") | ||
| os.makedirs(self._data_dir, exist_ok=True) | ||
|
|
||
| self._token_file = os.path.join(self._data_dir, "token.json") |
There was a problem hiding this comment.
Seems like this implementation only supports caching token for a single set of credentials.
I would suggest changing token.json to something like hash(username + password).json, which would resolve this issue
src/firebolt/common/token_storage.py
Outdated
|
|
||
| return self.encrypter.decrypt(res["token"]) | ||
|
|
||
| def cache_token(self, token: str) -> None: |
There was a problem hiding this comment.
It could be useful to also store token expiration here. This way we can reduce the amount of requests by skipping expired tokens
src/firebolt/common/token_storage.py
Outdated
| ) | ||
|
|
||
| @staticmethod | ||
| def generate_salt() -> str: |
There was a problem hiding this comment.
nit: separate method
.github/workflows/code-check.yml
Outdated
|
|
||
| - name: Type check with mypy | ||
| run: | | ||
| mypy src |
There was a problem hiding this comment.
I believe mypy is a part of pre-commit now
| assert ( | ||
| token | ||
| == TokenSecureStorage( | ||
| username="username", password="password" |
There was a problem hiding this comment.
nit: **settings like in others
| if exc.response.status_code == codes.UNAUTHORIZED: | ||
| # Do not raise an exception on UNAUTHORIZED | ||
| # It should be responsibility of authentication mechanism | ||
| return |
There was a problem hiding this comment.
What if the token is expired after auth and before command execution?
.github/workflows/code-check.yml
Outdated
| with: | ||
| extra_args: --all-files | ||
|
|
||
| - name: Type check with mypy |
There was a problem hiding this comment.
Mypy checks are already performed inside pre-commit
|
Kudos, SonarCloud Quality Gate passed! |
| @fixture(autouse=True) | ||
| def global_fake_fs() -> None: | ||
| with Patcher(): | ||
| yield |
There was a problem hiding this comment.
To be honest, I preferred explicitly adding fs to the relevant tests. That way there's no hidden functionality, you know which tests are hitting a fake file system and which do not.
But up to you, I'm not too worried about it.
There was a problem hiding this comment.
This is required because now any authentication leaves token cache artifact file, which also causes tests to fail
There was a problem hiding this comment.
We would need to add fake fs to almost every integration test and to a lot of unit tests
3 participants
On the first successful authentication, get the token and save it in app_config_dir token file. The token itself is encrypted using the username and password tuple. Hence, the token cannot be accessed, by someone who doesn’t know the username and the password.
During establishing the connection, we check, whether something is written in the app_config_dir token file. If yes, we decrypt it using the username and password. In case of successful decryption, we use this token for connection. If the connection failed, we use username, password as a fallback option.
For the encryption/decryption symmetric encryption Fernet is used with cryptography python library.