From eb59f17a75cf1a7a089b31f1986a6b6e2ef4ad3b Mon Sep 17 00:00:00 2001 From: JS Ng Date: Sun, 4 Oct 2026 12:10:17 +0800 Subject: [PATCH] refactor: revoke tokens via client library's oauth.revoke (api#80) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Logout's revoke_token raw-POSTed {auth_url}/oauth/revoke with a form-encoded RFC 7009 payload because the client library had no revocation surface. campus-api-python#80 models it (auth.oauth.revoke, JSON body — the server accepts both on all OAuth endpoints); the CLI now builds a device-mode auth root against the issuing endpoint (AuthRoot + CampusRequest are public exports) and delegates. The best-effort contract is unchanged: any APIError or transport failure — or the library being unavailable at all — still reports False so logout clears local credentials with only the 'revocation unavailable' note. The signature is unchanged too, so logout_cmd and the integration tests that mock revoke_token are untouched. Also drops the now-unused revoke_url from get_auth_urls and rewrites the four revoke unit tests against the library seam (plus a new library-unavailable degradation test). Relocks campus-api-python aece106 -> 0c0a36b (the previous pin predated even PR #70; the 0.3.0 version string is unchanged across api#78-#85). --- campus_cli/auth/common.py | 39 +++++++++++++++-------- poetry.lock | 2 +- tests/unit/test_login.py | 66 ++++++++++++++++++++++++--------------- 3 files changed, 67 insertions(+), 40 deletions(-) diff --git a/campus_cli/auth/common.py b/campus_cli/auth/common.py index bda3b35..79c60e7 100644 --- a/campus_cli/auth/common.py +++ b/campus_cli/auth/common.py @@ -102,7 +102,6 @@ def get_auth_urls(auth_url: str | None = None) -> dict: return { "device_code_url": f"{base_url}/oauth/device_authorize", "token_url": f"{base_url}/oauth/token", - "revoke_url": f"{base_url}/oauth/revoke", } @@ -112,9 +111,15 @@ def revoke_token( """ Revoke a token via the auth server's revocation endpoint (RFC 7009). + Goes through the client library's OAuth resource + (`auth.oauth.revoke`, campus-api-python#80): the endpoint targets + the given auth endpoint (or the CLI's current target), and the + server accepts JSON on all OAuth endpoints. + Best-effort by design: logout must still succeed when the server is unreachable or deployed without /oauth/revoke, so any failure is - reported as False instead of raising. + reported as False instead of raising — including when the client + library is unavailable, matching get_api_client's degradation. Args: token: The access or refresh token to revoke. @@ -127,20 +132,28 @@ def revoke_token( Returns: True if the server confirmed revocation, False otherwise. """ - urls = get_auth_urls(auth_url) + base_url = auth_url if auth_url is not None else resolve_auth_url() try: - response = requests.post( - urls["revoke_url"], - data={ - "token": token, - "token_type_hint": token_type_hint, - "client_id": PUBLIC_OAUTH_CLIENT_ID, - }, - timeout=10, + from campus_python import AuthRoot, CampusRequest, errors + except ImportError: + return False + + try: + # Device mode: no credentials, no Authorization header — the + # public revoke endpoint takes the client_id in the body. + auth = AuthRoot( + json_client=CampusRequest( + base_url=base_url, mode="device", timeout=10 + ) + ) + auth.oauth.revoke( + token, + client_id=PUBLIC_OAUTH_CLIENT_ID, + token_type_hint=token_type_hint, ) - return response.status_code == 200 - except requests.RequestException: + return True + except (errors.APIError, requests.RequestException): return False diff --git a/poetry.lock b/poetry.lock index 4194c02..6fdddb8 100644 --- a/poetry.lock +++ b/poetry.lock @@ -163,7 +163,7 @@ flask = "^3.0.0" type = "git" url = "https://github.com/nyjc-computing/campus-api-python.git" reference = "main" -resolved_reference = "aece1067471cbc27c83b29ebc96db3e666a4e62a" +resolved_reference = "0c0a36b6abaea9ce06f34cebf9af4af4913e51a5" [[package]] name = "campus-suite" diff --git a/tests/unit/test_login.py b/tests/unit/test_login.py index 5632ac7..f718e6a 100644 --- a/tests/unit/test_login.py +++ b/tests/unit/test_login.py @@ -4,6 +4,7 @@ import pytest import requests +from campus_python import errors from campus_cli.auth.common import revoke_token from campus_cli.auth.login import ( @@ -81,53 +82,66 @@ def test_request_device_code_network_error(): def test_revoke_token_sends_rfc7009_payload_and_reports_success(): - """A 200 response confirms revocation and the payload follows RFC 7009.""" - response = mock.Mock(spec=requests.Response, status_code=200) - with mock.patch( - "campus_cli.auth.common.requests.post", return_value=response - ) as mock_post: + """A clean library call confirms revocation with the RFC 7009 args.""" + with mock.patch.multiple( + "campus_python", AuthRoot=mock.DEFAULT, CampusRequest=mock.DEFAULT + ) as mocked: + auth_root = mocked["AuthRoot"].return_value assert revoke_token("tok-123", "refresh_token") is True - mock_post.assert_called_once() - assert mock_post.call_args.kwargs["data"] == { - "token": "tok-123", - "token_type_hint": "refresh_token", - "client_id": "guest", - } + auth_root.oauth.revoke.assert_called_once_with( + "tok-123", client_id="guest", token_type_hint="refresh_token" + ) + mocked["CampusRequest"].assert_called_once_with( + base_url=mock.ANY, mode="device", timeout=10 + ) def test_revoke_token_reports_failure_on_http_error(): - """Non-200 responses (e.g. endpoint not deployed) mean not revoked.""" - response = mock.Mock(spec=requests.Response, status_code=404) - with mock.patch( - "campus_cli.auth.common.requests.post", return_value=response - ): + """API errors (e.g. a deployment without the endpoint) mean not revoked.""" + with mock.patch.multiple( + "campus_python", AuthRoot=mock.DEFAULT, CampusRequest=mock.DEFAULT + ) as mocked: + auth_root = mocked["AuthRoot"].return_value + auth_root.oauth.revoke.side_effect = errors.NotFoundError( + status_code=404, error_description="no revoke endpoint" + ) assert revoke_token("tok-123", "access_token") is False def test_revoke_token_reports_failure_on_network_error(): """Network errors degrade to False instead of raising from logout.""" - with mock.patch( - "campus_cli.auth.common.requests.post", - side_effect=requests.ConnectionError("connection refused"), - ): + with mock.patch.multiple( + "campus_python", AuthRoot=mock.DEFAULT, CampusRequest=mock.DEFAULT + ) as mocked: + auth_root = mocked["AuthRoot"].return_value + auth_root.oauth.revoke.side_effect = requests.ConnectionError( + "connection refused" + ) + assert revoke_token("tok-123", "refresh_token") is False + + +def test_revoke_token_reports_failure_when_library_unavailable(): + """A missing client library degrades to False (logout still clears).""" + with mock.patch.dict("sys.modules", {"campus_python": None}): assert revoke_token("tok-123", "refresh_token") is False def test_revoke_token_targets_issuing_endpoint_when_given(): """auth_url overrides the current target for the revocation request.""" - response = mock.Mock(spec=requests.Response, status_code=200) - with mock.patch( - "campus_cli.auth.common.requests.post", return_value=response - ) as mock_post: + with mock.patch.multiple( + "campus_python", AuthRoot=mock.DEFAULT, CampusRequest=mock.DEFAULT + ) as mocked: assert revoke_token( "tok-123", "access_token", auth_url="https://auth-old.example.com/auth/v1", ) is True - assert mock_post.call_args.args[0] == ( - "https://auth-old.example.com/auth/v1/oauth/revoke" + mocked["CampusRequest"].assert_called_once_with( + base_url="https://auth-old.example.com/auth/v1", + mode="device", + timeout=10, )