henry3260 commented on PR #63604: URL: https://github.com/apache/airflow/pull/63604#issuecomment-4062553388
> Hey @henry3260, nice improvement — replacing the raw `dict[str, Any]` claims with a typed Pydantic model makes the token structure self-documenting and catches malformed tokens earlier. A few things I wanted to flag: > > **The Cadwyn version change may not have any effect.** `TIToken` is an internal security dependency injected via FastAPI's DI system — it's not an API request or response model. Cadwyn's `schema()` migration instructions only apply to models that appear in route signatures. Since `TIToken` never shows up in any endpoint's request/response body, `ValidateTaskIdentityTokenClaims` would be inert. All other version changes in the codebase operate on actual API models (`DagRun`, `TIRunContext`, etc.). I'd suggest removing the version change to avoid giving the impression that `TIToken` is part of the versioned API surface. > > **`TokenScope` duplicates `TokenType` in security.py.** The PR introduces `TokenScope = Literal["execution", "workload"]` in `token.py`, but `security.py:86` already has `TokenType = Literal["execution", "workload"]` with `VALID_TOKEN_TYPES = frozenset(get_args(TokenType))`. If someone adds a new token type, they'd need to update both. Worth unifying these into a single source of truth. > > **The error message for invalid scopes changes.** Previously, an invalid scope produced a clean "Invalid token scope" message from `require_auth`. Now it's caught earlier in `JWTBearer.__call__` and produces a Pydantic `ValidationError` dump, which is less human-readable. Not a blocker since it's still a 403, but worth being aware of for debugging. > > **A couple of test gaps:** > > * No test for the primary validation benefit — a `sub` that's not a valid UUID being rejected by the Pydantic `UUID` type. That's arguably the most valuable thing the typed schema adds. > * No test for `TIClaims` rejecting incomplete claim sets (e.g., missing `exp`). PyJWT would catch this first in production, but a unit test for the model itself would document the contract. > > The core change is solid and the `extra="allow"` config is the right call for forward compatibility. Just needs the version change reconsidered and the type duplication cleaned up. Indeed, We don't intend to include `TIToken `in Cadwyn's version facet. It's just that any changes to the files under `execution_api/datamodels/` will force the hook (`check-execution-api-versions`) to add a corresponding record to `execution_api/versions/`, even if the model doesn't appear in any route's request/response. To avoid creating a project that "looks like a version change" but actually has no external impact, I've kept `token.py` in its original `TokenScope` definition and haven't introduced a new name. This eliminates the need for a Cadwyn version entry and prevents the misconception that TIToken is part of the external version facet. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
