fix: default backend exception_class and drop unreachable serializer branch - #142
Open
CuriousLearner wants to merge 1 commit into
Open
fix: default backend exception_class and drop unreachable serializer branch#142CuriousLearner wants to merge 1 commit into
CuriousLearner wants to merge 1 commit into
Conversation
…branch BaseBackend left exception_class as None, so a custom backend that never set it made send_security_code_and_generate_session_token raise 'catching classes that do not inherit from BaseException is not allowed' instead of logging the provider error. None of the custom backend examples in the documentation set the attribute, so backends written from the docs hit this on any send failure. Default it to Exception on the class, so it applies even when a backend does not call super().__init__. The serializer checked 'verification is None' before SESSION_TOKEN_INVALID, but the backend only returns that status together with a None verification, leaving the 'Session Token mis-match' branch unreachable. Merge the equivalent conditions into the existing 'Security code is not valid' error, which also avoids naming which of phone_number, session_token or security_code was rejected, and update the docs that promised an error the API never returned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two defects in the verification error paths, both found by cross-checking the documentation against running code.
exception_classdefaulted toNone.BaseBackend.__init__setself.exception_class = None, andsend_security_code_and_generate_session_tokencatchesservice.backend.exception_class. A backend that never set the attribute therefore raisedTypeError: catching classes that do not inherit from BaseException is not allowedon any send failure, masking the real provider error. None of the custom backend examples indocs/customization.rst,docs/troubleshooting.rst, ordocs/configuration.rstset it, so backends written from the documentation hit this. It is now a class attribute defaulting toException, so it applies even to backends that do not callsuper().__init__(). The built-in Twilio and Nexmo backends continue to narrow it to their provider's error type.The
SESSION_TOKEN_INVALIDserializer branch was unreachable.validate_security_codereturns that status only together with aNoneverification, and the serializer testedverification is Nonefirst, so"Session Token mis-match"could never be returned. The two equivalent conditions are merged into the existing"Security code is not valid"error. Reporting them identically also avoids revealing which ofphone_number,session_token, orsecurity_codewas rejected. Callers that need to distinguish the cases still get the specific status fromverify_security_code().Docs that promised the unreachable error are corrected, and the serializer error list now includes the brute-force message, which was previously undocumented.
Test plan
test_provider_error_is_logged_when_backend_omits_exception_classbuilds a backend shaped like the documented examples and asserts the provider error is logged rather than raised. Verified it fails with the oldNonedefault.phone_verify/serializers.pygoes from 97% to 100% coverage, confirming the removed branch was the only uncovered line.