Fix user presence by not overwriting error with OK() (#521)

* Fix user presence by not overwriting error with OK()

* revert debugging change to TOUCH_TIMEOUT_MS

* fix up incomplete merge

* rename variable to more understandable name

* Add tests to test user_presence

src/api/user_presence.rs

@@ -15,6 +15,7 @@
 use crate::clock::ClockInt;
 use embedded_time::duration::Milliseconds;
 
+#[derive(Debug)]
 pub enum UserPresenceError {
     /// User explicitly declined user presence check.
     Declined,

src/ctap/mod.rs

@@ -349,8 +349,14 @@ fn check_user_presence(env: &mut impl Env, channel: Channel) -> Result<(), Ctap2
         // accordingly, so that all wait_with_timeout invocations are separated by
         // equal time intervals. That way token indicators, such as LEDs, will blink
         // with a consistent pattern.
-        result = send_keepalive_up_needed(env, channel, KEEPALIVE_DELAY);
+        let keepalive_result = send_keepalive_up_needed(env, channel, KEEPALIVE_DELAY);
-        if result.is_err() {
+        if keepalive_result.is_err() {
+            debug_ctap!(
+                env,
+                "Sending keepalive failed with error {:?}",
+                keepalive_result.as_ref().unwrap_err()
+            );
+            result = keepalive_result;
             break;
         }
     }
@@ -1517,6 +1523,7 @@ mod test {
     use super::pin_protocol::{authenticate_pin_uv_auth_token, PinProtocol};
     use super::*;
     use crate::api::customization;
+    use crate::api::user_presence::UserPresenceResult;
     use crate::env::test::TestEnv;
     use crate::test_helpers;
     use cbor::{cbor_array, cbor_array_vec, cbor_map};
@@ -3746,6 +3753,30 @@ mod test {
         assert_eq!(response, Err(Ctap2StatusCode::CTAP2_ERR_NOT_ALLOWED));
     }
 
+    #[test]
+    fn test_check_user_presence() {
+        // This TestEnv always returns successful user_presence checks.
+        let mut env = TestEnv::new();
+        let response = check_user_presence(&mut env, DUMMY_CHANNEL);
+        assert!(matches!(response, Ok(_)));
+    }
+
+    #[test]
+    fn test_check_user_presence_timeout() {
+        // This will always return timeout.
+        fn user_presence_timeout() -> UserPresenceResult {
+            Err(UserPresenceError::Timeout)
+        }
+
+        let mut env = TestEnv::new();
+        env.user_presence().set(user_presence_timeout);
+        let response = check_user_presence(&mut env, DUMMY_CHANNEL);
+        assert!(matches!(
+            response,
+            Err(Ctap2StatusCode::CTAP2_ERR_USER_ACTION_TIMEOUT)
+        ));
+    }
+
     #[test]
     fn test_channel_interleaving() {
         let mut env = TestEnv::new();

tools/vendor_hid_test.py

@@ -341,16 +341,22 @@ class CancelTests(unittest.TestCase):
         self.fido,
         'https://example.com',
         user_interaction=CliInteraction(cid + 1, connection))
-    # TODO(https://github.com/google/OpenSK/issues/519): This should be an
+    with self.assertRaises(ClientError) as context:
-    # exception
+      client.make_credential(self.create_options['publicKey'])
-    client.make_credential(self.create_options['publicKey'])
+
+    self.assertEqual(context.exception.code, ClientError.ERR.TIMEOUT)
+    self.assertEqual(context.exception.cause.code,
+                     ctap.CtapError.ERR.USER_ACTION_TIMEOUT)
 
   def test_timeout(self):
     client = Fido2Client(
         self.fido, 'https://example.com', user_interaction=CliInteraction())
-    # TODO(https://github.com/google/OpenSK/issues/519): This should be an
+    with self.assertRaises(ClientError) as context:
-    # exception
+      client.make_credential(self.create_options['publicKey'])
-    client.make_credential(self.create_options['publicKey'])
+
+    self.assertEqual(context.exception.code, ClientError.ERR.TIMEOUT)
+    self.assertEqual(context.exception.cause.code,
+                     ctap.CtapError.ERR.USER_ACTION_TIMEOUT)
 
 
 if __name__ == '__main__':