Yubico / yubikit-ios

Yubico Mobile iOS SDK - YubiKit
Apache License 2.0
198 stars 44 forks source link

modification to the NFC acceptance flow? #15

Closed rudyrichter closed 4 years ago

rudyrichter commented 4 years ago

I was taking a look at the NFC flow and it currently leaves us in a bad state if the user has supplied a key that isn't an allowed key. It accepts any read key as a signal to dismiss the apple NFC reader sheet, when that sheet could be used to present error messages back to the user, such as ("please try a different key, the key you supplied isn't valid).

I had envisioned this being exposed through a delegate method such that when the key is read the host app can be supplied with information for validating the key (in our case we'd make use of the fido2Service from this delegate callback to see if the key is valid, if valid we can return true and allow the NFCSession to close; if not valid we can return false and a NSString * that the YKNFCSession can then set on the NFCSession.alertMessage (per the apple docs that string can be updated while on screen to reflect changes).

does this sound like a better flow than the current one where the sheet just disappear upon reading a key?

I'm also noticing that when my key is accepted by my app, the sheet remains on screen until i pull the key away from the device. Is this intentional?

imakhalova commented 4 years ago

Thank you @rudyrichter for you request. Let me try to rephrase the issues you've got to confirm that I understand them: 1) You've got 2 YubiKeys and 1 has credential to login within a service, while another one does not. You use the key that is "empty" and your application closes NFC sheet after that. While you expect NFC session still open and suggest user to use another key.

Response: That's interesting case that currently SDK doesn't support, I agree. It invalidates session if there was some error (for example, tag was lost or NFC listener timed out). And the case when you pull the "empty" key away from the phone is considered as "error" case - tag was lost. So it's impossible to connect 2 keys during 1 open session. Thank you, I put this feature request into backlog. We can notify user/application about tag deactivation and leave it up to user whether he wants to close session or not. As well as add ability to update NFCSession.alertMessage from initial one to custom at any point of time (by exposing method for that).

2) And another issue: when you tap your key NFC sheet doesn't get closed until you pull the key away from device.

Response: SDK doesn't know when to close NFC sheet, so if you want to notify NFC API that you're done with communication (you've got all information you need from tag) app needs to call stopSession (which will invalidate and close the NFC sheet)

imakhalova commented 4 years ago

Fixing it in this commit: https://github.com/Yubico/yubikit-ios/commit/227278e69dcc003d490a4a4f4e6aa40800c256f5

Now you can receive multiple tags/devices during one session. Call stopSession() when application doesn't need it anymore. Update alert message with setAlertMessage.

rudyrichter commented 4 years ago

@imakhalova,

Thanks, that's perfect!