santigimeno / node-pcsclite

Bindings over pcsclite to access Smart Cards
ISC License
170 stars 125 forks source link

direct share protocol fix #66

Closed samo4 closed 7 years ago

samo4 commented 7 years ago

Hello, according to two manuals (links below) when you use SCardConnect with SCARD_SHARE_DIRECT share mode, the protocol must be set to 0. Currently if 0 is passed to protocol, the librarary reverted to default values.

http://downloads.acs.com.hk/drivers/cn/PMA_ACR38x_v5.0.pdf https://www.acs.com.hk/download-manual/1141/PMA_ACx30.pdf

samo4 commented 7 years ago

Yes, I agree that this is the safest option. options.protocol = options.protocol

if you pass undefined, I think the c++ code will complain nicely.

Do I submit another pull req.

On 22 February 2017 at 12:50, Santiago Gimeno notifications@github.com wrote:

@santigimeno commented on this pull request.

In lib/pcsclite.js https://github.com/santigimeno/node-pcsclite/pull/66#discussion_r102444656 :

@@ -90,6 +90,10 @@ CardReader.prototype.connect = function(options, cb) { options.share_mode = options.share_mode || this.SCARD_SHARE_EXCLUSIVE; options.protocol = options.protocol || this.SCARD_PROTOCOL_T0 | this.SCARD_PROTOCOL_T1;

  • if (options.share_mode = this.SCARD_SHARE_DIRECT) {
  • options.protocol = 0;
  • }

I think would prefer allowing option.protocol to be 0. If the user passes an invalid value, then I guess connect should fail. What do you think?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/santigimeno/node-pcsclite/pull/66#pullrequestreview-23198716, or mute the thread https://github.com/notifications/unsubscribe-auth/AE80M6JDpVQtmEl5hQ9Nnzw22VnJTxDtks5rfCEcgaJpZM4MIh37 .

santigimeno commented 7 years ago

@samo4 I would prefer something like this:

if (typeof options.protocol === 'undefined' || options.protocol === null) {
    options.protocol = this.SCARD_PROTOCOL_T0 | this.SCARD_PROTOCOL_T1;
}

as per the documentation protocol defaults to that value.

santigimeno commented 7 years ago

oh, and you can reuse this PR, just updating your branch would do

santigimeno commented 7 years ago

Landed in https://github.com/santigimeno/node-pcsclite/commit/98a52690159ee1529f5d9054eb415c74e0fe64b3 and published as 0.5.0. Thanks!