-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
chore(security): Updating assert logic #10034
chore(security): Updating assert logic #10034
Conversation
9f9c526
to
4eb4e21
Compare
Codecov Report
@@ Coverage Diff @@
## master #10034 +/- ##
==========================================
+ Coverage 64.08% 68.93% +4.84%
==========================================
Files 584 584
Lines 31054 31077 +23
Branches 3180 3180
==========================================
+ Hits 19901 21422 +1521
+ Misses 10975 9546 -1429
+ Partials 178 109 -69
Continue to review full report at Codecov.
|
superset/security/manager.py
Outdated
if not self.datasource_access(datasource): | ||
raise SupersetSecurityException( | ||
self.get_datasource_access_error_object(datasource), | ||
from superset import db |
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.
I wonder if there should be any assertion logic to ensure that the right combination of parameters are defined. Note except for database
and table
these are all mutually exclusive. I did consider having an argument database_and_table
which would be an Optional[Tuple["Database, "Table']]
but I wasn't sold on the idea and thus opted for the additional docstring context.
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.
An optional approach here would be to define DatasourceMixin
which provides a datasource, and add it to BaseViz
, QueryContext
and Datasource
. in this case we could simplify the signature of this method by combining viz
, datasource
and query_context
into one single datasource: DatasourceMixin
. The same could be done to query
and database
(DatabaseMixin
). This could help clarify what the diffefent arguments mean.
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.
It's tempting to think of something like use a **kwargs
then use a dict to route the named parameters to smaller methods that would implement the security assertion logic. A bit crazy, but has it's upsides, cleaner signature, forced breaking logic into smaller chunks, easy to extend.
Not 100% sure if it's doable,
@@ -2656,8 +2668,7 @@ def fetch_datasource_metadata(self) -> FlaskResponse: | |||
if not datasource: | |||
return json_error_response(DATASOURCE_MISSING_ERR) | |||
|
|||
# Check permission for datasource |
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.
Annexed self explanatory comment.
94a0fc7
to
7c05667
Compare
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.
one other question: does this handle queries run within SQL Lab too? I'm assume that's what the "datasource" part does, but wanted to confirm
) | ||
try: | ||
self.raise_for_access(datasource=datasource) | ||
except SupersetSecurityException: |
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.
If there's a different exception thrown, then this fails open. Is that secure?
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.
@etr2460 in theory (by construction) the raise_for_access
method should only throw a SupersetSecurityException
exception. Generally catching scoped (as opposed to general) exceptions is preferred.
7c05667
to
06efb7d
Compare
@dpgaspar and @villebro I was wondering whether you had any updated thoughts regarding this approach. Note I do believe that long term many constructs of the existing security manager (from a data access perspective) will need to change as the constructs of database, schema, datasource access is somewhat regimented especially within the world of row level access (exists within Apache Superset) and column level access (exists at Airbnb in the form of metric level access). |
@@ -450,7 +450,7 @@ def data(self) -> Response: | |||
except KeyError: | |||
return self.response_400(message="Request is incorrect") | |||
try: | |||
security_manager.assert_query_context_permission(query_context) | |||
query_context.raise_for_access() |
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.
not changed in this PR, but this try/catch seems kinda weird, I would've expected this api to be wrapped in the handle_api_exception
decorator that i believe automatically generates the proper json error response from an exception
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.
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.
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.
superset/security/manager.py
Outdated
@@ -232,7 +233,8 @@ def can_access_all_databases(self) -> bool: | |||
|
|||
def can_access_database(self, database: "Database") -> bool: | |||
""" | |||
Return True if the user can access the Superset database, False otherwise. | |||
Return True if the user can access all the tables within the Superset database, |
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.
I though there was merit in being more explicit about what it means to be able to access a database, schema, etc. It's not overly apparent without the comment.
b4ae299
to
997f1fd
Compare
@john-bodley this needs a rebase |
997f1fd
to
3c5f3ef
Compare
@villebro I've rebased. Note in the second commit I also extended the logic to replace the |
634943c
to
95fe0ec
Compare
1aef994
to
bfb0965
Compare
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.
LGTM. My only concern is the fairly loaded method raise_for_access
method, which in current form feels slightly difficult to approach for first-timers, and was slightly difficult to follow with many branching if-statements. I'm not sure how you feel about the mixin approach, but it could help remove some complexity in this method, although would arguably introduce some bloat elsewhere.
superset/security/manager.py
Outdated
if not self.datasource_access(datasource): | ||
raise SupersetSecurityException( | ||
self.get_datasource_access_error_object(datasource), | ||
from superset import db |
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.
An optional approach here would be to define DatasourceMixin
which provides a datasource, and add it to BaseViz
, QueryContext
and Datasource
. in this case we could simplify the signature of this method by combining viz
, datasource
and query_context
into one single datasource: DatasourceMixin
. The same could be done to query
and database
(DatabaseMixin
). This could help clarify what the diffefent arguments mean.
@villebro I agree that the The mixin is an interesting idea, though I sense it mightn’t simply the logic in |
I'm aware of this change, will take a look today |
superset/security/manager.py
Outdated
if not self.datasource_access(datasource): | ||
raise SupersetSecurityException( | ||
self.get_datasource_access_error_object(datasource), | ||
from superset import db |
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.
It's tempting to think of something like use a **kwargs
then use a dict to route the named parameters to smaller methods that would implement the security assertion logic. A bit crazy, but has it's upsides, cleaner signature, forced breaking logic into smaller chunks, easy to extend.
Not 100% sure if it's doable,
superset/security/manager.py
Outdated
|
||
if not (schema_perm and self.can_access("schema_access", schema_perm)): | ||
datasources = SqlaTable.query_datasources_by_name( | ||
db.session, database, table_.table, schema=table_.schema |
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.
Regarding the from superset import db
We could replace it by self.get_session
bfb0965
to
24cf5fd
Compare
* chore(security): Updating assert logic * Deprecating rejected_tables Co-authored-by: John Bodley <[email protected]> (cherry picked from commit aefef9c)
* chore(security): Updating assert logic * Deprecating rejected_tables Co-authored-by: John Bodley <[email protected]>
SUMMARY
This PR updates the security manager assertion logic making it more flexible for deployments to configure their security manager. Currently there exists two types of methods:
can_access_*
, e.g.can_access_datasource
, which returns a boolean in response to whether the user can access said resource.assert_*_permission
, e.g.assert_datasource_permission
, which returnsNone
but raises an exception if the user cannot access said resource.The issue with this approach is that some methods like
access_datasource
have all the logic andassert_datasource_permission
callscan_access_datasource
and raises an exception if the response isFalse
, whereas others likecan_access_table
have no assert equivalent.Additional context as to why a user cannot access said resource is lost when the logic resides in the
can_access_*
and thus the preferred approach is to have the logic in theassert_*_permission
method and thus thecan_access_*
methods (which are mostly just convenience methods) can take the form,Note I’ve only translated a few of the
can_access_*
methods. I think in the future it could make sense that more of these methods could leverage this pattern depending on how deployments with custom security managers would want to proceed.Secondly the term
assert
seems strange as one would expect it to raise anAssertionError
if the assertion fails, whereas these methods raise aSupersetSecurityException
, hence I opted for therequests
approach (which usesraise_for_status
) to call these methodsraise_for_access
and thus it seems clearer than an exception may be raised (and thus should be handled if necessary). I used the termaccess
rather thanpermission
for consistency with thecan_access_*
methods. I also added a wrapper function to various classes so rather than,the logic is now,
Finally rather than having a slew of
raise_for_*_access
which can add to the confusion (and introduce unnecessary complexity) if a deployment needs to override these, i.e., they intrinsically need to know how these interact, I defined a singleraise_for_access
method which contains all the logic regardless of the resource type (table, datasource, visualization, etc.). This can be helpful say if a user cannot access to an outright datasource but due to column level security can access the datasource in the context of a visualization (where the columns are defined).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
CI and added unit tests.
ADDITIONAL INFORMATION