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

feat(AIP-84): Add auth for ui config #47487

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Mar 7, 2025

Why

#42360

blocked by #47208 merged


^ 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:API Airflow's REST/HTTP API label Mar 7, 2025
@Lee-W Lee-W changed the title Feature/aip 84/auth/UI/config feat(AIP-84): Add auth for ui config Mar 7, 2025
@Lee-W Lee-W force-pushed the feature/AIP-84/auth/ui/config branch from 822b7aa to fcd084f Compare March 7, 2025 09:34
@Lee-W Lee-W marked this pull request as ready for review March 7, 2025 09:35
@Lee-W Lee-W added the full tests needed We need to run full set of tests for this PR to merge label Mar 7, 2025
@Lee-W Lee-W force-pushed the feature/AIP-84/auth/ui/config branch from fcd084f to 66b26b2 Compare March 7, 2025 10:09
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.

We need to adapt the UI, because this will break it.

The config call from the UI is done before loading the layout (router level), and therefore after successful authentication, we do not reach the code that handle the token and issue a ui/config query before that, resulting in a 401 loop. 😬

@pierrejeambrun
Copy link
Member

I'll take a look to try to unblock this, thanks for the PR.

@Lee-W Lee-W marked this pull request as draft March 7, 2025 14:42
@Lee-W
Copy link
Member Author

Lee-W commented Mar 7, 2025

got it. Thanks @pierrejeambrun . let me mark it as draft first

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Mar 10, 2025

#47562 this will fix the issue and needs to be merged first. Tested with this PR change and mine, things are working as expected.

@pierrejeambrun pierrejeambrun marked this pull request as ready for review March 10, 2025 12:29
@Lee-W
Copy link
Member Author

Lee-W commented Mar 10, 2025

#47562 this will fix the issue and needs to be merged first. Tested with this PR change and mine, things are working as expected.

I tried to take a look... but not familiar with that part.... I will rebase once that's merged. Thanks!

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Mar 10, 2025

#47562 merged! rebased

@pierrejeambrun pierrejeambrun force-pushed the feature/AIP-84/auth/ui/config branch from 2351ba4 to 7337430 Compare March 10, 2025 16:23
@pierrejeambrun
Copy link
Member

Static check need fixing (maybe you should run precommit locally to avoid this, especially when the API CI is full test -> very long)

@Lee-W
Copy link
Member Author

Lee-W commented Mar 10, 2025

hmmm... weird. changed nothing since the last CI green.

@Lee-W Lee-W force-pushed the feature/AIP-84/auth/ui/config branch from 7337430 to 7a1fa41 Compare March 11, 2025 00:01
@Lee-W
Copy link
Member Author

Lee-W commented Mar 11, 2025

nothing can be fixed by running pre-commit locally 🤔 let me push it again and see how it works

@Lee-W
Copy link
Member Author

Lee-W commented Mar 11, 2025

As this is approved and CI is green, gonna merge it

@Lee-W Lee-W added the AIP-84 Modern Rest API label Mar 11, 2025
@Lee-W Lee-W merged commit 74d7625 into apache:main Mar 11, 2025
90 checks passed
@Lee-W Lee-W deleted the feature/AIP-84/auth/ui/config branch March 11, 2025 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-84 Modern Rest API area:API Airflow's REST/HTTP API full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants