AIP-81 | AIP-84 | Include Token Generation Endpoints in FAB #47043

bugraoz93 · 2025-02-24T22:40:11Z

closes: #46916
relates: #47055

Remaining Work:

In between making this global and implementing custom expiration logic for fab. - Done
Move it to a new structure with FastAPI routes, datamodels. - Done
Unit tests. - Done

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

vincbeck

I might miss some context here but do we need to make these changes in all auth managers in regards to AIP-81? I thought we were going to use simple auth manager as auth manager for AIP-81 but from this PR I guess we want to use the auth manager configured in the environment?

providers/fab/src/airflow/providers/fab/auth_manager/api_endpoints/login.py

providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py

providers/fab/src/airflow/providers/fab/auth_manager/api_endpoints/login.py

bugraoz93 · 2025-02-25T19:02:32Z

I might miss some context here but do we need to make these changes in all auth managers in regards to AIP-81? I thought we were going to use simple auth manager as auth manager for AIP-81 but from this PR I guess we want to use the auth manager configured in the environment?

Thanks a lot for the early feedback! :)

For AIP-81, in the end, people will get their tokens from profile pages but if they are using FAB, they won't be able to use CLI. Since the integration with FastAPI is now happening in FAB, I thought, why not support both if we can at the same time.

bugraoz93 · 2025-02-25T22:35:09Z

The skeleton is here now. Please check when you have time Vincent.
If it looks good, I will move on with integrating into FastAPI. Otherwise (preferably), I can do it in the next PR maybe, it would be cleaner and easier to review.

providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/services/login.py

bugraoz93 · 2025-02-25T23:44:39Z

I will check the tests

.pre-commit-config.yaml

providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py

vincbeck

A doc in FAB provider would super useful to explain to users how to use this API in order to get a token. This will be the only way for users to call Rest API so it will be definitely super useful!

airflow/auth/managers/base_auth_manager.py

providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py

bugraoz93 · 2025-02-27T19:33:00Z

A doc in FAB provider would super useful to explain to users how to use this API in order to get a token. This will be the only way for users to call Rest API so it will be definitely super useful!

Thanks for the quick reviews! I will address them soon. I agree, it would be really useful, I will also include documentation. This will be kind of a breaking change in the API usage. Should we include newsfragments too? That would be also useful to point users to check the documentation.

I am surprised that the unit tests are fine in my local and not in the CI. I need to check the environment and dependencies and maybe image rebuild.

vincbeck · 2025-02-27T19:44:44Z

A doc in FAB provider would super useful to explain to users how to use this API in order to get a token. This will be the only way for users to call Rest API so it will be definitely super useful!

Thanks for the quick reviews! I will address them soon. I agree, it would be really useful, I will also include documentation. This will be kind of a breaking change in the API usage. Should we include newsfragments too? That would be also useful to point users to check the documentation.

I am surprised that the unit tests are fine in my local and not in the CI. I need to check the environment and dependencies and maybe image rebuild.

Newsfragment is a good idea :)

bugraoz93 · 2025-03-02T19:26:04Z

I will fix the static check and test soon. It seems it still needs a couple of touches

providers/fab/docs/auth-manager/api-authentication.rst

providers/fab/tests/unit/fab/auth_manager/api_fastapi/routes/test_login.py

…specs to pre-commit, exclude openapi specs from licence check, include unit tests

…with methods, remove refresh token logic from FAB

…enapi specs to pre-commit

… comparison to keep out of API scope, update docstring, update default values directly from conf, remove not necessary imports

…est checking providers module have tests, fix docs

bugraoz93 · 2025-03-03T20:48:27Z

Thanks for the review and all the comments, Vincent! :)

vincbeck

LGTM. I think the documentation can be updated and improved based on the discussion I started here. But that can be done in a separate PR

bugraoz93 · 2025-03-03T21:39:18Z

LGTM. I think the documentation can be updated and improved based on the discussion I started here. But that can be done in a separate PR

Thanks! I was thinking about documentation and whether should we wait for the discussion you started or not. I agree, let's improve it iteratively whenever it is needed. I can create follow-ups

vincbeck · 2025-03-03T22:47:12Z

The errors are unrelated to this PR. Merging

…7043)

boring-cyborg bot added the provider:fab label Feb 24, 2025

bugraoz93 added the area:auth label Feb 24, 2025

bugraoz93 changed the title ~~[WIP] Include CLI Token Renewal endpoint in FAB~~ [WIP] AIP-81 Include CLI Token Renewal endpoint in FAB Feb 24, 2025

bugraoz93 mentioned this pull request Feb 25, 2025

AIP-84 Support Auth Backends for API authentication #47055

Closed

1 task

vincbeck reviewed Feb 25, 2025

View reviewed changes

bugraoz93 force-pushed the feat/46916/expiration-config-cli-fab branch from b78c71a to 00a2416 Compare February 25, 2025 22:30

bugraoz93 changed the title ~~[WIP] AIP-81 Include CLI Token Renewal endpoint in FAB~~ [WIP] AIP-81 Include Token Renewal endpoints in FAB Feb 25, 2025

bugraoz93 changed the title ~~[WIP] AIP-81 Include Token Renewal endpoints in FAB~~ AIP-81 Include Token Renewal endpoints in FAB Feb 25, 2025

bugraoz93 marked this pull request as ready for review February 25, 2025 22:37

bugraoz93 requested review from ashb, jedcunningham and potiuk as code owners February 25, 2025 22:37

bugraoz93 force-pushed the feat/46916/expiration-config-cli-fab branch from 00a2416 to 8bfd861 Compare February 25, 2025 23:15

bugraoz93 commented Feb 25, 2025

View reviewed changes

providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/services/login.py Outdated Show resolved Hide resolved

vincbeck reviewed Feb 26, 2025

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py Outdated Show resolved Hide resolved

bugraoz93 force-pushed the feat/46916/expiration-config-cli-fab branch 2 times, most recently from 29629a2 to 24703c0 Compare February 27, 2025 02:04

vincbeck reviewed Feb 27, 2025

View reviewed changes

bugraoz93 changed the title ~~AIP-81 Include Token Renewal endpoints in FAB~~ AIP-81 | AIP-84 | Include Token Generation Endpoints in FAB Feb 28, 2025

bugraoz93 force-pushed the feat/46916/expiration-config-cli-fab branch from 24703c0 to 5ebf893 Compare March 2, 2025 18:27

bugraoz93 force-pushed the feat/46916/expiration-config-cli-fab branch from 5ebf893 to 1d64d2e Compare March 2, 2025 20:08

vincbeck reviewed Mar 3, 2025

View reviewed changes

bugraoz93 added 2 commits March 3, 2025 21:43

Include token CLI Token renewal endpoint

615f887

Remove added comment, Move to New FastAPI structure, add new openapi …

511f68e

…specs to pre-commit, exclude openapi specs from licence check, include unit tests

bugraoz93 added 10 commits March 3, 2025 21:43

Fix unit tests

c26650c

Fix pre-commit changes

c2008f6

Accept credentials and authenticate user, update simple auth manager …

4716d23

…with methods, remove refresh token logic from FAB

Run generate openapi specs and include Simple and FAB Auth Manager op…

66163e7

…enapi specs to pre-commit

Fix password comparison logic, use security manager to check password…

f6127db

… comparison to keep out of API scope, update docstring, update default values directly from conf, remove not necessary imports

Update SimpleAuthManager default to auth_jwt_expiration_time conf

dc50a32

Initial documentation update and adding newsfragment

24ea93c

Fix unit test mock check_password, add datamodel as an exception to t…

82e0b63

…est checking providers module have tests, fix docs

Mark test as db tests

f2cda5f

Fix document and include assert in tests

92cbcae

bugraoz93 force-pushed the feat/46916/expiration-config-cli-fab branch from e896c77 to 92cbcae Compare March 3, 2025 20:45

vincbeck approved these changes Mar 3, 2025

View reviewed changes

vincbeck merged commit 54016ec into apache:main Mar 3, 2025
85 of 88 checks passed

shahar1 pushed a commit to shahar1/airflow that referenced this pull request Mar 5, 2025

AIP-81 | AIP-84 | Include Token Generation Endpoints in FAB (apache#4…

f72c135

…7043)

nailo2c pushed a commit to nailo2c/airflow that referenced this pull request Apr 4, 2025

AIP-81 | AIP-84 | Include Token Generation Endpoints in FAB (apache#4…

a2f676a

…7043)

This was referenced Apr 6, 2025

Status of testing Providers that were prepared on April 06, 2025 #48842

Closed

Status of testing Providers that were prepared on April 10, 2025 #49066

Closed

AIP-81 | AIP-84 | Include Token Generation Endpoints in FAB #47043

AIP-81 | AIP-84 | Include Token Generation Endpoints in FAB #47043

Uh oh!

Conversation

bugraoz93 commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincbeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bugraoz93 commented Feb 25, 2025

Uh oh!

bugraoz93 commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bugraoz93 commented Feb 25, 2025

Uh oh!

Uh oh!

Uh oh!

vincbeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bugraoz93 commented Feb 27, 2025

Uh oh!

vincbeck commented Feb 27, 2025

Uh oh!

bugraoz93 commented Mar 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bugraoz93 commented Mar 3, 2025

Uh oh!

vincbeck left a comment

Choose a reason for hiding this comment

Uh oh!

bugraoz93 commented Mar 3, 2025

Uh oh!

vincbeck commented Mar 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bugraoz93 commented Feb 24, 2025 •

edited

Loading

bugraoz93 commented Feb 25, 2025 •

edited

Loading