Permission in PUBLIC role removes API permission from ADMIN role.

[rodmaz]

🐛 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,
...

4. Now add permission can read on Dashboard in the role Public.

5. 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

• superset version: 2.1.0 and 2.1.1rc2

• python version: 3.8.17

• node.js version: v16.20.0

• I have checked the superset logs for python stacktraces and included it here as text if there are any.

• I have reproduced the issue with at least the latest released version of superset.

• I have checked the issue tracker for the same issue and I haven't found one similar.

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]

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]

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]

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]

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]

@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]

@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]

@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]

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 <#24837 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIUQUZMAEQD3LI25IYGDN6TYOGPI7AVCNFSM6AAAAAA24AUQIWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBZHEZTOMJZHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ss-ravi]

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]

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]

@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.
#26470

This issue also seems to be related:
#25890

[rodmaz]

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]

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

[rodmaz]

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]

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

[ss-ravi]

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

[rusackas]

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]

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

[lbuchli]

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

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" ]

#!/bin/bash
superset fab create-admin --username "admin" --firstname Superset --lastname Admin --email "admin@superset.com" --password "admin"
cat > /tmp/public-role-perms.json <<EOF
[{"name": "Public", "permissions": [
    {"permission": {"name": "can_read"}, "view_menu": {"name": "Dashboard"}}
]}]
EOF
superset fab import-roles -p /tmp/public-role-perms.json
superset db upgrade
superset superset init
/bin/sh -c /usr/bin/run-server.sh

test script (will succeed if bug is there)

import requests
import json

login_data = {
    "password": "admin",
    "provider": "db",
    "refresh": True,
    "username": "admin"
}

session = requests.Session()
session.headers.update({
    'Accept': 'Application/json',
    'Content-Type': 'Application/json',
    'Referer': "http://superset:8088/",
})


r = session.post(
    "http://superset:8088/api/v1/security/login",
    data=json.dumps(login_data)
)
assert r.ok
tokens = r.json()
session.headers.update({'Authorization': f"Bearer {tokens['access_token']}"})
r = session.get("http://superset:8088/api/v1/security/csrf_token")
assert r.ok
session.headers.update({'X-CSRFToken': r.json()["result"]})


dashboard_data = json.dumps({
    "slug": "test-slug",
    "dashboard_title": "This is a test dashboard",
    "is_managed_externally": False,
    "json_metadata": "{ \"chart_configuration\": {} }",
    "owners": [],
    "published": True,
    "position_json": "{}",
})

r = session.post("http://superset:8088/api/v1/dashboard/", data=dashboard_data)
assert r.ok

# now superset still thinks that there is no dashboard (we don't have perms to see it bcs of https://github.com/apache/superset/issues/24837)
assert session.get("http://superset:8088//api/v1/dashboard/").json()["count"] == 0

# ... but if we try to add one, it complains that one exists already
r = session.post("http://superset:8088//api/v1/dashboard/", data=dashboard_data)
assert r.status_code == 422  # UNPROCESSABLE ENTITY
assert r.json() == {
    "message": {
        "slug": ["Must be unique"]
    }
}

[kleky]

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.

[FynnMazurkiewicz]

TLDR: Patch available!

This bug is still present in version 4.0.1. The workaround suggested by @lbuchli did not work for me either.

Bug summary
Whenever you set the "can_read" permission of "Dashboard" as part of the public permissions, the API of that endpoint will start ignoring the JWT entirely and always assume that the public user makes the request. In fact, this bug not only occurs for this endpoint, but for all endpoints. As soon as any item is part of the public read permissions, the JWT is ignored. This breaks the JWT authorization, as the API ignores any given token and always assumes public user permissions. For example, the dashboard endpoint will always return only dashboards available to the public user, even if you supply a JWT Bearer token of a user that has access to more dashboards (such as the admin role).

Root cause
I have completed a root cause analysis of this bug. It is caused by an issue in FlaskAppBuilder, the underlying framework also built by the lovely @dpgaspar. In the security decorator _protect(), that is called to set the authorization context on any endpoint, there is a special condition set for items that are considered public (meaning the public permissions can read them). I'll quote the relevant parts for discussion:

flask_appbuilder/security/decorators.py:98

# Check if the resource is public
if current_app.appbuilder.sm.is_item_public(permission_str, class_permission_name):
    return f(self, *args, **kwargs)

# if no browser login then verify JWT
if not (self.allow_browser_login or allow_browser_login):
    verify_jwt_in_request()

# Verify resource access
if current_app.appbuilder.sm.has_access(permission_str, class_permission_name):
    return f(self, *args, **kwargs)

In this decorator, it is first checked whether the item is public and if it is, the endpoint function is immediately called - with no further JWT token handling. You can see the function call to verify_jwt_in_request() is only called when the public condition check fails.

I am not sure if this can be considered a bug in FAB itself, so I won't open a bug issue there. I do see however, that this might cause issues a lot and I highly question this order is correct. @dpgaspar What do you think?

Patch
Until there is a proper fix implemented by @dpgaspar or any other maintainer, its possible to hack around this by attempting a JWT login by overwriting is_item_public in the SecurityManager and adding a call to verify_jwt_in_request there.

from flask_jwt_extended import verify_jwt_in_request
from superset.security import SupersetSecurityManager
class MySecurityManager(SupersetSecurityManager):
    ...
    def is_item_public(self, permission_name, view_name):
        verify_jwt_in_request(optional=True) # Attempt to parse any existing JWT and fail silently
        return super().is_item_public(permission_name, view_name)
    ...