Skip to content

Conversation

@dpgaspar
Copy link
Member

SUMMARY

Fixes SQLLab role permissions, missing access to fetching database function names. Opted for declaring explicitly the necessary permissions that make up SQLLab role.

Previously a user with SQLLab role (could be Gamma + SQLLab), would see the following toast:

Screenshot 2021-04-27 at 11 26 33

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #14372 (bf9c45d) into master (03e4a5b) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14372      +/-   ##
==========================================
- Coverage   77.12%   76.88%   -0.24%     
==========================================
  Files         954      954              
  Lines       48158    48159       +1     
  Branches     5991     5991              
==========================================
- Hits        37140    37026     -114     
- Misses      10821    10936     +115     
  Partials      197      197              
Flag Coverage Δ
hive ?
mysql 81.03% <100.00%> (+<0.01%) ⬆️
postgres 81.07% <100.00%> (+<0.01%) ⬆️
presto ?
python 81.15% <100.00%> (-0.45%) ⬇️
sqlite 80.67% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
superset/constants.py 100.00% <ø> (ø)
superset/security/manager.py 91.50% <100.00%> (+0.02%) ⬆️
superset/db_engines/hive.py 0.00% <0.00%> (-82.15%) ⬇️
superset/db_engine_specs/hive.py 70.80% <0.00%> (-16.80%) ⬇️
superset/db_engine_specs/presto.py 83.36% <0.00%> (-6.53%) ⬇️
superset/views/database/mixins.py 81.03% <0.00%> (-1.73%) ⬇️
superset/connectors/sqla/models.py 88.37% <0.00%> (-1.70%) ⬇️
superset/db_engine_specs/base.py 87.68% <0.00%> (-0.44%) ⬇️
superset/models/core.py 88.85% <0.00%> (-0.28%) ⬇️
superset/utils/core.py 88.62% <0.00%> (-0.13%) ⬇️

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 03e4a5b...bf9c45d. Read the comment docs.

and pvm.permission.name == "can_list"
)
)
return (pvm.permission.name, pvm.view_menu.name) in self.SQLLAB_PERMISSION_VIEWS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that this removes self.USER_MODEL_VIEWS can_list permission - am I misreading?

Copy link
Member Author

@dpgaspar dpgaspar Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not, I could not find any valid reason for this permission. My guess is that it's a left over from the old Query History view, that view was populating a user dropdown list.

We now use: /api/v1/query/related/user

@dpgaspar dpgaspar requested a review from willbarrett April 28, 2021 08:13
Copy link
Member

@willbarrett willbarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dpgaspar dpgaspar merged commit 6541a03 into apache:master Apr 29, 2021
@dpgaspar dpgaspar deleted the danielgaspar/ch8253/sqllab-role-is-missing-permissions branch April 29, 2021 14:58
amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request May 3, 2021
* master: (38 commits)
  refactor(native-filters): allow cascading only for filter_select (apache#14441)
  test(maximize-chart): Add tests to maximize chart action (apache#14371)
  fix: fixing mysql error message (apache#14416)
  feat: Logic added to limiting factor column in Query model (apache#13521)
  change relationship (apache#14435)
  fix: bootstrap data permissions (apache#14348)
  fix: parse simple string error message values (apache#14360)
  chore: add stack trace to all calls of logger.error (apache#14382)
  update README with new docs and recordings (apache#14432)
  Renamed impyla from implya in impala.mdx and Renamed PIP package impyla from impala in index.mdx (apache#14425)
  fix(native-filters): fix filter scope error (apache#14426)
  feat: Adding limiting_factor column to Query model (apache#14234)
  feat: Add etag caching to dashboard APIs (apache#14357)
  chore: Moves Card to the components folder (apache#14139)
  feat: Dynamic imports for the Icons component (apache#14318)
  feat: Support env vars configuration for WebSocket server (apache#14398)
  fix: SQLLab role permissions (apache#14372)
  fix(native-filters): always show filters without dataset (apache#14409)
  fix error getting partitionQuery from table.partition (apache#14369)
  refactor: Boostrap to AntD - Tabs (apache#14048)
  ...
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* fix: SQLLab role permissions

* add missing perm

* fix tests

* fix security test

* fix security test

* fix tests
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/M 🚢 1.2.0 First shipped in 1.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants