AIP-84 | Add Auth for Import Error #47270
Conversation
boring-cyborg
bot
added
area:API
jason810496
force-pushed
the
feature/AIP-84/auth/import_error
branch
from
98c6342 to
06de40c
Compare
jason810496
requested review from
ephraimbuddy and
pierrejeambrun
as code owners
|
@jason810496 looks like the on-going discussion has been resolved. Could you please confirm? if that's the case, let's update the description 🙂 |
Oh yes, the discussion I mentioned has been resolved. |
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
Looks good, need the dagId filter indeed.
Lee-W
force-pushed
the
feature/AIP-84/auth/import_error
branch
from
06de40c to
8de5122
Compare
|
just rebase with the latest main, we should be able to use |
Lee-W
force-pushed
the
feature/AIP-84/auth/import_error
branch
from
8de5122 to
53d25ff
Compare
|
Hey @jason810496 , just rebased and resolved conflicts. will you have some bandwidth to wrap it up? if not, I'll take a look tomorrow. Thanks! |
I might not be able to do it today, but I will be available tomorrow to work on it. I will ping you before I start working on it! |
|
No worries. Just want to ensure we don’t work on the same thing. Will comment before I start working on it as well 🙂 |
|
Hi @Lee-W I start working on it. |
jason810496
force-pushed
the
feature/AIP-84/auth/import_error
branch
from
53d25ff to
ada4066
Compare
Lee-W
force-pushed
the
feature/AIP-84/auth/import_error
branch
from
bdc8188 to
cd4e2db
Compare
Lee-W
force-pushed
the
feature/AIP-84/auth/import_error
branch
from
cd4e2db to
3a30770
Compare
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
Nice, just a few suggestions, but looking good.
jason810496
force-pushed
the
feature/AIP-84/auth/import_error
branch
from
3a30770 to
98c11f2
Compare
|
Thanks @Lee-W and @pierrejeambrun for the review, I just resolved all comments and refactored the logic in router and the test as well. |
Lee-W
left a comment
Lee-W
left a comment
There was a problem hiding this comment.
I did a quick review. The test is failing. I'll try to fix it later today if it hasn't been addressed yet. Thanks!
Lee-W
force-pushed
the
feature/AIP-84/auth/import_error
branch
3 times, most recently
from
2d852fb to
d30e259
Compare
- Remove requires_access_dag depends
- Refactor get_file_dag_ids helper
- Rename get_permitted_dag_ids to get_authorized_dag_ids
- Refactor get_import_errors
- Early return if the user has access to all DAGs
- Add subquery to get corresponding file_dag_ids for each fileloc in single query
- Make test more readable by adding separate comment - Add set auth_manager attribute helper - Refactor test setup
Lee-W
force-pushed
the
feature/AIP-84/auth/import_error
branch
2 times, most recently
from
bbc3c17 to
4cf5554
Compare
Lee-W
force-pushed
the
feature/AIP-84/auth/import_error
branch
from
4cf5554 to
0741dab
Compare
|
Thanks @Lee-W for the final test refactor ! |
* AIP-84 | Add Auth for Import Error
* fix: remove outdated requires_access_view
* Add permitted dags with import_error
* Add test for import error
* Refactor import_error
- Remove requires_access_dag depends
- Refactor get_file_dag_ids helper
- Rename get_permitted_dag_ids to get_authorized_dag_ids
- Refactor get_import_errors
- Early return if the user has access to all DAGs
- Add subquery to get corresponding file_dag_ids for each fileloc in single query
* Refactor test_import_error
- Make test more readable by adding separate comment
- Add set auth_manager attribute helper
- Refactor test setup
* Remove get_file_dag_ids helper
* Remove 403 in api doc, early return for get_import_error router
* feat(import_error): rewrite groupby logic as array_agg is only supported in pg
* test(api_fastapi): refactor test cases
* style: fix type checking
---------
Co-authored-by: Wei Lee <weilee.rx@gmail.com>
* AIP-84 | Add Auth for Import Error
* fix: remove outdated requires_access_view
* Add permitted dags with import_error
* Add test for import error
* Refactor import_error
- Remove requires_access_dag depends
- Refactor get_file_dag_ids helper
- Rename get_permitted_dag_ids to get_authorized_dag_ids
- Refactor get_import_errors
- Early return if the user has access to all DAGs
- Add subquery to get corresponding file_dag_ids for each fileloc in single query
* Refactor test_import_error
- Make test more readable by adding separate comment
- Add set auth_manager attribute helper
- Refactor test setup
* Remove get_file_dag_ids helper
* Remove 403 in api doc, early return for get_import_error router
* feat(import_error): rewrite groupby logic as array_agg is only supported in pg
* test(api_fastapi): refactor test cases
* style: fix type checking
---------
Co-authored-by: Wei Lee <weilee.rx@gmail.com>
4 participants
related: #42360
What
I have added the
requires_access_dagdependency for the Import Error endpoints as well.However, the implementation still depends on the ongoing discussion in #47062 (comment). ( I'm not sure if such granularity is needed for the Simple Auth Manager. If it is, I will add the necessary logic forget_readable_dagsas well. )For reference, here are the Import Error implementations in the Legacy API:https://github.com/apache/airflow/blob/v2-10-test/airflow/api_connexion/endpoints/import_error_endpoint.py
Update:
Yes, we should respect
readable_dag_ids, so this PR is still depends on AIP-84 | Add Auth for Dags #47433 as it introduceReadableDagsFilterDep.