Vamsi-klu commented on PR #63604: URL: https://github.com/apache/airflow/pull/63604#issuecomment-4061377000
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. -- 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]
