-
Notifications
You must be signed in to change notification settings - Fork 14.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
Add schema level access control on csv upload #5787
Changes from 12 commits
1876168
3f19778
dd33783
ff12706
ad8b16e
ae7bce2
9a85831
e421c17
6f6d158
2805853
7952913
e87cf4d
00e19cd
1c72ee4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ | |
from wtforms.ext.sqlalchemy.fields import QuerySelectField | ||
from wtforms.validators import DataRequired, NumberRange, Optional | ||
|
||
from superset import app, db | ||
from superset import app, db, security_manager | ||
from superset.models import core as models | ||
|
||
config = app.config | ||
|
@@ -49,10 +49,52 @@ def filter_not_empty_values(value): | |
|
||
class CsvToDatabaseForm(DynamicForm): | ||
# pylint: disable=E0211 | ||
def csv_enabled_dbs(): | ||
return db.session.query( | ||
def csv_allowed_dbs(): | ||
csv_allowed_dbs = [] | ||
csv_enabled_dbs = db.session.query( | ||
models.Database).filter_by( | ||
allow_csv_upload=True).all() | ||
allow_csv_upload=True).all() | ||
for csv_enabled_db in csv_enabled_dbs: | ||
if CsvToDatabaseForm.at_least_one_schema_is_allowed(csv_enabled_db): | ||
csv_allowed_dbs.append(csv_enabled_db) | ||
return csv_allowed_dbs | ||
|
||
@staticmethod | ||
def at_least_one_schema_is_allowed(database): | ||
""" | ||
If the user has access to the database or all datasource | ||
1. if schemas_allowed_for_csv_upload is empty | ||
a) if database does not support schema | ||
user is able to upload csv without specifying schema name | ||
b) if database supports schema | ||
user is able to upload csv to any schema | ||
2. if schemas_allowed_for_csv_upload is not empty | ||
a) if database does not support schema | ||
This situation is impossible and upload will fail | ||
b) if database supports schema | ||
user is able to upload to schema in schemas_allowed_for_csv_upload | ||
elif the user does not access to the database or all datasource | ||
1. if schemas_allowed_for_csv_upload is empty | ||
a) if database does not support schema | ||
user is unable to upload csv | ||
b) if database supports schema | ||
user is unable to upload csv | ||
2. if schemas_allowed_for_csv_upload is not empty | ||
a) if database does not support schema | ||
This situation is impossible and user is unable to upload csv | ||
b) if database supports schema | ||
user is able to upload to schema in schemas_allowed_for_csv_upload | ||
""" | ||
if (security_manager.database_access(database) or | ||
security_manager.all_datasource_access()): | ||
return True | ||
else: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch. thanks |
||
schemas = database.get_schema_access_for_csv_upload() | ||
if (schemas and | ||
security_manager.schemas_accessible_by_user( | ||
database, schemas, False)): | ||
return True | ||
return False | ||
|
||
name = StringField( | ||
_('Table Name'), | ||
|
@@ -66,8 +108,14 @@ def csv_enabled_dbs(): | |
FileRequired(), FileAllowed(['csv'], _('CSV Files Only!'))]) | ||
con = QuerySelectField( | ||
_('Database'), | ||
query_factory=csv_enabled_dbs, | ||
query_factory=csv_allowed_dbs, | ||
get_pk=lambda a: a.id, get_label=lambda a: a.database_name) | ||
schema = StringField( | ||
_('Schema'), | ||
description=_('Specify a schema (if database flavour supports this).'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: "flavour" is British English, it would be better to have US English ("flavor") as the default language. |
||
validators=[Optional()], | ||
widget=BS3TextFieldWidget(), | ||
filters=[lambda x: x or None]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did not notice that. I just copy-pasted to move these lines of code for a better order showing on the UI. Will take a look. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Took a quick look. I think the original author wrote this because that if no |
||
sep = StringField( | ||
_('Delimiter'), | ||
description=_('Delimiter used by CSV file (for whitespace use \s+).'), | ||
|
@@ -83,12 +131,6 @@ def csv_enabled_dbs(): | |
('fail', _('Fail')), ('replace', _('Replace')), | ||
('append', _('Append'))], | ||
validators=[DataRequired()]) | ||
schema = StringField( | ||
_('Schema'), | ||
description=_('Specify a schema (if database flavour supports this).'), | ||
validators=[Optional()], | ||
widget=BS3TextFieldWidget(), | ||
filters=[lambda x: x or None]) | ||
header = IntegerField( | ||
_('Header Row'), | ||
description=_( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -638,19 +638,19 @@ class Database(Model, AuditMixinNullable, ImportMixin): | |
expose_in_sqllab = Column(Boolean, default=False) | ||
allow_run_sync = Column(Boolean, default=True) | ||
allow_run_async = Column(Boolean, default=False) | ||
allow_csv_upload = Column(Boolean, default=True) | ||
allow_csv_upload = Column(Boolean, default=False) | ||
allow_ctas = Column(Boolean, default=False) | ||
allow_dml = Column(Boolean, default=False) | ||
force_ctas_schema = Column(String(250)) | ||
allow_multi_schema_metadata_fetch = Column(Boolean, default=True) | ||
extra = Column(Text, default=textwrap.dedent("""\ | ||
{ | ||
"metadata_params": {}, | ||
"engine_params": {} | ||
"engine_params": {}, | ||
"schemas_allowed_for_csv_upload": [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: leave a trailing comma on the last line, it makes it easier to sort lines and for better There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
""")) | ||
perm = Column(String(1000)) | ||
|
||
impersonate_user = Column(Boolean, default=False) | ||
export_fields = ('database_name', 'sqlalchemy_uri', 'cache_timeout', | ||
'expose_in_sqllab', 'allow_run_sync', 'allow_run_async', | ||
|
@@ -908,6 +908,7 @@ def get_extra(self): | |
extra = json.loads(self.extra) | ||
except Exception as e: | ||
logging.error(e) | ||
raise e | ||
return extra | ||
|
||
def get_table(self, table_name, schema=None): | ||
|
@@ -931,6 +932,9 @@ def get_pk_constraint(self, table_name, schema=None): | |
def get_foreign_keys(self, table_name, schema=None): | ||
return self.inspector.get_foreign_keys(table_name, schema) | ||
|
||
def get_schema_access_for_csv_upload(self): | ||
return self.get_extra().get('schemas_allowed_for_csv_upload', []) | ||
|
||
@property | ||
def sqlalchemy_uri_decrypted(self): | ||
conn = sqla.engine.url.make_url(self.sqlalchemy_uri) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
{% extends 'appbuilder/general/model/edit.html' %} | ||
|
||
{% block tail_js %} | ||
{{ super() }} | ||
<script> | ||
var db = $("#con"); | ||
var schema = $("#schema"); | ||
|
||
// this element is a text input | ||
// copy it here so it can be reused later | ||
var any_schema_is_allowed = schema.clone(); | ||
|
||
update_schemas_allowed_for_csv_upload(db.val()); | ||
db.change(function(){ | ||
update_schemas_allowed_for_csv_upload(db.val()); | ||
}); | ||
|
||
function update_schemas_allowed_for_csv_upload(db_id) { | ||
$.ajax({ | ||
method: "GET", | ||
url: "/superset/schema_access_for_csv_upload", | ||
data: {db_id: db_id}, | ||
dataType: 'json', | ||
contentType: "application/json; charset=utf-8" | ||
}).done(function(data) { | ||
change_schema_field_in_formview(data) | ||
}).fail(function(error) { | ||
var errorMsg = error.responseJSON.error; | ||
alert("ERROR: " + errorMsg); | ||
}); | ||
} | ||
|
||
function change_schema_field_in_formview(schemas_allowed){ | ||
if (schemas_allowed && schemas_allowed.length > 0) { | ||
var dropdown_schema_lists = '<select id="schema" name="schema" required>'; | ||
schemas_allowed.forEach(function(schema_allowed) { | ||
dropdown_schema_lists += ('<option value="' + schema_allowed + '">' + schema_allowed + '</option>'); | ||
}); | ||
dropdown_schema_lists += '</select>'; | ||
$("#schema").replaceWith(dropdown_schema_lists); | ||
} else { | ||
$("#schema").replaceWith(any_schema_is_allowed) | ||
} | ||
} | ||
</script> | ||
{% endblock %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,4 +4,5 @@ | |
{% block tail_js %} | ||
{{ super() }} | ||
{{ macros.testconn() }} | ||
{{ macros.expand_extra_textarea() }} | ||
{% endblock %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,4 +4,5 @@ | |
{% block tail_js %} | ||
{{ super() }} | ||
{{ macros.testconn() }} | ||
{{ macros.expand_extra_textarea() }} | ||
{% endblock %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -844,6 +844,7 @@ def get_or_create_main_db(): | |
dbobj.set_sqlalchemy_uri(conf.get('SQLALCHEMY_DATABASE_URI')) | ||
dbobj.expose_in_sqllab = True | ||
dbobj.allow_run_sync = True | ||
dbobj.allow_csv_upload = True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @youngyjd why is this explicitly set? Note in general one would not want to have users upload to the @mistercrunch personally I don't think we should be setting any of these fields if the
i.e., all these settings should be initialized as |
||
db.session.add(dbobj) | ||
db.session.commit() | ||
return dbobj | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,10 +154,10 @@ class DatabaseView(SupersetModelView, DeleteMixin, YamlExportMixin): # noqa | |
'modified', 'allow_csv_upload', | ||
] | ||
add_columns = [ | ||
'database_name', 'sqlalchemy_uri', 'cache_timeout', 'extra', | ||
'expose_in_sqllab', 'allow_run_sync', 'allow_run_async', 'allow_csv_upload', | ||
'database_name', 'sqlalchemy_uri', 'cache_timeout', 'expose_in_sqllab', | ||
'allow_run_sync', 'allow_run_async', 'allow_csv_upload', | ||
'allow_ctas', 'allow_dml', 'force_ctas_schema', 'impersonate_user', | ||
'allow_multi_schema_metadata_fetch', | ||
'allow_multi_schema_metadata_fetch', 'extra', | ||
] | ||
search_exclude_columns = ( | ||
'password', 'tables', 'created_by', 'changed_by', 'queries', | ||
|
@@ -203,14 +203,19 @@ class DatabaseView(SupersetModelView, DeleteMixin, YamlExportMixin): # noqa | |
'When allowing CREATE TABLE AS option in SQL Lab, ' | ||
'this option forces the table to be created in this schema'), | ||
'extra': utils.markdown( | ||
'JSON string containing extra configuration elements. ' | ||
'The ``engine_params`` object gets unpacked into the ' | ||
'JSON string containing extra configuration elements.<br/>' | ||
'1. The ``engine_params`` object gets unpacked into the ' | ||
'[sqlalchemy.create_engine]' | ||
'(http://docs.sqlalchemy.org/en/latest/core/engines.html#' | ||
'sqlalchemy.create_engine) call, while the ``metadata_params`` ' | ||
'gets unpacked into the [sqlalchemy.MetaData]' | ||
'(http://docs.sqlalchemy.org/en/rel_1_0/core/metadata.html' | ||
'#sqlalchemy.schema.MetaData) call. ', True), | ||
'#sqlalchemy.schema.MetaData) call.<br/>' | ||
'2. The ``schemas_allowed_for_csv_upload`` is a comma separated list ' | ||
'of schemas that CSVs are allowed to upload to. ' | ||
'Specify it as **"schemas_allowed": ["public", "csv_upload"]**. ' | ||
'If database flavour does not support schema or any schema is allowed ' | ||
'to be accessed, just leave the list empty', True), | ||
'impersonate_user': _( | ||
'If Presto, all the queries in SQL Lab are going to be executed as the ' | ||
'currently logged on user who must have permission to run them.<br/>' | ||
|
@@ -225,6 +230,8 @@ class DatabaseView(SupersetModelView, DeleteMixin, YamlExportMixin): # noqa | |
'Duration (in seconds) of the caching timeout for this database. ' | ||
'A timeout of 0 indicates that the cache never expires. ' | ||
'Note this defaults to the global timeout if undefined.'), | ||
'allow_csv_upload': _( | ||
'If selected, please set the schemas allowed for csv upload in Extra.'), | ||
} | ||
label_columns = { | ||
'expose_in_sqllab': _('Expose in SQL Lab'), | ||
|
@@ -302,6 +309,7 @@ class DatabaseAsync(DatabaseView): | |
|
||
class CsvToDatabaseView(SimpleFormView): | ||
form = CsvToDatabaseForm | ||
form_template = 'superset/form_view/csv_to_database_view/edit.html' | ||
form_title = _('CSV to Database configuration') | ||
add_columns = ['database', 'schema', 'table_name'] | ||
|
||
|
@@ -313,9 +321,21 @@ def form_get(self, form): | |
form.skip_blank_lines.data = True | ||
form.infer_datetime_format.data = True | ||
form.decimal.data = '.' | ||
form.if_exists.data = 'append' | ||
form.if_exists.data = 'fail' | ||
|
||
def form_post(self, form): | ||
database = form.con.data | ||
schema_name = form.schema.data or '' | ||
|
||
if not self.is_schema_allowed(database, schema_name): | ||
message = _('Database "{0}" Schema "{1}" is not allowed for csv uploads. ' | ||
'Please contact Superset Admin'.format(database.database_name, | ||
schema_name)) | ||
flash( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be written in a single language: flash(message, 'danger') There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. woops. I must have copy-paste from somewhere and then made some changes but did not aware of the styling issue. thanks |
||
message, | ||
'danger') | ||
return redirect('/csvtodatabaseview/form') | ||
|
||
csv_file = form.csv_file.data | ||
form.csv_file.data.filename = secure_filename(form.csv_file.data.filename) | ||
csv_filename = form.csv_file.data.filename | ||
|
@@ -349,6 +369,15 @@ def form_post(self, form): | |
flash(message, 'info') | ||
return redirect('/tablemodelview/list/') | ||
|
||
def is_schema_allowed(self, database, schema): | ||
if not database.allow_csv_upload: | ||
return False | ||
schemas = database.get_schema_access_for_csv_upload() | ||
if schemas: | ||
return schema in schemas | ||
return (security_manager.database_access(database) or | ||
security_manager.all_datasource_access()) | ||
|
||
|
||
appbuilder.add_view_no_menu(CsvToDatabaseView) | ||
|
||
|
@@ -2741,6 +2770,44 @@ def slice_query(self, slice_id): | |
link=security_manager.get_datasource_access_link(viz_obj.datasource)) | ||
return self.get_query_string_response(viz_obj) | ||
|
||
@api | ||
@has_access_api | ||
@expose('/schema_access_for_csv_upload') | ||
def schemas_access_for_csv_upload(self): | ||
""" | ||
This method exposes an API endpoint to | ||
get the schema access control settings for csv upload in this database | ||
""" | ||
if not request.args.get('db_id'): | ||
return json_error_response( | ||
'No database is allowed for your csv upload') | ||
|
||
db_id = int(request.args.get('db_id')) | ||
database = ( | ||
db.session | ||
.query(models.Database) | ||
.filter_by(id=db_id) | ||
.one() | ||
) | ||
try: | ||
schemas_allowed = database.get_schema_access_for_csv_upload() | ||
if (security_manager.database_access(database) or | ||
security_manager.all_datasource_access()): | ||
return self.json_response(schemas_allowed) | ||
# the list schemas_allowed should not be empty here | ||
# and the list schemas_allowed_processed returned from security_manager | ||
# should not be empty either, | ||
# otherwise the database should have been filtered out | ||
# in CsvToDatabaseForm | ||
schemas_allowed_processed = security_manager.schemas_accessible_by_user( | ||
database, schemas_allowed, False) | ||
return self.json_response(schemas_allowed_processed) | ||
except Exception: | ||
return json_error_response(( | ||
'Failed to fetch schemas allowed for csv upload in this database! ' | ||
'Please contact Superset Admin!\n\n' | ||
'The error message returned was:\n{}').format(traceback.format_exc())) | ||
|
||
|
||
appbuilder.add_view_no_menu(Superset) | ||
|
||
|
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.
Nice!