Add more informative auth error messages - #2338
Conversation
…fied" Users with an unentitled resource plan (not in RESOURCE_PLANS_ID_ALLOWED) were told only "Credentials couldn't be verified", this could lead to confusion, since their credentials were valid; the plan was not entitled. Changes: - Client: surface the server's message instead of discarding it, so the real reason reaches the user. - Gateway: reword the entitlement failure to explain the cause and next step, without leaking the plan id (kept in logs only). On top of this, other auth UX improvements: - Gateway: catch RuntimeFunctionsException in the auth backend and re-raise a friendly, CRN-free AuthenticationFailed instead of an opaque 500. - Fix typos in the deactivated-user message. - Add tests for the disallowed-plan and Runtime-API-failure paths, asserting no plan id / CRN / status code leaks to the user.
| # The Runtime API failed while resolving the user's accessible functions. | ||
| # Details (status codes, CRN) are already logged by FunctionAccessClient; | ||
| # surface a generic, CRN-free message instead of an opaque 500. | ||
| logger.warning("Could not resolve accessible functions: %s", error.message) |
There was a problem hiding this comment.
Should we log this as an error or a warning? I don't know the expectations in this point. I have the feeling that this is not an expected behavior and we should probably show an error here (from our side).
| except QiskitServerlessException as reason: | ||
| raise QiskitServerlessException("Credentials couldn't be verified.") from reason | ||
| raise QiskitServerlessException(f"Credentials couldn't be verified: {reason}") from reason |
There was a problem hiding this comment.
I think the whole try/except could be removed. I guess the original idea was specifically to hide the source of the exception behind a more generic one. But if we're going to show the reason now (which I agree, the safe_json_request already prints the right reason in the whole code), we can simply not catch anything and let the QiskitServerlessException raised in safe_json_request reach the user...
There was a problem hiding this comment.
True, it doesn't add much, but I kept it because if we see "Credentials couldn't be verified" it's easy to know where it was raised in our side of the code. I guess I am so used to seeing it that I would find it confusing to not see "Credentials couldn't be verified"
| # The Runtime API failed while resolving the user's accessible functions. | ||
| # Details (status codes, CRN) are already logged by FunctionAccessClient; | ||
| # surface a generic, CRN-free message instead of an opaque 500. | ||
| logger.warning("Could not resolve accessible functions: %s", error.message) |
There was a problem hiding this comment.
- I would use a logger.exception(error). 2) Do we have a test for this?
There was a problem hiding this comment.
I think we are commenting the same line Berto haha. For me it is an error (I'm not aware about exception for traces 🤔)
There was a problem hiding this comment.
Sure, I didn't pay much attention to the logs, for me the key thing was raising the error below. We can have logger.exception
New notes for PRs that were missing reno files: - #2338: informative auth error messages (surface server reason to caller) - #2300: hide cross-author job result on provider job listing - #2345: surface non-JSON error responses (fix JSON1001 mislead) Move notes that were accidentally placed in 0.33.0/ after that release: - ray-optional-client (#2332) - bump-qiskit-2.2-drop-symengine (#2333) - fix-eu-de-region-auth (#2325) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
New notes for PRs that were missing reno files: - #2338: informative auth error messages (surface server reason to caller) - #2300: hide cross-author job result on provider job listing - #2345: surface non-JSON error responses (fix JSON1001 mislead) Move notes that were accidentally placed in 0.33.0/ after that release: - ray-optional-client (#2332) - bump-qiskit-2.2-drop-symengine (#2333) - fix-eu-de-region-auth (#2325)
5 participants
Summary
Users with an unentitled resource plan (not in RESOURCE_PLANS_ID_ALLOWED) were told only "Credentials couldn't be verified", this could lead to confusion, since their credentials were valid; the plan was not entitled. This PR fixes that, together with a few other auth improvements.
Details and comments
Changes:
On top of this, other auth UX improvements: