[GitHub] [incubator-superset] justin-barton commented on issue #8699: [SIP-29] Add support for row-level security

[GitBox <gi...@apache.org>]

justin-barton commented on issue #8699: [SIP-29] Add support for row-level security
URL: https://github.com/apache/incubator-superset/pull/8699#issuecomment-560147328
 
 
   > I think it is important to support belonging to multiple roles early on. Think AD/LDAP in a corporate setting; not uncommon to belong to hundreds of groups. Regarding implementation, I would propose just adding a column "role_based_filters" or similar to the tables table with the metadata:
   > 
   > ```json
   > {
   >   "dept": {
   >     "default": "false",
   >     "roles": {
   >       "finance": "dept_id = 1",
   >       "risk": "dept_id = 2"
   >     }
   >   },
   >   "duration": {
   >     "default": "report_date >= current_timestamp() - 1",
   >     "roles": {
   >       "finance": "report_date >= current_timestamp() - 30"
   >     }
   >   }
   > }
   > ```
   > 
   > In this example, users that don't belong to any group would get a WHERE clause that returns zero rows due to the "false" clause (`SELECT col FROM table WHERE FALSE` -> no rows), and by default only the last days data would be available. If the user belongs to the "risk" Role, they would see only "dept_id = 2" for the last day (default clause for "duration"), whereas "finance" would see "dept_id = 1" for the last 30 days. Belonging to both would return data for both departments with 30 days of data.
   > 
   > One could later add the same column to the charts table, making it possible to introduce the same functionality on a per chart basis. With regards to the filter statements, I would propose using the same filter format that's currently used for `adhoc_filters`, which would enable us to leverage existing React components that allow for a much more user friendly means to add filters. To introduce the functionality, I would break the SIP into two parts; first introducing the backend functionality, i.e. adding the new column to table, making it possible to edit the filters by poking at the table metadata, and later adding proper UI functionality for editing the metadata.
   
   @villebro I've been looking through your suggestions and I believe that there are a number of potential bugs / unintended consequences:
   
   1. When someone belongs to multiple groups, setting the default behaviour to OR together all the permissions seems like a dangerous assumption. This errs on the side of being maximally permissive, which is usually not advisable. The current setup forces the user to define what happens at intersections by creating another role for any combined roles and explicitly setting the clauses.
   
   2. Even if we were to OR together permissions, using a key-based JSON approach would likely not be the right one. By way of example, let's say that on table `exports`: 
   ```
   Role A has filters: country='Freedonia' AND item='Apples'
   Role B has filters: country='Ruritania' AND item='Oranges'
   ```
   For someone that has Role A and Role B, ORing together at a key-level would give the user access to not only reports on Freedonia Apple exports and Ruritania Orange exports (as you might expect) but also Ruritania Apple exports and Freedonia Orange exports (which may not be expected/desired in a lot of use cases).
   
   3. Having users directly manage large JSON objects to control row-based security is unduly cumbersome and inaccessible to some users, so a UI / SQL approach is preferable.
   
   4. The concept of defaults (and how to use them) again requires assumptions that will not fit a lot of use cases. For example, your proposed setup only works where the default is more restrictive than all of the roles. If there were a case where the default was to show last 15 days (`report_date >= current_timestamp() - 15`) but one of the roles was to restrict to only seeing the most recent day's data (`report_date >= current_timestamp() - 1`) then your setup would actually escalate permissions.
   
   5. The proposal to break the SIP into two parts, the first being only a back-end change to tables, means that most users will not have use of row-level security when this is released, which defeats the purpose of the SIP.
   
   
   In answer to the above, I would suggest one of the following two approaches:
   
   A. Accept the pull request as-is, continuing to force users to explicitly define the permissions for any intersection cases (since there aren't any defaults that will work in all scenarios and that would lead to predictable behaviour for the user)
   
   B. Modify the current pull request to allow for roles to be combined by ORing together the entire clause in brackets, e.g.: 
   `Role A & Role B: WHERE (country='Freedonia' AND item='Apples') OR (country='Ruritania' AND item='Oranges')`
   
   With a preference for A.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org