villebro commented on code in PR #27880: URL: https://github.com/apache/superset/pull/27880#discussion_r1550163063
########## superset/db_engine_specs/README.md: ########## @@ -547,65 +547,53 @@ Alternatively, it's also possible to impersonate users by implementing the `upda Support for authenticating to a database using personal OAuth2 access tokens was introduced in [SIP-85](https://github.com/apache/superset/issues/20300). The Google Sheets DB engine spec is the reference implementation. -To add support for OAuth2 to a DB engine spec, the following attribute and methods are needed: +To add support for OAuth2 to a DB engine spec, the following attributes are needed: ```python class BaseEngineSpec: + supports_oauth2 = True oauth2_exception = OAuth2RedirectError - @staticmethod - def is_oauth2_enabled() -> bool: - return False - - @staticmethod - def get_oauth2_authorization_uri(state: OAuth2State) -> str: - raise NotImplementedError() - - @staticmethod - def get_oauth2_token(code: str, state: OAuth2State) -> OAuth2TokenResponse: - raise NotImplementedError() - - @staticmethod - def get_oauth2_fresh_token(refresh_token: str) -> OAuth2TokenResponse: - raise NotImplementedError() + oauth2_scope = " ".join([ + "https://example.org/scope1";, + "https://example.org/scope2";, + ]) + oauth2_authorization_request_uri = "https://example.org/authorize"; + oauth2_token_request_uri = "https://example.org/token"; ``` The `oauth2_exception` is an exception that is raised by `cursor.execute` when OAuth2 is needed. This will start the OAuth2 dance when `BaseEngineSpec.execute` is called, by returning the custom error `OAUTH2_REDIRECT` to the frontend. If the database driver doesn't have a specific exception, it might be necessary to overload the `execute` method in the DB engine spec, so that the `BaseEngineSpec.start_oauth2_dance` method gets called whenever OAuth2 is needed. -The first method, `is_oauth2_enabled`, is used to inform if the database supports OAuth2. This can be dynamic; for example, the Google Sheets DB engine spec checks if the Superset configuration has the necessary section: - -```python -from flask import current_app - +The DB engine should implement logic in either `get_url_for_impersonation` or `update_impersonation_config` to update the connection with the personal access token. See the Google Sheets DB engine spec for a reference implementation. Review Comment: This may be splitting hairs, but I perceive user impersonation to be a very specific execution form, which means "query is executed using a service account, but is executed as if a particular user us executing it". In the case of OAuth2, the query is in fact executed by the user, not the service account, and makes it more secure. I also agree that consolidating these methods could make sense, as having separate methods for mutating the URL and the other for params to `create_engine` often causes confusion for me, and are IMO really the same thing. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
