-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
[security] allow security manager provide error message #5500
[security] allow security manager provide error message #5500
Conversation
e4375f2
to
f87585d
Compare
f87585d
to
a0c4858
Compare
Codecov Report
@@ Coverage Diff @@
## master #5500 +/- ##
==========================================
- Coverage 63.37% 63.29% -0.08%
==========================================
Files 349 349
Lines 22110 22114 +4
Branches 2455 2455
==========================================
- Hits 14013 13998 -15
- Misses 8083 8102 +19
Partials 14 14
Continue to review full report at Codecov.
|
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.
Overall LGTM. I agree with the idea of moving this logic to the security manager. I just had one question regarding one of the checks.
@@ -1260,9 +1255,11 @@ def explore(self, datasource_type=None, datasource_id=None): | |||
flash(DATASOURCE_MISSING_ERR, 'danger') | |||
return redirect(error_redirect) | |||
|
|||
if not security_manager.datasource_access(datasource): | |||
if config.get('ENABLE_ACCESS_REQUEST') and ( |
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.
Why was this check added?
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.
A check is done at a lower point (in explore_json) This check here is strictly for redirecting users who don't have access to the access request page.
(cherry picked from commit 3b6cafc)
This PR
@john-bodley @michellethomas @mistercrunch