979767 - Supporting web application triggering based on HCI event EVT_TRANSACTION

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Ming Yin]

Attachment: EVT_TRANSACTION Handling_in_Payment_Scenario.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Comment 1

[Ming Yin]

The MozSecureElementManager.webidl atBug 879861 needs to be extended to allow the certified web applications to listen to the HCI event EVT_TRANSACTION. This features is useful for NFC payment scenario,

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

As requested by Ming, assign to Sidd.

Comment 3

[Ming Yin]

Some design issues:

.............................................................................
Issue-1: How to subscribe to certain transaction events(EVT_TRANSACTION)
.............................................................................

According to GSMA NFC Handset APIs Requirement Specification v4.1(http://www.gsma.com/digitalcommerce/wp-content/uploads/2013/12/TS26v4-1.pdf), the certified web application will construct following  transaction event in order to listen to specific "EVT_TRANSACTION".  

    event.action: com.gsma.services.nfc.action.TRANSACTION_EVENT
    event.uri: nfc://secure:0/<SEName>/<AID>  

SEName reflects the originating SE. It should be compliant with SIMAlliance Open Mobile APIs.
AID reflects the originating UICC applet identifier.

When AID is omitted from the URI, the application is registered to any “EVT_TRANSACTION” events sent from the specified Secure Element. For UICC, SEName must follow the format hereafter: “SIM” [smartcard slot] (e.g. SIM1, SIM2, … SIMn).


..............................................................
Issue-2: The data structure of the incoming transaction events
..............................................................

On reception of EVT_TRANSACTION, the SE Manager will notfiy the web application about the transaction event. The payload of the event data shall follow the definition in ETSI TS 102.622, which is made up of  a list of parameters coded in BER-TLV fields. The tag and length of these fields are defined in following table.

Description |  Tag | Length
--------------------------------
AID         | '81' | 5 to 16
--------------------------------
Parameters  | '82' | 0 to 255
--------------------------------


..............................................................
Issue-3: Securing transaction events
..............................................................

1)Since “EVT_TRANSACTION” messages are sensitive data, intercepting these events might help a malicious application to lure a user into entering sensitive information into a fake UI. Usage of “EVT_TRANSACTION” events must request certain permissions. All of them must be set to “dangerous” protection level.

2)Transaction events link an web application and an applet installed on a Secure Element. For this reason, securing them shall be done with the same rules that restrict applet access by the Web application through the SE Access WebAPI. If an application is registered to any “EVT_TRANSACTION”, by omitting the AID in the Intent, it will receive the events of any applets to those accessible using the “Access Control Function”.

Comment 4

[Sandip Kamat]

Quick Note: Currently all NFC payment user stories are covered under: Bug 979152, Bug 979154, Bug 979157, Bug 979158, 884478. All the dependencies for those should be marked in those meta bugs.

Comment 5

[Garner Lee]

Attachment: WIP: add HCI Event Transaction

Comment 6

[Garner Lee]

Attachment: WebIDL: add HCI Event Transaction

Move event callback to mozNFC. While SE triggered, like any other user transactions (ex: SMS, email, a phone call), NFC is simply another delivery path for that transaction event notification.

Comment 7

[Kamil Leszczuk [:kamituel]]

Attachment: HCI event handling flow in tap2pay scenario.

Comment 8

[Garner Lee]

In last NFC workweek, there's a larger discusson on how to register the eSE or sim card AIDs so that the right web application (or application category at least) is notified. Assuming a non-rooted NFC enabled device, this can either be in the manifest (so it can be verified), dynamically at runtime, "route.xml", and/or also by a vetted whitelist on the secure element application itself somewhere.

One obvious example is if the user has more than one wallet they want to use (perhaps depending on context: business trip wallet, every day transaction wallet, restricted vacation only wallet, etc). The user story may influence the API/security design.

Comment 9

[Ming Yin]

(In reply to Garner Lee from comment #8)
> One obvious example is if the user has more than one wallet they want to use
> (perhaps depending on context: business trip wallet, every day transaction
> wallet, restricted vacation only wallet, etc). The user story may influence
> the API/security design.

Good point!  Security needs to be considered for this event handling.

Since “EVT_TRANSACTION” messages are sensitive transaction data, Intercepting these events might help a malicious application to lure a user into entering sensitive information into a fake UI. 

The NFC stack shall therefore implement GlobalPlatform Secure Element Access Control standard to check that the recipient app has been signed with an authorised certificate. This check is performed at the time the event is being forwarded from the lower layers to the target application. If no application is authorised as per “Access Control” check, then the event is discarded.

Comment 10

[Garner Lee]

Attachment: HCI Event Transaction Proposal

Comment 11

[Garner Lee]

Attachment: HCI Event Transaction (from google doc)

Comment 12

[Garner Lee]

Attachment: 0001-Bug-979767-Add-NFC-HCI-Event-Transaction-notificatio.patch

This patch file is a WIP that is still using events to send HCI EVENT TRANSACTION to registered applications. nfc_worker.js is the entry point of the new nfcd message.

Comment 13

[Garner Lee]

Attachment: 0001-Bug-979767-Add-NFC-HCI-Event-Transaction-notificatio.patch

WIP update. Rewrite and rebase to head (still events).

Comment 14

[Garner Lee]

Attachment: (v2) 0001-Bug-979767-Add-NFC-HCI-Event-Transaction-notificatio.patch

Handle event clearing in child process shutdown.

Comment 15

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Garner,
Can you set the target milestone to either sprint2 or sprint3?

Comment 16

[Henry Chang [:hchang]]

Hi Garner,

I am going to show you how to implement the filter:

1) Add your own system message, say 'hci-transaction', 
   to SystemMessagePermissionChecker.jsm [1] The system message could 
   bind a permission if you want.

