Consolidate HTTP 401/403 Responses for Public API Routes#43932
Consolidate HTTP 401/403 Responses for Public API Routes#43932kaxil merged 2 commits intoapache:mainfrom
Conversation
kaxil
requested review from
bbovenzi,
ephraimbuddy,
jscheffl,
pierrejeambrun and
ryanahamilton
as code owners
There was a problem hiding this comment.
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
|
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 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
I will take a stab at it as a separate PR |
|
Here is one attempt: #43990 |
Moved 401 & 403 responses to top-level router so we can avoid having to add it too all the public api routers
* 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Jed Cunningham <[email protected]>
5 participants
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.rstor{issue_number}.significant.rst, in newsfragments.