[WIP] feat(auth): add algorithm property to Signer base class and update jw…#17662
[WIP] feat(auth): add algorithm property to Signer base class and update jw…#17662ohmayr wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an algorithm property to the base Signer class and its RSA implementations, and updates the JWT encoding logic to dynamically retrieve the algorithm from the signer using getattr. The review feedback points out a potential issue when signer is a mock object, as getattr(signer, "algorithm", None) would return a truthy mock object and bypass the fallback to "RS256". It suggests explicitly checking if the retrieved algorithm is a string.
| if getattr(signer, "algorithm", None): | ||
| header.update({"alg": signer.algorithm}) | ||
| else: | ||
| header.update({"alg": "RS256"}) |
There was a problem hiding this comment.
Using getattr(signer, "algorithm", None) can lead to unexpected behavior when signer is a mock object (e.g., created with mock.create_autospec(crypt.Signer, instance=True) or mock.Mock()). In Python, accessing any attribute on a mock object returns another mock object, which is truthy. This causes getattr(signer, "algorithm", None) to return a mock object instead of None, which prevents the code from falling back to "RS256" and can break existing tests or cause serialization errors.
To avoid this, explicitly verify that the returned algorithm is a string using isinstance(algorithm, str).
| if getattr(signer, "algorithm", None): | |
| header.update({"alg": signer.algorithm}) | |
| else: | |
| header.update({"alg": "RS256"}) | |
| algorithm = getattr(signer, "algorithm", None) | |
| if isinstance(algorithm, str): | |
| header.update({"alg": algorithm}) | |
| else: | |
| header.update({"alg": "RS256"}) |
…account_info factory
…t.encode
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