diff --git a/campus_python/__init__.py b/campus_python/__init__.py index 5fa7495..985c896 100644 --- a/campus_python/__init__.py +++ b/campus_python/__init__.py @@ -159,6 +159,9 @@ def _get_token_from_session( self.auth.credentials["campus"][login_session.user_id].update( token=token ) + # Return the refreshed token — user_creds still holds the + # pre-refresh (now rotated) access token. + return token return user_creds.token def revoke_session(self) -> None: diff --git a/campus_python/api/v1/timetable.py b/campus_python/api/v1/timetable.py index db2b5c6..9f49bd2 100644 --- a/campus_python/api/v1/timetable.py +++ b/campus_python/api/v1/timetable.py @@ -114,7 +114,8 @@ def new(self, metadata: dict, data: dict) -> dict: } ) resp.raise_for_status() - return resp.json() + # POST /timetable/ wraps the created resource in a data envelope + return resp.json()["data"] def list(self, **filters: typing.Any) -> "list[campus.model.TimetableMetadata]": """List timetables matching the provided filters. @@ -129,7 +130,7 @@ def list(self, **filters: typing.Any) -> "list[campus.model.TimetableMetadata]": resp.raise_for_status() return [ campus.model.TimetableMetadata.from_resource(item) - for item in resp.json() + for item in resp.json()["data"] ] class Timetable(Resource): @@ -146,13 +147,18 @@ def metadata(self) -> "Timetables.Timetable.Metadata": return Timetables.Timetable.Metadata("metadata", parent=self) def get(self) -> campus.model.Timetable: - """Get the metadata for this timetable.""" + """Get this timetable (metadata and labeled entries). + + GET /timetable// wraps the resource in a + ``{"timetable": ...}`` envelope. + """ resp = self.client.get( self.make_path(end_slash=True) - # json={}, ) resp.raise_for_status() - return resp.json() + return campus.model.Timetable.from_resource( + resp.json()["timetable"] + ) class Entries(Resource): """Entries for a single timetable.""" @@ -178,15 +184,20 @@ def get(self) -> campus.model.TimetableMetadata: """ resp = self.client.get(self.make_path()) resp.raise_for_status() - return campus.model.Timetable.from_resource(resp.json()) + return campus.model.TimetableMetadata.from_resource(resp.json()) - def update(self, **kwargs) -> None: + def update(self, *, start_date: str, end_date: str) -> None: """Update the metadata for this timetable. - Args: - **kwargs: Fields to update (e.g., start_date, end_date) + The API replaces both dates together: each is required. - Raises: - NotImplementedError: Not yet implemented + Args: + start_date: New start date (schema.DateTime-compatible) + end_date: New end date (schema.DateTime-compatible) """ - raise NotImplementedError("TODO: Student to implement") + resp = self.client.patch(self.make_path(), json={ + "start_date": start_date, + "end_date": end_date, + }) + resp.raise_for_status() + return None diff --git a/campus_python/auth/v1/__init__.py b/campus_python/auth/v1/__init__.py index bdad825..9afb6ac 100644 --- a/campus_python/auth/v1/__init__.py +++ b/campus_python/auth/v1/__init__.py @@ -169,11 +169,12 @@ def finalize( # 3. Finalize session and get target target = self.sessions[auth_session.id].finalize() - # 4. Ensure user exists - self.users[auth_session.user_id] + # 4. Ensure user exists (the server provisions the user record + # during verify_login; a missing user is a hard error here) + self.users[auth_session.user_id].get() # 5. Create login session - ls = self.logins.new( + self.logins.new( user_id=auth_session.user_id, device_id=uid.generate_category_uid("device", length=16), agent_string=flask.request.headers.get("User-Agent", ""), diff --git a/campus_python/auth/v1/clients.py b/campus_python/auth/v1/clients.py index a18f6a3..0ce2172 100644 --- a/campus_python/auth/v1/clients.py +++ b/campus_python/auth/v1/clients.py @@ -145,13 +145,15 @@ def get( self, vault: str | None = None ) -> JsonDict: + # GET rule is //access/ (trailing slash) and the + # auth app sets strict_slashes — the slash is required. if vault: resp = self.client.get( - self.make_path(), + self.make_path(end_slash=True), query={"vault": vault} ) else: - resp = self.client.get(self.make_path()) + resp = self.client.get(self.make_path(end_slash=True)) # Raise error if status code is not 2XX or 3XX resp.raise_for_status() return resp.json() diff --git a/campus_python/auth/v1/logins.py b/campus_python/auth/v1/logins.py index 6f6fdad..c104002 100644 --- a/campus_python/auth/v1/logins.py +++ b/campus_python/auth/v1/logins.py @@ -77,7 +77,7 @@ def new( resp.raise_for_status() loginsession = campus.model.LoginSession.from_resource(resp.json()) flask.session[self._session_key] = loginsession.id - return campus.model.LoginSession.from_resource(resp.json()) + return loginsession class Login(Resource): """A single login session resource.""" @@ -92,7 +92,9 @@ def revoke(self) -> None: ) # Raise error if status code is not 2XX or 3XX resp.raise_for_status() - del flask.session[self.parent._session_key] # type: ignore + # The key may be absent (expired cookie, new worker) — + # revocation must not fail on local cleanup. + flask.session.pop(self.parent._session_key, None) # type: ignore def get(self) -> campus.model.LoginSession: resp = self.client.get( diff --git a/campus_python/auth/v1/oauth.py b/campus_python/auth/v1/oauth.py index 2dc9e08..6d9b13e 100644 --- a/campus_python/auth/v1/oauth.py +++ b/campus_python/auth/v1/oauth.py @@ -21,6 +21,11 @@ class OAuth(ResourceRoot): Reference: https://datatracker.ietf.org/doc/html/rfc8628 """ + # All OAuth endpoints live under /auth/v1/oauth (campus/auth/routes/ + # oauth.py); the auth app enables strict_slashes, so the slash-less + # relative paths built here must match the route rules exactly. + url_prefix = "/auth/v1/oauth" + def __init__(self, root: ResourceRoot): super().__init__(json_client=root.client) self._root = root @@ -49,9 +54,7 @@ def request_device_code( json_body = { "client_id": client_id, } - # Use /oauth prefix (not /auth/v1) for device authorization endpoints - device_code_path = "/oauth/device_authorize" - resp = self.client.post(device_code_path, json=json_body) + resp = self.client.post(self.make_path("device_authorize"), json=json_body) resp.raise_for_status() return resp.json() @@ -86,39 +89,40 @@ def poll_for_token( "client_id": client_id, "device_code": device_code, } - token_path = "/oauth/token" - resp = self.client.post(token_path, json=json_body) + resp = self.client.post(self.make_path("token"), json=json_body) # Handle OAuth error responses if resp.status_code == 400: error_data = resp.json() error = error_data.get("error", "") - # Map RFC 8628 errors to AuthenticationError + # Map RFC 8628 errors to AuthenticationError; the OAuth error + # code travels in details so callers can read it back via the + # APIError.oauth_error property. if error == "authorization_pending": raise errors.AuthenticationError( error_description="Authorization pending", - error_code="authorization_pending" + details={"oauth_error": "authorization_pending"} ) elif error == "slow_down": raise errors.AuthenticationError( error_description="Slow down", - error_code="slow_down" + details={"oauth_error": "slow_down"} ) elif error == "expired_token": raise errors.AuthenticationError( error_description="Device code has expired", - error_code="expired_token" + details={"oauth_error": "expired_token"} ) elif error == "access_denied": raise errors.AuthenticationError( error_description="Access denied by user", - error_code="access_denied" + details={"oauth_error": "access_denied"} ) else: raise errors.AuthenticationError( error_description=error_data.get("error_description", "Unknown error"), - error_code=error + details={"oauth_error": error} ) resp.raise_for_status() @@ -149,7 +153,8 @@ def authorize_device( "user_code": user_code, "user_id": user_id, } - authorize_path = "/oauth/device/authorize" - resp = self.client.post(authorize_path, json=json_body) + resp = self.client.post( + self.make_path("device/authorize"), json=json_body + ) resp.raise_for_status() return resp.json() diff --git a/campus_python/auth/v1/sessions.py b/campus_python/auth/v1/sessions.py index 39654ee..7f2f23e 100644 --- a/campus_python/auth/v1/sessions.py +++ b/campus_python/auth/v1/sessions.py @@ -22,7 +22,7 @@ class CampusSessions(ResourceCollection): @property def _session_key(self) -> str: - provider = self.path.split("/")[-1] + provider = self.path.rstrip("/").split("/")[-1] return f"{provider}_session_id" def __getitem__(self, session_id: str) -> "CampusSessions.Session": @@ -102,8 +102,12 @@ def finalize(self) -> str: self.make_path(end_slash=True) ) resp.raise_for_status() - # Remove session ID from Flask session using correct key - del flask.session[cast(CampusSessions, self.parent)._session_key] + # Remove session ID from Flask session using correct key. + # The key may be absent (expired cookie, new worker, lost + # session) — clearing it must not fail the finalization (#59). + flask.session.pop( + cast(CampusSessions, self.parent)._session_key, None + ) body = resp.json() return body["target"] diff --git a/tests/unit/test_auth_clients.py b/tests/unit/test_auth_clients.py index 5041637..183b23e 100644 --- a/tests/unit/test_auth_clients.py +++ b/tests/unit/test_auth_clients.py @@ -158,5 +158,29 @@ def test_update_empty_allowed_scopes_is_sent(self): ) +class TestClientAccessTrailingSlash(unittest.TestCase): + """ClientAccess.get() must request /access/ with the trailing slash: + the auth app sets strict_slashes, so the slash-less path 404s.""" + + def setUp(self): + self.auth, self.client = make_auth() + response = Mock() + response.json.return_value = {"access": []} + self.client.get.return_value = response + + def test_get_requests_trailing_slash(self): + self.auth.clients["cid123"].access.get() + self.client.get.assert_called_once_with( + "/auth/v1/clients/cid123/access/" + ) + + def test_get_with_vault_requests_trailing_slash(self): + self.auth.clients["cid123"].access.get(vault="google") + self.client.get.assert_called_once_with( + "/auth/v1/clients/cid123/access/", + query={"vault": "google"}, + ) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/test_nested_resources.py b/tests/unit/test_nested_resources.py index 6049345..e80df70 100644 --- a/tests/unit/test_nested_resources.py +++ b/tests/unit/test_nested_resources.py @@ -133,7 +133,7 @@ def test_entries_list_gets_entries_endpoint(self): def test_metadata_get_gets_metadata_endpoint(self): with patch.object( - campus.model.Timetable, "from_resource", return_value=Mock() + campus.model.TimetableMetadata, "from_resource", return_value=Mock() ): self.api.timetable["tt-1"].metadata.get() self.client.get.assert_called_once_with( diff --git a/tests/unit/test_oauth_device_flow.py b/tests/unit/test_oauth_device_flow.py new file mode 100644 index 0000000..7deed64 --- /dev/null +++ b/tests/unit/test_oauth_device_flow.py @@ -0,0 +1,122 @@ +"""Unit tests for the OAuth device-flow resource (RFC 8628). + +All OAuth endpoints live under /auth/v1/oauth on the auth service +(campus/auth/routes/oauth.py, branch ``weekly``) and the auth app sets +``strict_slashes``, so the client must POST the exact /auth/v1/oauth/... +paths — absolute /oauth/... paths 404 against real deployments. + +OAuth error responses must raise AuthenticationError carrying the OAuth +error code in ``details`` (readable via ``APIError.oauth_error``), not a +TypeError from an unsupported ``error_code=`` kwarg. +""" + +import unittest +from unittest.mock import Mock + +from campus_python import errors +from campus_python.auth.v1 import AuthRoot + + +def make_auth() -> tuple[AuthRoot, Mock]: + """Create an AuthRoot backed by a mock JSON client.""" + client = Mock() + return AuthRoot(json_client=client), client + + +class TestDeviceFlowPaths(unittest.TestCase): + """Device-flow requests must target the /auth/v1/oauth endpoints.""" + + def setUp(self): + self.auth, self.client = make_auth() + + def test_request_device_code_posts_device_authorize_endpoint(self): + self.auth.oauth.request_device_code(client_id="campus-cli") + self.client.post.assert_called_once_with( + "/auth/v1/oauth/device_authorize", + json={"client_id": "campus-cli"}, + ) + + def test_poll_for_token_posts_token_endpoint(self): + response = Mock() + response.status_code = 200 + response.json.return_value = {"access_token": "tok123"} + self.client.post.return_value = response + self.auth.oauth.poll_for_token( + client_id="campus-cli", device_code="dev123" + ) + self.client.post.assert_called_once_with( + "/auth/v1/oauth/token", + json={ + "grant_type": "urn:ietf:params:oauth:grant-type:device_code", + "client_id": "campus-cli", + "device_code": "dev123", + }, + ) + + def test_authorize_device_posts_device_authorize_endpoint(self): + self.auth.oauth.authorize_device(user_code="ABCD-1234", user_id="user1") + self.client.post.assert_called_once_with( + "/auth/v1/oauth/device/authorize", + json={"user_code": "ABCD-1234", "user_id": "user1"}, + ) + + +class TestPollForTokenErrors(unittest.TestCase): + """RFC 8628 error responses map to AuthenticationError, not TypeError.""" + + def setUp(self): + self.auth, self.client = make_auth() + + def make_error_response(self, error: str) -> Mock: + response = Mock() + response.status_code = 400 + response.json.return_value = { + "error": error, + "error_description": f"desc: {error}", + } + self.client.post.return_value = response + return response + + def test_authorization_pending(self): + self.make_error_response("authorization_pending") + with self.assertRaises(errors.AuthenticationError) as ctx: + self.auth.oauth.poll_for_token( + client_id="campus-cli", device_code="dev123" + ) + self.assertEqual(ctx.exception.oauth_error, "authorization_pending") + + def test_slow_down(self): + self.make_error_response("slow_down") + with self.assertRaises(errors.AuthenticationError) as ctx: + self.auth.oauth.poll_for_token( + client_id="campus-cli", device_code="dev123" + ) + self.assertEqual(ctx.exception.oauth_error, "slow_down") + + def test_expired_token(self): + self.make_error_response("expired_token") + with self.assertRaises(errors.AuthenticationError) as ctx: + self.auth.oauth.poll_for_token( + client_id="campus-cli", device_code="dev123" + ) + self.assertEqual(ctx.exception.oauth_error, "expired_token") + + def test_access_denied(self): + self.make_error_response("access_denied") + with self.assertRaises(errors.AuthenticationError) as ctx: + self.auth.oauth.poll_for_token( + client_id="campus-cli", device_code="dev123" + ) + self.assertEqual(ctx.exception.oauth_error, "access_denied") + + def test_unknown_error(self): + self.make_error_response("something_else") + with self.assertRaises(errors.AuthenticationError) as ctx: + self.auth.oauth.poll_for_token( + client_id="campus-cli", device_code="dev123" + ) + self.assertEqual(ctx.exception.oauth_error, "something_else") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/test_sessions.py b/tests/unit/test_sessions.py new file mode 100644 index 0000000..428d90e --- /dev/null +++ b/tests/unit/test_sessions.py @@ -0,0 +1,90 @@ +"""Unit tests for the auth sessions resource (issue #59). + +Session.finalize() and Login.revoke() must not raise KeyError when the +Flask session key is absent (expired cookie, new worker, lost session) — +the remote operation has already succeeded by the time local session +state is cleared. The provider-qualified Flask session key must also be +"campus_session_id" (the trailing-slash path used to yield the empty +provider, storing state under "_session_id"). +""" + +import unittest +from unittest.mock import Mock + +import flask + +from campus_python.auth.v1 import AuthRoot + + +def make_auth() -> tuple[AuthRoot, Mock]: + """Create an AuthRoot backed by a mock JSON client.""" + client = Mock() + return AuthRoot(json_client=client), client + + +def session_context(): + """A Flask request context with a writable session.""" + app = flask.Flask(__name__) + app.secret_key = "test-secret" + return app.test_request_context() + + +def ok_response(body: dict) -> Mock: + response = Mock() + response.json.return_value = body + return response + + +class TestSessionKey(unittest.TestCase): + """The Flask session key is provider-qualified.""" + + def setUp(self): + self.auth, _ = make_auth() + + def test_session_key_names_the_provider(self): + sessions = self.auth.sessions + self.assertEqual(sessions._session_key, "campus_session_id") + + +class TestSessionFinalize(unittest.TestCase): + """finalize() must tolerate an absent Flask session key (#59).""" + + def setUp(self): + self.auth, self.client = make_auth() + self.client.delete.return_value = ok_response( + {"target": "https://app.example.org/after-login"} + ) + + def test_finalize_removes_session_key(self): + with session_context(): + flask.session["campus_session_id"] = "sess-1" + target = self.auth.sessions["sess-1"].finalize() + self.assertEqual(target, "https://app.example.org/after-login") + self.assertNotIn("campus_session_id", flask.session) + + def test_finalize_succeeds_without_session_key(self): + with session_context(): + target = self.auth.sessions["sess-1"].finalize() + self.assertEqual(target, "https://app.example.org/after-login") + + +class TestLoginRevoke(unittest.TestCase): + """Login.revoke() must tolerate an absent Flask session key.""" + + def setUp(self): + self.auth, self.client = make_auth() + self.client.delete.return_value = ok_response({}) + + def test_revoke_removes_session_key(self): + with session_context(): + flask.session["logins_login_id"] = "login-1" + self.auth.logins["login-1"].revoke() + self.assertNotIn("logins_login_id", flask.session) + + def test_revoke_succeeds_without_session_key(self): + with session_context(): + self.auth.logins["login-1"].revoke() + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/test_timetable.py b/tests/unit/test_timetable.py new file mode 100644 index 0000000..bce3a20 --- /dev/null +++ b/tests/unit/test_timetable.py @@ -0,0 +1,106 @@ +"""Unit tests for the timetable resource (issue #54). + +The api service wraps timetable list/create responses in a +``{"data": ...}`` envelope and the by-id GET in ``{"timetable": ...}`` +(campus/api/routes/timetable.py, branch ``weekly``); the client must +unwrap them instead of returning the envelope (or, for list(), crashing +while iterating it). +""" + +import unittest +from unittest.mock import Mock, patch + +import campus.model + +from campus_python.api.v1 import ApiRoot + + +def make_api() -> tuple[ApiRoot, Mock]: + """Create an ApiRoot backed by a mock JSON client.""" + client = Mock() + return ApiRoot(json_client=client), client + + +class TestTimetablesList(unittest.TestCase): + """Timetables.list() must unwrap the API's data envelope.""" + + def setUp(self): + self.api, self.client = make_api() + + def test_list_unwraps_data_envelope(self): + metadata = [{"id": "uid-timetable-1", "start_date": "2026-01-01"}] + self.client.get.return_value.json.return_value = {"data": metadata} + with patch.object( + campus.model.TimetableMetadata, "from_resource", return_value=Mock() + ) as from_resource: + self.api.timetable.list() + self.client.get.assert_called_once_with("/api/v1/timetable/", query=None) + from_resource.assert_called_once_with(metadata[0]) + + def test_list_returns_empty_list_for_empty_envelope(self): + self.client.get.return_value.json.return_value = {"data": []} + self.assertEqual(self.api.timetable.list(), []) + + +class TestTimetablesNew(unittest.TestCase): + """Timetables.new() must unwrap the created resource.""" + + def setUp(self): + self.api, self.client = make_api() + + def test_new_unwraps_data_envelope(self): + resource = {"id": "uid-timetable-1"} + self.client.post.return_value.json.return_value = {"data": resource} + result = self.api.timetable.new(metadata={}, data={"lessongroups": {}}) + self.client.post.assert_called_once_with( + "/api/v1/timetable/", + json={"metadata": {}, "data": {"lessongroups": {}}}, + ) + self.assertEqual(result, resource) + + +class TestTimetableGet(unittest.TestCase): + """Timetables.Timetable.get() must unwrap the timetable envelope.""" + + def setUp(self): + self.api, self.client = make_api() + + def test_get_unwraps_timetable_envelope(self): + resource = {"id": "uid-timetable-1"} + self.client.get.return_value.json.return_value = {"timetable": resource} + with patch.object( + campus.model.Timetable, "from_resource", return_value=Mock() + ) as from_resource: + self.api.timetable["tt-1"].get() + self.client.get.assert_called_once_with("/api/v1/timetable/tt-1/") + from_resource.assert_called_once_with(resource) + + +class TestTimetableMetadata(unittest.TestCase): + """Metadata.get() parses a TimetableMetadata; update() PATCHes both dates.""" + + def setUp(self): + self.api, self.client = make_api() + + def test_get_parses_timetable_metadata(self): + resource = {"id": "uid-timetable-1", "start_date": "2026-01-01"} + self.client.get.return_value.json.return_value = dict(resource) + with patch.object( + campus.model.TimetableMetadata, "from_resource", return_value=Mock() + ) as from_resource: + self.api.timetable["tt-1"].metadata.get() + from_resource.assert_called_once_with(resource) + + def test_update_patches_both_dates(self): + """The API requires start_date and end_date together.""" + self.api.timetable["tt-1"].metadata.update( + start_date="2026-01-01", end_date="2026-12-31" + ) + self.client.patch.assert_called_once_with( + "/api/v1/timetable/tt-1/metadata", + json={"start_date": "2026-01-01", "end_date": "2026-12-31"}, + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/test_token_refresh.py b/tests/unit/test_token_refresh.py new file mode 100644 index 0000000..778d3cb --- /dev/null +++ b/tests/unit/test_token_refresh.py @@ -0,0 +1,52 @@ +"""Unit test for Campus._get_token_from_session token rotation. + +After exchanging the refresh token, the refreshed token must be the one +returned — the credentials resource still holds the pre-refresh access +token, and refresh-token grants rotate (single-use) on the server. +""" + +import os +import unittest +from unittest.mock import MagicMock, Mock, patch + +from campus_python import Campus + + +class TestGetTokenFromSession(unittest.TestCase): + """The refreshed token, not the stale credentials token, is returned.""" + + def test_returns_refreshed_token_after_refresh(self): + with patch.dict(os.environ, {"CLIENT_ID": "cid", "CLIENT_SECRET": "sec"}): + campus = Campus(timeout=5) + auth = campus.auth + + old_token = Mock() + old_token.is_expired.return_value = True + new_token = Mock() + + auth._logins = Mock() + auth._logins.from_session.return_value = Mock(user_id="user1") + + creds_resource = Mock() + creds_resource.get.return_value = Mock(token=old_token) + provider = MagicMock() + provider.__getitem__.return_value = creds_resource + creds_collection = MagicMock() + creds_collection.__getitem__.return_value = provider + auth._credentials = creds_collection + + with patch.object( + type(auth), "token", return_value=new_token + ) as token_mock: + result = campus._get_token_from_session(force_refresh=True) + + self.assertIs(result, new_token) + token_mock.assert_called_once_with( + grant_type="refresh_token", + refresh_token=old_token.refresh_token, + ) + creds_resource.update.assert_called_once_with(token=new_token) + + +if __name__ == "__main__": + unittest.main()