-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
fix: revert back to use security manager authz for dashboard when get by uuid #23330
Conversation
56cf087
to
9d017f0
Compare
9d017f0
to
044603e
Compare
Codecov Report
@@ Coverage Diff @@
## master #23330 +/- ##
==========================================
+ Coverage 65.81% 67.58% +1.76%
==========================================
Files 1910 1907 -3
Lines 73471 73507 +36
Branches 7975 7977 +2
==========================================
+ Hits 48357 49677 +1320
+ Misses 23065 21782 -1283
+ Partials 2049 2048 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 62 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
superset/dashboards/dao.py
Outdated
dashboard = query.one_or_none() | ||
except ValueError: | ||
# if it's slug or uuid, which is more specific, just get it | ||
dashboard = Dashboard.get(id_or_slug) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will not apply any security settings in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the check from security manager will still apply at below, as the base defense line, even though we have the cls.base_filter applied on int id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, can you add an integration test for the API where the request by uuid?
@@ -41,21 +42,28 @@ class DashboardDAO(BaseDAO): | |||
|
|||
@classmethod | |||
def get_by_id_or_slug(cls, id_or_slug: Union[int, str]) -> Dashboard: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we change this method name to get_by_id_slug_or_uuid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a better naming could just be get_by_identifier
but it could touches too many files if we want to change every id_or_slug
to identifier
. Note that uuid is also id in general though. So I think this naming should be fine.
except ValueError: | ||
return False | ||
|
||
|
||
def id_or_slug_filter(id_or_slug: Union[int, str]) -> BinaryExpression: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we change this method name also?
SUMMARY
The patch breaks some folks functionality around report schedule for draft dashboard, permalink. This PR should:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION