Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
bugraoz93 commented on PR #51657: URL: https://github.com/apache/airflow/pull/51657#issuecomment-2972514492 Thanks Vincent! It has been a great time spent to learn and connecting all the pieces for both Keycloak and our internal authentication. I only worked on the backend API part in other authentication managers, so I missed completely what and how the entire authentication system works. Sure, Vincent, I will update the PR to include only the `refresh_token` logic. I will update the PR today or tomorrow. Today, I have a full day plan, so I will try the night or tomorrow morning to update the PR :) -- 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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
bugraoz93 commented on code in PR #51657:
URL: https://github.com/apache/airflow/pull/51657#discussion_r2146775833
##
providers/keycloak/src/airflow/providers/keycloak/auth_manager/routes/login.py:
##
@@ -48,39 +45,81 @@ def login_callback(request: Request):
code = request.query_params.get("code")
if not code:
return HTMLResponse("Missing code", status_code=400)
-
-client = _get_keycloak_client()
redirect_uri = request.url_for("login_callback")
-
-tokens = client.token(
-grant_type="authorization_code",
+token = KeycloakAuthManagerLogin.refresh_token(
Review Comment:
Thanks a lot, Vincent! Clear :) I will try the UI part too but if I see it
will take too much time, I will ping you on that part after finishing all the
others :)
--
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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
vincbeck commented on code in PR #51657:
URL: https://github.com/apache/airflow/pull/51657#discussion_r2145510678
##
providers/keycloak/src/airflow/providers/keycloak/auth_manager/routes/login.py:
##
@@ -48,39 +45,81 @@ def login_callback(request: Request):
code = request.query_params.get("code")
if not code:
return HTMLResponse("Missing code", status_code=400)
-
-client = _get_keycloak_client()
redirect_uri = request.url_for("login_callback")
-
-tokens = client.token(
-grant_type="authorization_code",
+token = KeycloakAuthManagerLogin.refresh_token(
Review Comment:
Let me try to be more clear :)
> Should we create UI pieces in Keycloak or core ui? Asking because the
wording Optional method to refresh token across auth managers
This is not UI work but backend work. What I have in mind is adding a method
in the auth manager interface
(`airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py`).
This method would look like this:
```
def refresh_token(self) -> str | None:
return None
```
This way, each auth manager implementation can override this method in order
to implement a refresh token logic. By default it does nothing.
Then you would need to call this method in a new Middleware. If this method
return something (not None) then this is the new token we need to replace with
the old one. If so, we need to add this token to the response as cookie the
same we do it when logging in
https://github.com/apache/airflow/blob/main/providers/keycloak/src/airflow/providers/keycloak/auth_manager/routes/login.py#L71.
Then we also need to a small modification on the UI to save this cookie, I
can take care of that if you want.
In the `KeycloakAuthManager`, you need to override the method
`refresh_token` and call Keycloak APIs to refresh the token if necessary.
> Do we want to move the code from Keycloak that will be persisted in the
Cookie to local storage afterwards, similar to the JWT internal token? Asking
this because we don't persist anything in the Cookie after all, they are
generally in the local storage
Core Airflow save the token in local storage, it should stay that way. The
way the token is transmitted between auth manager and core Airflow is via
cookie, we should keep using the mechanism.
I hope it helps :)
--
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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
vincbeck commented on code in PR #51657: URL: https://github.com/apache/airflow/pull/51657#discussion_r2145479928 ## providers/keycloak/provider.yaml: ## @@ -67,3 +67,11 @@ config: version_added: 1.0.0 example: ~ default: "http://host.docker.internal:48080"; + all_admins: Review Comment: Let's delete it. I dont think it is necessary -- 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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
vincbeck commented on PR #51657: URL: https://github.com/apache/airflow/pull/51657#issuecomment-2970909955 No worries at all Bugra, and thanks a lot for your hard work, I understand Keycloak can be confusing and auth in general is not easy to grasp at first sight. Let's do it step by step, could you update this PR to focus on on the refresh token logic? We'll see other points later. I think that will simplify the review. -- 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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
bugraoz93 commented on code in PR #51657:
URL: https://github.com/apache/airflow/pull/51657#discussion_r2145437638
##
providers/keycloak/src/airflow/providers/keycloak/auth_manager/routes/login.py:
##
@@ -48,39 +45,81 @@ def login_callback(request: Request):
code = request.query_params.get("code")
if not code:
return HTMLResponse("Missing code", status_code=400)
-
-client = _get_keycloak_client()
redirect_uri = request.url_for("login_callback")
-
-tokens = client.token(
-grant_type="authorization_code",
+token = KeycloakAuthManagerLogin.refresh_token(
Review Comment:
Thanks a lot for the review, Vincent! I had a learning curve in some parts
of Keycloak, which is why it took a while. They are all working. I have tested
all the parts other than token expire auto login and finding the problem with
JWT token generation because I saw there was a problem with segments, and
mostly unit tests remain. I think I only missed the part where it should be in
this PR :sweat_smile: I understand what is needed, just interpreted a bit wrong
and mixed with all_admins and included, let me fix this.
I may struggle on the UI end. If we can split and you can help with the UI
part, I would be happy to cover/help on all backend sides :) I am still happy
to do my best. I have two questions for the UI
- Should we create UI pieces in Keycloak or core ui? Asking because the
wording `Optional method to refresh token across auth managers`
- Do we want to move the code from Keycloak that will be persisted in the
Cookie to local storage afterwards, similar to the JWT internal token? Asking
this because we don't persist anything in the Cookie after all, they are
generally in the local storage
--
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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
bugraoz93 commented on code in PR #51657:
URL: https://github.com/apache/airflow/pull/51657#discussion_r2145437638
##
providers/keycloak/src/airflow/providers/keycloak/auth_manager/routes/login.py:
##
@@ -48,39 +45,81 @@ def login_callback(request: Request):
code = request.query_params.get("code")
if not code:
return HTMLResponse("Missing code", status_code=400)
-
-client = _get_keycloak_client()
redirect_uri = request.url_for("login_callback")
-
-tokens = client.token(
-grant_type="authorization_code",
+token = KeycloakAuthManagerLogin.refresh_token(
Review Comment:
Thanks a lot for the review, Vincent! I had a learning curve in some parts
of Keycloak, which is why it took a while. They are all working. I have tested,
and mostly unit tests remain. I think I only missed the part where it should be
in this PR :sweat_smile: I understand what is needed, just interpreted a bit
wrong and mixed with all_admins and included, let me fix this.
I may struggle on the UI end. If we can split and you can help with the UI
part, I would be happy to cover/help on all backend sides :) I am still happy
to do my best. I have two questions for the UI
- Should we create UI pieces in Keycloak or core ui? Asking because the
wording `Optional method to refresh token across auth managers`
- Do we want to move the code from Keycloak that will be persisted in the
Cookie to local storage afterwards, similar to the JWT internal token? Asking
this because we don't persist anything in the Cookie after all, they are
generally in the local storage
--
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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
bugraoz93 commented on PR #51657: URL: https://github.com/apache/airflow/pull/51657#issuecomment-2970813880 > I think this PR should be separated in multiple PRs, you are trying to implement multiple different features. I started to work on the create token API ([Keycloak auth manager (view)](https://github.com/orgs/apache/projects/500/views/1?pane=issue&itemId=112609811)) but it seems you want to work on it as well? I should have communicated this with you better. My bad, Vincent! I first started adding these features because I thought we would have these endpoints to manage login, similar to FAB and SAM but then realised that we should manage the login and refresh part separately. Those things just stayed. I can also help on those parts if there isn't too much duplicate work already on your side :sweat_smile: -- 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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
bugraoz93 commented on code in PR #51657:
URL: https://github.com/apache/airflow/pull/51657#discussion_r2145396665
##
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##
@@ -263,3 +275,15 @@ def _get_headers(access_token):
"Authorization": f"Bearer {access_token}",
"Content-Type": "application/x-www-form-urlencoded",
}
+
+def get_url_logout(self, **kwargs) -> str:
+base_url = conf.get("api", "base_url", fallback="/")
+return urljoin(base_url, f"{AUTH_MANAGER_FASTAPI_APP_PREFIX}/logout")
+
+def get_redirected_logout_url(self, url: str) -> str:
Review Comment:
Nowhere :) I moved this part to the service and I forgot to delete it, but
maybe it should be handled in the auth manager's logout.
--
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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
bugraoz93 commented on code in PR #51657: URL: https://github.com/apache/airflow/pull/51657#discussion_r2145393059 ## providers/keycloak/provider.yaml: ## @@ -67,3 +67,11 @@ config: version_added: 1.0.0 example: ~ default: "http://host.docker.internal:48080"; + all_admins: Review Comment: I initially understood it as is, but then I misinterpreted after checking the SAM and thought we also want all admins here. I am okay to delete it. Also not strong opinion to keep it. > (look how is made the SAM middleware when all admins is on) -- 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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
bugraoz93 commented on code in PR #51657: URL: https://github.com/apache/airflow/pull/51657#discussion_r2145390742 ## providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py: ## @@ -216,6 +221,7 @@ def _is_authorized( resource_id: str | None = None, attributes: dict[str, str | None] | None = None, ) -> bool: +return True Review Comment: I forgot to delete this. The token was giving segmentation error so I bypass for the implementation since it is related to authorisation and not authentication -- 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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
vincbeck commented on code in PR #51657:
URL: https://github.com/apache/airflow/pull/51657#discussion_r2145021688
##
providers/keycloak/src/airflow/providers/keycloak/auth_manager/routes/login.py:
##
@@ -48,39 +45,81 @@ def login_callback(request: Request):
code = request.query_params.get("code")
if not code:
return HTMLResponse("Missing code", status_code=400)
-
-client = _get_keycloak_client()
redirect_uri = request.url_for("login_callback")
-
-tokens = client.token(
-grant_type="authorization_code",
+token = KeycloakAuthManagerLogin.refresh_token(
Review Comment:
How does it work? You are refreshing the token when you are logging in? Have
you tested it? Let's take an example. As a user, I am using keycloak auth
manager and I am authenticated. After 5 minutes my token expires, from that
point all the calls I make to Keycloak server are denied. How the refresh logic
will work here?
What I had in mind:
- Create middleware before each response
- This middleware call a new method in auth manager like : refresh_token.
Optional method to refresh token across auth managers
- If this method returns something (not none), this is the new token
- It token is returned, add it as cookie in the response
- Update the front-end to override the token if present in cookie
This way, the token will be refreshed without the user knowing. It will be
totally transparent to the user, it would happen in the background.
--
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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
vincbeck commented on code in PR #51657:
URL: https://github.com/apache/airflow/pull/51657#discussion_r2145022477
##
providers/keycloak/src/airflow/providers/keycloak/auth_manager/routes/login.py:
##
@@ -48,39 +45,81 @@ def login_callback(request: Request):
code = request.query_params.get("code")
if not code:
return HTMLResponse("Missing code", status_code=400)
-
-client = _get_keycloak_client()
redirect_uri = request.url_for("login_callback")
-
-tokens = client.token(
-grant_type="authorization_code",
+token = KeycloakAuthManagerLogin.refresh_token(
Review Comment:
If you are unsure how to do it, I am happy to take this piece
--
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]
Re: [PR] Add Keycloak endpoints, middleware, service and logout [airflow]
vincbeck commented on code in PR #51657:
URL: https://github.com/apache/airflow/pull/51657#discussion_r2145003432
##
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##
@@ -216,6 +221,7 @@ def _is_authorized(
resource_id: str | None = None,
attributes: dict[str, str | None] | None = None,
) -> bool:
+return True
Review Comment:
?
##
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##
@@ -245,6 +251,12 @@ def _is_authorized(
def _get_token_url(server_url, realm):
return f"{server_url}/realms/{realm}/protocol/openid-connect/token"
+@staticmethod
+def get_keycloak_logout_url(server_url, realm):
Review Comment:
```suggestion
def _get_keycloak_logout_url(server_url, realm):
```
##
providers/keycloak/provider.yaml:
##
@@ -67,3 +67,11 @@ config:
version_added: 1.0.0
example: ~
default: "http://host.docker.internal:48080";
+ all_admins:
Review Comment:
I am not against it, but do we want this option? I dont think it is needed
##
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##
@@ -263,3 +275,15 @@ def _get_headers(access_token):
"Authorization": f"Bearer {access_token}",
"Content-Type": "application/x-www-form-urlencoded",
}
+
+def get_url_logout(self, **kwargs) -> str:
+base_url = conf.get("api", "base_url", fallback="/")
+return urljoin(base_url, f"{AUTH_MANAGER_FASTAPI_APP_PREFIX}/logout")
+
+def get_redirected_logout_url(self, url: str) -> str:
Review Comment:
Where is it used?
--
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]
[PR] Add Keycloak endpoints, middleware, service and logout [airflow]
bugraoz93 opened a new pull request, #51657:
URL: https://github.com/apache/airflow/pull/51657
---
**^ Add meaningful description above**
Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)**
for more information.
In case of fundamental code changes, an Airflow Improvement Proposal
([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals))
is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party
License Policy](https://www.apache.org/legal/resolved.html#category-x).
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
[airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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]
