mirror of
https://github.com/langgenius/dify.git
synced 2026-05-09 21:28:25 +08:00
fix(openapi): /apps/permitted hardening + naming
- fail-fast on missing subject_email/subject_issuer for dfoe_ bearers (was silently coercing None -> empty string and sending a malformed query upstream). - document the has_more contract: total comes from EE inner-API unfiltered count; locally-dropped archived rows leave len(items) < limit even when has_more=True. - gate tenant-name lookup in /apps on non-empty rows so empty filter results skip the wasted scalar query. - rename AppListPermittedApi -> AppPermittedListApi for word-order consistency with AppPermittedListQuery. - tests: positive mode acceptance and explicit dfoa_ non-carrier assertion.
This commit is contained in:
parent
04ebf8a92f
commit
d1c1c04615
@ -1,11 +1,7 @@
|
||||
"""GET /openapi/v1/apps and per-app reads (single, parameters, describe).
|
||||
"""GET /openapi/v1/apps and per-app reads.
|
||||
|
||||
Read endpoints attach via `AppReadResource`, which stacks
|
||||
`validate_bearer + require_scope` as method_decorators. List order is
|
||||
innermost-first: `validate_bearer` is last in the list and ends up
|
||||
outermost, so it sets `g.auth_ctx` before `require_scope` reads it.
|
||||
The OAuth bearer pipeline is reserved for /run (which gates on
|
||||
webapp_auth ACL).
|
||||
Decorator order: `method_decorators` is innermost-first. `validate_bearer`
|
||||
is last → outermost → sets `g.auth_ctx` before `require_scope` reads it.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@ -44,9 +40,6 @@ from libs.oauth_bearer import (
|
||||
from models import App, Tenant
|
||||
from models.model import AppMode, Tag, TagBinding
|
||||
|
||||
# Shared decorator stack for `apps:read`-scoped endpoints. List order is
|
||||
# innermost-first; `validate_bearer` (last) wraps outermost so it sets
|
||||
# `g.auth_ctx` before `require_scope` reads it.
|
||||
_APPS_READ_DECORATORS = [
|
||||
require_scope(Scope.APPS_READ),
|
||||
validate_bearer(accept=ACCEPT_USER_ANY),
|
||||
@ -62,17 +55,13 @@ _EMPTY_PARAMETERS: dict[str, Any] = {
|
||||
|
||||
|
||||
class AppReadResource(Resource):
|
||||
"""Base for `/apps/<id>` read endpoints. Stacks bearer auth + scope check
|
||||
on every method, then exposes `_load()` so subclasses don't repeat the
|
||||
SSO-guard / app-load / membership-check ritual."""
|
||||
"""Base for per-app read endpoints; subclasses call `_load()` for SSO/membership/exists checks."""
|
||||
|
||||
method_decorators = _APPS_READ_DECORATORS
|
||||
|
||||
def _load(self, app_id: str) -> tuple[App, AuthContext]:
|
||||
ctx = g.auth_ctx
|
||||
# Per-app reads are account-only; SSO subjects 404 to avoid leaking
|
||||
# ID space (and dfoe_ already lacks apps:read scope, so this is
|
||||
# defensive against future scope changes).
|
||||
# Account-only; SSO subjects 404 (don't leak ID space).
|
||||
if ctx.subject_type != SubjectType.ACCOUNT or ctx.account_id is None:
|
||||
raise NotFound("app not found")
|
||||
|
||||
@ -151,12 +140,7 @@ class AppDescribeApi(AppReadResource):
|
||||
|
||||
|
||||
class AppListQuery(BaseModel):
|
||||
"""Query-param validator for `GET /openapi/v1/apps`.
|
||||
|
||||
`mode` is a closed set (AppMode) — invalid values surface as 422
|
||||
instead of returning silently-empty data. `workspace_id` is required;
|
||||
page / limit have numeric bounds; name / tag have length caps.
|
||||
"""
|
||||
"""`mode` is a closed enum — unknown values 422 instead of silently-empty data."""
|
||||
|
||||
workspace_id: str
|
||||
page: int = Field(1, ge=1)
|
||||
@ -173,8 +157,7 @@ class AppListApi(Resource):
|
||||
def get(self):
|
||||
ctx = g.auth_ctx
|
||||
if ctx.subject_type != SubjectType.ACCOUNT or ctx.account_id is None:
|
||||
# An account-required endpoint reachable only via dfoa_ in practice
|
||||
# (dfoe_ lacks apps:read). Defensive guard for future scope shifts.
|
||||
# Defensive: dfoe_ lacks apps:read today, but guard against future scope shifts.
|
||||
return PaginationEnvelope[AppListRow].build(page=1, limit=0, total=0, items=[]).model_dump(mode="json"), 200
|
||||
|
||||
try:
|
||||
@ -185,8 +168,6 @@ class AppListApi(Resource):
|
||||
workspace_id = query.workspace_id
|
||||
require_workspace_member(ctx, workspace_id)
|
||||
|
||||
tenant_name = db.session.execute(sa.select(Tenant.name).where(Tenant.id == workspace_id)).scalar_one_or_none()
|
||||
|
||||
page = query.page
|
||||
limit = query.limit
|
||||
mode = query.mode.value if query.mode else None
|
||||
@ -221,6 +202,12 @@ class AppListApi(Resource):
|
||||
.all()
|
||||
)
|
||||
|
||||
tenant_name: str | None = None
|
||||
if rows:
|
||||
tenant_name = db.session.execute(
|
||||
sa.select(Tenant.name).where(Tenant.id == workspace_id)
|
||||
).scalar_one_or_none()
|
||||
|
||||
items = [
|
||||
AppListRow(
|
||||
id=str(r.id),
|
||||
|
||||
@ -6,7 +6,7 @@ import sqlalchemy as sa
|
||||
from flask import g, request
|
||||
from flask_restx import Resource
|
||||
from pydantic import BaseModel, ConfigDict, Field, ValidationError
|
||||
from werkzeug.exceptions import UnprocessableEntity
|
||||
from werkzeug.exceptions import InternalServerError, UnprocessableEntity
|
||||
|
||||
from controllers.openapi import openapi_ns
|
||||
from controllers.openapi._models import (
|
||||
@ -28,11 +28,7 @@ from services.enterprise.app_permitted_service import list_permitted_apps
|
||||
|
||||
|
||||
class AppPermittedListQuery(BaseModel):
|
||||
"""Query-param validator for `GET /openapi/v1/apps/permitted`.
|
||||
|
||||
Strict — `extra='forbid'` rejects `workspace_id`, `tag`, and any other
|
||||
param not explicitly allowed for this surface. Returns 422 on violation.
|
||||
"""
|
||||
"""Strict (`extra='forbid'`) — rejects `workspace_id`/`tag`/etc. that are valid on /apps but not here."""
|
||||
|
||||
model_config = ConfigDict(extra="forbid")
|
||||
|
||||
@ -43,7 +39,7 @@ class AppPermittedListQuery(BaseModel):
|
||||
|
||||
|
||||
@openapi_ns.route("/apps/permitted")
|
||||
class AppListPermittedApi(Resource):
|
||||
class AppPermittedListApi(Resource):
|
||||
method_decorators = [
|
||||
require_scope(Scope.APPS_READ_PERMITTED),
|
||||
validate_bearer(accept=ACCEPT_USER_EXT_SSO),
|
||||
@ -57,9 +53,13 @@ class AppListPermittedApi(Resource):
|
||||
raise UnprocessableEntity(exc.json())
|
||||
|
||||
ctx = g.auth_ctx
|
||||
# Fail-fast: empty subject would silently corrupt the EE allow-list query.
|
||||
if not ctx.subject_email or not ctx.subject_issuer:
|
||||
raise InternalServerError("malformed external_sso bearer: missing subject identity")
|
||||
|
||||
page_result = list_permitted_apps(
|
||||
subject_email=ctx.subject_email or "",
|
||||
subject_issuer=ctx.subject_issuer or "",
|
||||
subject_email=ctx.subject_email,
|
||||
subject_issuer=ctx.subject_issuer,
|
||||
page=query.page,
|
||||
limit=query.limit,
|
||||
mode=query.mode.value if query.mode else None,
|
||||
@ -85,8 +85,7 @@ class AppListPermittedApi(Resource):
|
||||
for r in page_result.data:
|
||||
app = apps_by_id.get(r.app_id)
|
||||
if not app or app.status != "normal":
|
||||
# Allow-list referenced an app that no longer exists or was archived;
|
||||
# filter it out rather than emit a partial row.
|
||||
# Skip allow-list entries where the app is missing/archived.
|
||||
continue
|
||||
tenant = tenants_by_id.get(str(app.tenant_id))
|
||||
items.append(
|
||||
@ -95,7 +94,7 @@ class AppListPermittedApi(Resource):
|
||||
name=app.name,
|
||||
description=app.description,
|
||||
mode=app.mode,
|
||||
tags=[], # tags are tenant-scoped; not surfaced cross-tenant
|
||||
tags=[], # tenant-scoped; not surfaced cross-tenant
|
||||
updated_at=app.updated_at.isoformat() if app.updated_at else None,
|
||||
created_by_name=None, # cross-tenant author leak prevention
|
||||
workspace_id=str(app.tenant_id),
|
||||
@ -103,6 +102,7 @@ class AppListPermittedApi(Resource):
|
||||
)
|
||||
)
|
||||
|
||||
# total/has_more reflect the EE-side allow-list; len(items) may be < limit when local rows are dropped.
|
||||
env = PaginationEnvelope[AppListRow].build(
|
||||
page=query.page, limit=query.limit, total=page_result.total, items=items
|
||||
)
|
||||
|
||||
@ -42,3 +42,10 @@ def test_query_validates_mode_against_app_mode():
|
||||
def test_query_clamps_limit_at_max():
|
||||
with pytest.raises(ValidationError):
|
||||
AppPermittedListQuery.model_validate({"limit": 500})
|
||||
|
||||
|
||||
def test_query_accepts_valid_mode():
|
||||
"""Pin the happy path: AppMode values pass."""
|
||||
q = AppPermittedListQuery.model_validate({"mode": "chat"})
|
||||
assert q.mode is not None
|
||||
assert q.mode.value == "chat"
|
||||
|
||||
@ -17,3 +17,13 @@ def test_dfoe_token_kind_carries_apps_read_permitted():
|
||||
registry = build_registry(MagicMock(), MagicMock())
|
||||
dfoe = next(k for k in registry.kinds() if k.prefix == "dfoe_")
|
||||
assert Scope.APPS_READ_PERMITTED in dfoe.scopes
|
||||
|
||||
|
||||
def test_dfoa_token_kind_does_not_carry_apps_read_permitted():
|
||||
"""dfoa_ relies on Scope.FULL umbrella; the explicit permitted scope
|
||||
is reserved for dfoe_."""
|
||||
from libs.oauth_bearer import Scope, build_registry
|
||||
|
||||
registry = build_registry(MagicMock(), MagicMock())
|
||||
dfoa = next(k for k in registry.kinds() if k.prefix == "dfoa_")
|
||||
assert Scope.APPS_READ_PERMITTED not in dfoa.scopes
|
||||
|
||||
Loading…
Reference in New Issue
Block a user