Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate HTTP 401/403 Responses for Public API Routes #43932

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Nov 12, 2024

Moved 401 & 403 responses to top-level router so we can avoid having to add it too all the public api routers


^ 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 newsfragments.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Nov 12, 2024
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Indeed that's something I also thought of doing. That's a nice addition.

The only issue is that we have unauthenticated routes such as get_version, get_health.

Maybe we can just assume that they also need authentication from now on ? (That's breaking and we can document it here #43378

@ashb
Copy link
Member

ashb commented Nov 12, 2024

There's no problem with saying those endpoints return a 401/403 even if they don't tbh -- the error responses doesn't really mean much to clients if the endpoint never returns it.

And as for auth vs anon, there's another way you are meant to specify security/auth requirements in Open API spec, so this doesn't affect that

@pierrejeambrun
Copy link
Member

There's no problem with saying those endpoints return a 401/403 even if they don't tbh -- the error responses doesn't really mean much to clients if the endpoint never returns it.

And as for auth vs anon, there's another way you are meant to specify security/auth requirements in Open API spec, so this doesn't affect that

Are you referring to the security parameter in the openapi spec ? Because we are using that field in the legacy as a global one, not setting it on a per route basis. (We could do that in the new spec but that needs to be developed)

Also I do not fully agree, as a client if the document states that the endpoint can return 401 and 403, I expect it to be an authenticated endpoint, and I will try to provide credentials to it. Also I will also write code to handle those 401, 403 on the client side...for nothing. I just think it's not ideal, but I agree that's not really a big deal so if that sounds reasonable to you, I'm perfectly fine merging that. (That's also much easier for us :))

@jscheffl
Copy link
Contributor

There's no problem with saying those endpoints return a 401/403 even if they don't tbh -- the error responses doesn't really mean much to clients if the endpoint never returns it.

And as for auth vs anon, there's another way you are meant to specify security/auth requirements in Open API spec, so this doesn't affect that

Are you referring to the security parameter in the openapi spec ? Because we are using that field in the legacy as a global one, not setting it on a per route basis. (We could do that in the new spec but that needs to be developed)

Also I do not fully agree, as a client if the document states that the endpoint can return 401 and 403, I expect it to be an authenticated endpoint, and I will try to provide credentials to it. Also I will also write code to handle those 401, 403 on the client side...for nothing. I just think it's not ideal, but I agree that's not really a big deal so if that sounds reasonable to you, I'm perfectly fine merging that. (That's also much easier for us :))

To make the "open API endpoints" explicit could we add a "anonymous" router for these both endpoints and separate them on the top level from the authenticated ones?

Moved 401 & 403 responses to top-level router so we can avoid having to add it too all the public api routers
@kaxil
Copy link
Member Author

kaxil commented Nov 13, 2024

There's no problem with saying those endpoints return a 401/403 even if they don't tbh -- the error responses doesn't really mean much to clients if the endpoint never returns it.

And as for auth vs anon, there's another way you are meant to specify security/auth requirements in Open API spec, so this doesn't affect that

Are you referring to the security parameter in the openapi spec ? Because we are using that field in the legacy as a global one, not setting it on a per route basis. (We could do that in the new spec but that needs to be developed)
Also I do not fully agree, as a client if the document states that the endpoint can return 401 and 403, I expect it to be an authenticated endpoint, and I will try to provide credentials to it. Also I will also write code to handle those 401, 403 on the client side...for nothing. I just think it's not ideal, but I agree that's not really a big deal so if that sounds reasonable to you, I'm perfectly fine merging that. (That's also much easier for us :))

To make the "open API endpoints" explicit could we add a "anonymous" router for these both endpoints and separate them on the top level from the authenticated ones?

I will take a stab at it as a separate PR

@kaxil kaxil merged commit 0d49535 into apache:main Nov 13, 2024
52 checks passed
@kaxil kaxil deleted the move-401-403 branch November 13, 2024 21:38
@kaxil
Copy link
Member Author

kaxil commented Nov 13, 2024

Here is one attempt: #43990

amoghrajesh pushed a commit to astronomer/airflow that referenced this pull request Nov 14, 2024
Moved 401 & 403 responses to top-level router so we can avoid having to add it too all the public api routers
vatsrahul1001 added a commit to astronomer/airflow that referenced this pull request Nov 14, 2024
pierrejeambrun pushed a commit that referenced this pull request Nov 14, 2024
* AIP-84: Migrating GET Assets to fastAPI

* matching response to legacy

* Adding unit tests - part 1

* Update airflow/api_fastapi/common/parameters.py

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

* fixing the dag_ids filter

* fixing the dag_ids filter

* Adding unit tests - part 2

* fixing unit tests & updating parameter type

* review comments pierre

* fixing last commit

* fixing unit tests

* migrating get assets events endpoint to fastapi

* fixing test response

* Adding tests for filtering

* address review comments

* fixing test parametrize

* address review comments

* address review comments

* removing http 401 and 403 as its now added in  root router in #43932

---------

Co-authored-by: Amogh <amoghrajesh1999@gmail.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants