Repository navigation
Fix #241: Refresh Keycloak tokens during long operations - #242
dhruvv16-hash wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The current 401 refresh+retry implementation can retry unsafe (non-idempotent) write requests and the retry guard is implemented incorrectly, risking repeated retries and violating acceptance criteria.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR implements a shared Keycloak token lifecycle mechanism so long-running SW360 operations can transparently refresh client-credentials tokens and (on 401) refresh+retry requests without requiring each command to duplicate auth logic.
Changes:
- Centralizes SW360 login/token acquisition in
ScriptBase.login(app_args=..., write_access=...)and updates multiple commands to use it. - Introduces
KeycloakAuth(requests.auth.AuthBase) to proactively refresh tokens and retry 401s. - Adds unit tests for the new
KeycloakAuthhelper.
File summaries
| File | Description |
|---|---|
capycli/common/script_base.py |
Extends login() to optionally build/attach KeycloakAuth and reduce per-command token boilerplate. |
capycli/common/keycloak_auth.py |
Adds a requests auth interceptor for proactive refresh and 401-triggered refresh+retry. |
tests/test_keycloak_auth.py |
Introduces initial unit tests for KeycloakAuth behavior. |
capycli/project/show_vulnerabilities.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/project/show_project.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/project/show_licenses.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/project/show_ecc.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/project/project_component_check.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/project/get_license_info.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/project/find_project.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/project/create_project.py |
Switches to centralized login(app_args=..., write_access=True). |
capycli/project/check_prerequisites.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/bom/map_bom.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/bom/findsources.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/bom/create_components.py |
Switches to centralized login(app_args=..., write_access=True). |
capycli/bom/check_bom.py |
Switches to centralized login(app_args=..., write_access=False). |
capycli/bom/check_bom_item_status.py |
Switches to centralized login(app_args=..., write_access=False). |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def handle_401(self, r, **kwargs): | ||
| if r.status_code == 401 and not getattr(r, '_sw360_retried', False): | ||
| if LOG.isEnabledFor(logging.DEBUG): | ||
| LOG.debug("Received 401 Unauthorized, attempting token refresh...") | ||
|
|
||
| if self._refresh_token(): | ||
| # Consume content of response so we can reuse the connection | ||
| r.content | ||
| r.close() | ||
|
|
||
| # Create a new request based on the old one | ||
| new_req = r.request.copy() | ||
| new_req.headers['Authorization'] = 'Bearer ' + self.token | ||
| new_req._sw360_retried = True | ||
|
|
||
| if LOG.isEnabledFor(logging.DEBUG): | ||
| LOG.debug("Retrying request after token refresh.") | ||
|
|
||
| _r = r.connection.send(new_req, **kwargs) | ||
| _r.history.append(r) | ||
| _r.request = new_req | ||
| return _r | ||
| return r |
| def login(self, token: str = "", url: str = "", oauth2: bool = False, app_args: Any = None, write_access: bool = False) -> bool: | ||
| """Login to SW360""" | ||
| self.sw360_url = os.environ.get("SW360ServerUrl", "") | ||
| sw360_api_token = os.environ.get("SW360ProductionToken", "") | ||
|
|
||
| # Token abstraction via Keycloak | ||
| keycloak_auth = None | ||
| if app_args: |
| def test_handle_401(self): | ||
| with patch('capycli.common.keycloak_auth.SW360Keycloak') as mock_kc: | ||
| instance = mock_kc.return_value | ||
| instance.get_keycloak_token.return_value = "new_token" | ||
|
|
||
| auth = KeycloakAuth("http://localhost", "client", "secret", False, "old_token") | ||
|
|
||
| from requests.models import PreparedRequest | ||
| r = Response() | ||
| r.status_code = 401 | ||
| r.request = PreparedRequest() | ||
| r.request.method = "GET" | ||
| r.request.url = "http://localhost" | ||
| r.request.headers = {} | ||
| r.connection = MagicMock() | ||
|
|
||
| mock_new_resp = Response() | ||
| mock_new_resp.status_code = 200 | ||
| r.connection.send.return_value = mock_new_resp | ||
|
|
||
| new_r = auth.handle_401(r) | ||
|
|
||
| assert r.connection.send.called | ||
| assert new_r == mock_new_resp |
| import pytest | ||
| import responses | ||
| from unittest.mock import patch, MagicMock | ||
|
|
||
| from capycli.common.keycloak_auth import KeycloakAuth | ||
| from requests.models import Request, Response | ||
|
|
|
|
@dhruvv16-hash, great to see that you worked on reducing code duplication, thanks for that! However, it's very hard to get the idea of your changes as there's one commit mixing the refactoring to a central method with semantic changes. May I suggest that we do this in two or more steps, probably aligned to points 1 to 3 of your "key changes" above (testing can easily go into the same commit as it's clearly separated in other files anyways)? First have a pure refactoring commit which only centralizes the code without doing any semantical changes. This one should be rather mechanical and easy to review. And as 2nd step, then the actual refresh implementation, probably also split up into two or more commits which are easy to review each? It's up to you whether you want to immediately work on all changes in one PR or if just start with point 1 for now, then we review and merge it and then you can continue with the actual semantic change based on the merged central code. That would be very helpful, at least for me when reviewing things! |
74c157a to
08de381
Compare
|
Thanks @gernot-h for the suggestion! I've force-pushed the branch to split the changes into two distinct commits:
Let me know if this makes it easier to review! |
|
Hi @gernot-h, Don't you think it would be better to handle refresh token for long running operations in the sw360python library, won't that be a more central solution? Or do we have a reason to handle it here? |
Hi @berke581! The basic Keycloak handling is actually done in the sw360python library, we just need to call it repeatedly to refresh the token. I however also suggested a different solution back in #221 (comment) which would have prevented any Keycloak code in capycli, but @tngraf decided we should better have it integrated directly. And with the problem at hand, tokens expiring during a capycli run, I also see no easy solution to complete abstract away token refresh. Do you have a concrete idea how we could make this better? |
|
@dhruvv16-hash, I'm currently reviewing your refactoring commit. So far, it looks correct, it even fixes a hidden bug in findsources.py where the result of However, the new ScriptBase.login() method is now rather confusing as it combines two ways of being called, either using the Are you willing to continue working on this or shall I take over here with the refactoring part? |
4724b2d to
00bda48
Compare
|
Thanks @gernot-h! I am more than happy to continue working on this. I've just pushed a new commit that reverts \ScriptBase.login()\ entirely back to its original signature (so no tests or legacy usages should break) and introduces a new \login_with_args(self, app_args, write_access)\ method to cleanly wrap the Keycloak logic. I also updated all the callers in the \�om\ and \project\ commands to route through \login_with_args. Let me know how it looks now! |
|
@dhruvv16-hash, I just enabled the test/check actions and it seems your latest changes broke test suite as well as mypy. As we can't enable the pipelines in general for external contributions due to security reasons, do you have a local development setup where you can run mypy and pytest to assure the checks are green? |
| token = getattr(app_args, "sw360_token", "") | ||
| oauth2 = getattr(app_args, "oauth2", False) |
There was a problem hiding this comment.
Is there a specific reason why you always use getattr(app_args, "foo") instead of the more simple app_args.foo? I'd say it's a good thing to crash if some option is undefined instead of silently using a default value in case there's a spelling error somewhere?
| if hasattr(app_args, "sw360_token"): | ||
| app_args.sw360_token = kc_token | ||
| if hasattr(app_args, "oauth2"): | ||
| app_args.oauth2 = True |
There was a problem hiding this comment.
Similar question here: why use hasattr()?
|
@dhruvv16-hash, as I had a hard time reviewing your commits, I rebased your branch locally to merge commit 1 and commit 3 into one commit, so that we first have the full refactoring, then the feature. During that, I also noticed that your last commit removes assignment of the session auth: > git diff fix-issue-241 dhruv/fix-issue-241diff --git a/capycli/common/script_base.py b/capycli/common/script_base.py
index f5ecab3..1d9c5d0 100644
--- a/capycli/common/script_base.py
+++ b/capycli/common/script_base.py
@@ -119,9 +119,6 @@ class ScriptBase:
success = self.login(token, url, oauth2)
if success and keycloak_auth and self.client:
self.client.keycloak_auth = keycloak_auth
- client_session = getattr(self.client, "session", None)
- if client_session:
- client_session.auth = keycloak_auth
return success
def analyze_token(self, token: str) -> None:I guess this was not intended? Shall I push my updated branch here or to a new PR and we have a common look on it? |
I put together a small POC in sw360python for an opt-in SW360KeycloakClient, which extends SW360. Existing callers can keep passing a static token to SW360; callers using the new client would provide client_id and client_secret instead. The Keycloak client obtains a token, caches it, and requests a replacement when it’s within a configurable window of expiry. It handles both normal session requests and methods that read api_headers directly. It also uses its own session so it doesn’t change the shared default session used by existing clients. This would require some changes in capycli, but we thought handling token refresh in the library might be a more suitable approach. Could you take a look at this commit and let me know what you think, @gernot-h: berke581/sw360python@5da20c6 |
|
Thanks for the feedback @gernot-h! I've fixed the test suite failures. The issue was that 5 of the unit tests ( Regarding the assignment of the session auth, you are absolutely right — removing it was unintended! I have re-added Let me know how it looks now! |
Fixes #241
Description
This PR addresses issue #241 by implementing a centralized token lifecycle abstraction that transparently refreshes Keycloak tokens during long operations and automatically retries 401 Unauthorized errors.
Key Changes:
main()method of every individual command intocapycli.common.script_base.ScriptBase.login. This significantly reduces boilerplate (about 500 lines removed).requests.auth.AuthBase: AKeycloakAuthrequest interceptor has been implemented and attached to the SW360requests.Session. It tracks token expiration (refreshing shortly before expiry) and catches 401 Unauthorized responses to perform at most one token refresh and safely retry the failed request.KeycloakAuthinterceptor is attached and existing behavior is preserved.KeycloakAuthintest_keycloak_auth.py.Note: As per ponytail rules, "deletion over addition", so centralizing the token logic into ScriptBase yielded a net decrease in lines of code.