Set JWT token to localStorage from cookies

[gopidesupavan]

closes #47080

I added the JWT to localStorage. Since the Axios interceptor already retrieves the token from localStorage in requests here https://github.com/apache/airflow/blob/main/airflow/ui/src/main.tsx#L50 , I thought it would be a good place to update localStorage after a LoginResponse. This way, subsequent redirects will consider the updated token from localStorage.

Tested:

Login with redirect to connections

After login redirected to connections page with Authorization header.

---

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

[gopidesupavan]

Happy to take any other suggestion and will apply, i am not very good with front end stuff :)

[pierrejeambrun]

Nice.

To successfully close the issue, we need to do the same for other AuthManagers (FabAuthManager, etc...).

Also we need to remove the front-end code that handles the token from the URL as it will not be needed anymore. (in /airflow/airflow/ui/src/layouts/BaseLayout.tsx)

[gopidesupavan]

Nice.

To successfully close the issue, we need to do the same for other AuthManagers (FabAuthManager, etc...).

Also we need to remove the front-end code that handles the token from the URL as it will not be needed anymore. (in /airflow/airflow/ui/src/layouts/BaseLayout.tsx)

cool will update it :)

[vincbeck]

That is going to be hard to do it for other managers such as FAB auth manager. The front-end is controlled and generated by Flask itself. That's why I used querystring to pass the token but I agree that is a bad idea in terms of security. I do not necessarily have a solution though

[vincbeck]

The concern I have this solution as well is that each auth manager should then save the JWT token in local storage. I do not think this is a good idea. I'd like core Airflow to do it, not auth managers. Ideally I would like to have the JWT token in the request and then Airflow saving it. As described in #47080, instead of putting the JWT token in querystring, why dont we try in headers?

[gopidesupavan]

The concern I have this solution as well is that each auth manager should then save the JWT token in local storage. I do not think this is a good idea. I'd like core Airflow to do it, not auth managers. Ideally I would like to have the JWT token in the request and then Airflow saving it. As described in #47080, instead of putting the JWT token in querystring, why dont we try in headers?

Yes vincent, something headers would be good idea, i am trying this option, will update.

[gopidesupavan]

I have tried to use headers, but no luck, ended up doing with html render, If this is fine please let me know.

[ashb]

What about other auth managers? How do they set the token?

Did we consider setting the token in a cookie that the "main" app (not the UI for the specific auth manager) then deletes once it's put in to local storage? That way the approach can be universal, rather than each auth manager needing to do something similar.

[gopidesupavan]

What about other auth managers? How do they set the token?

Did we consider setting the token in a cookie that the "main" app (not the UI for the specific auth manager) then deletes once it's put in to local storage? That way the approach can be universal, rather than each auth manager needing to do something similar.

That also sounds like good plan ash, was only tried with headers. But let me check cookies as well.

[gopidesupavan]

Have removed now search params token extraction as it not required i think anymore.

[vincbeck]

What about other auth managers? How do they set the token?

Did we consider setting the token in a cookie that the "main" app (not the UI for the specific auth manager) then deletes once it's put in to local storage? That way the approach can be universal, rather than each auth manager needing to do something similar.

Strongly agree on this one. Auth managers should pass somehow the JWT token and then core Airflow saves it. We should not rely on auth managers to save the JWT tokens, this would create some problems in the long run

[gopidesupavan]

What about other auth managers? How do they set the token?
Did we consider setting the token in a cookie that the "main" app (not the UI for the specific auth manager) then deletes once it's put in to local storage? That way the approach can be universal, rather than each auth manager needing to do something similar.

Strongly agree on this one. Auth managers should pass somehow the JWT token and then core Airflow saves it. We should not rely on auth managers to save the JWT tokens, this would create some problems in the long run

The auth manager sets the token in cookies, and the main app retrieves it using document.cookie and stores it in localStorage. Then, it sets the expiration date for the token cookie. Does this align the expectation ?

[ashb]

The auth manager sets the token in cookies, and the main app retrieves it using document.cookie and stores it in localStorage. Then, it sets the expiration date for the token cookie. Does this align the expectation ?

Ideally the auth_manager will return the token to the main app so that it can set the cookie. If that is not possible then we at least need to document the requirement that "if you write a custom auth manager, it must set cookie X"

[vincbeck]

I think this change fine now. except the tests failures related to k8s are unstable, even in CI its failing. will try to look.

Those errors are unrelated to this PR, you can ignore them

[gopidesupavan]

I think this change fine now. except the tests failures related to k8s are unstable, even in CI its failing. will try to look.

Those errors are unrelated to this PR, you can ignore them

Okay cool, will wait for anyone to look, as this changes also include in front end.

[pierrejeambrun]

Nice! looks good. Thanks. Just one suggestion

[gopidesupavan]

Merging now its finally green :)

[vincbeck]

Really nice job! 👏

[potiuk]

Nice indeed