apache / superset

Apache Superset is a Data Visualization and Data Exploration Platform
https://superset.apache.org/
Apache License 2.0
62.7k stars 13.84k forks source link

Permission in PUBLIC role removes API permission from ADMIN role. #24837

Open rodmaz opened 1 year ago

rodmaz commented 1 year ago

:bug: Something strange is happening to Superset v2.1.0 regarding permissions. Why adding a permission like can read on dashboards for role Public removes this permission for role Admin?

How to reproduce the bug

  1. Make sure Public role has NO permission.
  2. Using curl and a token from an Admin user, query the API for available dashboards:
curl --location 'http://localhost:8088/api/v1/dashboard/' \
--header 'Authorization: Bearer xxxxx'

We should get a list of current available dashboards. (So far so good).

{
    "count": 10,
    "description_columns": {},
    "ids": [
        9,
        10,
        8,
...
  1. Now add permission can read on Dashboard in the role Public.
Screenshot 2023-07-28 at 17 28 06
  1. Perform the same query as step 2 and (surprisingly!) we see no more Dashboards in the API.
{
  "count": 0, 
  "description_columns": {}, 
  "ids": [], 
  "label_columns"
  ...

Expected results

Adding permission in Public role should not affect users using the API with role Admin.

Actual results

Adding permission in role Public REMOVES permission from users with role Admin.

Environment

Additional context

Public is the role defined for guest users:

# Embedded config options
GUEST_ROLE_NAME = "Public"
GUEST_TOKEN_JWT_SECRET = "test-guest-secret-change-me"
GUEST_TOKEN_JWT_ALGO = "HS256"
GUEST_TOKEN_HEADER_NAME = "X-GuestToken"
GUEST_TOKEN_JWT_EXP_SECONDS = 300  # 5 minutes
# Guest token audience for the embedded superset, either string or callable
GUEST_TOKEN_JWT_AUDIENCE: Optional[Union[Callable[[], str], str]] = None
mdeshmu commented 1 year ago

It is not recommended to modify default roles. You should clone the default roles and make modifications to them. Are you able to replicate this in cloned roles?

rodmaz commented 1 year ago

We are only adding permissions to the Public role since it is a requirement when we embed dashboards. That’s the overall approach documented. We just didn’t expect it to break the API. Public role is our guest role.

Should we create a new role for guest users instead?

rodmaz commented 1 year ago

We managed to fix this by using another custom role for the GUEST_ROLE_NAME and leaving PUBLIC alone. Still odd, unexpected behavior but this works fine.

ahmed-adly-khalil commented 1 year ago

i'm having same issue, why adding permission to the public rule removes it from the admin and on the api level only? !!

@rodmaz can you reopen this issue? creating custom role works for users who login to superset UI, however it doesn't for embedded dashboards because embedded has to use the public role and enabled permissions for public role basically disables the API access to any resource

nytai commented 1 year ago

@ahmed-adly-khalil Embedded doesn't have the use the Public role, you can configure it to use whichever role you want with GUEST_ROLE_NAME as @rodmaz has done. This definitely seems like an issue, but there is a workaround.

@dpgaspar any thoughts on what might be happening here?

ahmed-adly-khalil commented 1 year ago

@nytai Yeah GUEST_ROLE_NAME fixed the problem for me and i can do both dashboard embedding and use the API, thanks a lot. I still think this is a bug and it's great to be fixed since this behaviour is mostly unexpected

tcape commented 10 months ago

@rodmaz @ahmed-adly-khalil Could either of you please provide your config.py and superset_config.py values you have set? I cannot get embedding and API to both work. It's always either one or the other.

ahmed-adly-khalil commented 10 months ago

Just use a different role for embracing not public

On Fri, Jan 12, 2024 at 3:54 PM Tim Capehart @.***> wrote:

@rodmaz https://github.com/rodmaz @ahmed-adly-khalil https://github.com/ahmed-adly-khalil Could either of you provide your config.py and superset_config.py values you have set? I cannot get embedding and API to both work. It's always either one or the other.

— Reply to this email directly, view it on GitHub https://github.com/apache/superset/issues/24837#issuecomment-1889937198, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIUQUZMAEQD3LI25IYGDN6TYOGPI7AVCNFSM6AAAAAA24AUQIWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBZHEZTOMJZHA . You are receiving this because you were mentioned.Message ID: @.***>

ss-ravi commented 9 months ago

I have the same problem. In my case I need the can read on Chart permission in the public role so I can embed the chart in my app without having to login. But when I add this to the role, the /chart api stops working. For me, the GUEST_ROLE_NAME workaround does not seem to work, as the embed chart functionality does not use the guest token (and there is no sdk for it). Does someone know what can I do to solve it?

rusackas commented 6 months ago

Just checking in to see if anyone can recapitulate the issue here. I'm tempted to close it as stale, but it's probably gone stale because it's hard to tell if there's a real bug here, or if this is a question of how to configure roles for embedding (in which case, it should be a Discussion, not an Issue). Thoughts?

ss-ravi commented 6 months ago

@rusackas Adding permissions to the Public role makes the api requests using the Admin role stop working, so it does look like a bug. I remember I also make an issue about that. Feel free to close it as duplicate if it is going to be handled in this one. https://github.com/apache/superset/issues/26470

This issue also seems to be related: https://github.com/apache/superset/issues/25890

rodmaz commented 6 months ago

This is a well described and rather serious bug I would say, when users add permissions for role Public removes permissions for role Admin.

rusackas commented 6 months ago

@rodmaz a serious bug on 2.1.0, or have you upgraded since then?

rodmaz commented 6 months ago

We haven't upgraded yet but plan to do so soon. I would say, if this is not a bug it should be definitely better documented. Adding permissions in one role ending up removing permissions from another is not something users really expect TBH.

rusackas commented 6 months ago

Agreed... it's definitely a bug if it's happening in supported versions. That just needs to be validated.

ss-ravi commented 6 months ago

@rusackas I haven't tried it on the 4.0.0 version, but it does happen on 3.0.2.

rusackas commented 5 months ago

Good to know... but we currently support 3.1.x and 4.0.x for the time being. If anyone can validate using those versions, that would be even better still.

aibunny commented 5 months ago

For superset version 3.1.x I didn't experience this issue but for 4.0.x I am experiencing this issue.

lbuchli commented 4 months ago

I played around a bit with this and found a hacky solution, which could also be a pointer to implementing better solution:

In superset_config.py:

from superset.security.manager import SupersetSecurityManager
from flask import Flask
from flask_login import login_user
from flask_jwt_extended import JWTManager

class CustomSecurityManager(SupersetSecurityManager):

    def create_jwt_manager(self, app: Flask) -> JWTManager:
        def _load_user_jwt(_jwt_header, jwt_data):
            user = self.load_user_jwt(_jwt_header, jwt_data)
            login_user(user)  # sets g.user to jwt provided user
            return user
        jwt_manager = JWTManager()
        jwt_manager.init_app(app)
        jwt_manager.user_lookup_loader(_load_user_jwt)
        return jwt_manager

CUSTOM_SECURITY_MANAGER = CustomSecurityManager

I'm by no means an expert with flask permission stuff, so I cannot say whether this is a good idea from a security point of view. In short, it is a patch for this function, which sets g.user, although this has no effect for some reason I haven't looked into. Instead using flask_logins login_user, it works.

I also have absolutely no clue why this problem only pops up when setting PUBLIC role perm can read on Dashboard.

I hope this helps somebody and if someone thinks that this solution is a bad idea, please say so.

This is tested with apache/superset:4.0.1, using the following setup:

Setup

```Dockerfile FROM apache/superset:4.0.1 USER root RUN pip install psycopg2-binary COPY ./superset-init.sh /superset-init.sh COPY superset_config.py /app/ ENV SUPERSET_CONFIG_PATH /app/superset_config.py USER superset ENTRYPOINT [ "/superset-init.sh" ] ``` ```bash #!/bin/bash superset fab create-admin --username "admin" --firstname Superset --lastname Admin --email "admin@superset.com" --password "admin" cat > /tmp/public-role-perms.json <

kleky commented 2 months ago

I can confirm this occurs on 4.0.0 and is causing us serious pain. We sync DB schema from Cube Cloud to Superset, which uses the REST API. The workaround by changing the GUEST_ROLE_NAME worked sometimes, but not on other instances and it makes no sense why.