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.

Sign up for GitHub

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

Jump to bottom

[security] Adding docstrings and type hints #7952

Merged
merged 1 commit into from Aug 5, 2019
Merged

[security] Adding docstrings and type hints #7952

merged 1 commit into from Aug 5, 2019

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Jul 31, 2019 •
edited

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

This PR adds doc-strings and type hints to the security manager to help to provide more context and consistency. Note I've tried to ensure that the code remains unchanged however:

  1. I renamed a couple of methods/variables for accuracy. Note further polish could be applied.
  2. Given the vast number of methods in the SupersetSecurityManager (it's becoming almost intractable) I renamed private methods with a leading _ (underscore).
  3. The Mypy type hints surfaced a couple of small issues during linting.

A few notes/observations:

  1. I get the sense there's too many surfaces where security datasource checks happen (many occur on the view) and thus we susceptible for potentially forgetting to add them for new routes. Moving forward it seems like these checks should probably occur closer to the engine where appropriate.
  2. I've tried to be as descriptive as possible but it took me quite some time to decipher what the various objects were. There may be merit in the future of being more explicit between Druid datasources and SQL tables.
  3. I think some of the private methods could probably be refactored.
  4. The SQL table names seem to be a mess, currently these can be defined as:
  • str: [[cluster.]schema.]table
  • Tuple[str, str]: (schema, table)
  • DatasourceName

I believe there would be merit in aligning on one mechanism for defining SQL tables throughout Superset to help mitigate bugs/inaccuracies (note we've addresses issues related to this in the past). This is especially vital as it relates to security and a hardened security model should be one of Superset's core tenets. A dataclass class may be a viable route.

  1. I'm not certain whether the get_schema_perm logic is correct as I believe that the database parameter can be Union[Database, str]. I'm not sure if that was intentional

TEST PLAN

CI.

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

REVIEWERS

to: @DiggidyDave @etr2460 @michellethomas @mistercrunch @villebro

@john-bodley john-bodley force-pushed the john-bodley--security-manager-add-docstrings-and-typing branch 5 times, most recently from df0b27e to 576947e Compare July 31, 2019 16:58
@codecov-io
Copy link

Codecov Report

Merging #7952 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7952      +/-   ##
==========================================
+ Coverage   65.61%   65.62%   +<.01%     
==========================================
  Files         469      469              
  Lines       22381    22388       +7     
  Branches     2432     2432              
==========================================
+ Hits        14686    14692       +6     
- Misses       7574     7575       +1     
  Partials      121      121
Impacted Files Coverage Δ
superset/views/core.py 71.22% <66.66%> (ø) ⬆️
superset/security.py 74.29% <84.12%> (+0.33%) ⬆️

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 10f00cd...576947e. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jul 31, 2019 •
edited

Codecov Report

Merging #7952 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7952      +/-   ##
==========================================
+ Coverage   65.61%   65.62%   +<.01%     
==========================================
  Files         469      469              
  Lines       22381    22388       +7     
  Branches     2432     2432              
==========================================
+ Hits        14686    14692       +6     
- Misses       7574     7575       +1     
  Partials      121      121
Impacted Files Coverage Δ
superset/views/core.py 71.22% <66.66%> (ø) ⬆️
superset/security.py 74.29% <84.12%> (+0.33%) ⬆️

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 10f00cd...8cccd26. Read the comment docs.

@john-bodley john-bodley force-pushed the john-bodley--security-manager-add-docstrings-and-typing branch from 576947e to d6a8259 Compare July 31, 2019 18:27
john-bodley
john-bodley commented Jul 31, 2019
View reviewed changes
superset/security.py
def datasource_access_by_fullname(self, database, table_in_query, schema):
table_schema, table_name = self.get_schema_and_table(table_in_query, schema)
return self.datasource_access_by_name(database, table_name, schema=table_schema)
def _datasource_access_by_fullname(
Copy link
Member Author

Choose a reason for hiding this comment

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

Note I find the naming of this method somewhat confusing and there's also a method called _datasource_access_by_name, i.e, without the full.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this is super confusing. I'm probably an outlier among those reading this, but I don't know what "fallback sql schema" means, would it be appropriate to clarify that here in the param docs? I realize it might be part of the superset codebase vernacular and maybe shouldn't be repeated everywhere it is used, if that's the case then cool. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

@DiggidyDave I added more documentation regarding the "fallback SQL schema".

@john-bodley john-bodley force-pushed the john-bodley--security-manager-add-docstrings-and-typing branch from d6a8259 to 8cccd26 Compare July 31, 2019 19:20
@DiggidyDave
Copy link
Contributor

I believe there would be merit in aligning on one mechanism for defining SQL tables throughout Superset ... A dataclass class may be a viable route.

👍 agreed! if something is important enough that folks have invented 3 ways of doing it, it probably should have a first-class type :-)

Thanks for doing all of this work

@john-bodley john-bodley force-pushed the john-bodley--security-manager-add-docstrings-and-typing branch 2 times, most recently from 3ab7175 to 5b728d4 Compare July 31, 2019 21:10
@john-bodley john-bodley force-pushed the john-bodley--security-manager-add-docstrings-and-typing branch from 5b728d4 to e463d86 Compare August 1, 2019 00:04
villebro
villebro approved these changes Aug 2, 2019
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Awesome work 👍 Apart from the small comment about I202 LGTM.

superset/security.py
if TYPE_CHECKING:
from superset.models.core import Database, BaseDatasource

from superset.utils.core import DatasourceName # noqa: I202
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should add ignoring of I202 to tox.ini? @sturmer ran intro a few of those in #7643 and it seems flake8 and black aren't aligned in their thinking on newlines.

Copy link
Member Author

Choose a reason for hiding this comment

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

@villebro I can create another PR which ignores I202 and removes all references.

superset/security.py
`all_datasource_access` permission""".format(
datasource.name
)
def get_datasource_access_error_msg(self, datasource: "BaseDatasource") -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this in quotes?

Edit: googled it, learned something new 👍

@john-bodley john-bodley merged commit f7af50c into apache:master Aug 5, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DiggidyDave DiggidyDave DiggidyDave left review comments

@villebro villebro villebro approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 0.34.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@john-bodley @codecov-io @DiggidyDave @villebro @mistercrunch