Skip to content

Change logic in auth with swagger ui - #53597

Merged
jason810496 merged 7 commits into
apache:mainfrom
kyungjunleeme:feature/temp-fix-422
Aug 1, 2025
Merged

Change logic in auth with swagger ui#53597
jason810496 merged 7 commits into
apache:mainfrom
kyungjunleeme:feature/temp-fix-422

Conversation

@kyungjunleeme

@kyungjunleemekyungjunleeme commented Jul 21, 2025

Copy link
Copy Markdown
Contributor

Descritpion

422 root cause related with this https://github.com/fastapi/fastapi/issues/1431

first)
I added this logic to manually convert the request body to a LoginBody instance when it’s received as a dictionary (e.g., from application/x-www-form-urlencoded).

changed my mind)
I think that airflow needs to support both application/json and application/x-www-form-urlencoded content types.

This helps prevent FastAPI from raising a 422 Unprocessable Entity error due to automatic validation failure.

#53567 related. (first this pr merged -> 53597)

image
  • "Using the RequestBody option in Swagger UI causes a 422 error."
image - "Only accept application/json content-type" image - swagger UI

Result

image - Requestbody image - Authroization header image - content-type: application/json, application/x-www-form-urlencoded

cf) https://github.com/koxudaxi/datamodel-code-generator


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@boring-cyborgboring-cyborgBot added the area:API Airflow's REST/HTTP API label Jul 21, 2025

@vincbeckvincbeck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Can you update tests to cover that?

@kyungjunleemekyungjunleeme changed the title Change logic in auth with swagger uIChange logic in auth with swagger uiJul 21, 2025
@kyungjunleeme
kyungjunleeme marked this pull request as draft July 22, 2025 02:04
@kyungjunleeme
kyungjunleeme marked this pull request as ready for review July 22, 2025 03:31
@kyungjunleeme

kyungjunleeme commented Jul 22, 2025

Copy link
Copy Markdown
ContributorAuthor

I found CI bug. -> I will make 2 PR

  1. id: generate-airflowctl-datamodelsFIX - pre-commit scope (generate-airflowctl-datamodels) #53624
image
  1. id: ts-compile-lint-simple-auth-manager-uiChange pre-commit propety(v1) #53636
image

ashb
ashb previously requested changes Jul 22, 2025

@ashbashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check out the settings we have in the Execution API sub-application.

Comment threadairflow-core/src/airflow/api_fastapi/auth/managers/simple/routes/login.py Outdated
@kyungjunleeme
kyungjunleeme marked this pull request as draft July 22, 2025 15:38
@kyungjunleeme

Copy link
Copy Markdown
ContributorAuthor

Reflections on the Feedback and Purpose of the PR

After receiving the feedback yesterday, I spent some time reflecting on Airflow's philosophy and current internal structure.
It helped me understand the intention behind the feedback more clearly.

From my perspective, there seems to be a distinction between the authentication mechanisms used for user-facing APIs and those used internally between components within Airflow.

The reason I created this PR was to improve the developer experience for users interacting with the Airflow API through Swagger UI, making it easier to test and trigger endpoints directly.

During this process, I noticed that a 422 Unprocessable Entity error was occurring when using application/x-www-form-urlencoded content type.
To address this, I made a small change to support form-based input without modifying Airflow’s core structure or authentication logic.
The goal was to make the authentication endpoint more user-friendly, especially for those exploring the API via tools like Swagger.

While working on this, I also observed that JWT token handling in Airflow is not yet fully unified across different parts of the system.
I believe this presents an opportunity to submit a follow-up PR that would refactor and unify the token handling logic,
ensuring consistency and paving the way for smoother integration of OAuth2.0 and SSO in the future.


(Note: I drafted my thoughts and structure in Korean first, based on my understanding of the feedback and the current state of the codebase.
ChatGPT helped only with polishing the English writing and markdown formatting — all technical reasoning and intent came from my own review.)

If there's anything I may have misunderstood, I’d really appreciate your clarification.

@ashb

@kyungjunleeme
kyungjunleeme marked this pull request as ready for review July 23, 2025 02:30
@kyungjunleeme
kyungjunleeme marked this pull request as draft July 23, 2025 05:48
@kyungjunleeme

kyungjunleeme commented Jul 23, 2025

Copy link
Copy Markdown
ContributorAuthor

Comment threadairflow-core/src/airflow/api_fastapi/auth/managers/simple/utils.py Outdated

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! LGTM.

@bugraoz93bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks @kyungjunleeme ! Small suggestion to speed up the procedures, please resolve any thread you think you addressed. This can make merging a lot easier since not everyone checking the PR has the full context of all of the threads :)

@kyungjunleeme

Copy link
Copy Markdown
ContributorAuthor

@bugraoz93 Thank you, I followed your instructions

@jason810496

jason810496 commented Aug 1, 2025

Copy link
Copy Markdown
Member

It seems that this PR still needs @ashb's approval before it can be merged.
(Due to GitHub restrictions, if any reviewer has requested changes, the PR requires that specific person's approval to unblock and proceed with the merge.)

Update: I can dismissed review from ash

@kyungjunleeme

Copy link
Copy Markdown
ContributorAuthor

Ah ha thank you for telling me! :)

@jason810496
jason810496 merged commit ff36fb5 into apache:mainAug 1, 2025
103 checks passed
ferruzzi pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Aug 7, 2025
* ADD: test code for application/x-www-form-urlencoded
* ADD: FORM
* ADD: header_content_type_json_or_form_depends
* NEW: parse_login_body
* CHG: create_token
* DEL: parametrize
fweilun pushed a commit to fweilun/airflow that referenced this pull request Aug 11, 2025
* ADD: test code for application/x-www-form-urlencoded
* ADD: FORM
* ADD: header_content_type_json_or_form_depends
* NEW: parse_login_body
* CHG: create_token
* DEL: parametrize
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:APIAirflow's REST/HTTP API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@kyungjunleeme@jason810496@ashb@bugraoz93@vincbeck