Test and fix app selection and release apdu-dispatch v0.4.1

dispatch/CHANGELOG.md

@@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 -
 
+## [0.4.1]
+
+- Call `App::deselect` for the deselected app, not for the selected app ([#36](https://github.com/trussed-dev/apdu-dispatch/issues/36))
+
 ## [0.4.0]
 
 - Update dependencies:
@@ -71,7 +75,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Initial release
 
 
-[Unreleased]: https://github.com/trussed-dev/apdu-dispatch/compare/0.3.1...HEAD
+[Unreleased]: https://github.com/trussed-dev/apdu-dispatch/compare/dispatch-0.4.1...HEAD
+[0.4.1]: https://github.com/trussed-dev/apdu-dispatch/compare/dispatch-0.4.0...dispatch-0.4.1
+[0.4.0]: https://github.com/trussed-dev/apdu-dispatch/compare/0.3.1...dispatch-0.4.0
 [0.3.1]: https://github.com/trussed-dev/apdu-dispatch/compare/0.3.0...0.3.1
 [0.3.0]: https://github.com/trussed-dev/apdu-dispatch/compare/0.2.0...0.3.0
 [0.2.0]: https://github.com/trussed-dev/apdu-dispatch/compare/0.1.2...0.2.0

dispatch/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "apdu-dispatch"
-version = "0.4.0"
+version = "0.4.1"
 description = "Dispatch layer after nfc-device and usbd-ccid"
 
 authors.workspace = true

dispatch/src/dispatch.rs

@@ -121,20 +121,14 @@ impl<'pipe> ApduDispatch<'pipe> {
     // It would be nice to store `current_app` instead of constantly looking up by AID,
     // but that won't work due to ownership rules
     fn find_app<'a, 'b>(
-        aid: Option<&Aid>,
+        aid: &Aid,
         apps: &'a mut [&'b mut dyn App],
     ) -> Option<&'a mut &'b mut dyn App> {
-        // match aid {
-        //     Some(aid) => apps.iter_mut().find(|app| aid.starts_with(app.rid())),
-        //     None => None,
-        // }
-        aid.and_then(move |aid| {
-            debug!("matching {:?}", aid);
-            apps.iter_mut().find(|app| {
-                // aid.starts_with(app.aid().truncated())
-                debug!("...against {:?}", app.aid());
-                app.aid().matches(aid)
-            })
+        debug!("matching {:?}", aid);
+        apps.iter_mut().find(|app| {
+            // aid.starts_with(app.aid().truncated())
+            debug!("...against {:?}", app.aid());
+            app.aid().matches(aid)
         })
     }
 
@@ -376,10 +370,8 @@ impl<'pipe> ApduDispatch<'pipe> {
         // For PIV, "SELECT" is NOP if it was already selected, but this is
         // not necessarily the case for other apps
 
-        // if there is a selected app with a different AID, deselect it
-
         // select specified app in any case
-        if let Some(app) = Self::find_app(Some(&aid), apps) {
+        if let Some(app) = Self::find_app(&aid, apps) {
             info!("Selected app");
             let mut response = response::Data::new();
             let result = match &self.buffer.raw {
@@ -389,10 +381,11 @@ impl<'pipe> ApduDispatch<'pipe> {
                 _ => panic!("Unexpected buffer state."),
             };
 
+            // if there is a selected app with a different AID, deselect it
             let old_aid = self.current_aid.replace(aid);
             if let Some(old_aid) = old_aid {
                 if old_aid != aid {
-                    let app = Self::find_app(self.current_aid.as_ref(), apps).unwrap();
+                    let app = Self::find_app(&old_aid, apps).unwrap();
                     // for now all apps will be happy with this.
                     app.deselect();
                 }
@@ -409,7 +402,11 @@ impl<'pipe> ApduDispatch<'pipe> {
     fn handle_app_command(&mut self, apps: &mut [&mut dyn App], interface: Interface) {
         // if there is a selected app, send it the command
         let mut response = response::Data::new();
-        if let Some(app) = Self::find_app(self.current_aid.as_ref(), apps) {
+        if let Some(app) = self
+            .current_aid
+            .as_ref()
+            .and_then(|aid| Self::find_app(aid, apps))
+        {
             let result = match &self.buffer.raw {
                 RawApduBuffer::Request(apdu) => app.call(interface, apdu.as_view(), &mut response),
                 _ => panic!("Unexpected buffer state."),

dispatch/tests/select.rs

@@ -0,0 +1,204 @@
+use apdu_app::{App, Result};
+use apdu_dispatch::{
+    dispatch::ApduDispatch,
+    interchanges::{Channel, Data, Requester},
+};
+use heapless::VecView;
+use hex_literal::hex;
+use iso7816::{command::CommandView, Aid, Interface};
+
+const AID_PIV: &[u8] = &hex!("A000000308000010000100");
+const AID_OPENPGP: &[u8] = &hex!("D276000124010304000FFFFFFFFF0000");
+
+#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
+struct Calls {
+    select: usize,
+    deselect: usize,
+    call: usize,
+}
+
+struct TestApp {
+    aid: Aid,
+    calls: Calls,
+}
+
+impl TestApp {
+    fn new(aid: Aid) -> Self {
+        Self {
+            aid,
+            calls: Calls::default(),
+        }
+    }
+}
+
+impl iso7816::App for TestApp {
+    fn aid(&self) -> Aid {
+        self.aid
+    }
+}
+
+impl App for TestApp {
+    fn select(
+        &mut self,
+        _interface: Interface,
+        _apdu: CommandView<'_>,
+        _reply: &mut VecView<u8>,
+    ) -> Result {
+        self.calls.select += 1;
+        Ok(())
+    }
+
+    fn deselect(&mut self) {
+        self.calls.deselect += 1;
+    }
+
+    fn call(
+        &mut self,
+        _interface: Interface,
+        _apdu: CommandView<'_>,
+        _reply: &mut VecView<u8>,
+    ) -> Result {
+        self.calls.call += 1;
+        Ok(())
+    }
+}
+
+struct Channels {
+    contact: Channel,
+    contactless: Channel,
+}
+
+impl Channels {
+    fn new() -> Self {
+        Self {
+            contact: Channel::new(),
+            contactless: Channel::new(),
+        }
+    }
+}
+
+struct Dispatch<'a, const N: usize> {
+    contact_rq: Requester<'a>,
+    contactless_rq: Requester<'a>,
+    dispatch: ApduDispatch<'a>,
+    apps: [TestApp; N],
+}
+
+impl<'a, const N: usize> Dispatch<'a, N> {
+    fn new(channels: &'a Channels, aids: [Aid; N]) -> Self {
+        let (contact_rq, contact_rp) = channels.contact.split().unwrap();
+        let (contactless_rq, contactless_rp) = channels.contactless.split().unwrap();
+        let dispatch = ApduDispatch::new(contact_rp, contactless_rp);
+        let apps = aids.map(TestApp::new);
+        Self {
+            contact_rq,
+            contactless_rq,
+            dispatch,
+            apps,
+        }
+    }
+
+    fn send_command(&mut self, interface: Interface, command: &[u8]) -> Option<Data> {
+        let rq = match interface {
+            Interface::Contact => &mut self.contact_rq,
+            Interface::Contactless => &mut self.contactless_rq,
+        };
+        rq.request(command.try_into().unwrap()).unwrap();
+        let mut apps: Vec<&mut dyn App> = self.apps.iter_mut().map(|app| app as _).collect();
+        self.dispatch.poll(&mut apps);
+        rq.take_response()
+    }
+
+    fn calls(&self) -> [Calls; N] {
+        self.apps.each_ref().map(|app| app.calls)
+    }
+}
+
+fn select(aid: &[u8]) -> Vec<u8> {
+    let mut cmd = Vec::from(hex!("00A40400"));
+    cmd.push(aid.len().try_into().unwrap());
+    cmd.extend(aid);
+    cmd
+}
+
+#[test]
+fn test_simple() {
+    let aids = [Aid::try_new(AID_PIV).unwrap()];
+    let mut calls = [Calls::default()];
+    let channels = Channels::new();
+    let mut dispatch = Dispatch::new(&channels, aids);
+    assert_eq!(dispatch.calls(), calls);
+
+    let dummy_cmd = hex!("00000000");
+    let select_cmd = select(AID_PIV);
+
+    let response = dispatch.send_command(Interface::Contact, &dummy_cmd);
+    assert_eq!(response.unwrap(), &hex!("6A82"));
+    assert_eq!(dispatch.calls(), calls);
+
+    let response = dispatch.send_command(Interface::Contact, &select_cmd);
+    assert_eq!(response.unwrap(), &hex!("9000"));
+    calls[0].select += 1;
+    assert_eq!(dispatch.calls(), calls);
+
+    let response = dispatch.send_command(Interface::Contact, &select_cmd);
+    assert_eq!(response.unwrap(), &hex!("9000"));
+    calls[0].select += 1;
+    assert_eq!(dispatch.calls(), calls);
+
+    let response = dispatch.send_command(Interface::Contact, &dummy_cmd);
+    assert_eq!(response.unwrap(), &hex!("9000"));
+    calls[0].call += 1;
+    assert_eq!(dispatch.calls(), calls);
+}
+
+#[test]
+fn test_multi() {
+    let aids = [
+        Aid::try_new(AID_PIV).unwrap(),
+        Aid::try_new(AID_OPENPGP).unwrap(),
+    ];
+    let mut calls = [Calls::default(), Calls::default()];
+    let channels = Channels::new();
+    let mut dispatch = Dispatch::new(&channels, aids);
+    assert_eq!(dispatch.calls(), calls);
+
+    let dummy_cmd = hex!("00000000");
+    let select_piv_cmd = select(AID_PIV);
+    let select_openpgp_cmd = select(AID_OPENPGP);
+
+    let response = dispatch.send_command(Interface::Contact, &dummy_cmd);
+    assert_eq!(response.unwrap(), &hex!("6A82"));
+    assert_eq!(dispatch.calls(), calls);
+
+    let response = dispatch.send_command(Interface::Contact, &select_piv_cmd);
+    assert_eq!(response.unwrap(), &hex!("9000"));
+    calls[0].select += 1;
+    assert_eq!(dispatch.calls(), calls);
+
+    let response = dispatch.send_command(Interface::Contact, &select_piv_cmd);
+    assert_eq!(response.unwrap(), &hex!("9000"));
+    calls[0].select += 1;
+    assert_eq!(dispatch.calls(), calls);
+
+    let response = dispatch.send_command(Interface::Contact, &dummy_cmd);
+    assert_eq!(response.unwrap(), &hex!("9000"));
+    calls[0].call += 1;
+    assert_eq!(dispatch.calls(), calls);
+
+    let response = dispatch.send_command(Interface::Contact, &select_openpgp_cmd);
+    assert_eq!(response.unwrap(), &hex!("9000"));
+    calls[0].deselect += 1;
+    calls[1].select += 1;
+    assert_eq!(dispatch.calls(), calls);
+
+    let response = dispatch.send_command(Interface::Contact, &select_openpgp_cmd);
+    assert_eq!(response.unwrap(), &hex!("9000"));
+    calls[1].select += 1;
+    assert_eq!(dispatch.calls(), calls);
+
+    let response = dispatch.send_command(Interface::Contact, &dummy_cmd);
+    assert_eq!(response.unwrap(), &hex!("9000"));
+    calls[1].call += 1;
+    assert_eq!(dispatch.calls(), calls);
+}