vincbeck commented on code in PR #47432:
URL: https://github.com/apache/airflow/pull/47432#discussion_r1992264451
##########
providers/amazon/tests/unit/amazon/aws/auth_manager/router/test_login.py:
##########
@@ -119,7 +119,9 @@ def test_login_callback_successful(self):
)
assert response.status_code == 303
assert "location" in response.headers
- assert
response.headers["location"].startswith("http://localhost:8080/?token=";)
+ print(response.cookies)
Review Comment:
Please remove
##########
airflow/api_fastapi/auth/managers/simple/ui/src/login/Login.tsx:
##########
@@ -40,7 +40,10 @@ export const Login = () => {
const onSuccess = (data: LoginResponse) => {
// Redirect to appropriate page with the token
const next = searchParams.get("next")
- globalThis.location.replace(`${next ?? ""}?token=${data.jwt_token}`);
+
+ localStorage.setItem("token", data.jwt_token);
Review Comment:
We should be consistent across all auth managers. Auth manager should pass
the token via cookie and not set the local storage themselves. I know this
feels like adding a step for nothing but that would simplify things
##########
docs/apache-airflow/core-concepts/auth-manager/index.rst:
##########
@@ -92,6 +92,11 @@ Some reasons you may want to write a custom auth manager
include:
* You'd like to use an auth manager that leverages an identity provider from
your preferred cloud provider.
* You have a private user management tool that is only available to you or
your organization.
+.. note::
Review Comment:
Thanks for updating the doc, I think this is indeed very important. Could
you please create a sub section of "Authentication related BaseAuthManager
methods" and put it in there? It would make more sense
##########
providers/fab/src/airflow/providers/fab/www/views.py:
##########
@@ -70,7 +71,9 @@ class FabIndexView(IndexView):
def index(self):
if g.user is not None and g.user.is_authenticated:
token = get_auth_manager().get_jwt_token(g.user)
- return redirect(urljoin(conf.get("api", "base_url"),
f"?token={token}"), code=302)
+ response = make_response(redirect(f"{conf.get('api',
'base_url')}", code=302))
+ response.set_cookie("_token", token)
Review Comment:
```suggestion
response.set_cookie("_token", token, httponly=True, secure=True,
samesite="Lax")
```
##########
providers/amazon/src/airflow/providers/amazon/aws/auth_manager/router/login.py:
##########
@@ -80,8 +79,11 @@ def login_callback(request: Request):
username=saml_auth.get_nameid(),
email=attributes["email"][0] if "email" in attributes else None,
)
- url = urljoin(conf.get("api", "base_url"),
f"?token={get_auth_manager().get_jwt_token(user)}")
- return RedirectResponse(url=url, status_code=303)
+ url = conf.get("api", "base_url")
+ token = get_auth_manager().get_jwt_token(user)
+ response = RedirectResponse(url=url, status_code=303)
+ response.set_cookie("_token", token)
Review Comment:
```suggestion
response.set_cookie("_token", token, httponly=True, secure=True,
samesite="Lax")
```
##########
providers/amazon/src/airflow/providers/amazon/aws/auth_manager/router/login.py:
##########
@@ -80,8 +79,11 @@ def login_callback(request: Request):
username=saml_auth.get_nameid(),
email=attributes["email"][0] if "email" in attributes else None,
)
- url = urljoin(conf.get("api", "base_url"),
f"?token={get_auth_manager().get_jwt_token(user)}")
- return RedirectResponse(url=url, status_code=303)
+ url = conf.get("api", "base_url")
+ token = get_auth_manager().get_jwt_token(user)
+ response = RedirectResponse(url=url, status_code=303)
+ response.set_cookie("_token", token)
Review Comment:
I would also save the string "_token" as a constant as well. In
`base_auth_manager`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
