You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2023/04/21 20:47:07 UTC
[GitHub] [superset] michael-s-molina opened a new pull request, #23777: feat(revert): Re-introduces the RLS page
michael-s-molina opened a new pull request, #23777:
URL: https://github.com/apache/superset/pull/23777
### SUMMARY
This PR re-introduces the changes made in https://github.com/apache/superset/pull/22325 that were reverted because it introduced breaking changes to master.
This PR is not an exact revert of the revert because it adapts the files to the pages structure. It also worth mentioning that the menu option to the RLS page is currently triggering a reload and we should work on a follow-up to make it fully compatible with the SPA project.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Check the original PR for screenshots.
### TESTING INSTRUCTIONS
Check the original PR for test instructions.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23777:
URL: https://github.com/apache/superset/pull/23777#discussion_r1175599671
##########
superset/row_level_security/api.py:
##########
@@ -0,0 +1,349 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from typing import Any
+
+from flask import request, Response
+from flask_appbuilder.api import expose, protect, rison, safe
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from flask_babel import ngettext
+from marshmallow import ValidationError
+
+from superset import app
+from superset.commands.exceptions import (
+ DatasourceNotFoundValidationError,
+ RolesNotFoundValidationError,
+)
+from superset.connectors.sqla.models import RowLevelSecurityFilter
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
+from superset.dao.exceptions import DAOCreateFailedError, DAOUpdateFailedError
+from superset.extensions import event_logger
+from superset.row_level_security.commands.bulk_delete import BulkDeleteRLSRuleCommand
+from superset.row_level_security.commands.create import CreateRLSRuleCommand
+from superset.row_level_security.commands.exceptions import RLSRuleNotFoundError
+from superset.row_level_security.commands.update import UpdateRLSRuleCommand
+from superset.row_level_security.schemas import (
+ get_delete_ids_schema,
+ RLSListSchema,
+ RLSPostSchema,
+ RLSPutSchema,
+ RLSShowSchema,
+)
+from superset.views.base_api import (
+ BaseSupersetModelRestApi,
+ requires_json,
+ statsd_metrics,
+)
+
+logger = logging.getLogger(__name__)
+
+
+class RLSRestApi(BaseSupersetModelRestApi):
+ datamodel = SQLAInterface(RowLevelSecurityFilter)
+ include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | {
+ RouteMethod.RELATED,
+ "bulk_delete",
+ }
+ resource_name = "rowlevelsecurity"
+ class_permission_name = "Row Level Security"
+ openapi_spec_tag = "Row Level Security"
+ method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP
+ allow_browser_login = True
+
+ list_columns = [
+ "id",
+ "name",
+ "filter_type",
+ "tables.id",
+ "tables.table_name",
+ "roles.id",
+ "roles.name",
+ "clause",
+ "changed_on_delta_humanized",
+ "group_key",
+ ]
+ order_columns = [
+ "name",
+ "filter_type",
+ "clause",
+ "changed_on_delta_humanized",
+ "group_key",
+ ]
+ add_columns = [
+ "name",
+ "description",
+ "filter_type",
+ "tables",
+ "roles",
+ "group_key",
+ "clause",
+ ]
+ show_columns = [
+ "name",
+ "description",
+ "filter_type",
+ "tables.id",
+ "tables.schema",
+ "tables.table_name",
+ "roles.id",
+ "roles.name",
+ "group_key",
+ "clause",
+ ]
+ search_columns = (
+ "name",
+ "description",
+ "filter_type",
+ "tables",
+ "roles",
+ "group_key",
+ "clause",
+ )
+ edit_columns = add_columns
+
+ show_model_schema = RLSShowSchema()
+ list_model_schema = RLSListSchema()
+ add_model_schema = RLSPostSchema()
+ edit_model_schema = RLSPutSchema()
+
+ allowed_rel_fields = {"tables", "roles"}
+ base_related_field_filters = app.config["RLS_BASE_RELATED_FIELD_FILTERS"]
Review Comment:
@michael-s-molina sure, will do π
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] mayurnewase commented on pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "mayurnewase (via GitHub)" <gi...@apache.org>.
mayurnewase commented on PR #23777:
URL: https://github.com/apache/superset/pull/23777#issuecomment-1519456549
awesome!!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] EugeneTorap commented on a diff in pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "EugeneTorap (via GitHub)" <gi...@apache.org>.
EugeneTorap commented on code in PR #23777:
URL: https://github.com/apache/superset/pull/23777#discussion_r1174257577
##########
superset/connectors/sqla/views.py:
##########
@@ -270,107 +269,15 @@ class SqlMetricInlineView( # pylint: disable=too-many-ancestors
edit_form_extra_fields = add_form_extra_fields
-class RowLevelSecurityListWidget(
- SupersetListWidget
-): # pylint: disable=too-few-public-methods
- template = "superset/models/rls/list.html"
Review Comment:
@michael-s-molina We can also remove this html file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23777:
URL: https://github.com/apache/superset/pull/23777#discussion_r1175511834
##########
superset/row_level_security/api.py:
##########
@@ -0,0 +1,349 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from typing import Any
+
+from flask import request, Response
+from flask_appbuilder.api import expose, protect, rison, safe
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from flask_babel import ngettext
+from marshmallow import ValidationError
+
+from superset import app
+from superset.commands.exceptions import (
+ DatasourceNotFoundValidationError,
+ RolesNotFoundValidationError,
+)
+from superset.connectors.sqla.models import RowLevelSecurityFilter
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
+from superset.dao.exceptions import DAOCreateFailedError, DAOUpdateFailedError
+from superset.extensions import event_logger
+from superset.row_level_security.commands.bulk_delete import BulkDeleteRLSRuleCommand
+from superset.row_level_security.commands.create import CreateRLSRuleCommand
+from superset.row_level_security.commands.exceptions import RLSRuleNotFoundError
+from superset.row_level_security.commands.update import UpdateRLSRuleCommand
+from superset.row_level_security.schemas import (
+ get_delete_ids_schema,
+ RLSListSchema,
+ RLSPostSchema,
+ RLSPutSchema,
+ RLSShowSchema,
+)
+from superset.views.base_api import (
+ BaseSupersetModelRestApi,
+ requires_json,
+ statsd_metrics,
+)
+
+logger = logging.getLogger(__name__)
+
+
+class RLSRestApi(BaseSupersetModelRestApi):
+ datamodel = SQLAInterface(RowLevelSecurityFilter)
+ include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | {
+ RouteMethod.RELATED,
+ "bulk_delete",
+ }
+ resource_name = "rowlevelsecurity"
+ class_permission_name = "Row Level Security"
+ openapi_spec_tag = "Row Level Security"
+ method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP
+ allow_browser_login = True
+
+ list_columns = [
+ "id",
+ "name",
+ "filter_type",
+ "tables.id",
+ "tables.table_name",
+ "roles.id",
+ "roles.name",
+ "clause",
+ "changed_on_delta_humanized",
+ "group_key",
+ ]
+ order_columns = [
+ "name",
+ "filter_type",
+ "clause",
+ "changed_on_delta_humanized",
+ "group_key",
+ ]
+ add_columns = [
+ "name",
+ "description",
+ "filter_type",
+ "tables",
+ "roles",
+ "group_key",
+ "clause",
+ ]
+ show_columns = [
+ "name",
+ "description",
+ "filter_type",
+ "tables.id",
+ "tables.schema",
+ "tables.table_name",
+ "roles.id",
+ "roles.name",
+ "group_key",
+ "clause",
+ ]
+ search_columns = (
+ "name",
+ "description",
+ "filter_type",
+ "tables",
+ "roles",
+ "group_key",
+ "clause",
+ )
+ edit_columns = add_columns
+
+ show_model_schema = RLSShowSchema()
+ list_model_schema = RLSListSchema()
+ add_model_schema = RLSPostSchema()
+ edit_model_schema = RLSPutSchema()
+
+ allowed_rel_fields = {"tables", "roles"}
+ base_related_field_filters = app.config["RLS_BASE_RELATED_FIELD_FILTERS"]
Review Comment:
It makes sense to me. Can you open a follow-up with this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23777:
URL: https://github.com/apache/superset/pull/23777#discussion_r1174582595
##########
superset/connectors/sqla/views.py:
##########
@@ -270,107 +269,15 @@ class SqlMetricInlineView( # pylint: disable=too-many-ancestors
edit_form_extra_fields = add_form_extra_fields
-class RowLevelSecurityListWidget(
- SupersetListWidget
-): # pylint: disable=too-few-public-methods
- template = "superset/models/rls/list.html"
Review Comment:
Good point. Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #23777:
URL: https://github.com/apache/superset/pull/23777#issuecomment-1518367496
Thanks for the un-revert @michael-s-molina , and thanks @mayurnewase for the awesome work on this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23777:
URL: https://github.com/apache/superset/pull/23777#discussion_r1174893780
##########
superset/row_level_security/api.py:
##########
@@ -0,0 +1,349 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from typing import Any
+
+from flask import request, Response
+from flask_appbuilder.api import expose, protect, rison, safe
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from flask_babel import ngettext
+from marshmallow import ValidationError
+
+from superset import app
+from superset.commands.exceptions import (
+ DatasourceNotFoundValidationError,
+ RolesNotFoundValidationError,
+)
+from superset.connectors.sqla.models import RowLevelSecurityFilter
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
+from superset.dao.exceptions import DAOCreateFailedError, DAOUpdateFailedError
+from superset.extensions import event_logger
+from superset.row_level_security.commands.bulk_delete import BulkDeleteRLSRuleCommand
+from superset.row_level_security.commands.create import CreateRLSRuleCommand
+from superset.row_level_security.commands.exceptions import RLSRuleNotFoundError
+from superset.row_level_security.commands.update import UpdateRLSRuleCommand
+from superset.row_level_security.schemas import (
+ get_delete_ids_schema,
+ RLSListSchema,
+ RLSPostSchema,
+ RLSPutSchema,
+ RLSShowSchema,
+)
+from superset.views.base_api import (
+ BaseSupersetModelRestApi,
+ requires_json,
+ statsd_metrics,
+)
+
+logger = logging.getLogger(__name__)
+
+
+class RLSRestApi(BaseSupersetModelRestApi):
+ datamodel = SQLAInterface(RowLevelSecurityFilter)
+ include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | {
+ RouteMethod.RELATED,
+ "bulk_delete",
+ }
+ resource_name = "rowlevelsecurity"
+ class_permission_name = "Row Level Security"
+ openapi_spec_tag = "Row Level Security"
+ method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP
+ allow_browser_login = True
+
+ list_columns = [
+ "id",
+ "name",
+ "filter_type",
+ "tables.id",
+ "tables.table_name",
+ "roles.id",
+ "roles.name",
+ "clause",
+ "changed_on_delta_humanized",
+ "group_key",
+ ]
+ order_columns = [
+ "name",
+ "filter_type",
+ "clause",
+ "changed_on_delta_humanized",
+ "group_key",
+ ]
+ add_columns = [
+ "name",
+ "description",
+ "filter_type",
+ "tables",
+ "roles",
+ "group_key",
+ "clause",
+ ]
+ show_columns = [
+ "name",
+ "description",
+ "filter_type",
+ "tables.id",
+ "tables.schema",
+ "tables.table_name",
+ "roles.id",
+ "roles.name",
+ "group_key",
+ "clause",
+ ]
+ search_columns = (
+ "name",
+ "description",
+ "filter_type",
+ "tables",
+ "roles",
+ "group_key",
+ "clause",
+ )
+ edit_columns = add_columns
+
+ show_model_schema = RLSShowSchema()
+ list_model_schema = RLSListSchema()
+ add_model_schema = RLSPostSchema()
+ edit_model_schema = RLSPutSchema()
+
+ allowed_rel_fields = {"tables", "roles"}
+ base_related_field_filters = app.config["RLS_BASE_RELATED_FIELD_FILTERS"]
Review Comment:
In #22526 we introduced `EXTRA_RELATED_QUERY_FILTERS ` which already has filters for `role` and `user`. I think it might make sense deprecate `RLS_BASE_RELATED_FIELD_FILTERS` and leverage the more global config parameter here by doing the following:
- assume that `EXTRA_RELATED_QUERY_FILTERS["role"]` also applies to RLS
- Reuse the existing `DatasourceFilter` for tables
I tried this and it nicely filtered the roles and tables to match what's available on the datasets page and elsewhere (I don't see why we'd want to have a different dataset filter for RLS than what the regular RBAC model provides, unless we want to add _additional_ filters in RLS):
```python
from superset.views.base import DatasourceFilter
from superset.views.filters import BaseFilterRelatedRoles
base_related_field_filters = {
"tables": [["id", DatasourceFilter, lambda: []]],
"roles": [["id", BaseFilterRelatedRoles, lambda: []]],
}
```
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23777:
URL: https://github.com/apache/superset/pull/23777#issuecomment-1518327821
## [Codecov](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#23777](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (afffb4c) into [master](https://codecov.io/gh/apache/superset/commit/35f36a20ffe075b97ff1eef26e1876b6c4e9619a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (35f36a2) will **decrease** coverage by `11.35%`.
> The diff coverage is `65.49%`.
> :exclamation: Current head afffb4c differs from pull request most recent head b8f9334. Consider uploading reports for the commit b8f9334 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23777 +/- ##
===========================================
- Coverage 67.87% 56.53% -11.35%
===========================================
Files 1925 1932 +7
Lines 74389 74642 +253
Branches 8108 8108
===========================================
- Hits 50494 42201 -8293
- Misses 21818 30364 +8546
Partials 2077 2077
```
| Flag | Coverage Ξ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.91% <65.49%> (+0.07%)` | :arrow_up: |
| python | `59.12% <65.49%> (-23.53%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `52.80% <65.49%> (+0.07%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Ξ | |
|---|---|---|
| [superset-frontend/src/views/routes.tsx](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL3JvdXRlcy50c3g=) | `51.11% <ΓΈ> (ΓΈ)` | |
| [superset/views/sql\_lab/views.py](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3Mvc3FsX2xhYi92aWV3cy5weQ==) | `56.57% <ΓΈ> (-7.24%)` | :arrow_down: |
| [superset/dao/base.py](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFvL2Jhc2UucHk=) | `83.67% <20.00%> (-11.79%)` | :arrow_down: |
| [superset/row\_level\_security/commands/update.py](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcm93X2xldmVsX3NlY3VyaXR5L2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `41.66% <41.66%> (ΓΈ)` | |
| [superset/row\_level\_security/commands/create.py](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcm93X2xldmVsX3NlY3VyaXR5L2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `46.66% <46.66%> (ΓΈ)` | |
| [...uperset/row\_level\_security/commands/bulk\_delete.py](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcm93X2xldmVsX3NlY3VyaXR5L2NvbW1hbmRzL2J1bGtfZGVsZXRlLnB5) | `52.00% <52.00%> (ΓΈ)` | |
| [superset/row\_level\_security/api.py](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcm93X2xldmVsX3NlY3VyaXR5L2FwaS5weQ==) | `60.39% <60.39%> (ΓΈ)` | |
| [superset/connectors/sqla/views.py](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3ZpZXdzLnB5) | `88.78% <90.90%> (-0.02%)` | :arrow_down: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.30% <100.00%> (-0.56%)` | :arrow_down: |
| [superset/initialization/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvaW5pdGlhbGl6YXRpb24vX19pbml0X18ucHk=) | `91.43% <100.00%> (+0.05%)` | :arrow_up: |
| ... and [3 more](https://codecov.io/gh/apache/superset/pull/23777?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [298 files with indirect coverage changes](https://codecov.io/gh/apache/superset/pull/23777/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: Weβre building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina merged pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #23777:
URL: https://github.com/apache/superset/pull/23777
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23777:
URL: https://github.com/apache/superset/pull/23777#discussion_r1174893780
##########
superset/row_level_security/api.py:
##########
@@ -0,0 +1,349 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from typing import Any
+
+from flask import request, Response
+from flask_appbuilder.api import expose, protect, rison, safe
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from flask_babel import ngettext
+from marshmallow import ValidationError
+
+from superset import app
+from superset.commands.exceptions import (
+ DatasourceNotFoundValidationError,
+ RolesNotFoundValidationError,
+)
+from superset.connectors.sqla.models import RowLevelSecurityFilter
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
+from superset.dao.exceptions import DAOCreateFailedError, DAOUpdateFailedError
+from superset.extensions import event_logger
+from superset.row_level_security.commands.bulk_delete import BulkDeleteRLSRuleCommand
+from superset.row_level_security.commands.create import CreateRLSRuleCommand
+from superset.row_level_security.commands.exceptions import RLSRuleNotFoundError
+from superset.row_level_security.commands.update import UpdateRLSRuleCommand
+from superset.row_level_security.schemas import (
+ get_delete_ids_schema,
+ RLSListSchema,
+ RLSPostSchema,
+ RLSPutSchema,
+ RLSShowSchema,
+)
+from superset.views.base_api import (
+ BaseSupersetModelRestApi,
+ requires_json,
+ statsd_metrics,
+)
+
+logger = logging.getLogger(__name__)
+
+
+class RLSRestApi(BaseSupersetModelRestApi):
+ datamodel = SQLAInterface(RowLevelSecurityFilter)
+ include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | {
+ RouteMethod.RELATED,
+ "bulk_delete",
+ }
+ resource_name = "rowlevelsecurity"
+ class_permission_name = "Row Level Security"
+ openapi_spec_tag = "Row Level Security"
+ method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP
+ allow_browser_login = True
+
+ list_columns = [
+ "id",
+ "name",
+ "filter_type",
+ "tables.id",
+ "tables.table_name",
+ "roles.id",
+ "roles.name",
+ "clause",
+ "changed_on_delta_humanized",
+ "group_key",
+ ]
+ order_columns = [
+ "name",
+ "filter_type",
+ "clause",
+ "changed_on_delta_humanized",
+ "group_key",
+ ]
+ add_columns = [
+ "name",
+ "description",
+ "filter_type",
+ "tables",
+ "roles",
+ "group_key",
+ "clause",
+ ]
+ show_columns = [
+ "name",
+ "description",
+ "filter_type",
+ "tables.id",
+ "tables.schema",
+ "tables.table_name",
+ "roles.id",
+ "roles.name",
+ "group_key",
+ "clause",
+ ]
+ search_columns = (
+ "name",
+ "description",
+ "filter_type",
+ "tables",
+ "roles",
+ "group_key",
+ "clause",
+ )
+ edit_columns = add_columns
+
+ show_model_schema = RLSShowSchema()
+ list_model_schema = RLSListSchema()
+ add_model_schema = RLSPostSchema()
+ edit_model_schema = RLSPutSchema()
+
+ allowed_rel_fields = {"tables", "roles"}
+ base_related_field_filters = app.config["RLS_BASE_RELATED_FIELD_FILTERS"]
Review Comment:
In #22526 we introduced `EXTRA_RELATED_QUERY_FILTERS ` which already has filters for `role` and `user`. I think it might make sense deprecate `RLS_BASE_RELATED_FIELD_FILTERS` and leverage the more global config parameter here by doing the following:
- assume that `EXTRA_RELATED_QUERY_FILTERS["role"]` also applies to RLS
- Reuse the existing `DatasourceFilter` for tables
I tried this and it nicely filtered the roles and tables to match what's available on the datasets page and elsewhere:
```python
from superset.views.base import DatasourceFilter
from superset.views.filters import BaseFilterRelatedRoles
base_related_field_filters = {
"tables": [["id", DatasourceFilter, lambda: []]],
"roles": [["id", BaseFilterRelatedRoles, lambda: []]],
}
```
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #23777: feat(revert): Re-introduces the RLS page
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23777:
URL: https://github.com/apache/superset/pull/23777#discussion_r1174893780
##########
superset/row_level_security/api.py:
##########
@@ -0,0 +1,349 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from typing import Any
+
+from flask import request, Response
+from flask_appbuilder.api import expose, protect, rison, safe
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from flask_babel import ngettext
+from marshmallow import ValidationError
+
+from superset import app
+from superset.commands.exceptions import (
+ DatasourceNotFoundValidationError,
+ RolesNotFoundValidationError,
+)
+from superset.connectors.sqla.models import RowLevelSecurityFilter
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
+from superset.dao.exceptions import DAOCreateFailedError, DAOUpdateFailedError
+from superset.extensions import event_logger
+from superset.row_level_security.commands.bulk_delete import BulkDeleteRLSRuleCommand
+from superset.row_level_security.commands.create import CreateRLSRuleCommand
+from superset.row_level_security.commands.exceptions import RLSRuleNotFoundError
+from superset.row_level_security.commands.update import UpdateRLSRuleCommand
+from superset.row_level_security.schemas import (
+ get_delete_ids_schema,
+ RLSListSchema,
+ RLSPostSchema,
+ RLSPutSchema,
+ RLSShowSchema,
+)
+from superset.views.base_api import (
+ BaseSupersetModelRestApi,
+ requires_json,
+ statsd_metrics,
+)
+
+logger = logging.getLogger(__name__)
+
+
+class RLSRestApi(BaseSupersetModelRestApi):
+ datamodel = SQLAInterface(RowLevelSecurityFilter)
+ include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | {
+ RouteMethod.RELATED,
+ "bulk_delete",
+ }
+ resource_name = "rowlevelsecurity"
+ class_permission_name = "Row Level Security"
+ openapi_spec_tag = "Row Level Security"
+ method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP
+ allow_browser_login = True
+
+ list_columns = [
+ "id",
+ "name",
+ "filter_type",
+ "tables.id",
+ "tables.table_name",
+ "roles.id",
+ "roles.name",
+ "clause",
+ "changed_on_delta_humanized",
+ "group_key",
+ ]
+ order_columns = [
+ "name",
+ "filter_type",
+ "clause",
+ "changed_on_delta_humanized",
+ "group_key",
+ ]
+ add_columns = [
+ "name",
+ "description",
+ "filter_type",
+ "tables",
+ "roles",
+ "group_key",
+ "clause",
+ ]
+ show_columns = [
+ "name",
+ "description",
+ "filter_type",
+ "tables.id",
+ "tables.schema",
+ "tables.table_name",
+ "roles.id",
+ "roles.name",
+ "group_key",
+ "clause",
+ ]
+ search_columns = (
+ "name",
+ "description",
+ "filter_type",
+ "tables",
+ "roles",
+ "group_key",
+ "clause",
+ )
+ edit_columns = add_columns
+
+ show_model_schema = RLSShowSchema()
+ list_model_schema = RLSListSchema()
+ add_model_schema = RLSPostSchema()
+ edit_model_schema = RLSPutSchema()
+
+ allowed_rel_fields = {"tables", "roles"}
+ base_related_field_filters = app.config["RLS_BASE_RELATED_FIELD_FILTERS"]
Review Comment:
In #22526 we introduced `EXTRA_RELATED_QUERY_FILTERS ` which already has filters for `role` and `user`. I think it might make sense deprecate `RLS_BASE_RELATED_FIELD_FILTERS` and leverage the more global config parameter here by doing the following:
- assume that `EXTRA_RELATED_QUERY_FILTERS["role"]` also applies to RLS
- Add `table` to `EXTRA_RELATED_QUERY_FILTERS`.
I tried this and it nicely filtered the roles and tables to match what's available on the datasets page and elsewhere:
```python
from superset.views.base import DatasourceFilter
from superset.views.filters import BaseFilterRelatedRoles
base_related_field_filters = {
"tables": [["id", DatasourceFilter, lambda: []]],
"roles": [["id", BaseFilterRelatedRoles, lambda: []]],
}
```
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org