feat: Authorizing guest access to embedded dashboards #17757

suddjian · 2021-12-15T05:02:51Z

SUMMARY

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

This is the second of the embedded dashboard (#17187) access control PRs. The first dealt with authentication, creating and reading the guest token and setting up a guest user. This one adds new rules to allow guest users access to authorized dashboards.

We haven't added an embedded dashboard model, yet. So for now, if the EMBEDDED_SUPERSET flag is on, all dashboards are considered "embeddable" if the guest token permits access. That means that the id used is the id of the dashboard. In the future, the id in the token will reference a row of the embedded_dashboards table instead, and some of these checks will need to be modified.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Don't try to QA this PR, test #17530 instead.

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/common/request_contexed_based.py

codecov · 2021-12-15T05:29:34Z

Codecov Report

Merging #17757 (5c3a5fb) into embedded (4c74e53) will increase coverage by 0.62%.
The diff coverage is 97.01%.

@@             Coverage Diff              @@
##           embedded   #17757      +/-   ##
============================================
+ Coverage     66.39%   67.02%   +0.62%     
============================================
  Files          1571     1572       +1     
  Lines         61801    63382    +1581     
  Branches       6243     6243              
============================================
+ Hits          41031    42479    +1448     
- Misses        19172    19305     +133     
  Partials       1598     1598

Flag	Coverage Δ
hive	`53.86% <35.82%> (+0.60%)`	⬆️
javascript	`50.93% <ø> (ø)`
mysql	`82.59% <97.01%> (+0.46%)`	⬆️
postgres	`82.64% <97.01%> (+0.45%)`	⬆️
presto	`53.82% <35.82%> (+0.72%)`	⬆️
python	`83.07% <97.01%> (+0.44%)`	⬆️
sqlite	`82.34% <97.01%> (+0.47%)`	⬆️

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

Impacted Files	Coverage Δ
superset/views/core.py	`77.74% <0.00%> (ø)`
superset/security/manager.py	`92.57% <97.77%> (+0.75%)`	⬆️
superset/common/request_contexed_based.py	`100.00% <100.00%> (+20.00%)`	⬆️
superset/config.py	`91.58% <100.00%> (-0.24%)`	⬇️
superset/dashboards/filters.py	`95.45% <100.00%> (+0.53%)`	⬆️
superset/security/api.py	`95.45% <100.00%> (ø)`
superset/security/guest_token.py	`97.22% <100.00%> (+3.47%)`	⬆️
superset/views/base.py	`74.49% <100.00%> (-0.43%)`	⬇️
superset/commands/importers/v1/examples.py	`36.20% <0.00%> (-2.43%)`	⬇️
... and 16 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 4c74e53...5c3a5fb. Read the comment docs.

superset/security/manager.py

suddjian · 2021-12-15T05:52:07Z

superset/security/guest_token.py

@@ -50,7 +58,7 @@ class GuestUser(AnonymousUserMixin):
    def is_authenticated(self) -> bool:
        """
        This is set to true because guest users should be considered authenticated,
-        at least in most places. The treatment of this flag is pretty inconsistent.
+        at least in most places. The treatment of this flag is kind of inconsistent.


lol apparently this got downgraded to "kind of" somewhere along the way

superset/dashboards/filters.py

superset/security/manager.py

villebro

Code and tests look very good, but to be able to get a full understanding of the functionality here I would need to get fully into this feature by reviewing and testing the previous PRs first. Let me know if that's needed.

superset/security/api.py

superset/security/manager.py

craig-rueda · 2022-01-21T19:08:39Z

superset/security/manager.py

@@ -1289,33 +1308,60 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:

        try:
            token = self.parse_jwt_guest_token(raw_token)
+            if token.get("user") is None:


Can you add tests to cover these cases?

craig-rueda · 2022-01-21T19:12:51Z

Overall code looks good - I think you just need a few more tests to make sure a few more areas are covered (security logic test coverage should be kept high generally). One other thing that might become an issue down the road is token size. From what I understand, there's a resources key that is embedded in tokens. If this list gets too big, the JWT itself will be too big to pass in as a header.

* 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 <1858430+suddjian@users.noreply.github.com> * comment out origins checks * move embedded for core to dashboard * pylint * isort Co-authored-by: David Aaron Suddjian <aasuddjian@gmail.com> Co-authored-by: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com> * 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 <lily@preset.io> * 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 <lily@preset.io> * SupersetClient guest token test * Apply suggestions from code review Co-authored-by: Lily Kuang <lily@preset.io> Co-authored-by: Lily Kuang <lily@preset.io>

* 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 <1858430+suddjian@users.noreply.github.com> * comment out origins checks * move embedded for core to dashboard * pylint * isort Co-authored-by: David Aaron Suddjian <aasuddjian@gmail.com> Co-authored-by: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com> * 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 <lily@preset.io> * 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 <lily@preset.io> * SupersetClient guest token test * Apply suggestions from code review Co-authored-by: Lily Kuang <lily@preset.io> Co-authored-by: Lily Kuang <lily@preset.io>

suddjian added 4 commits December 14, 2021 20:37

helper methods and dashboard access

a21a417

guest token dashboard authz

ad3572e

adjust csrf exempt list

4fd8715

eums don't work that way

7353826

superset-github-bot bot added the preset-io label Dec 15, 2021

pull-request-size bot added the size/L label Dec 15, 2021

suddjian commented Dec 15, 2021

View reviewed changes

superset/common/request_contexed_based.py Show resolved Hide resolved

suddjian commented Dec 15, 2021

View reviewed changes

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

suddjian commented Dec 15, 2021

View reviewed changes