2) Create an XPCOM component which implements nsISystemMessagesConfigurator [2]

3) Name the contract id of the component you create 
  '@mozilla.org/dom/system-messages/configurator/hci-transaction;1'
   Note that the contract id MUST match the system message name 
   because the system message internal would look up the contract id 
   based on the system message name.

4) Finally, implement the function "shouldDispatch". Promise.defer 
   is a approach to translate an asynchronous operation into a promise. 
   It typically looks like this:

   let deferred = Promise.defer();
   someAsyncFunc(function callback() {
     deferred.resolve(true); // returns true to indicate 'should dispatch'
   });
   return deferred.promise;

5) The content of manifest could be fetched by [3].

Please feel free to ask me if you need further help or information.
Thanks!

[1] http://hg.mozilla.org/mozilla-central/file/83ced53b4298/dom/messages/SystemMessagePermissionsChecker.jsm#l29
[2] http://hg.mozilla.org/mozilla-central/file/83ced53b4298/dom/messages/interfaces/nsISystemMessagesInternal.idl#l60
[3] http://hg.mozilla.org/mozilla-central/file/83ced53b4298/dom/interfaces/apps/nsIAppsService.idl#l29

Comment 17

[Garner Lee]

Attachment: v3 - 0001-Bug-979767-Supporting-web-application-triggering-bas.patch

New WIP using system messages for the benifit of demo apps. It's not yet pulling in the manifest and/or other apps for inspection from the previous comment from Henry (Thanks!) since there's no data/topic filter defined yet.

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Garner Lee from comment #17)
> Created attachment 8476123 [details] [diff] [review]
> v3 - 0001-Bug-979767-Supporting-web-application-triggering-bas.patch
> 
> New WIP using system messages for the benifit of demo apps. It's not yet
> pulling in the manifest and/or other apps for inspection from the previous
> comment from Henry (Thanks!) since there's no data/topic filter defined yet.

