feat: Backlog/monorepo refactor 2025/authenticate library #607
Conversation
dennisweil
left a comment
dennisweil
left a comment
There was a problem hiding this comment.
I don't quite understand why an internal webserver is needed for the authentication.
| # :coding: utf-8 | ||
| # :copyright: Copyright (c) 2024 ftrack | ||
|
|
||
| import logging |
There was a problem hiding this comment.
Small, unimportant detail reg. code styling:
Imports should be alphabetically ordered.
| :param redirect_port: The port on which the local web server will run. | ||
| """ | ||
| try: | ||
| self._server_url: str = url_checker( |
There was a problem hiding this comment.
This check should be moved into a setter method.
We might only want to check for the validity of the URL when actually trying to authenticate. We can safely tolerate a non-valid url unless we try to contact the server.
There was a problem hiding this comment.
Lets talk about this one. I wanted to avoid a setter method here as I don't want to give access to anyone to change the server url.
There was a problem hiding this comment.
Agreed on leaving it protected until we have another necessity.
| logging.error(f"Invalid server URL: {e}") | ||
| raise | ||
| self._credential_provider_instance: "CredentialProvider" = ( | ||
| credential_provider_factory.create_credential_provider() |
There was a problem hiding this comment.
Naming this methong simply create would be sufficient. The name/type of the factory class already makes it quite obvious what it will create.
There was a problem hiding this comment.
I would prefer to make it specific. If we start using create build or this kind of generic names, it ends up beign very difficult to navigate through the code.
There was a problem hiding this comment.
Agreed to change it to a more generic name "make".
| def redirect_uri(self): | ||
| return self._redirect_uri | ||
|
|
||
| def browser_authenticate(self) -> None: |
There was a problem hiding this comment.
We might have different methods for authentication. Naming should follow a pattern of .... I'd recommend to rename the method to authenticate_browser and in extension also rename the other authentication methods according to this schema.
There was a problem hiding this comment.
I don't see the difference?
I mean I currently have browser_authenticate and credential_authenticate as methods. why flipping the order will make it best practice?
There was a problem hiding this comment.
Agreed to change this to authenticate_browser and authenticate_credential. Because this way you can .authenticate_... and it will show you the available possibilities.
| server_thread.join() | ||
| except Exception as e: | ||
| logging.error( | ||
| f"An error occurred during browser authentication: {e}" |
There was a problem hiding this comment.
Instead of adding the actual exception into the log string, it's better to use the keyword argument exc_info=True, which will automatically add the error and the stack trace to the logging message.
General Discussion topic:
Where should exception handling happen? I have a small preference for deferring the handling of exceptions into the usage code, rather than the library code. We should raise here, but handling the exception should be done outside of this scope.
There was a problem hiding this comment.
Lets talk about this.
There was a problem hiding this comment.
Agreed on following exc_info "pattern". And agreed on always raising on libraries to propagate the error.
| @@ -0,0 +1,120 @@ | |||
| # :coding: utf-8 | |||
There was a problem hiding this comment.
Concistency:
Use the TYPE_CHECKING mechanism for type hints as in the other files.
There was a problem hiding this comment.
If you talk about the credentialProvider import. I can't use type_checking because I need it on the :
isinstance(value, CredentialProvider...
There was a problem hiding this comment.
We will maintain consistency in the Typing checking using strings.
| logging.basicConfig(level=logging.INFO) | ||
|
|
||
|
|
||
| class SessionProvider: |
There was a problem hiding this comment.
Consistency:
Using consistent patterns. This should also have a factory.
There was a problem hiding this comment.
Why? this is equivalent to the authenticate module, not to the credential or webserver helpers.
There was a problem hiding this comment.
Agreed to use it as it is for now until we start using it and we see what needs it has, then we will rename it or adapt it.
| self._session = self.load_session() | ||
| return self._session | ||
|
|
||
| def load_session( |
There was a problem hiding this comment.
load_session implies that we're dealing with a stored/cached session while in reality, we're creating a new session.
Naming it new_session or create_session makes the intent clearer.
If the load refers to the cretendials, we should name it as new_session_from_stored_credentials.
| logging.basicConfig(level=logging.INFO) | ||
|
|
||
|
|
||
| def wait_for_event_hub_connection( |
There was a problem hiding this comment.
I'd propose to use a more generic retry library. This is a good one:
https://github.com/jd/tenacity
There was a problem hiding this comment.
I think might not be worth having a new dependency to a library here... But lets discuss it.
There was a problem hiding this comment.
Agreed on improving names to connect to the event hub thread
|
|
||
|
|
||
| # Generic URL Checker Dispatcher | ||
| def url_checker(url: str, checkers: list) -> str: |
There was a problem hiding this comment.
If we have a function like this, we should use it and implement our ftrack checkers to be provided to this function.
However, the functionality seems generic enough, that python's builtin functionality should be more than enough:
url_valid = all([check_function(url) for check_function in checker_functions])
There was a problem hiding this comment.
Interesting, lets talk about this.
There was a problem hiding this comment.
Lets remove the url_checker and use the ftrack_url_checker everywhere.
It is needed to redirect the webbrowser authentication and read the credentials from there. |
dennisweil
left a comment
dennisweil
left a comment
There was a problem hiding this comment.
Approved except for a small discussion point regarding singular/plura.
lluisCM
deleted the
backlog/monorepo-refactor-2025/authenticate-library
branch
Resolves :
This PR has been tested on :
Overview
Purpose:
Authenticate and session library and Authenticator and session tools basic functionality is in.
Scope:
Implementation Details
Approach:
Reasoning:
Changes:
Trade-offs:
Testing
Tests Added:
Manual Testing:
Testing Environment:
Notes for Reviewers
Focus Areas:
Dependencies:
Known Issues: