vincbeck commented on code in PR #51657:
URL: https://github.com/apache/airflow/pull/51657#discussion_r2219417815
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_auth.py:
##########
@@ -91,3 +92,38 @@ def test_should_respond_307(
assert response.status_code == 307
assert response.headers["location"] == expected_redirection
+
+
+class TestRefresh(TestAuthEndpoint):
+ @pytest.mark.parametrize(
+ "params",
+ [
+ {},
+ {"next": None},
+ {"next": "http://localhost:8080"},
+ {"next": "http://localhost:8080";, "other_param": "something_else"},
+ ],
+ )
+ @patch("airflow.api_fastapi.core_api.routes.public.auth.is_safe_url",
return_value=True)
+ def test_should_respond_307(self, mock_is_safe_url, test_client, params):
+ response = test_client.get("/auth/refresh", follow_redirects=False,
params=params)
+
+ assert response.status_code == 307
+ assert (
+ response.headers["location"] ==
f"{AUTH_MANAGER_LOGIN_URL}?next={params.get('next')}"
Review Comment:
You should test here you are redirected to the refresh token URL from the
auth manager. Please test when the URL is None as well
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/auth.py:
##########
@@ -55,3 +55,20 @@ def logout(request: Request, next: None | str = None) ->
RedirectResponse:
logout_url = request.app.state.auth_manager.get_url_login()
return RedirectResponse(logout_url)
+
+
+@auth_router.get(
+ "/refresh",
+
responses=create_openapi_http_exception_doc([status.HTTP_307_TEMPORARY_REDIRECT]),
+)
+def refresh(request: Request, next: None | str = None) -> RedirectResponse:
+ """Refresh the authentication token."""
+ refresh_url = request.app.state.auth_manager.get_url_refresh()
Review Comment:
`refresh_url` can be `None`. In that case, we should redirect to logout?
This scenario is when a user gets an expired token, they are redirected here
but the auth manager the environment is using does not support refresh token.
##########
airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py:
##########
@@ -132,6 +132,15 @@ def get_url_logout(self) -> str | None:
"""
return None
+ def get_url_refresh(self) -> str | None:
+ """
+ Return the URL to refresh the authentication token.
+
+ This is used to refresh the authentication token when it expires.
+ The default implementation returns None, which means that no refresh
URL is provided.
Review Comment:
```suggestion
The default implementation returns None, which means that the auth
manager does not support refresh token.
```
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/routes/login.py:
##########
@@ -70,3 +72,33 @@ def login_callback(request: Request):
secure = bool(conf.get("api", "ssl_cert", fallback=""))
response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=secure)
return response
+
+
+@login_router.get("/refresh")
+def refresh(
+ request: Request, user: Annotated[KeycloakAuthManagerUser,
Depends(get_user)]
+) -> RedirectResponse:
+ """Refresh the token."""
+ client = KeycloakAuthManager.get_keycloak_client()
+
+ if not user or not user.refresh_token:
+ raise HTTPException(
+ status_code=400, detail="User is not a valid Keycloak user or has
no refresh token"
+ )
+
+ tokens = client.refresh_token(user.refresh_token)
+ userinfo = client.userinfo(tokens["access_token"])
Review Comment:
Do we need it? Does it change between tokens? We already have his
information in `user` right?
--
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]
