You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "y-sudharshan (via GitHub)" <gi...@apache.org> on 2026/02/01 07:20:42 UTC
[PR] Fix Keycloak double-slash URL bug (#61121) [airflow]
y-sudharshan opened a new pull request, #61305:
URL: https://github.com/apache/airflow/pull/61305
Fixes #61121 - Fix KeycloakAuthManager double-slash URL construction when server_url has trailing slash
## Description
- Normalize server_url in _get_token_url to prevent double-slashes
- Add .rstrip('/') to handle trailing slashes in server_url configuration
- Add comprehensive tests for URL normalization scenarios
- Resolves compatibility issue with Keycloak 26.4+ strict path validation
When server_url has a trailing slash (e.g., 'https://host/auth/'), the previous implementation would create invalid URLs with double-slashes (e.g., 'https://host/auth//realms/...'), which Keycloak 26.4+ rejects with HTTP 400 'missingNormalization' error.
This fix allows users to configure server_url with or without trailing slashes while ensuring properly normalized URLs are always generated.
## Changes
- **keycloak_auth_manager.py**: Added URL normalization with `.rstrip('/')` in `_get_token_url()` method
- **test_keycloak_auth_manager.py**: Added 4 new test methods covering URL normalization scenarios:
- `test_get_token_url_without_trailing_slash` - Baseline behavior
- `test_get_token_url_with_trailing_slash` - Main bug scenario
- `test_get_token_url_with_multiple_trailing_slashes` - Edge case handling
- `test_get_token_url_with_root_path` - Root URL scenario
## Testing
- ✅ All ruff linting checks passed
- ✅ Code properly formatted per project standards
- ✅ All new tests pass
- ✅ Backward compatible with existing configurations
## Impact
- **Backward Compatible**: Yes - works with or without trailing slash in configuration
- **Breaking Changes**: None
- **Affected Components**: Keycloak auth manager token URL generation
Fixes #61121
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: GitHub Copilot
---
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Keycloak double-slash URL bug (#61121) [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #61305:
URL: https://github.com/apache/airflow/pull/61305#issuecomment-3836409230
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Keycloak double-slash URL bug (#61121) [airflow]
Posted by "y-sudharshan (via GitHub)" <gi...@apache.org>.
y-sudharshan commented on code in PR #61305:
URL: https://github.com/apache/airflow/pull/61305#discussion_r2755081073
##########
providers/keycloak/tests/unit/keycloak/auth_manager/test_keycloak_auth_manager.py:
##########
@@ -559,3 +559,25 @@ def test_get_keycloak_client_with_no_credentials(self, mock_keycloak_openid, aut
client_secret_key="client_secret",
)
assert client == mock_keycloak_openid.return_value
+
+ def test_get_token_url_without_trailing_slash(self, auth_manager):
Review Comment:
Sure. Thanks for the review. In the most recent commit, I have implemented 4 separate test methods into a single parametrized test
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Keycloak double-slash URL bug (#61121) [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #61305:
URL: https://github.com/apache/airflow/pull/61305
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Keycloak double-slash URL bug (#61121) [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #61305:
URL: https://github.com/apache/airflow/pull/61305#discussion_r2754725479
##########
providers/keycloak/tests/unit/keycloak/auth_manager/test_keycloak_auth_manager.py:
##########
@@ -559,3 +559,25 @@ def test_get_keycloak_client_with_no_credentials(self, mock_keycloak_openid, aut
client_secret_key="client_secret",
)
assert client == mock_keycloak_openid.return_value
+
+ def test_get_token_url_without_trailing_slash(self, auth_manager):
Review Comment:
I like those tests but could be one test with multiple parameter using `@pytest.mark.parametrize`. Can you please do that?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Keycloak double-slash URL bug (#61121) [airflow]
Posted by "SameerMesiah97 (via GitHub)" <gi...@apache.org>.
SameerMesiah97 commented on code in PR #61305:
URL: https://github.com/apache/airflow/pull/61305#discussion_r2751927745
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##########
@@ -412,7 +412,10 @@ def _is_batch_authorized(
@staticmethod
def _get_token_url(server_url, realm):
- return f"{server_url}/realms/{realm}/protocol/openid-connect/token"
+ # Normalize server_url by removing trailing slashes to prevent double-slash URLs
+ # when constructing paths. This is required for Keycloak 26.4+ which strictly
+ # rejects non-normalized paths with HTTP 400.
+ return f"{server_url.rstrip('/')}/realms/{realm}/protocol/openid-connect/token"
Review Comment:
This comment seems too verbose . It can be easily inferred from the code that trailing forward slashes are being removed. I would shorten it to this:
`# Normalize server_url to avoid double slashes (required for Keycloak 26.4+ strict path validation).`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org