[DavidAmsellem]

Follow-up to @ntorqulu's note in #378: /v4/auth/cas/v2/login has the same problem as /v3/login had.

What

When the request carries a ticket, the endpoint calls CASClient.verify_ticket(). python-cas parses the serviceValidate response without catching parse errors, so if the CAS server answers with something that isn't a valid CAS document, the exception reaches FastAPI and the endpoint returns a 500: ParseError for a body that isn't XML (an HTML error page, an empty body), IndexError for an empty serviceResponse, AttributeError for an authenticationSuccess with no user, and ValueError for a document without the CAS namespace.

This catches those four exceptions around verify_ticket(), logs the problem as a warning, and treats it as an unverified ticket: the endpoint returns the existing 401 Can't verify user response, as /v3/login does after #378. Other errors, such as a network failure reaching the CAS server, are not caught and still surface as before. Tracked in #383.

Tests

The ticket path had no tests. TestCASV2Login.test_ticket_validation patches only CASClientV2.get_verification_response (the HTTP call to the CAS server), so python-cas's own parsing runs:

• an authenticationSuccess response: 200 with the user and a token
• an authenticationFailure response: 401
• a response that isn't XML, an empty serviceResponse, an authenticationSuccess with no user, and a document without the CAS namespace: 401 (500 before this change)

The last four cases fail without the change, on both SQLite and PostgreSQL. The full suite passes locally (749 tests, rebased onto main after #378), and ruff check passes on the changed files. I added a line to CHANGELOG.md under Unreleased.

AI use: I used Claude Code (Opus 5.5) to help analyse the code, plan the change and write the tests. I reviewed every line, ran the full test suite locally against SQLite and PostgreSQL, and checked that the new test cases fail without the change.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.22%. Comparing base (6c77caf) to head (9dc9bfe).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #381      +/-   ##
==========================================
+ Coverage   82.16%   82.22%   +0.06%     
==========================================
  Files         113      113              
  Lines       10009    10015       +6     
==========================================
+ Hits         8224     8235      +11     
+ Misses       1785     1780       -5     

Flag	Coverage Δ
fast-tests	82.22% <100.00%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ntorqulu]

Hi @DavidAmsellem, just some minor comments about exception handling of the verify_ticket call.

[ntorqulu]

verify_ticket calls cas.CASClientV2.verify_response. This endpoint can return ParseError, (None, None, None), IndexError, AttributeError or ValueError.

I would do something like this, adding a logger.warning to be able to check the error raised in the logs:

Suggested change

	except ET.ParseError:
	except (ET.ParseError, IndexError, AttributeError, ValueError) as exc:
	logger.warning("Unexpected CAS ticket validation response: %s", exc)
	user = attributes = pgtiou = None

And bound all user, attributes, pgtiou to prevent any future error from a refactor.
WDYT?

[DavidAmsellem]

Thanks @ntorqulu, good call. I checked each case against python-cas's CASClientV2.verify_response, and they all raise as you said:

response	result
not XML, or an empty body	ParseError
empty <cas:serviceResponse>	IndexError
<cas:authenticationSuccess> with no <cas:user>	AttributeError
no CAS namespace	ValueError
<cas:authenticationFailure>	(None, None, None)

Done in 35e188d: the except now catches (ET.ParseError, IndexError, AttributeError, ValueError) and logs Unexpected CAS ticket validation response: %s as a warning. The parametrized test has a case for each of the three new exceptions, and those cases fail without the change (500 instead of 401).

One deviation from the suggestion: user = attributes = pgtiou = None fails the CI's ruff check with F841, because attributes and pgtiou aren't read anywhere after the call, and the tuple form user, attributes, pgtiou = None, None, None is flagged too. So the except only sets user = None. If you'd rather keep all three bound, I can do it with a # noqa: F841, but I didn't see noqa used elsewhere in the package, so I left it out.

Thanks for rebasing the branch and linking #383. I put the change on top of your commit and only reworded the v4 CHANGELOG line to "not a valid CAS document", since it now covers more than malformed XML. The full suite passes locally (749 tests), and ruff check passes on the changed files.

[ntorqulu]

Perfect, thanks @DavidAmsellem. Will do a quick look at the new changes, but I agree on avoid setting unused variables to None if this raises a ruff error.
Better not use the # noqa: F841, this is why we're implementing ruff!

[ntorqulu]

Hi @DavidAmsellem, all perfect from my side.
While reviewing it I noticed that the function is mixing sync calls inside an async function, and that there's no timeout for the function call, which can make workers stall here.
Not a huge bug, but thanks for fixing the exception handling here so we can pay attention to this part of the code!
I'll write another issue to check with more detail the "/cas/v2/login" endpoint and merge this pr!