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.

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

[SIP-29] Add support for row-level security #8644

Closed
justin-barton opened this issue Nov 24, 2019 · 45 comments
Closed

[SIP-29] Add support for row-level security #8644

justin-barton opened this issue Nov 24, 2019 · 45 comments
Labels
enhancement:request Enhancement request submitted by anyone from the community inactive Inactive for >= 30 days sip Superset Improvement Proposal

Comments

@justin-barton
Copy link

[SIP-29] Proposal to add support for row-level security

Motivation

Many BI applications, particularly in multi-tenancy scenarios, require support for row-level security. That is, the ability to show different slices of a table to users based on some user attribute.

This feature has been requested in Superset several times, including:
#660
#5128
#7887
#8023

Proposed Change

Primarily, this feature would require two sub-features:

  1. The ability to add user attributes
  2. The ability to filter queries based on these attributes

The proposed solution would be to add an 'attributes' property to roles that would essentially be a user-defined key-value store:
Screenshot 2019-11-24 02 35 54

and to make those role attributes available as values for Query Filters:
Screenshot 2019-11-24 02 38 26

New or Changed Public Interfaces

  • New 'attributes' property added to roles
  • Changes to Query Filters to allow access to attribute values (e.g by macros like WHERE column={{role.attributes.key}}
  • Minor UI changes associated with the above

New dependencies

N/A

Migration Plan and Compatibility

TBD

Rejected Alternatives

Pre-processing data into multiple tables or views:

  • This solution does not scale well, and requires all attribute values to be known a priori
  • Requires duplicate dashboards and charts

Reverse proxy

  • Issues with identifying the user initiating the query
@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label #enhancement to this issue, with a confidence of 0.98. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@issue-label-bot issue-label-bot bot added the enhancement:request Enhancement request submitted by anyone from the community label Nov 24, 2019
@axuew
Copy link
Contributor

axuew commented Nov 25, 2019

I also need this function very much. At present, I solve this problem by developing custom data permissions through jinja template

@toop
Copy link
Contributor

toop commented Nov 25, 2019

There are security problems in this plan: Use query filters to allow access to attribute values ,The current login user can delete the filter conditions in the query. If you do not give explore permission, the current user will lack the explore interaction function.

@thunter009
Copy link
Contributor

I also need this function very much. At present, I solve this problem by developing custom data permissions through jinja template

Can you elaborate on your approach?

@altef
Copy link
Contributor

altef commented Nov 27, 2019

I've been looking into this with an eye toward implementing it, using a slightly different approach:

Proposed Change

  1. Add a new model to describe row level security filters, which references a Table and a Role. So when adding a row level security filter, you specify a particular Role and Table.

  2. Add the filters in to the query when applicable. I've modified the query function here to add any relevant to the WHERE clause.

  3. Create a UI for row level security filters. I haven't started on this yet, in case I'm missing something and this method proves ill-advised.

New or Changed Public Interfaces

  • New row level security filters interface

New dependencies

N/A

Migration Plan and Compatibility

TBD

It all seems to be working as expected at this point. Does anyone foresee any obvious issues with this method?

@justin-barton
Copy link
Author

I've been looking into this with an eye toward implementing it, using a slightly different approach:

Proposed Change

  1. Add a new model to describe row level security filters, which references a Table and a Role. So when adding a row level security filter, you specify a particular Role and Table.
  2. Add the filters in to the query when applicable. I've modified the query function here to add any relevant to the WHERE clause.
  3. Create a UI for row level security filters. I haven't started on this yet, in case I'm missing something and this method proves ill-advised.

New or Changed Public Interfaces

  • New row level security filters interface

New dependencies

N/A

Migration Plan and Compatibility

TBD

It all seems to be working as expected at this point. Does anyone foresee any obvious issues with this method?

This sounds ideal. @toop any concerns with this revised approach?

@axuew
Copy link
Contributor

axuew commented Nov 30, 2019

@thunter009 I basically follow @altef and define a model to store the roles and data permissions of the account. For some roles, the data permissions are not controlled, while the normal permissions control the data of the query according to the roles and data permissions.You end up adding this custom permission control to the front of the filter, and then interacting with the data you see on the screen.
However, direct control through tables is not currently supported, only through sqllab

@axuew
Copy link
Contributor

axuew commented Nov 30, 2019

@altef :I think just adding a component to the diagram (as shown in the initial screenshot of @justin-barton ) that is controlled by permissions is a good solution to the user interaction and security issues.
👍

@altef
Copy link
Contributor

altef commented Nov 30, 2019

Proposed solution

  1. Row level security filters added to the Security menu:
    menu

  2. Row level security list
    list

  3. Row level security interface
    edit

That allows for the management of the RowLevelSecurityFilters model. Additionally, for convenience, I've added it as a related view for tables.

table-related-view

After logging in with a user assigned to that role, I can still supply additional filters:

additional-filters

The generated SQL includes the additional filters AND the clause supplied by the row level security filter(s).

generated-sql

Everything seems to be working as expected on my end, and I'd be happy to provide a pull request if no one has any objections to this implementation?

@axuew
Copy link
Contributor

axuew commented Nov 30, 2019

@altef How do you resolve different client_ids for the same role in your current solution?

@altef
Copy link
Contributor

altef commented Nov 30, 2019

@axuew Different clients would just have different Roles assigned to them. Each client can potentially have any number of users, and all would be assigned the role for that client.
However, since the clause can be whatever you want injected into the query's WHERE, it should be fairly flexible and you can handle things however suits you.

@axuew
Copy link
Contributor

axuew commented Nov 30, 2019

@altef Your solution is good enough so far, but I recommend that you use the expression client_id={g.u.ser.id.dp} in a model that stores different ids for users to resolve the complexity of permission configuration.

@altef
Copy link
Contributor

altef commented Nov 30, 2019

@axuew I'm sorry, I don't think I follow. Could you rephrase?

In our case this method seems suitable because the field we check against can vary by table and is often a string. So the filter's clause would often be something like advertiser_name IN ("foo", "bar"), for roles who should have access to advertisers foo and bar.

@toop
Copy link
Contributor

toop commented Nov 30, 2019

I've been looking into this with an eye toward implementing it, using a slightly different approach:

Proposed Change

  1. Add a new model to describe row level security filters, which references a Table and a Role. So when adding a row level security filter, you specify a particular Role and Table.
  2. Add the filters in to the query when applicable. I've modified the query function here to add any relevant to the WHERE clause.
  3. Create a UI for row level security filters. I haven't started on this yet, in case I'm missing something and this method proves ill-advised.

New or Changed Public Interfaces

  • New row level security filters interface

New dependencies

N/A

Migration Plan and Compatibility

TBD
It all seems to be working as expected at this point. Does anyone foresee any obvious issues with this method?

This sounds ideal. @toop any concerns with this revised approach?

Agree to this revised !This good ideal !!!
Some proposals :
1、Dynamic multi conditional support, such as:“department = {user}.department,client_id =4”
2、Can support the datasources`s tables
3、Can add user attributes
4、FAB's empowerment view is too ugly and does not support 18in, improve FAB's authority view

Thanks!!!
@altef @justin-barton

@altef
Copy link
Contributor

altef commented Nov 30, 2019

Essentially, we're giving each role access to a subset of the table. So we could have one role: Company A, who has the clause company_id=12
And another role: Last 30 days, who has the clause date_field > DATE_SUB(NOW(), INTERVAL 30 DAY)

And for a specific user of Company A, if they should only be able to see the last 30 days worth of data, apply both roles.

Since a row level security filter's clause is arbitrary, it can also support multiple conditions: client_id = 6 AND advertiser="foo", etc. You can throw whatever you want in there to define the subset of the table you want the role in question to have access to.

@toop
Copy link
Contributor

toop commented Nov 30, 2019

We are looking forward to your revised !Thank you very much. @altef

@axuew
Copy link
Contributor

axuew commented Nov 30, 2019

@altef Your plan does solve all my problems and is flexible.However, if you configure one role for each user, it will be difficult to maintain because of the large number of users and roles. Therefore, I suggest to optimize the configuration for the user level after you merge the code

@stale
Copy link

stale bot commented Jan 30, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Jan 30, 2020
@altef
Copy link
Contributor

altef commented Jan 30, 2020

Update: I've created a pull request which adds this feature; working through the process of getting it accepted. I think we're close. Currently awaiting another review.

@stale stale bot removed the inactive Inactive for >= 30 days label Jan 30, 2020
@liangsieng
Copy link

hi, have this released?

@altef
Copy link
Contributor

altef commented Mar 13, 2020

@i2cute it's been merged in, i'm not sure how that'll be reflected in the release schedule though
(Apparently it's in 0.36.0rc1, according to durchgedreht)

@AleVzT
Copy link

AleVzT commented Apr 21, 2020

Proposed solution

  1. Row level security filters added to the Security menu:
    menu
  2. Row level security list
    list
  3. Row level security interface
    edit

That allows for the management of the RowLevelSecurityFilters model. Additionally, for convenience, I've added it as a related view for tables.

table-related-view

After logging in with a user assigned to that role, I can still supply additional filters:

additional-filters

The generated SQL includes the additional filters AND the clause supplied by the row level security filter(s).

generated-sql

Everything seems to be working as expected on my end, and I'd be happy to provide a pull request if no one has any objections to this implementation?

How can I add the filter_fila option in the security menu. I use version 0.27

@amitmiran137
Copy link
Member

use ENABLE_ROW_LEVEL_SECURITY = True in you superset_config.py file
@AleVzT

@AleVzT
Copy link

AleVzT commented Apr 27, 2020

utilizar ENABLE_ROW_LEVEL_SECURITY = Trueen su archivo superset_config.py
@AleVzT

the superset_config.py file does not exist in version 0.27. config.py is called but the ENABLE_ROW_LEVEL_SECURITY statement is missing

@AleVzT
Copy link

AleVzT commented Apr 27, 2020

@altef @amitNielsen @justin-barton

@amitmiran137
Copy link
Member

amitmiran137 commented Apr 27, 2020

The row level security feature only exist from version 0.36
And you also cherry-pick it with the following fix:
important role bug fix

Otherwise feature will just not work

@stale
Copy link

stale bot commented Jun 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Jun 26, 2020
@stale stale bot closed this as completed Jul 3, 2020
@xcoder123
Copy link

Hi,

I'm running the latest 0.36 version

But when I enable the ENABLE_ROW_LEVEL_SECURITY

Nothing appears in menu. With logger I can see the enable flag is set to true

I thought maybe I need to set permissions for my gropup, similar to list LogModelView, maybe there is list RowLevelSecurityView.

image

It doesn't seem to be the case

@altef
Copy link
Contributor

altef commented Jul 12, 2020

Hi @xcoder123 - does your user have the Admin role?

@xcoder123
Copy link

HI @altef

Yes indeed, my user is part of admin group.

What is interesting it doesn't show up in the menu:
image

One more thing I want to quickly note, this might relate to the issue.

I'm running the superset in docker, with pyodbc with MSSQL support.
In addition I have implemented the JWT token authorization that has been flowing around the git issue tracker.

Where I have my custom security manager. Though switching it off, doesn't seem to help.

Thank You

@altef
Copy link
Contributor

altef commented Jul 12, 2020

@xcoder123
Copy link

Hi @altef

I managed to find what was wrong. It was misconfigured docker-compose file from our side.

For some reason superset-init and superset-worker was commented out by me.

Thank you for quick response.

@altef
Copy link
Contributor

altef commented Jul 13, 2020

That is a relief - thanks for letting me know!

@redmonki
Copy link

redmonki commented Jul 21, 2020

@altef is the feature only enabled for queries executed from "charts"? I am not seeing the row level filters applied on the queries run from SQL Lab.

@mistercrunch
Copy link
Member

We should clarify the docs on that topic if it's not clear there.

@altef
Copy link
Contributor

altef commented Jul 21, 2020

It applies the restriction in connectors/sqla/models.py:get_sqla_query(), so unless SQL Lab uses that (I haven't used SQL Lab yet, but it seems unlikely) it probably doesn't.
How does SQL Lab work? Restricting arbitrary SQL would at the very least require parsing of that SQL so as to appropriately limit any subqueries and joins, in addition to the base table.

@vrnvikas
Copy link

vrnvikas commented Aug 7, 2020

@altef - I am facing a similar issue where I added the flag for enabling ROW_LEVEL in superset config but still I am not able to see this option enabled in UI. I tried it in charts also no luck there also. I tried this in 0.36 plus 0.37rc4 not able to get this working.

@altef
Copy link
Contributor

altef commented Aug 7, 2020

Hi @vrnvikas - just to be clear, did you use ROW_LEVEL or ENABLE_ROW_LEVEL_SECURITY?

@vrnvikas
Copy link

vrnvikas commented Aug 7, 2020

Hi @vrnvikas - just to be clear, did you use ROW_LEVEL or ENABLE_ROW_LEVEL_SECURITY?

I used "ENABLE_ROW_LEVEL_SECURITY" only.

@altef
Copy link
Contributor

altef commented Aug 7, 2020

I've heard a few people recently with similar issues1,2. It seems like running superset init has solved it for them!

@vrnvikas
Copy link

vrnvikas commented Aug 8, 2020

I've heard a few people recently with similar issues1,2. It seems like running superset init has solved it for them!

thanks @altef running superset init solved the issue.

@Nj-kol
Copy link
Contributor

Nj-kol commented Aug 20, 2020

Yes, by default ENABLE_ROW_LEVEL_SECURITY is False, if you change this, you must do superset init. This is because this feature creates some backend Database artifacts

@Kamakshi12
Copy link

After "ENABLE_ROW_LEVEL_SECURITY : True" and running superset init , it's not reflecting in superset. I am using superset version 0.37.2 and running this on a Linux system.

P.S. However I tried the same on a windows machine, it worked.
What could possibly make it work in Linux ?

@villebro
Copy link
Member

On more recent Superset versions, the config variable ENABLE_ROW_LEVEL_SECURITY has been moved to a FEATURE_FLAG called ROW_LEVEL_SECURITY. Please check UPGRADING.md for details.

@Kamakshi12
Copy link

As mentioned in "Updating.md", I have included "ROW_LEVEL_SECURITY : True" (also tried "ENABLE_ROW_LEVEL_SECURITY : True") under the Feature_Flag Dictionary. After doing so and running superset init also it is not reflecting in superset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement:request Enhancement request submitted by anyone from the community inactive Inactive for >= 30 days sip Superset Improvement Proposal
Projects
Status: Implemented / Done
Development

No branches or pull requests