From f209c188c13f32236374eadc251380003517d1ef Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Thu, 24 Sep 2026 18:59:53 -0400 Subject: [PATCH] fix(3cx): adopt a new client secret only after login succeeds A candidate secret is tried before it replaces the current one, so a revoked secret cannot stick and a later revert to a working secret still takes effect. A failed re-login after 401 returns the original API response. --- src/shared/shared/three_cx_client.py | 41 ++++++++------ tests/shared/test_three_cx_client.py | 83 +++++++++++++++++++++++++++- 2 files changed, 105 insertions(+), 19 deletions(-) diff --git a/src/shared/shared/three_cx_client.py b/src/shared/shared/three_cx_client.py index d463f69..87a1855 100644 --- a/src/shared/shared/three_cx_client.py +++ b/src/shared/shared/three_cx_client.py @@ -47,7 +47,6 @@ class ThreeCXClient: self._auth_mode = auth_mode self._client_id = auth_kwargs.get("client_id") self._client_secret = auth_kwargs.get("client_secret") - self._retired_secrets: set[str] = set() self._token_expires_at = 0.0 self._auth_lock = threading.RLock() self.session = requests.Session() @@ -68,20 +67,24 @@ class ThreeCXClient: def use_client_secret(self, client_secret: str) -> None: """Point this client at ``client_secret`` and log in again if needed. - The compare-and-set runs under ``_auth_lock``. A secret this client has - already replaced is ignored, so a slower caller still holding the - pre-rotation secret cannot write it back. + A different secret is logged in with before it replaces the current one. + A candidate that 3CX rejects leaves the working secret in place, so a + slower caller still holding a revoked secret cannot clobber a good one, + and a later revert to a secret 3CX accepts still takes effect. """ with self._auth_lock: - if ( - client_secret != self._client_secret - and client_secret not in self._retired_secrets - ): - if self._client_secret: - self._retired_secrets.add(self._client_secret) - self._client_secret = client_secret - self._token_expires_at = 0.0 - self._refresh_expired_token() + if client_secret == self._client_secret: + self._refresh_expired_token() + return + try: + self._authenticate_oauth(self._client_id, client_secret) + except Exception: + logger.warning( + "3CX login with a new client secret failed; keeping the current secret" + ) + self._refresh_expired_token() + return + self._client_secret = client_secret def ensure_fresh_token(self) -> None: """Fetch a new access token when the current one is missing or near expiry.""" @@ -105,9 +108,15 @@ class ThreeCXClient: self.ensure_fresh_token() response = self._raw_request(method, url, **kwargs) if self._auth_mode == "oauth" and response.status_code == 401: - with self._auth_lock: - self._token_expires_at = 0.0 - self._authenticate_oauth(self._client_id, self._client_secret) + try: + with self._auth_lock: + self._token_expires_at = 0.0 + self._authenticate_oauth(self._client_id, self._client_secret) + except Exception: + logger.warning( + "3CX re-login after 401 failed; returning the original response" + ) + return response response = self._raw_request(method, url, **kwargs) return response diff --git a/tests/shared/test_three_cx_client.py b/tests/shared/test_three_cx_client.py index 0a4ff5a..75fcf23 100644 --- a/tests/shared/test_three_cx_client.py +++ b/tests/shared/test_three_cx_client.py @@ -286,15 +286,71 @@ def test_oauth_client_reauths_when_client_secret_changes(): ) first = tcx.oauth_client("test.3cx.us", "cid", "old-secret") second = tcx.oauth_client("test.3cx.us", "cid", "new-secret") - stale = tcx.oauth_client("test.3cx.us", "cid", "old-secret") - assert first is second is stale - assert stale.session.headers["Authorization"] == "Bearer tok-new" + assert first is second + assert second.session.headers["Authorization"] == "Bearer tok-new" posts = _token_posts() assert len(posts) == 2 assert "client_secret=new-secret" in posts[1].request.body tcx._oauth_clients.clear() +@responses.activate +def test_oauth_client_accepts_a_reverted_client_secret(): + from shared import three_cx_client as tcx + + tcx._oauth_clients.clear() + for token in ("tok-a", "tok-b", "tok-a-again"): + responses.add( + responses.POST, + f"{BASE}/connect/token", + json={"access_token": token, "expires_in": 3600}, + status=200, + ) + responses.add( + responses.GET, + f"{BASE}/xapi/v1/Queues/Pbx.GetByNumber(number='801')", + json={"Id": 83}, + status=200, + ) + client = tcx.oauth_client("test.3cx.us", "cid", "secret-a") + tcx.oauth_client("test.3cx.us", "cid", "secret-b") + reverted = tcx.oauth_client("test.3cx.us", "cid", "secret-a") + assert reverted is client + assert reverted.session.headers["Authorization"] == "Bearer tok-a-again" + assert reverted.get_queue("801")["Id"] == 83 + posts = _token_posts() + assert len(posts) == 3 + assert "client_secret=secret-a" in posts[2].request.body + tcx._oauth_clients.clear() + + +@responses.activate +def test_oauth_client_keeps_current_secret_when_candidate_login_fails(): + from shared import three_cx_client as tcx + + tcx._oauth_clients.clear() + responses.add( + responses.POST, + f"{BASE}/connect/token", + json={"access_token": "tok-current", "expires_in": 3600}, + status=200, + ) + responses.add( + responses.POST, + f"{BASE}/connect/token", + json={"access_token": "tok-new", "expires_in": 3600}, + status=200, + ) + responses.add(responses.POST, f"{BASE}/connect/token", status=401) + client = tcx.oauth_client("test.3cx.us", "cid", "current-secret") + tcx.oauth_client("test.3cx.us", "cid", "new-secret") + kept = tcx.oauth_client("test.3cx.us", "cid", "revoked-secret") + assert kept is client + assert kept.session.headers["Authorization"] == "Bearer tok-new" + assert kept._client_secret == "new-secret" + tcx._oauth_clients.clear() + + @responses.activate def test_oauth_request_retries_once_after_401(): _stub_oauth() @@ -321,3 +377,24 @@ def test_oauth_request_retries_once_after_401(): assert client.get_queue("801")["Id"] == 83 assert client.session.headers["Authorization"] == "Bearer tok-refreshed" assert len(_token_posts()) == 2 + + +@responses.activate +def test_oauth_401_returns_original_response_when_relogin_fails(): + import pytest + from requests import HTTPError + + _stub_oauth() + responses.add(responses.POST, f"{BASE}/connect/token", status=401) + responses.add( + responses.GET, + f"{BASE}/xapi/v1/Queues/Pbx.GetByNumber(number='801')", + status=401, + ) + client = ThreeCXClient( + domain="test.3cx.us", auth_mode="oauth", client_id="c", client_secret="s" + ) + with pytest.raises(HTTPError) as raised: + client.get_queue("801") + assert "Queues/Pbx.GetByNumber" in str(raised.value) + assert raised.value.response.status_code == 401