feat: Guest token (for embedded dashboard auth) #17517

suddjian · 2021-11-23T01:00:23Z

Note: This is a PR into a feature branch, not into master. #17530

SUMMARY

Adds an endpoint to create a guest token, a permission can_grant_guest_token, and methods in security manager to use the guest token as authn. Authorization is left out, for now.

We have decided to call this a "guest token" rather than an "embed token" because it could conceivably have future uses beyond viewing embedded dashboards.

Note that the token is not yet used for actually checking access to anything. It is only parsed to determine a "logged in" state.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

tested.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

superset/security/api.py

superset/security/manager.py

…d-token

codecov · 2021-12-02T21:13:28Z

Codecov Report

Merging #17517 (51cc290) into embedded (ee71eb8) will increase coverage by 0.00%.
The diff coverage is 90.17%.

@@            Coverage Diff             @@
##           embedded   #17517    +/-   ##
==========================================
  Coverage     68.10%   68.11%            
==========================================
  Files          1653     1655     +2     
  Lines         66292    66497   +205     
  Branches       7107     7124    +17     
==========================================
+ Hits          45150    45292   +142     
- Misses        19253    19308    +55     
- Partials       1889     1897     +8

Flag	Coverage Δ
hive	`81.81% <92.52%> (+0.03%)`	⬆️
javascript	`56.26% <40.00%> (-0.03%)`	⬇️
mysql	`82.18% <92.52%> (+0.03%)`	⬆️
postgres	`82.24% <92.52%> (+0.03%)`	⬆️
python	`82.58% <92.52%> (+0.03%)`	⬆️
sqlite	`81.92% <92.52%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...-frontend/src/components/UiConfigContext/index.tsx	`70.00% <40.00%> (ø)`
superset/security/manager.py	`91.82% <90.90%> (-0.15%)`	⬇️
superset/security/api.py	`95.45% <92.30%> (-4.55%)`	⬇️
superset/security/guest_token.py	`93.75% <93.75%> (ø)`
superset/config.py	`91.74% <100.00%> (+0.13%)`	⬆️
...nd/src/explore/components/DataTablesPane/index.tsx	`76.92% <0.00%> (-0.75%)`	⬇️
...et-frontend/src/components/EditableTitle/index.tsx	`73.07% <0.00%> (-0.61%)`	⬇️
...nd/plugins/plugin-chart-table/src/controlPanel.tsx	`21.34% <0.00%> (-0.50%)`	⬇️
...et-ui-chart-controls/src/shared-controls/index.tsx	`38.53% <0.00%> (-0.36%)`	⬇️
...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx	`67.15% <0.00%> (-0.26%)`	⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee71eb8...51cc290. Read the comment docs.

superset/security/manager.py

craig-rueda · 2021-12-08T19:01:58Z

superset/security/manager.py

+            logger.warning(ex)
+            return None
+        else:
+            user = token["user"]


Can you add a method to delegate this to? I'm thinking that folks may want to override the claims, etc. and that will make it easier.

I'm not sure how exactly you're saying folks might want to override things. They could add extra properties or verifications already by overriding parse_jwt_guest_token, calling super(), and then running their own code.

We could also delegate the creation of claims to allow putting extra stuff into the token.

If you override, you'll need to clobber the contents of this method. It's likely that extra claims will need to be added, etc. I'm sure this works as-is - just future-proofing

I think I get what you mean. I'm going to modify the GuestUser class to take the token instead of specific fields, and then extra claims can be added by subclassing GuestUser. That should cover those concerns.

superset/security/manager.py

craig-rueda

Looks good - left a few structural suggestions

This reverts commit df2f49a.

This reverts commit 883dff3.

* feat(dashboard): embedded dashboard UI configuration (#17175) (#17450) * setup embedded provider * update ui configuration * fix test * feat: Guest token (for embedded dashboard auth) (#17517) * generate an embed token * improve existing tests * add some auth setup, and rename token * fix the stuff for compatibility with external request loaders * docs, standard jwt claims, tweaks * black * lint * tests, and safer token decoding * linting * type annotation * prettier * add feature flag * quiet pylint * apparently typing is a problem again * Make guest role name configurable * fake being a non-anonymous user * just one log entry * customizable algo * lint * lint again * 403 works now! * get guest token from header instead of cookie * Revert "403 works now!" This reverts commit df2f49a. * fix tests * Revert "Revert "403 works now!"" This reverts commit 883dff3. * rename method * correct import * feat: entry for embedded dashboard (#17529) * create entry for embedded dashboard in webpack * add cookies * lint * token message handshake * guestTokenHeaderName * use setupClient instead of calling configure * rename the webpack chunk * simplified handshake * embedded entrypoint: render a proper app * make the embedded page accept anonymous connections * format * lint * fix test # Conflicts: # superset-frontend/src/embedded/index.tsx # superset/views/core.py * lint * Update superset-frontend/src/embedded/index.tsx Co-authored-by: David Aaron Suddjian <[email protected]> * comment out origins checks * move embedded for core to dashboard * pylint * isort Co-authored-by: David Aaron Suddjian <[email protected]> Co-authored-by: David Aaron Suddjian <[email protected]> * feat: Authorizing guest access to embedded dashboards (#17757) * helper methods and dashboard access * guest token dashboard authz * adjust csrf exempt list * eums don't work that way * Remove unnecessary import * move row level security tests to their own file * a bit of refactoring * add guest token security tests * refactor tests * clean imports * variable names can be too long apparently * missing argument to get_user_roles * don't redefine builtins * remove unused imports * fix test import * default to global user when getting roles * missing import * mock it * test get_user_roles * infer g.user for ease of tests * remove redundant check * tests for guest user security manager fns * use algo to get rid of warning messages * tweaking access checks * fix guest token security tests * missing imports * more tests * more testing and also some small refactoring * move validation out of parsing * fix dashboard access check again * add more test Co-authored-by: Lily Kuang <[email protected]> * feat: Row Level Security rules for guest tokens (#17836) * helper methods and dashboard access * guest token dashboard authz * adjust csrf exempt list * eums don't work that way * Remove unnecessary import * move row level security tests to their own file * a bit of refactoring * add guest token security tests * refactor tests * clean imports * variable names can be too long apparently * missing argument to get_user_roles * don't redefine builtins * remove unused imports * fix test import * default to global user when getting roles * missing import * mock it * test get_user_roles * infer g.user for ease of tests * remove redundant check * tests for guest user security manager fns * use algo to get rid of warning messages * tweaking access checks * fix guest token security tests * missing imports * more tests * more testing and also some small refactoring * move validation out of parsing * fix dashboard access check again * rls rules for guest tokens * test guest token rls rules * more flexible rls rules * lint * fix tests * fix test * defaults * fix some tests * fix some tests * lint Co-authored-by: Lily Kuang <[email protected]> * SupersetClient guest token test * Apply suggestions from code review Co-authored-by: Lily Kuang <[email protected]> Co-authored-by: Lily Kuang <[email protected]>

* feat(dashboard): embedded dashboard UI configuration (apache#17175) (apache#17450) * setup embedded provider * update ui configuration * fix test * feat: Guest token (for embedded dashboard auth) (apache#17517) * generate an embed token * improve existing tests * add some auth setup, and rename token * fix the stuff for compatibility with external request loaders * docs, standard jwt claims, tweaks * black * lint * tests, and safer token decoding * linting * type annotation * prettier * add feature flag * quiet pylint * apparently typing is a problem again * Make guest role name configurable * fake being a non-anonymous user * just one log entry * customizable algo * lint * lint again * 403 works now! * get guest token from header instead of cookie * Revert "403 works now!" This reverts commit df2f49a. * fix tests * Revert "Revert "403 works now!"" This reverts commit 883dff3. * rename method * correct import * feat: entry for embedded dashboard (apache#17529) * create entry for embedded dashboard in webpack * add cookies * lint * token message handshake * guestTokenHeaderName * use setupClient instead of calling configure * rename the webpack chunk * simplified handshake * embedded entrypoint: render a proper app * make the embedded page accept anonymous connections * format * lint * fix test # Conflicts: # superset-frontend/src/embedded/index.tsx # superset/views/core.py * lint * Update superset-frontend/src/embedded/index.tsx Co-authored-by: David Aaron Suddjian <[email protected]> * comment out origins checks * move embedded for core to dashboard * pylint * isort Co-authored-by: David Aaron Suddjian <[email protected]> Co-authored-by: David Aaron Suddjian <[email protected]> * feat: Authorizing guest access to embedded dashboards (apache#17757) * helper methods and dashboard access * guest token dashboard authz * adjust csrf exempt list * eums don't work that way * Remove unnecessary import * move row level security tests to their own file * a bit of refactoring * add guest token security tests * refactor tests * clean imports * variable names can be too long apparently * missing argument to get_user_roles * don't redefine builtins * remove unused imports * fix test import * default to global user when getting roles * missing import * mock it * test get_user_roles * infer g.user for ease of tests * remove redundant check * tests for guest user security manager fns * use algo to get rid of warning messages * tweaking access checks * fix guest token security tests * missing imports * more tests * more testing and also some small refactoring * move validation out of parsing * fix dashboard access check again * add more test Co-authored-by: Lily Kuang <[email protected]> * feat: Row Level Security rules for guest tokens (apache#17836) * helper methods and dashboard access * guest token dashboard authz * adjust csrf exempt list * eums don't work that way * Remove unnecessary import * move row level security tests to their own file * a bit of refactoring * add guest token security tests * refactor tests * clean imports * variable names can be too long apparently * missing argument to get_user_roles * don't redefine builtins * remove unused imports * fix test import * default to global user when getting roles * missing import * mock it * test get_user_roles * infer g.user for ease of tests * remove redundant check * tests for guest user security manager fns * use algo to get rid of warning messages * tweaking access checks * fix guest token security tests * missing imports * more tests * more testing and also some small refactoring * move validation out of parsing * fix dashboard access check again * rls rules for guest tokens * test guest token rls rules * more flexible rls rules * lint * fix tests * fix test * defaults * fix some tests * fix some tests * lint Co-authored-by: Lily Kuang <[email protected]> * SupersetClient guest token test * Apply suggestions from code review Co-authored-by: Lily Kuang <[email protected]> Co-authored-by: Lily Kuang <[email protected]>

suddjian added 2 commits November 22, 2021 16:55

generate an embed token

26a84b1

improve existing tests

d284ab3

suddjian requested a review from lilykuang November 23, 2021 01:00

superset-github-bot bot added the preset-io label Nov 23, 2021

pull-request-size bot added the size/L label Nov 23, 2021

suddjian marked this pull request as draft November 23, 2021 01:00

suddjian commented Nov 23, 2021

View reviewed changes

superset/security/api.py Outdated Show resolved Hide resolved

suddjian added 2 commits November 24, 2021 14:47

add some auth setup, and rename token

7add90a

fix the stuff for compatibility with external request loaders

be4efd5

suddjian mentioned this pull request Nov 25, 2021

feat: embedded dashboard core #17530

Merged

9 tasks

lilykuang mentioned this pull request Nov 29, 2021

feat: entry for embedded dashboard #17529

Merged

9 tasks

lilykuang reviewed Nov 29, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

lilykuang reviewed Nov 29, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

lilykuang reviewed Nov 29, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

suddjian added 2 commits December 2, 2021 11:15

docs, standard jwt claims, tweaks

3e67e94

Merge branch 'embed-token' of github.com:preset-io/superset into embe…

b1f0e0f

…d-token

suddjian marked this pull request as ready for review December 2, 2021 20:41

suddjian requested a review from lilykuang December 2, 2021 20:44

suddjian changed the title ~~feat: Embed token (wip)~~ feat: Guest token Dec 2, 2021

suddjian changed the title ~~feat: Guest token~~ feat: Guest token (for embedded dashboard auth) Dec 2, 2021

suddjian added 2 commits December 2, 2021 12:48

black

4b34e68

lint

97f623a

lilykuang approved these changes Dec 2, 2021

View reviewed changes

suddjian added 5 commits December 3, 2021 14:29

tests, and safer token decoding

44af49c

linting

9ca729d

type annotation

0d704e6

Merge branch 'embedded' into embed-token

5694212

prettier

19c6538

suddjian requested a review from craig-rueda December 7, 2021 00:11

suddjian added 4 commits December 6, 2021 16:39

add feature flag

1ee824d

Merge branch 'embedded' into embed-token

2e1c22f

quiet pylint

cdb7909

apparently typing is a problem again

f67c8a8

craig-rueda reviewed Dec 8, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

craig-rueda reviewed Dec 8, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

craig-rueda reviewed Dec 8, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

craig-rueda approved these changes Dec 8, 2021

View reviewed changes

suddjian added 14 commits December 11, 2021 00:04

Make guest role name configurable

29358f7

fake being a non-anonymous user

2f3df2c

just one log entry

1920623

customizable algo

814d314

lint

271913e

lint again

45d016e

Merge branch 'embedded' into embed-token

d76bbf2

403 works now!

df2f49a

get guest token from header instead of cookie

bcba0fa

Revert "403 works now!"

883dff3

This reverts commit df2f49a.

fix tests

10ccc67

Revert "Revert "403 works now!""

4f7aec2

This reverts commit 883dff3.

Merge branch 'embedded' into embed-token

be0209b

rename method

51cc290

suddjian merged commit d705236 into apache:embedded Dec 14, 2021

suddjian deleted the embed-token branch December 15, 2021 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Guest token (for embedded dashboard auth) #17517

feat: Guest token (for embedded dashboard auth) #17517

suddjian commented Nov 23, 2021 •

edited

Loading

codecov bot commented Dec 2, 2021 •

edited

Loading

craig-rueda Dec 8, 2021

suddjian Dec 8, 2021 •

edited

Loading

craig-rueda Dec 9, 2021

suddjian Dec 11, 2021 •

edited

Loading

craig-rueda left a comment

feat: Guest token (for embedded dashboard auth) #17517

feat: Guest token (for embedded dashboard auth) #17517

Conversation

suddjian commented Nov 23, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Dec 2, 2021 • edited Loading

Codecov Report

craig-rueda Dec 8, 2021

Choose a reason for hiding this comment

suddjian Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

craig-rueda Dec 9, 2021

Choose a reason for hiding this comment

suddjian Dec 11, 2021 • edited Loading

Choose a reason for hiding this comment

craig-rueda left a comment

Choose a reason for hiding this comment

suddjian commented Nov 23, 2021 •

edited

Loading

codecov bot commented Dec 2, 2021 •

edited

Loading

suddjian Dec 8, 2021 •

edited

Loading

suddjian Dec 11, 2021 •

edited

Loading