You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "schollz (via GitHub)" <gi...@apache.org> on 2025/06/03 16:07:14 UTC

[PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

schollz opened a new pull request, #33673:
URL: https://github.com/apache/superset/pull/33673

   ### SUMMARY
   
   Change to the wording of the SDK. There is little documentation around `GUEST_TOKEN_JWT_AUDIENCE` and having it set is crucial to having the SDK work. 
   
   
   
   ### ADDITIONAL INFORMATION
   
   - [x] Has associated issue: https://github.com/apache/superset/issues/25740
   
   


-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "schollz (via GitHub)" <gi...@apache.org>.
schollz commented on PR #33673:
URL: https://github.com/apache/superset/pull/33673#issuecomment-2957146056

   Thanks @rusackas, working through it now.


-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "schollz (via GitHub)" <gi...@apache.org>.
schollz commented on code in PR #33673:
URL: https://github.com/apache/superset/pull/33673#discussion_r2132942502


##########
superset-embedded-sdk/README.md:
##########
@@ -139,6 +139,14 @@ with the secret set in configuration variable `GUEST_TOKEN_JWT_SECRET` (see conf
 }
 ```
 
+In this example, the configuration file includes the following setting:
+
+```python
+GUEST_TOKEN_JWT_AUDIENCE="superset"
+```
+
+If you're using a different audience value, be sure to update the GUEST_TOKEN_JWT_AUDIENCE variable accordingly.
+

Review Comment:
   Thanks, I added a bit of clarity there too. 



-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #33673:
URL: https://github.com/apache/superset/pull/33673#issuecomment-2950897784

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/33673?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   All modified and coverable lines are covered by tests :white_check_mark:
   > Project coverage is 51.76%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`2095f87`)](https://app.codecov.io/gh/apache/superset/commit/2095f87bc0836631bea2869d00667633dddc1997?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 1935 commits behind head on master.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #33673      +/-   ##
   ==========================================
   - Coverage   60.48%   51.76%   -8.73%     
   ==========================================
     Files        1931      558    -1373     
     Lines       76236    40278   -35958     
     Branches     8568     4221    -4347     
   ==========================================
   - Hits        46114    20850   -25264     
   + Misses      28017    18870    -9147     
   + Partials     2105      558    -1547     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/33673/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/33673/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `47.22% <ø> (-1.94%)` | :arrow_down: |
   | [javascript](https://app.codecov.io/gh/apache/superset/pull/33673/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/33673/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.97% <ø> (-2.83%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/33673/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `51.76% <ø> (-11.75%)` | :arrow_down: |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/33673/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.36%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/33673?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "schollz (via GitHub)" <gi...@apache.org>.
schollz commented on code in PR #33673:
URL: https://github.com/apache/superset/pull/33673#discussion_r2128741235


##########
superset-embedded-sdk/README.md:
##########
@@ -139,6 +139,14 @@ with the secret set in configuration variable `GUEST_TOKEN_JWT_SECRET` (see conf
 }
 ```
 
+In this example, the configuration file includes the following setting:
+
+```python
+GUEST_TOKEN_JWT_AUDIENCE="superset"
+```
+
+If you're using a different audience value, be sure to update the GUEST_TOKEN_JWT_AUDIENCE variable accordingly.
+

Review Comment:
   I can add it, but personally I think its a little confusing. `get_url_host()` returns the value of another configuration variable - `WEBDRIVER_BASEURL` which also has its own default value. For the sake of simplicity I'd lean towards a recommendation of making clear that setting `GUEST_TOKEN_JWT_AUDIENCE` is recommended for using the SDK.  



-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33673:
URL: https://github.com/apache/superset/pull/33673#discussion_r2128771284


##########
superset-embedded-sdk/README.md:
##########
@@ -139,6 +139,14 @@ with the secret set in configuration variable `GUEST_TOKEN_JWT_SECRET` (see conf
 }
 ```
 
+In this example, the configuration file includes the following setting:
+
+```python
+GUEST_TOKEN_JWT_AUDIENCE="superset"
+```
+
+If you're using a different audience value, be sure to update the GUEST_TOKEN_JWT_AUDIENCE variable accordingly.
+

Review Comment:
   @schollz totally agree with you that it's confusing to fallback to `get_url_host()` (I was genuinely surprised when I found that lol). In that case, what do you think about intentionally instructing users to set this config in their `superset_config.py` file? Above the JSON example we have:
   
   > Alternatively, a guest token can be created directly in your app with a json like the following, and then signed
   with the secret set in configuration variable `GUEST_TOKEN_JWT_SECRET` (see configuration file config.py)
   
   I'm thinking we could do something like:
   
   Alternatively, a guest token can be encoded directly in your app without interacting with the Superset API. Set up a secret via the `GUEST_TOKEN_JWT_SECRET` config and the token audience via the `GUEST_TOKEN_JWT_AUDIENCE` in your `superset_config.py` file, then sign the token using `GUEST_TOKEN_JWT_SECRET`. Example payload:
   ```
   <example JSON>
   ```
   The `aud` value must match the `GUEST_TOKEN_JWT_AUDIENCE` value.
   
   
   Just an example (might be a bit confusing) but thinking this way it's explicit that users should set values for these two configs.



-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #33673:
URL: https://github.com/apache/superset/pull/33673#discussion_r2132877031


##########
superset-embedded-sdk/README.md:
##########
@@ -139,6 +139,14 @@ with the secret set in configuration variable `GUEST_TOKEN_JWT_SECRET` (see conf
 }
 ```
 
+In this example, the configuration file includes the following setting:
+
+```python
+GUEST_TOKEN_JWT_AUDIENCE="superset"
+```
+
+If you're using a different audience value, be sure to update the GUEST_TOKEN_JWT_AUDIENCE variable accordingly.
+

Review Comment:
   Docs site PRs are warmly welcomed, if you want to add anything there :D 



-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "schollz (via GitHub)" <gi...@apache.org>.
schollz commented on code in PR #33673:
URL: https://github.com/apache/superset/pull/33673#discussion_r2128814035


##########
superset-embedded-sdk/README.md:
##########
@@ -139,6 +139,14 @@ with the secret set in configuration variable `GUEST_TOKEN_JWT_SECRET` (see conf
 }
 ```
 
+In this example, the configuration file includes the following setting:
+
+```python
+GUEST_TOKEN_JWT_AUDIENCE="superset"
+```
+
+If you're using a different audience value, be sure to update the GUEST_TOKEN_JWT_AUDIENCE variable accordingly.
+

Review Comment:
   Yeah that seems like a good idea. And along those lines I'll propose a small edit to `config.py` `GUEST_TOKEN_JWT_AUDIENCE` that calls out that it will default to `WEBDRIVER_BASEURL`. The `config.py` does serve as one of the best places of documentation so I'd love to keep that well informed too. 



-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #33673:
URL: https://github.com/apache/superset/pull/33673


-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #33673:
URL: https://github.com/apache/superset/pull/33673#issuecomment-2957131909

   Just needs some pre-commit touchups to pass. 
   
   Superset uses Git pre-commit hooks courtesy of [pre-commit](https://pre-commit.com/). To install run the following:
   ```
   pip3 install -r requirements/development.txt
   pre-commit install
   ```
   A series of checks will now run when you make a git commit.
   
   Alternatively it is possible to run pre-commit by running pre-commit manually:
   ```
   pre-commit run --all-files
   ``` 
   
   I can try to touch up the PR by hand if needed... the CI errors tell you what to fix ;)


-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33673:
URL: https://github.com/apache/superset/pull/33673#discussion_r2127885140


##########
superset-embedded-sdk/README.md:
##########
@@ -139,6 +139,14 @@ with the secret set in configuration variable `GUEST_TOKEN_JWT_SECRET` (see conf
 }
 ```
 
+In this example, the configuration file includes the following setting:
+
+```python
+GUEST_TOKEN_JWT_AUDIENCE="superset"
+```
+
+If you're using a different audience value, be sure to update the GUEST_TOKEN_JWT_AUDIENCE variable accordingly.
+

Review Comment:
   Do we want to talk about the default value? Checking `superset/config.py`, it's declared like this:
   ``` python
   GUEST_TOKEN_JWT_AUDIENCE: Callable[[], str] | str | None = None
   ```
   
   Then it seems in the `security_manager` we have:
   ``` python
   @staticmethod
   def _get_guest_token_jwt_audience() -> str:
       audience = current_app.config["GUEST_TOKEN_JWT_AUDIENCE"] or get_url_host()
       if callable(audience):
           audience = audience()
       return audience
   ```
   
   So it seems the URL host would be the default value if not specified in the config?



-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix: clarify GUEST_TOKEN_JWT_AUDIENCE usage in the SDK [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33673:
URL: https://github.com/apache/superset/pull/33673#discussion_r2128899104


##########
superset-embedded-sdk/README.md:
##########
@@ -139,6 +139,14 @@ with the secret set in configuration variable `GUEST_TOKEN_JWT_SECRET` (see conf
 }
 ```
 
+In this example, the configuration file includes the following setting:
+
+```python
+GUEST_TOKEN_JWT_AUDIENCE="superset"
+```
+
+If you're using a different audience value, be sure to update the GUEST_TOKEN_JWT_AUDIENCE variable accordingly.
+

Review Comment:
   awesome -- than you!



-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org