Improve OTP validation and logging#23
Conversation
gestrich
commented
Dec 2, 2022
- Add support for a user to tweak code to change the number of OTPs we will support (currently 2). This will help as we try to understand and mitigate Apple's push notification delays.
- Improve OTP error handling.
- Add better logging for the various types of OTP errors the can occur.
- Add new tests and test refactor.
| @@ -333,10 +333,18 @@ extension NightscoutService: RemoteDataService { | |||
|
|
|||
| public func validatePushNotificationSource(_ notification: [String: AnyObject]) -> Bool { | |||
There was a problem hiding this comment.
Log the precise errors when validating.
| @@ -74,41 +74,49 @@ public class OTPManager { | |||
| let issuerName = "Loop" | |||
| var tokenPeriod: TimeInterval | |||
| var passwordDigitCount = 6 | |||
There was a problem hiding this comment.
Allow the maxOTPsToAccept to be parameterized. This parameter will help with testing which can be agnostic to the number of OTPs, which was not previously.
This is also where users can select a different value in code which wasn't supported before (well you could previously change it here but there were other assumptions in the code that wouldn't respect it. This PR removes those assumptions)
| resetSecretKey() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This method will now throw so we can propagate the precise error that occurred and make sure to log it (from client call above).
| guard otpToValidate.count == passwordDigitCount else { | ||
| throw OTPValidationError.invalidFormat(otp: otpToValidate) | ||
| } | ||
|
|
There was a problem hiding this comment.
We can't assume we only store the last 2 passwords anymore so this needed changed to be more flexible.
| @@ -147,10 +155,10 @@ public class OTPManager { | |||
| return Token(name: secretKeyName, issuer: issuerName, generator: generator) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Return nil if we can't get the token rather than just returning an empty array to avoid ambiguity. An error may be preferred but this seems like an improvement.
| @@ -193,6 +201,26 @@ public class OTPManager { | |||
| return components.url?.absoluteString | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Define the different errors that may occur.
These will print in the logs. Eventually, I'd like these to make it to the UI in a notification as our errors are pretty generic. That's a future iteration though.
| @@ -21,77 +21,129 @@ class OTPManagerTestCase: XCTestCase { | |||
| // Put teardown code here. This method is called after the invocation of each test method in the class. | |||
| } | |||
There was a problem hiding this comment.
- Tests should support more than 2 OTPs.
- Added a new test for the case we fail to store used OTPs to disk.
- Miscellaneous test cleanup.
Add new tests and refactor. Improve OTP error handling for better logging.
e0953c3 to
96acc08
Compare
…ction Updated translations from lokalise on Tue May 12 15:47:31 PDT 2026