search

Yubico / yubikit-ios

Yubico Mobile iOS SDK - YubiKit
Apache License 2.0
199 stars 48 forks source link

Expose NFC error codes for error handling #14

Closed jumde closed 4 years ago

jumde commented 4 years ago

We need the NFC error codes to handle workflow for:

  1. Session invalidated by user
  2. System resource not available
  3. Tag connection lost
imakhalova commented 4 years ago

Thank you so much for your contribution and suggestion, I think the idea of allowing user to handle error/cancellation flow is great.

My comments on your change request: 1) Error should not be a property of the session. Each session can have multiple errors. And it's not what session should be aware of. But we can add optional delegate to YKFNFCSession class for that and method that will be invoked upon error. Something like:

2) Tag connection lost is returned by method that communicates with key ([NFCTagReaderSession transceive:tagUpdate:error:]) and it's already propagated to user with execute operations (each method of service allows to have completion block)

jumde commented 4 years ago

@imakhalova - Thanks for the feedback.

Error should not be a property of the session. Each session can have multiple errors. And it's not what session should be aware of. But we can add optional delegate to YKFNFCSession class for that and method that will be invoked upon error. Something like: (void)tagReaderSession:(YKFNFCSession )session didInvalidateWithError:(NSError )error

Using the property I was able to create KVO to trigger the error handling workflows when the error was triggered. I'm not sure if I can do that with a method. Do you have any suggestions?

imakhalova commented 4 years ago

Ok, I understand your case. I took a second look at the code and realize that singleton YubiKitManager won't allow to provide delegate in a proper way. I can add that Error property to YKFNFCSession, but it will be readonly property that you can check when session is closed to verify the reason why it is closed. Similar way you get services when session is open. That way you won't need to observe 2 independent properties, but have 1 observer on state only.

imakhalova commented 4 years ago

Exposed iso7816SessionError which you can check when iso7816SessionState is YKFNFCISO7816SessionStateClosed. See commit https://github.com/Yubico/yubikit-ios/commit/227278e69dcc003d490a4a4f4e6aa40800c256f5

You should get an error in case if session was invalidated by some error or cancellation (unless it was invalidated by stopSession method)

imakhalova commented 4 years ago

@jumde I haven't heard back from you. Can you please let me know if this solution would work for you.

Let me also explain why I decided to go another route to solve the issue rather than your proposal. 1) I was covering other reported issues and feedback and it was related to error handling, so I wasn't sure whether you've got time to polish your PR. 2) Exposing error object is usually better than just error code 3) When you invalidate session the error about cancellation will be returned (I don't think it's desired and user won't be able to filter it out from regular cancellation) 4) With 2 observers you would have to check an error and a state in both observers, making sure that it's synchronized, IMHO it would complicate consumer/app logic 5) Error might be returned for another session, different from what currently is launched, e.g. when you try to start session while there is one already running (not invalidated yet). You would want to filter them.