The DOM part for HCI event has been removed in this version, is this intensional?

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #18)
> (In reply to Garner Lee from comment #17)
> > Created attachment 8476123 [details] [diff] [review]
> > v3 - 0001-Bug-979767-Supporting-web-application-triggering-bas.patch
> > 
> > New WIP using system messages for the benifit of demo apps. It's not yet
> > pulling in the manifest and/or other apps for inspection from the previous
> > comment from Henry (Thanks!) since there's no data/topic filter defined yet.
> 
> The DOM part for HCI event has been removed in this version, is this
> intensional?
sorry, intentional

Comment 20

[Krzysztof Mioduszewski[:tauzen]]

DOM Events were just a temporary solution, until system message filter has landed. This uses system message 'nfc-hci-event-transaction' to deliver hci-evt-transaction to the web app. This is the target solution since it will always show the app/notification to the user. You can use the demo app to test this https://github.com/tauzen/HCI-Event-Demo.

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #20)
> DOM Events were just a temporary solution,
Please have DOM peers agree on this first.
The purpose of system message is to launch the app, just don't abuse it unless you're pretty sure what you're doing.

Comment 22

[Garner Lee]

Fabrice, care to comment on #21?

> Please have DOM peers agree on this first.
> The purpose of system message is to launch the app, just don't abuse it
> unless you're pretty sure what you're doing.

Right now, there's no direct DOM interaction anymore, just a system message to launch and deliver data.

One iteration of the code had previously set a special NFC only callback to know when to fire to the dynamically set event handler. I'm not sure what the preference is, though I believe the conclusion was "choose the solution that best fits FirefoxOS architecture first, then move it towards whatever standard emerges."

Comment 23

[Vincent Chang[:vchang][changyihsin]]

Linking the discussion in webapi here. 
https://groups.google.com/forum/#!topic/mozilla.dev.webapi/oER2OLg40pg

Comment 24

[[:fabrice] Fabrice Desré]

So, first I'm not a DOM peer ;) System messages have a dual role actually: delivering a message, and eventually waking up the app if it's not running.
I remember the "NFC only callback" setup being very very strange, so I'm in favor of going with system messages for now if they fit the use case.

Comment 25

[Garner Lee]

Hi smaug, DOM question from Comment #21, can/should we use system messages only? Or should our hardware/bottom up transaction message forward to an event handler as well? (I'm currently merging both solutions in the mean time...).

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

I'm not familiar with system messages.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

But what is wrong with events?

Comment 28

[Garner Lee]

(In reply to Olli Pettay [:smaug] from comment #27)
> But what is wrong with events?

Events don't launch applications.

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Garner Lee from comment #28)
> (In reply to Olli Pettay [:smaug] from comment #27)
> > But what is wrong with events?
> 
> Events don't launch applications.

I guess smaug's question is 
"After the app is launched by System message, why don't we pass DOM Event callback to the app?"

My concern on this (not using DOM callback) is:
For the following-up NFC API, CardEmulation API, Host Card Emulation API, they both share the same pattern of this, the Event Target app may not be launched yet. We need to dispatch a system message to wake it up. But if we don't pass DOM event to this app, only using system-message, maybe we will have some design problem in the future, for example, in the HCE case, the app should receive a "on-process-apdu" system message (See Android in [1]), the content of the system message should be looked like SECommand defined in Bug 879861, but the symbol is not available on Parent side, we will need a different dispatch strategy and possibly influence what this bug is doing now.  

Also the question to DOM peer and WebAPI reviewer is:
What's the distinction between a System-message and a DOM callback? Should we use it separately? Or should we use it both at the same time? For the case that when the event happens, the event target(the app) is not launched yet.


If we use System-message alone, I am not sure if that benefits the Web Platform, or it helps Web-Developer. For the Web Platform part, this means the event callback will pass by System-Message, and System message is only available on FirefoxOS(I know currently ServiceWorker is under going now). For the Web-developer, from the WebIDL (like in this bug), he may not know the event is passed through System-message, even his webapp is always running, he may spend a lot time digging through MDN or code to find out.
Also developer maybe confused by when to use DOM callback, and when to use System-message now.

Garner's team think they can get the data from the System-message, why will they listen to DOM callback?
I think this is a good question for the DOM, as this question is also existing on our FirefoxOS now.
Many Gaia apps will only listen to the System-message, they don't listen to the DOM calback. That means many efforts done in the DOM callback are not greatly appreciated.
Although the System message could work for their product, I do think it's our (Mozilla) job to think if this could work on our goal to build a Web Platform.

And I'd like to raise my concern that whether only system message is enough here, as this will greatly influence the design of NFC API in the future. And if we change the strategy in the future, at that time it may have impact on our partners (what Garner's team is doing right now).

[1]: http://developer.android.com/reference/android/nfc/cardemulation/HostApduService.html#processCommandApdu(byte[],%20android.os.Bundle)

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

For system messages you need to ask someone who designed that API. Like sicking or fabrice, I think.

Comment 31

[Garner Lee]

Attachment: 0001-Bug-979767-Add-NFC-HCI-Event-Transaction-notification_events_and_messages.patch

Hi Yoshi, to your question on why no events, in the interest of providing both possiblities for consideration earlier before branch, I've rebased HCI Event Transactions with both events and messages firing.

Known issues:
- Uint8Array wasn't codegening for some reason in a non-customEvent "HCIEventTransactionEvent", so I left the new Event out.
- We might want to convert some Uint8Arrays (the data comes unmodified from nfcd), to a hex string in events as well, which might be mildly more programmer friendly depending on how it's used with Secure Elements.
- Based on another NFC doc (I need to find it again...) this implementation blind fires events when the message is fired, and when the event hander is dynamically registered at runtime (apps can get one message and one event later). Without more process tracking (which could potentially be possible), it can't deliver just once at the moment.
- The to be implemented "Access Control Enforcer" is marked in a few potential locations to filter and restrict access.
- nfc (write) AND nfc-hci-events are needed in the manifest. This might be too restrictive. The callback currently requires the NFC navigator DOM to exist.
- Works on Flame devices to HID reader/Terminal Simulator.

I'm not obsoleting the system message only version yet.

Comment 32

[Garner Lee]

^^ My ideal case is somehow direct an event to a process(es) that might not be runing, like a generic system message, but for navigator object DOM events. Perhaps that the event is merely piggy-backing on the system message for delivery (the message can currently do filtering too).

> And I'd like to raise my concern that whether only system message is enough
> here, as this will greatly influence the design of NFC API in the future.
> And if we change the strategy in the future, at that time it may have impact
> on our partners (what Garner's team is doing right now).
> 
> [1]:
> http://developer.android.com/reference/android/nfc/cardemulation/
> HostApduService.html#processCommandApdu(byte[],%20android.os.Bundle)

Comment 33

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8477935 [details] [diff] [review]
0001-Bug-979767-Add-NFC-HCI-Event-Transaction-notification_events_and_messages.patch

Review of attachment 8477935 [details] [diff] [review]:
-----------------------------------------------------------------

If DOM peer and Fabrice don't think a DOM callback is mandatory, then we can remove DOM callback for now.
Next time when you change your WIP dramatically, please also add detail comment on it as others will find out after three versions of WIP, then the fourth one removes event and the reason I found it is "The DOM part of first three versions are for testing"

Please split your patches into : 
- System message,
- NFC part.

But if you still like to land DOM event part, please also split it to
- NFC WebIDL and DOM part.

Because WebIDL/DOM needs a DOM peer to review.

And if you update the binary protocol with nfcd(NfcMessageHandler), nfcd part needs to be updated as well,
but if you don't have too much time, please file another bug and make this bug depends on it,
Dimi and I will fix it on Monday.

::: dom/apps/src/PermissionsTable.jsm
@@ +340,5 @@
>                               certified: ALLOW_ACTION
>                             },
> +                           "nfc-hci-events": {
> +                             app: DENY_ACTION,
> +                             privileged: DENY_ACTION,

Not privileged?

::: dom/nfc/NfcContentHelper.js
@@ +56,5 @@
>    "NFC:CloseResponse",
>    "NFC:CheckP2PRegistrationResponse",
>    "NFC:PeerEvent",
>    "NFC:NotifySendFileStatusResponse",
> +  "NFC:DOMEvent",

Make a more clear name

::: dom/nfc/gonk/Nfc.js
@@ +632,5 @@
>        return null;
>      }
>  
> +    // Update the current sessionId before sending to the worker
> +    message.json.sessionId = this._currentSessionId;

sessionId is already set few lines above.

::: dom/nfc/gonk/NfcMessageHandler.cpp
@@ +33,5 @@
>  static const char* kInitializedNotification = "InitializedNotification";
>  static const char* kTechDiscoveredNotification = "TechDiscoveredNotification";
>  static const char* kTechLostNotification = "TechLostNotification";
> +static const char* kHCIEventTransactionNotification =
> +                     "HCIEventTransactionNotification";

shorter name is okay.

@@ +305,5 @@
>  bool
> +NfcMessageHandler::HCIEventTransactionNotification(const Parcel& aParcel, EventOptions& aOptions)
> +{
> +  aOptions.mType = NS_ConvertUTF8toUTF16(kHCIEventTransactionNotification);
> +  aOptions.mSessionId = -1;

no need sesson Id.

::: dom/nfc/nsINfcContentHelper.idl
@@ +36,5 @@
> +   *            Uint8Array payload
> +   *            Uint8Array aidOrigin 
> +   *          }
> +   */
> +   void notifyHCIEventTransaction(in nsIVariant data);

If you plan to do the DOM part, update the UUID and change nsINfcPeerEventListener to nsINfcEventListener.

@@ +157,5 @@
> +  *
> +  * @param appId
> +  *        Application ID to be registered
> +  */
> +  void registerTargetForHCIEventTransaction(in nsIDOMWindow window,

And make the original (un)registerTargetForPeerReady more generic, like registerTarget(window, appId, event);

But it seems AID is missing here?
HCI_EVT_Transaction should be fired to target with matching AID, right ?

::: dom/webidl/MozNFC.webidl
@@ +64,5 @@
>     [CheckPermissions="nfc-write"]
>     attribute EventHandler onpeerready;
>     [CheckPermissions="nfc-write"]
>     attribute EventHandler onpeerlost;
> +   [CheckPermissions="nfc-write"]

nfc-hci-event?

Also add nfc-hci-event permission to MozNFC.

@@ +65,5 @@
>     attribute EventHandler onpeerready;
>     [CheckPermissions="nfc-write"]
>     attribute EventHandler onpeerlost;
> +   [CheckPermissions="nfc-write"]
> +   attribute EventHandler onhcieventtransaction;

The parameters in this event should be documented.

Comment 34

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #33)
> And if you update the binary protocol with nfcd(NfcMessageHandler), nfcd
> part needs to be updated as well,
Dimi updated the nfcd in Bug 1057960, please use the PR there for nfcd.
The gecko and nfcd should be landed together, otherwise there will be some warning shown in logcat.

Comment 35

[Garner Lee]

(In reply to Yoshi Huang[:allstars.chh] from comment #34)
> (In reply to Yoshi Huang[:allstars.chh] from comment #33)
> > And if you update the binary protocol with nfcd(NfcMessageHandler), nfcd
> > part needs to be updated as well,

Thanks. Updated locally to match the patch version.

Comment 36

[Garner Lee]

> @@ +157,5 @@
> > +  *
> > +  * @param appId
> > +  *        Application ID to be registered
> > +  */
> > +  void registerTargetForHCIEventTransaction(in nsIDOMWindow window,
> 
> And make the original (un)registerTargetForPeerReady more generic, like
> registerTarget(window, appId, event);
> 
Ok (if doing events).

> But it seems AID is missing here?
> HCI_EVT_Transaction should be fired to target with matching AID, right ?
> 
There's more than one way. I was anticipating the application declares in the signed webapp.manifest what it will listen to "secure_element" section, though that's not really implemented either for the SE Access Control Enforcer. Taking a parameter means it might want an array of AIDs on the secure element. If there's no security to be gained, I can go with either, and just have ACE or similar code verify again.

Comment 37

[Vincent Chang[:vchang][changyihsin]]

Hi Garner, I have talked to Yoshi offline, and both of us agree that we can remove DOM callback for now based on comment 24 and comment 33. Please split your patches into system message and NFC parts and ask Fabrice and Yoshi for review.

Comment 38

[Garner Lee]

Attachment: (v4) Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

Comment 39

[Garner Lee]

Attachment: (v4) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Comment 40

[Garner Lee]

(In reply to Vincent Chang[:vchang] from comment #37)
> Hi Garner, I have talked to Yoshi offline, and both of us agree that we can
> remove DOM callback for now based on comment 24 and comment 33. 

Done. I'll update when I submit them to the try server.

Comment 41

[Garner Lee]

Hold on. I may have messed up the patch generation after rebase.

Comment 42

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8478813 [details] [diff] [review]
(v4) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Review of attachment 8478813 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/gonk/Nfc.js
@@ +462,5 @@
>          this._currentSessionId = null;
>  
>          break;
> +     case "HCIEventTransactionNotification":
> +        gMessageManager.notifyHCIEventTransaction(message);

gMessageManager is not used to do broadcast.
do the broadcast here.
And you didn't wrap the message?

How does WebApp will understand Origin and OriginIndex?
Shouldn't you wrap up to "sim1"?

::: dom/nfc/gonk/NfcGonkMessage.h
@@ +30,5 @@
>  enum NfcNotification {
>    Initialized = 2000,
>    TechDiscovered,
>    TechLost,
> +  HCIEventTransaction,

Please update NFCD_MINOR_VERSION as well.

::: dom/nfc/gonk/NfcOptions.h
@@ +93,5 @@
>      : mType(EmptyString()), mStatus(-1), mSessionId(-1), mRequestId(EmptyString()), mMajorVersion(-1), mMinorVersion(-1),
>        mIsReadOnly(-1), mCanBeMadeReadOnly(-1), mMaxSupportedLength(-1), mPowerLevel(-1)
> +  {
> +      mHciEventTransaction.mAidOrigin = 0;
> +      mHciEventTransaction.mAidOriginIndex = 0;

initial default value wisely.

::: dom/nfc/gonk/NfcService.cpp
@@ +162,5 @@
>      COPY_OPT_FIELD(mCanBeMadeReadOnly, -1)
>      COPY_OPT_FIELD(mMaxSupportedLength, -1)
>  
> +    COPY_OPT_FIELD(mHciEventTransaction.mAidOrigin, 0);
> +    COPY_OPT_FIELD(mHciEventTransaction.mAidOriginIndex, 1);

Now every event from nfcd will have this property.

::: dom/webidl/MozNFC.webidl
@@ +82,5 @@
> +    *   }
> +    * }
> +    */
> +   [CheckPermissions="nfc-hci-events"]
> +   attribute EventHandler onhcieventtransaction;

Remove this

::: dom/webidl/NfcOptions.webidl
@@ +11,5 @@
>  };
>  
> +dictionary HCIEventTransaction
> +{
> +  unsigned long aidOrigin;

origin.
The origin means the source of this HCIEventTransaction, not the source of the 'aid'.

@@ +12,5 @@
>  
> +dictionary HCIEventTransaction
> +{
> +  unsigned long aidOrigin;
> +  unsigned long aidOriginIndex;

ditto

Comment 43

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8478812 [details] [diff] [review]
(v4) Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

Review of attachment 8478812 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/messages/HCIEventTransactionSystemMessageConfigurator.js
@@ +37,5 @@
> +    if(!evtTransaction) {
> +      return false;
> +    }
> +    let aid = this._byteAIDToHex(evtTransaction.aid);
> +    let seName = 'SIM0'; // Bug 1043276: Get the SE name from nfcd.

NFC Daemon already passes the SE name to gecko.

Comment 44

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

See the origin passed from nfcd
https://github.com/mozilla-b2g/platform_system_nfcd/commit/f309e8373d338e7a83ac04a034c5910f912d95ff#diff-facc6f99589b3adeffaebc49d9b3a2ccR385

Comment 45

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (v5) Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

This is the latest version of Garners code with minor fixes from me.

Comment 46

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (v5) Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Same as above, this the latest version of Garners code with small fixes from me.

Comment 47

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8478987 [details] [diff] [review]
(v5) Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Review of attachment 8478987 [details] [diff] [review]:
-----------------------------------------------------------------

cancelling r? for the dict can be more simpler.
Also some handling should be done in NfcService layer as we try not to expose nfcd-gonk specific things to high level.
(I am also planing moving nfcd error code in NfcMessageHandler)

::: dom/nfc/gonk/Nfc.js
@@ +486,5 @@
> +    let evtTxn = message.hciEventTransaction;
> +    let seNameBase = NFC.NFC_SECURE_ELEMENT_NAMES[evtTxn.originType];
> +
> +    let msg = {
> +      seName: seNameBase + evtTxn.originIndex,

I'll suggest using 'origin', don't use 'seName'
1. In the beginning we try to differetiate SE API and NFC API, so we try not to put 'SE' in this event/callback to make it more clear this event is from NFC.
2. For impl you can broadcast the message directly without any renaming.

::: dom/nfc/gonk/NfcOptions.h
@@ +108,5 @@
>    int32_t mIsReadOnly;
>    int32_t mCanBeMadeReadOnly;
>    int32_t mMaxSupportedLength;
>    int32_t mPowerLevel;
> +  HCIEventTransactionStruct mHciEventTransaction;

Don't need another level abstraction.
move 'origin', 'aid' and 'payload' into EventOptions

::: dom/nfc/gonk/nfc_consts.js
@@ +118,5 @@
> +this.NFC_SECURE_ELEMENT_NAMES = {};
> +this.NFC_SECURE_ELEMENT_NAMES[NFC_SECURE_ELEMENT_SIM] = "SIM";
> +this.NFC_SECURE_ELEMENT_NAMES[NFC_SECURE_ELEMENT_ESE] = "ESE";
> +this.NFC_SECURE_ELEMENT_NAMES[NFC_SECURE_ELEMENT_ASSD] = "ASSD";
> +this.NFC_SECURE_ELEMENT_NAMES[NFC_SECURE_ELEMENT_UNKNOWN] = "UNKNOWN";

These should be defined in NfcGonkMessage.h

::: dom/webidl/NfcOptions.webidl
@@ +12,5 @@
>  
> +dictionary HCIEventTransaction
> +{
> +  long originType;
> +  long originIndex;

If we can process these two in NfcService, we can replace these two as DOMString, like DOMString origin

@@ +51,5 @@
>    long maxSupportedLength;
>  
>    long powerLevel;
> +
> +  HCIEventTransaction hciEventTransaction;

We could try to make this dict as flat as we can.
For example, when event is hci-event-transaction, this dict could be looked like
{
  type: "HCIEventTransaction",
  origin: "SIM1",
  aid: [..],
  payload: [..]
}

without introducing another level 'hciEventTransaction'
So you should move members of hciEventTransaction here to make it more simpler

Comment 48

[Garner Lee]

(In reply to Yoshi Huang[:allstars.chh] from comment #47)
Also some handling should be done in NfcService layer as we try not to
> expose nfcd-gonk specific things to high level.
> (I am also planing moving nfcd error code in NfcMessageHandler)

Can you clarify. Do you mean to have NfcMessageHandler as a pure(er) unmarshaller, and NfcService will process it more (which just takes/filters EventOptions right now)?

Comment 49

[Garner Lee]

Attachment: (v6) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

Patch rebased.

Comment 50

[Garner Lee]

Attachment: (v6) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Rebaed. hciEvtTransaction is flattened, and no longer sends an empty version of itself on unrelated events.

Question: Shoud I move creation of origin string "SIM1" in NfcService layer versus NfcMessageHandler? NfcGonkMessage.h isn't used by NfcService.cpp currently.

Comment 51

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8479438 [details] [diff] [review]
(v6) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Review of attachment 8479438 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/gonk/Nfc.js
@@ +483,5 @@
> +    /**
> +     * FIXME:
> +     * GSMA 6.0 7.4 UI Application triggering requirements
> +     * specifices the need to send a URL, but doesn't spec it fully.
> +     */

Better document the parameters here.
(I will file another bug for fixing tech-discovered)

::: dom/nfc/gonk/NfcGonkMessage.h
@@ +75,5 @@
>    FailEnableLowPowerMode = -30,
>    FailDisableLowPowerMode = -31,
>  };
>  
> +enum SecureElementTypeCode {

SecureElementOrigin should be enough

::: dom/nfc/gonk/NfcMessageHandler.cpp
@@ +314,5 @@
> +
> +  int32_t originType = aParcel.readInt32();
> +  int32_t originIndex = aParcel.readInt32();
> +
> +  // Move processing to NfcService.cpp?

yeah, moving NfcService is better.
And you don't have to include NfcGonkMessage.h.
Just list it by order
like the NfcTechString in NfcService.cpp

@@ +325,5 @@
> +      break;
> +    case SecureElementTypeCode::ASSD:
> +      aOptions.mOrigin.Assign(SE_ASSD_STR);
> +      break;
> +    case SecureElementTypeCode::UNKNOWN:

Don't have to do 'case UNKNOWN'.
default is enough

Comment 52

[[:fabrice] Fabrice Desré]

Comment on attachment 8479434 [details] [diff] [review]
(v6) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

Review of attachment 8479434 [details] [diff] [review]:
-----------------------------------------------------------------

Nothing major, but I'd like to see these changes before landing:
- Use consistently double quotes for strings in HCIEventTransactionSystemMessageConfigurator.js (it uses both ' and " currently).
- Move HCIEventTransactionSystemMessageConfigurator.js and the manifest to dom/nfc.
- Add unit tests for HCIEventTransactionSystemMessageConfigurator.js. That would help a lot to understand what the configurator does.

::: dom/apps/src/PermissionsTable.jsm
@@ +345,5 @@
>                               certified: ALLOW_ACTION
>                             },
> +                           "nfc-hci-events": {
> +                             app: DENY_ACTION,
> +                             privileged: ALLOW_ACTION,

We'll need security people to sign off on the privileged level.

::: dom/messages/HCIEventTransactionSystemMessageConfigurator.js
@@ +12,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "appsService",
> +                                   "@mozilla.org/AppsService;1",
> +                                   "nsIAppsService");
> +
> +let DEBUG = true;

nit: switch to false before landing.

@@ +32,5 @@
> +    debug("mustShowRunningApp returning true");
> +    return true;
> +  },
> +
> +  shouldDispatch: function shouldDispatch(manifestURL, pageURL, type, message, extra) {

nit: here an in other function, use aParamName

@@ +92,5 @@
> +    return (matchingRule) ? Promise.resolve() : Promise.reject();
> +  },
> +
> +  // FIXME: there is probably something which does this
> +  _byteAIDToHex: function _byteAIDToHex(arr) {

which type is arr? Is is a typed array or a 'normal' array?

::: dom/messages/moz.build
@@ +7,5 @@
>  DIRS += ['interfaces']
>  
>  EXTRA_COMPONENTS += [
> +    'HCIEventTransactionSystemMessage.manifest',
> +    'HCIEventTransactionSystemMessageConfigurator.js',

these should rather live in the dom/nfc directory

Comment 53

[Garner Lee]

Thanks for the review. I should get to fixing the issues soon in Part 1.

(In reply to Fabrice Desré [:fabrice] from comment #52)
> Comment on attachment 8479434 [details] [diff] [review]
> (v6) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch
> - Add unit tests for HCIEventTransactionSystemMessageConfigurator.js. That
> would help a lot to understand what the configurator does.

For the moment, it makes more sense in context of in laying the groundwork for Bug 884594, Access Control Element/Enforcer to the secure element (v2.2).

Comment 54

[Henry Chang [:hchang]]

Comment on attachment 8479434 [details] [diff] [review]
(v6) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

Review of attachment 8479434 [details] [diff] [review]:
-----------------------------------------------------------------

The minor issue I mentioned in yesterday's meeting.

::: dom/messages/HCIEventTransactionSystemMessageConfigurator.js
@@ +36,5 @@
> +  shouldDispatch: function shouldDispatch(manifestURL, pageURL, type, message, extra) {
> +    debug('message to dispatch: ' + JSON.stringify(message));
> +    debug('manifest url: ' + manifestURL);
> +    if(!message) {
> +      return false;

shouldDispatch needs to return a promise by design. [1]

[1] http://hg.mozilla.org/mozilla-central/file/0753f7b93ab7/dom/messages/interfaces/nsISystemMessagesInternal.idl#l73

Comment 55

[Garner Lee]

Attachment: (v7) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

Fixed nits, chars are '', strings "", debug is off, Moved files. No unit tests yet. Please note if required (Marionette tests on emulator is pending Bug: 1034998)

Sec review --> sec-approval? Seems kind of off. Paul?

Comment 56

[Garner Lee]

Attachment: (v7) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Fixed nits, and comments. Secure Element "UNKNOWN" is treated as "don't send it" for now.

Comment 57

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8479559 [details] [diff] [review]
(v7) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Review of attachment 8479559 [details] [diff] [review]:
-----------------------------------------------------------------

some files are missing? where's NfcOptions.webidl?

Comment 58

[Garner Lee]

Attachment: (v7.1) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

Reshuffle patch files.

Comment 59

[Garner Lee]

Attachment: (v7.1) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Move NfcOptions.webidl back.

Comment 60

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8479579 [details] [diff] [review]
(v7.1) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Review of attachment 8479579 [details] [diff] [review]:
-----------------------------------------------------------------

Forward r? to Smaug for the WebIDL change

::: dom/nfc/gonk/Nfc.js
@@ +477,5 @@
>    },
>  
> +  // HCI Event Transaction
> +  notifyHCIEventTransaction: function notifyHCIEventTransaction(message) {
> +    debug("notifyHCIEventTransaction: " + JSON.stringify(message));

remove, as onEvent already did this.

@@ +484,5 @@
> +     * FIXME:
> +     * GSMA 6.0 7.4 UI Application triggering requirements
> +     * specifies the need for the following parameters to be derived and sent.
> +     * One unclear spec is what the URI format "secure:0" refers to, given
> +     * seName can be something like "SIM0" or "SIM1".

like "SIM1", or "SIM2".

@@ +491,5 @@
> +     * 2) URI,  of the format:  nfc://secure:0/<SEName>/<AID>
> +     *     - SEName reflects the originating SE. It must be compliant with
> +     *       SIMAlliance Open Mobile APIs
> +     *     - AID reflects the originating UICC applet identifier
> +     * 3) AID - Application ID on a Secure Element

You already list AID in 2) URI.

::: dom/nfc/gonk/NfcGonkMessage.h
@@ +79,5 @@
> +enum SecureElementOrigin {
> +  SIM = 0,
> +  ESE = 1,
> +  ASSD = 2,
> +  UNKNOWN = 99,

remove UNKNOWN

::: dom/nfc/gonk/NfcService.cpp
@@ +39,5 @@
> +static const nsLiteralString SEOriginString[] = {
> +  NS_LITERAL_STRING("SIM"),
> +  NS_LITERAL_STRING("ESE"),
> +  NS_LITERAL_STRING("ASSD"),
> +  NS_LITERAL_STRING("UNKNOWN")

remove UNKNOWN

@@ +177,5 @@
> +    } else {
> +      // UNKNOWN is sent if not a transaction.
> +      mEvent.mOrigin.Assign(SEOriginString[size - 1]);
> +    }
> +    COPY_OPT_FIELD(mOrigin, SEOriginString[size - 1]);

if ((mEvent.mOriginType != -1) {
  mEvent.mOrigin.Assign(SEOriginString[mEvent.mOriginType]);
  mEvent.mOrigin.AppendInt(mEvent.mOriginIndex, 16 /* radix */);
}

or maybe we do need to include NfcGonkMessage.h here.
But now it should be okay,
I'll file another bug to make sure the type won't overflow.

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8479579 [details] [diff] [review]
(v7.1) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

(reviewing .webidl only)

>+NfcMessageHandler::HCIEventTransactionNotification(const Parcel& aParcel, EventOptions& aOptions)
>+{
>+  aOptions.mType = NS_ConvertUTF8toUTF16(kHCIEventTransactionNotification);
>+
>+  aOptions.mOriginType = aParcel.readInt32();
>+  aOptions.mOriginIndex = aParcel.readInt32();
>+
>+  int32_t aidLength = aParcel.readInt32();
>+  aOptions.mAid.AppendElements(
>+       static_cast<const uint8_t*>(aParcel.readInplace(aidLength)), aidLength);
... but could you use 2 spaces for indentation here and elsewhere.

>+    int size = sizeof(SEOriginString)/sizeof(nsLiteralString);
space before and after /

Comment 62

[[:fabrice] Fabrice Desré]

(In reply to Garner Lee from comment #53)
> Thanks for the review. I should get to fixing the issues soon in Part 1.
> 
> (In reply to Fabrice Desré [:fabrice] from comment #52)
> > Comment on attachment 8479434 [details] [diff] [review]
> > (v6) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch
> > - Add unit tests for HCIEventTransactionSystemMessageConfigurator.js. That
> > would help a lot to understand what the configurator does.
> 
> For the moment, it makes more sense in context of in laying the groundwork
> for Bug 884594, Access Control Element/Enforcer to the secure element (v2.2).

I don't understand what you mean there.

Comment 63

[[:fabrice] Fabrice Desré]

Comment on attachment 8479578 [details] [diff] [review]
(v7.1) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

Review of attachment 8479578 [details] [diff] [review]:
-----------------------------------------------------------------

Paul, can you check if you're fine with privileged apps getting that permission?

Comment 64

[Garner Lee]

Some background. For privilaged permissions, we have Bug 1042851 for that feature in NFC in general. The demo-hci-event app is actually certified only for the moment. I can back it off to certified only for now too.

Comment 65

[Garner Lee]

Attachment: (v8) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

Thanks fabrice. Updated.

Comment 66

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Garner Lee from comment #64)
> Some background. For privilaged permissions, we have Bug 1042851 for that
> feature in NFC in general. The demo-hci-event app is actually certified only
> for the moment. I can back it off to certified only for now too.

That's probably a good idea. Otherwise we need to figure out what guidance we need to give to marketplace security reviewers for when examining apps which use this permission.

Comment 67

[Garner Lee]

Attachment: (v8) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch

Updated to comment reviews. And removed UNKNOWN references. (TODO added for it).

Comment 68

[Garner Lee]

Awaiting sec comment.

Trying patches: https://tbpl.mozilla.org/?tree=Try&rev=1bb632754752

Comment 69

[Garner Lee]

Attachment: (v8.1) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch

Updated permissions. Re-restrict to certified only for now.

Comment 70

[Garner Lee]

Last try push was a bad. I'll re-submit once the tree re-opens, for the check-in.

Comment 71

[Garner Lee]

Trying: https://tbpl.mozilla.org/?tree=Try&rev=7a40c49e5d05

Comment 72

[Garner Lee]

Current failures appear unrelated to the change.

Mostly green on a limited run subset: https://tbpl.mozilla.org/?tree=Try&rev=959f63bda1e5

Comment 73

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0f8132707c46
https://hg.mozilla.org/integration/b2g-inbound/rev/98d9bdb538a8

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f8132707c46
https://hg.mozilla.org/mozilla-central/rev/98d9bdb538a8