-
Notifications
You must be signed in to change notification settings - Fork 22
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
fix(superset-auth): Retrieve CSRF token with jwt auth for Superset #233
Conversation
tests/cli/superset/main_test.py
Outdated
@@ -161,4 +162,4 @@ def test_superset_jwt_auth(mocker: MockerFixture) -> None: | |||
catch_exceptions=False, | |||
) | |||
|
|||
JWTAuth.assert_called_with("SECRET") | |||
JWTAuth.assert_called_with("SECRET", URL("http://localhost:8088/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baseurl is better suffix without /
. Then you can prefix api/v1/security/csrf_token/
with /
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, @zephyring! The baseurl
wasn't included in the constructor before, so I just copied what was sent to the runner.invoke
here:
runner.invoke(
superset_cli,
["--jwt-token=SECRET", "http://localhost:8088/", "export"],
catch_exceptions=False,
)
The last trailing slash is included in other tests (like here and here), do you think this should be updated in other places as well?
I performed a local test using both http://localhost:8088/
and http://localhost:8088
in the CLI command and printed the endpoint -- both scenarios produced the same URL (http://localhost:8088/api/v1/security/csrf_token/
) and the command worked properly.
Let me know your thoughts. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a convention to have base_url ending without /
. Not sure the other examples syntax for string / string is a good pattern to follow. maybe @betodealmeida ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work either way here, we're using yarl for URL composition and it should do the right thing.
src/preset_cli/auth/token.py
Outdated
super().__init__() | ||
self.token = token | ||
self.baseurl = baseurl | ||
|
||
def get_csrf_token(self, jwt: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to create a new Auth
class, maybe SupersetJWTAuth
, since the path api/v1/security/csrf_token/
is very specific to Superset. Otherwise this generic class becomes tightly coupled with Superset and it becomes hard to reuse it for other APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhhhh that makes sense! Just noticed this class is also used by dbt. Going to make these changes. Thank you!
By default, Superset requires an
X-CSRFToken
header for some API requests. This was already handled when using thesuperset-cli
command with basic auth (-u $username -p $password
) but it wasn't handled when using jwt auth (--jwt-token $jwt
).This PR adds logic to include the
CSRF
token in theget_headers
function.Fixes #183.