typing improvements#184
Conversation
8143db2 to
acd7917
Compare
|
Thanks for the PR! Unfortunately it can't be merged in its current form.
|
|
moved the return-timestamp bit to #191, if that looks ok I can also redo some of the typing bits. |
…mecode (#191) This contains the part of #184 that implements a method `verify_and_return_timecode` for TOTP. As its name suggests, this method verifies the given OTP, and if it matches, it will return the matching timecode. This can be used to prevent replay attacks, by storing the timecode on successful authentication, and ensuring that it always increases.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
=======================================
Coverage ? 91.34%
=======================================
Files ? 8
Lines ? 231
Branches ? 54
=======================================
Hits ? 211
Misses ? 9
Partials ? 11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for merging #191 ! |
| super().__init__(s=s, digits=digits, digest=digest, name=name, issuer=issuer) | ||
|
|
||
| def at(self, for_time: Union[int, datetime.datetime], counter_offset: int = 0) -> str: | ||
| def at(self, for_time: Union[float, datetime.datetime], counter_offset: int = 0) -> str: |
There was a problem hiding this comment.
Sorry, I don't understand. Why are you putting float here? The behavior of the algorithm is undefined for non-integer timestamps so I don't think the API should accept floats.
There was a problem hiding this comment.
i believe the commit message explains the reasoning? (it also links to docs about this.) note that tests already do pass floats. note that time.time() also returns a float.
There was a problem hiding this comment.
This package was first developed when type hints were not really a thing. Yes, it accepts floats and at this time it does not raise an error and attempts to do the least surprising thing when a float is passed in. But that does not mean it's a good idea to pass in a float. Passing a float to an algorithm that is only defined on integers is meaningless and should be assumed to lead to dangerously undefined behavior.
This is also why exhaustively typing the test suite is not a big priority; the test suite may deliberately violate type specs to test behavior that the type checker errors on. In this case the correct solution is to set a # type: ignore[...] on the call in the test suite that passes a float, instead of relaxing the type annotation.
This allows mypy to infer that the called methods actually exist. Note that two self.assertIsInstance got replaced by these; the tests will still fail on type mismatch.
This accidentally worked but the typing police does not like it.
This removes a bunch of test calls against compare_digest which is imported from the standard library, and not exposed to users of this library. Mypy also did not like this.
4 participants
This series of commits brings some typing improvements, but also adds the option to return the timecode that matched when verifying a TOTP, which is a feature in
passlib.totpbut that project appears to no longer be maintained.