Skip to content
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

feat(annotations): security permissions simplification #12014

Merged
merged 9 commits into from
Dec 16, 2020

Conversation

kkucharc
Copy link
Contributor

SUMMARY

Updated annotations and annotation layers security permissions.

Current state:

View Permission
AnnotationLayerModelView can_delete
AnnotationLayerModelView can_list
AnnotationLayerModelView can_show
AnnotationLayerModelView can_add
AnnotationLayerModelView can_edit
AnnotationModelView can_annotation
AnnotationModelView can_show
AnnotationModelView can_add
AnnotationModelView can_delete
AnnotationModelView can_edit
AnnotationModelView can_list

New state:

View Permission
Annotation can_write
Annotation can_read

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@kkucharc kkucharc force-pushed the annotations-permissions branch 2 times, most recently from 249c3cc to 0811867 Compare December 14, 2020 11:01
@dpgaspar dpgaspar added the risk:db-migration PRs that require a DB migration label Dec 14, 2020
@codecov-io
Copy link

codecov-io commented Dec 14, 2020

Codecov Report

Merging #12014 (19b9306) into master (7dac150) will increase coverage by 3.78%.
The diff coverage is 31.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12014      +/-   ##
==========================================
+ Coverage   63.81%   67.60%   +3.78%     
==========================================
  Files         956      957       +1     
  Lines       46845    46895      +50     
  Branches     4590     4590              
==========================================
+ Hits        29895    31704    +1809     
+ Misses      16766    15080    -1686     
+ Partials      184      111      -73     
Flag Coverage Δ
cypress 53.11% <0.00%> (?)
javascript 62.68% <100.00%> (+<0.01%) ⬆️
python 64.42% <26.82%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
...ions/c25cb2c78727_security_converge_annotations.py 0.00% <0.00%> (ø)
...ews/CRUD/annotationlayers/AnnotationLayersList.tsx 77.88% <100.00%> (+0.96%) ⬆️
superset/annotation_layers/annotations/api.py 84.96% <100.00%> (+0.11%) ⬆️
superset/annotation_layers/api.py 83.19% <100.00%> (+0.14%) ⬆️
superset/views/annotations.py 77.27% <100.00%> (+1.46%) ⬆️
superset/db_engine_specs/sqlite.py 65.62% <0.00%> (-9.38%) ⬇️
superset/utils/celery.py 96.42% <0.00%> (-3.58%) ⬇️
...et-frontend/src/SqlLab/components/LimitControl.tsx 89.36% <0.00%> (-1.95%) ⬇️
superset/result_set.py 96.69% <0.00%> (-1.66%) ⬇️
...-frontend/src/datasource/ChangeDatasourceModal.tsx 80.26% <0.00%> (-1.08%) ⬇️
... and 181 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 7dac150...19b9306. Read the comment docs.

@dpgaspar
Copy link
Member

@kkucharc you need to change the permission name on the tests also: spec/javascripts/views/CRUD/annotationlayers/AnnotationLayersList_spec.jsx

@dpgaspar dpgaspar merged commit 9c8b65d into apache:master Dec 16, 2020
@hajdbo
Copy link
Contributor

hajdbo commented Mar 20, 2021

The db upgrade "c25cb2c78727, security converge annotations" failed on my instance.
Somehow, there were role definition duplicates and _find_pvm() would go into exception because of these duplicates:

  File "superset/migrations/shared/security_converge.py", line 227, in migrate_roles
    old_pvm = _find_pvm(session, old_pvm_key.view, old_pvm_key.permission)
  File "superset/migrations/shared/security_converge.py", line 158, in _find_pvm
    return (
  File "sqlalchemy/orm/query.py", line 3413, in one_or_none
    raise orm_exc.MultipleResultsFound(
sqlalchemy.orm.exc.MultipleResultsFound: Multiple rows were found for one_or_none()

To fix it, I had to manually remove these three lines in the table:

sqlite>  SELECT ab_permission_view.id AS ab_permission_view_id, ab_permission_view.permission_id AS ab_permission_view_permission_id, ab_permission_view.view_menu_id AS ab_permission_view_view_menu_id FROM ab_permission_view JOIN ab_permission ON ab_permission.id = ab_permission_view.permission_id JOIN ab_view_menu ON ab_view_menu.id = ab_permission_view.view_menu_id WHERE ab_view_menu.name = 'AnnotationModelView' and ab_permission.name = 'can_show';
284|9|87
286|9|87
sqlite> delete from ab_permission_view where id=286;
sqlite>  SELECT ab_permission_view.id AS ab_permission_view_id, ab_permission_view.permission_id AS ab_permission_view_permission_id, ab_permission_view.view_menu_id AS ab_permission_view_view_menu_id FROM ab_permission_view JOIN ab_permission ON ab_permission.id = ab_permission_view.permission_id JOIN ab_view_menu ON ab_view_menu.id = ab_permission_view.view_menu_id WHERE ab_view_menu.name = 'AnnotationModelView' and ab_permission.name = 'can_add';
282|8|87
283|8|87
sqlite> delete from ab_permission_view where id=283;
sqlite>  SELECT ab_permission_view.id AS ab_permission_view_id, ab_permission_view.permission_id AS ab_permission_view_permission_id, ab_permission_view.view_menu_id AS ab_permission_view_view_menu_id FROM ab_permission_view JOIN ab_permission ON ab_permission.id = ab_permission_view.permission_id JOIN ab_view_menu ON ab_view_menu.id = ab_permission_view.view_menu_id WHERE ab_view_menu.name = 'AnnotationModelView' and ab_permission.name = 'can_list';
280|7|87
281|7|87
sqlite> delete from ab_permission_view where id=281;

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.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 risk:db-migration PRs that require a DB migration size/L 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants