feat: add support for extra rlsf #107

Ian2012 · 2023-06-29T13:57:41Z

Description

This PR allows importing extra row-level security filters, which in combination with #123 will allow users to have granular control over what users can see.

Use cases:

Bootstrap a new dev environment with custom filters
Call an external API, perform validations, and verify user permissions
Use data from some external DB to control which data a user has access to.
Among others

openedx-webhooks · 2023-06-29T13:57:46Z

Thanks for the pull request, @Ian2012! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

bmtcril

This seems ok to me, but @pomegranited knows a lot more about it than I do

pomegranited

@Ian2012 This is working fine, and a good addition to the config.

But I have some questions about the documentation, and a suggestion for the implementation. What do you think?

tutoraspects/templates/aspects/apps/superset/pythonpath/create_row_level_security.py

README.rst

pomegranited · 2023-07-03T08:37:42Z

tutoraspects/templates/aspects/apps/superset/pythonpath/create_row_level_security.py

-assert openedx_role, "{{SUPERSET_OPENEDX_ROLE_NAME}} role doesn't exist yet?"
-
-for (schema, table_name, group_key, clause, filter_type) in (
+row_level_security_filters = [
    (
        "{{ASPECTS_XAPI_DATABASE}}",
        "{{ASPECTS_XAPI_TABLE}}",


I see we actually do have a new datasources for transcript_usage and video_plays now.. don't we need RLSFs for them too?

And so maybe the details for these filters should all live in the tutor config, so here we can iterate over SUPERSET_ROW_LEVEL_SECURITY_FILTERS + SUPERSET_EXTRA_ROW_LEVEL_SECURITY_FILTERS?

Thanks for the review, I agree in all points

Ian2012 · 2023-07-04T15:09:49Z

@pomegranited it seems like tutor doesn't render nested fields, so this:

"SUPERSET_BASE_ROW_LEVEL_SECURITY_FILTERS", {
  "schema": "{{ ASPECTS_XAPI_DATABASE }}",
  "table_name": "{{ ASPECTS_XAPI_TABLE }}",
  "role_name": "{{ SUPERSET_OPENEDX_ROLE_NAME }}",
  "group_key": "{{ SUPERSET_ROW_LEVEL_SECURITY_XAPI_GROUP_KEY }}",
  "clause": "{{can_view_courses(current_username(),"
  "\"splitByChar(\"/\", course_id)[-1]\")}}",
  "filter_type": "Regular",
},

Is rendered as raw strings.

BASE_SECURITY_FILTERS = [
    (
        "{{ ASPECTS_XAPI_DATABASE }}",
        "{{ ASPECTS_XAPI_TABLE }}",
        "{{ SUPERSET_OPENEDX_ROLE_NAME }}",
        "{{ SUPERSET_ROW_LEVEL_SECURITY_XAPI_GROUP_KEY }}",
        '{{can_view_courses(current_username(),"splitByChar("/", course_id)[-1]")}}',
        "Regular",
    ),
]

This should be refactored, but I will go back to the old approach. And mark it for improvement.

Ian2012 · 2023-07-04T16:15:26Z

@pomegranited @bmtcril I've used a patch to add base security filters that will support interpolation, so I guess the setting SUPERSET_EXTRA_ROW_LEVEL_SECURITY_FILTERS is not needed anymore, because one can use the tutor patch to accomplish the same result and use the same "api" to define rlsf

pomegranited

@Ian2012 I started reviewing this too but ran out of time.. I'd specifically like to test the SUPERSET_EXTRA_JINJA_FEATURES patch you've documented to make sure it actually works.

But here's some comments, thanks for this!

README.rst

pomegranited · 2023-07-05T03:38:40Z

README.rst

+..  code-block:: python
+
+    def can_view_courses(username, field_name="course_id"):


Don't we need to provide yaml here?

Suggested change

.. code-block:: python

def can_view_courses(username, field_name="course_id"):

.. code-block:: python

SUPERSET_EXTRA_JINJA_FILTERS:

custom_can_view_courses: |

def can_view_courses(username, field_name="course_id"):

And so I think the rest of this code block needs to be indented too?

I was not clear with this documentation, I corrected and added a missing step for register the jinja filter using SUPERSET_EXTRA_JINJA_FILTERS

Ian2012 · 2023-07-05T21:29:12Z

@pomegranited This is ready for review, I will assign proper rlsf to the other datasets in another PR with some changes to the permissions

bmtcril

Just a couple of documentation nits. I tested this by creating a plugin with both patches, installing it, init-ing superset, and confirming that the sql was appended to the queries appropriately by both seeing the data change and by viewing the chart SQL.

bmtcril · 2023-07-06T20:42:11Z

README.rst

+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+If you add new datasources, tables, or fields to Superset, you may want to add new `row level security filters`_ 
+to restrict access to that data based on people's course roles. To apply custom row level security filters 


nit: "to restrict access to that data based on things like course roles, or organization"

Since you can use these on any column.

bmtcril · 2023-07-06T20:44:15Z

README.rst

+    superset-row-level-security: |
+        {
+            "schema": "{{ASPECTS_XAPI_DATABASE}}",
+            "table_name": "{{ASPECTS_XAPI_TABLE}}",


We should be really clear that the schema / table need to exist in the assets.yaml, so they can't filter on upstream tables that don't end up getting imported to Superset. I just fought with this for 30 mins trying to figure out why it thought a table didn't exist. Maybe a note about when you see AssertionError: {schema.table} table doesn't exist yet? to make sure it's exported to Superset.

bmtcril · 2023-07-06T20:58:39Z

Just a heads up on this: #153 I think it's probably a more general issue of "how do we deal with deleted assets" but I wanted to track it.

Ian2012 · 2023-07-07T17:48:16Z

@bmtcril ready, I cannot merge it

openedx-webhooks · 2023-07-07T20:57:33Z

@Ian2012 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 29, 2023

Ian2012 force-pushed the cag/extend-row-level-security branch 5 times, most recently from 274a9ba to 72a85fb Compare June 29, 2023 20:45

Ian2012 marked this pull request as ready for review June 29, 2023 20:45

Ian2012 force-pushed the cag/extend-row-level-security branch 4 times, most recently from 9cc35d5 to d72abc3 Compare June 29, 2023 20:56

bmtcril reviewed Jun 30, 2023

View reviewed changes

pomegranited requested changes Jul 3, 2023

View reviewed changes

Ian2012 force-pushed the cag/extend-row-level-security branch 4 times, most recently from 1bd1f80 to fac03dd Compare July 4, 2023 14:12

Ian2012 force-pushed the cag/extend-row-level-security branch 3 times, most recently from 5e14622 to 458eac4 Compare July 4, 2023 16:12

pomegranited reviewed Jul 5, 2023

View reviewed changes

itsjeyd added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Jul 5, 2023

Ian2012 force-pushed the cag/extend-row-level-security branch 4 times, most recently from ac87942 to c2b6fa7 Compare July 5, 2023 21:15

bmtcril approved these changes Jul 6, 2023

View reviewed changes

bmtcril mentioned this pull request Jul 6, 2023

bug: Removing custom Superset assets #153

Open

feat: add support for extra rlsf

892cba8

Ian2012 force-pushed the cag/extend-row-level-security branch from c2b6fa7 to 892cba8 Compare July 7, 2023 17:47

bmtcril merged commit d23c9ba into main Jul 7, 2023

bmtcril deleted the cag/extend-row-level-security branch July 7, 2023 20:57

itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for extra rlsf #107

feat: add support for extra rlsf #107

Ian2012 commented Jun 29, 2023 •

edited

Loading

openedx-webhooks commented Jun 29, 2023 •

edited

Loading

bmtcril left a comment

pomegranited left a comment

pomegranited Jul 3, 2023

Ian2012 Jul 3, 2023

Ian2012 commented Jul 4, 2023 •

edited

Loading

Ian2012 commented Jul 4, 2023

pomegranited left a comment

pomegranited Jul 5, 2023

Ian2012 Jul 5, 2023

Ian2012 commented Jul 5, 2023

bmtcril left a comment

bmtcril Jul 6, 2023

bmtcril Jul 6, 2023

bmtcril commented Jul 6, 2023

Ian2012 commented Jul 7, 2023

openedx-webhooks commented Jul 7, 2023

		.. code-block:: python

		def can_view_courses(username, field_name="course_id"):

feat: add support for extra rlsf #107

feat: add support for extra rlsf #107

Conversation

Ian2012 commented Jun 29, 2023 • edited Loading

Description

Use cases:

openedx-webhooks commented Jun 29, 2023 • edited Loading

bmtcril left a comment

Choose a reason for hiding this comment

pomegranited left a comment

Choose a reason for hiding this comment

pomegranited Jul 3, 2023

Choose a reason for hiding this comment

Ian2012 Jul 3, 2023

Choose a reason for hiding this comment

Ian2012 commented Jul 4, 2023 • edited Loading

Ian2012 commented Jul 4, 2023

pomegranited left a comment

Choose a reason for hiding this comment

pomegranited Jul 5, 2023

Choose a reason for hiding this comment

Ian2012 Jul 5, 2023

Choose a reason for hiding this comment

Ian2012 commented Jul 5, 2023

bmtcril left a comment

Choose a reason for hiding this comment

bmtcril Jul 6, 2023

Choose a reason for hiding this comment

bmtcril Jul 6, 2023

Choose a reason for hiding this comment

bmtcril commented Jul 6, 2023

Ian2012 commented Jul 7, 2023

openedx-webhooks commented Jul 7, 2023

Ian2012 commented Jun 29, 2023 •

edited

Loading

openedx-webhooks commented Jun 29, 2023 •

edited

Loading

Ian2012 commented Jul 4, 2023 •

edited

Loading