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

Use next_url instead of next #37225

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Conversation

vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Feb 7, 2024

Resolves #37224


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

@potiuk potiuk added this to the Airflow 2.8.2 milestone Feb 7, 2024
@vincbeck vincbeck merged commit daa2bce into apache:main Feb 7, 2024
57 checks passed
@vincbeck vincbeck deleted the vincbeck/next_url branch February 7, 2024 16:47
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Feb 20, 2024
ephraimbuddy pushed a commit that referenced this pull request Feb 20, 2024
(cherry picked from commit daa2bce)
ephraimbuddy pushed a commit that referenced this pull request Feb 22, 2024
(cherry picked from commit daa2bce)
@csp33
Copy link
Contributor

csp33 commented Mar 5, 2024

@vincbeck
This PR breaks the fix I implemented in #36833

Behavior when using next_url:

Grabacion.de.pantalla.2024-03-05.a.las.9.57.36.mov

Behavior when using next:

Grabacion.de.pantalla.2024-03-05.a.las.9.56.15.mov

Cc: @ephraimbuddy @abhishekbhakat @potiuk

@vincbeck
Copy link
Contributor Author

vincbeck commented Mar 5, 2024

I dont think it breaks what you did. It seems there is a bug I agree, but I dont think this PR breaks #36833.

EDIT: Hmmm, potentially indeed. My bad

@vincbeck
Copy link
Contributor Author

vincbeck commented Mar 5, 2024

Indeed. The issue is:

  • next should be used when used with url_for function
  • next_url should be used with the get_url_login method

Let me fix that mess, we should always use next for both cases. That will simplify the code.

@vincbeck
Copy link
Contributor Author

vincbeck commented Mar 5, 2024

Fix here: #37904. I could not update the signature of get_url_login, that would break backward compatibility. But it is fine, we jsut need to remember to use next_url with get_url_login and next with url_for.

@csp33, could you test it please?

@jamiekt
Copy link

jamiekt commented Feb 28, 2025

Hello,
We are running airflow 2.8.4 which should contain the fix in #37904 however we're still seeing the same behaviour. We suspect that the reason is that that fix applies to the FAB auth manager whereas we are using some other auth manager. When we login to to our airflow instances we authenticate via Microsoft Entra (aka Azure Active Directory).
If anyone knows where to go to fix this when authenticating via Entra I'd be very grateful if you could let me know. Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:fab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants