Closed
Bug 963533
Opened 11 years ago
Closed 10 years ago
B2G NFC: don't expose sessionToken
Categories
(Firefox OS Graveyard :: NFC, defect)
Tracking
(tracking-b2g:backlog)
RESOLVED
WONTFIX
tracking-b2g | backlog |
People
(Reporter: allstars.chh, Assigned: psiddh)
References
Details
Attachments
(4 files, 1 obsolete file)
Part 2: v1 - Remove the usage of session from Nfc APIs getNFCTag() & getNFCPeer(). r=khuey, sr=smaug
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Jonas mentioned this during WebAPI review. We should find a more consistent way to dispatch NFCTag or NFCPeer to the web, instead of exposing sessionToken and use mozNfc.getTag(sessionToken) or mozNfc.getPeer(sessionToken) to get the object. Also we have to consider about in ndef-tech-discovered, it seems we cannot pass NFCTag in that system message (Structure Clone). In that case, how should we pass NFCTag ?
Updated•11 years ago
|
No longer blocks: b2g-NFC-2.0
Comment 2•11 years ago
|
||
It may be possible to remove the session token. If we can ensure only a single instance of an application can be launched at once (manifest URL), the token may be hidden from the application. The token will need to be stored in the parent process, where it is already checked for validity currently.
Flags: needinfo?(dgarnerlee)
Comment 3•11 years ago
|
||
Sid, please look into whether the current framework for P2P can be extended to handle parent/content app exchange of sessionTokens.
Assignee: nobody → psiddh
Yes, I think we can leverage the existing P2P framework and do away with exposing 'sessoinTokens'
Comment 5•11 years ago
|
||
Note from a different discussion: For Activities, where the session tokens for tags arrive, we'd need a new/modified message type to route the NFC object to the W3C compiliant "ontechdiscovered" (create interface object at the application level with the internal sessionToken).
Attachment #8402146 -
Flags: superreview?(bugs)
Attachment #8402146 -
Flags: review?(khuey)
Assignee | ||
Comment 10•11 years ago
|
||
With the above attachments / changes, tested the following scenarios:- 1) Tag Usecases - Reading / Writing. Validated most of the APIs in the interface 'MozNFCTag'. 2) P2P Usecases - Tested onpeerfound & onpeerlost handlers. Validated all the APIs of the interface 'MozNFCPeer' 3) Tested scenarios when the Nfc gaia application is killed 4) Validated P2P sharing scenario (of URL) from browser. 5) Validated P2P sendFile scenario partly (as there is a regression caused by earlier changes) I have also attached Gaia patches for everyone's review. Part3 : (NFC) System App changes - I will issue a pull request for Alive. But attaching a patch for now Part 4: Gaia App changes to use NFC peer API ( getNFCPeer() )without 'session' + 1 test case is also fixed in contacts app. Once the patches get reviewed / landed, I will fix the Nfc wiki as well accordingly
Assignee | ||
Comment 11•11 years ago
|
||
For Gaia apps changes (Part 4), do I have to create separate patches per application (Browser, Contacts, Gallery etc..)(or) could it be accepted as one single patch with a reviewer. I will accordingly create a pull request for this change. Ken , can you please suggest ?
Flags: needinfo?(kchang)
Comment 12•11 years ago
|
||
(In reply to Siddartha P from comment #11) > For Gaia apps changes (Part 4), do I have to create separate patches per > application (Browser, Contacts, Gallery etc.. Yes, this way would be easier for reviewer to do a review. Thanks.
Flags: needinfo?(kchang)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8402145 -
Attachment is obsolete: true
Attachment #8402145 -
Flags: review?(allstars.chh)
Attachment #8403001 -
Flags: review?(allstars.chh)
Reporter | ||
Comment 14•11 years ago
|
||
Comment on attachment 8403001 [details] [diff] [review] Part 1: v1 - Do not expose session tokens to content process. r=allstars.chh Review of attachment 8403001 [details] [diff] [review]: ----------------------------------------------------------------- Can't this have test cases?
Attachment #8403001 -
Flags: review?(allstars.chh)
Comment 15•11 years ago
|
||
Comment on attachment 8402146 [details] [diff] [review] Part 2: v1 - Remove the usage of session from Nfc APIs getNFCTag() & getNFCPeer(). r=khuey, sr=smaug >--- a/dom/webidl/MozNfc.webidl >+++ b/dom/webidl/MozNfc.webidl >@@ -26,18 +26,18 @@ interface MozNfcManager { > */ > void notifySendFileStatus(octet status, DOMString requestId); > }; > > [JSImplementation="@mozilla.org/navigatorNfc;1", > NavigatorProperty="mozNfc", > Func="Navigator::HasNfcSupport"] > interface MozNfc : EventTarget { >- MozNFCTag getNFCTag(DOMString sessionId); >- MozNFCPeer getNFCPeer(DOMString sessionId); >+ MozNFCTag getNFCTag(); >+ MozNFCPeer getNFCPeer(); So how is this API supposed to work? When can one call getNFCTag() and will it return the same object if re-called right after? And what about calling it later?
Attachment #8402146 -
Flags: superreview?(bugs) → feedback?(psiddh)
Assignee | ||
Comment 16•11 years ago
|
||
This API getNFCTag() is supposed to be called when there is an active Nfc Tag (passive tag) in phone's near field. To give the context, When a tag is brought in close proximity to the FxOS phone, nfcd (Gonk) receives the event (with the data). nfcd relays this info to Gecko (Nfc.js - Parent Process) which in turn notifies System app about 'nfc-manager-tech-discovered' event. System manager through mozActivity forwards the event to selected application by the user. nfcd-->Nfc.js-->System App (Moz Activity) -->Nfc App1-->getNFCTag() Currently it will not return the same object when called say later. (Should a singleton like object be enforced?)
Attachment #8402146 -
Flags: feedback?(psiddh)
Comment 17•11 years ago
|
||
So how does the system app notify some selected application? Given that MozNfc is an EventTarget couldn't we dispatch an Event there which has .nfcTag or some such as a readonly property?
Assignee | ||
Comment 18•11 years ago
|
||
Gaia (nfc) Applications declare/register 'nfc-ndef-discovered' in their manifest.webapp. When the system app receives the event from lower layers (Nfc.js), it sends the mozActivity. If there are more than one registered application that declared 'nfc-ndef-discovered', a picker would be shown allowing user to make a selection. After user makes a selection, the application selected will be launched (if not launched already) or brought into foreground (if it is in background) This is what Gaia applications can do currently.. After registering in their manifest.webapp, navigator.mozSetMessageHandler('activity', NfcActivityHandler); . . function NfcActivityHandler(activity) { var activityName = activity.source.name; var data = activity.source.data; switch (activityName) { case 'nfc-tech-discovered': var nfcdom = window.navigator.mozNfc; var nfcTag = nfcdom.getNFCTag(); var readreq = nfcTag.readNDEF(); . . } If I understand your suggestion correctly, this is what you propose. Pls correct me otherwise var nfcdom = window.navigator.mozNfc; nfcdom.onnfctagfound = function(event) { // 'onnfctagfound' say, is new dom event handler var tag = nfcdom.nfcTag; // readonly attr (or) var tag = event.details; var readreq = tag.readNDEF(); } If we do adopt the notion of dispatching the event, we may lose out on showing a picker (MozActivity) and allowing user to make a choice. Also for the dom event to be dispatched, the application needs to be launched / or running in background. With the picker approach an application can be launched upon user selection Garner has raised this bug recently :https://bugzilla.mozilla.org/show_bug.cgi?id=991970 Ideally, getNFCTag() and getNFCPeer() should be removed in future. So yes, your suggestion is a good idea to get rid of getNFC* interfaces. Do you want me to raise separate bug to discuss the changes suggested by you , the user scenarios and its implication?
Comment 19•11 years ago
|
||
I guess in my mind nfcactivityhandler should be an event listener and an event would be dispatch when there is nfc available. But I'm not familiar with our activity API, so it depends on whether that activity stuff could use normal event handling. Could we even keep the current activity stuff (which is just giving us a 'nfc-tech-discovered' notification?) and add a new event which would be dispatched to mozNfc. The event would be dispatched after the notification.
Assignee | ||
Comment 20•11 years ago
|
||
I think , Activities are a kind of system messages internally.
> Could we even keep the current activity stuff (which is just giving us a 'nfc-tech-discovered'
> notification?) and add a new event which would be dispatched to mozNfc.
> The event would be dispatched after the notification.
I think, for Gaia developers, this may not be intuitive (Please correct me if I have not understood your point here. Maybe I am missing your point.) as they may expect some data / info as part of activityHandler.
Nfc Gaia apps will have to specify an entry in their manifest webapp to receive activity messages + register for an event (dom call) for example 'onnfctagfound'. There needs to be a glue in lower layers to bind these two messaging ways (Activities are System messages & EventHandlers are dom way) to ensure that there are no race conditions and ensure that dom event call does not happen before activity is launched. There needs to be some co-ordination in place. I think with the current design of Nfc this may be cumbersome. What do you think?
Updated•11 years ago
|
blocking-b2g: --- → backlog
Comment on attachment 8402146 [details] [diff] [review] Part 2: v1 - Remove the usage of session from Nfc APIs getNFCTag() & getNFCPeer(). r=khuey, sr=smaug It looks like Olli is handling this. If I'm wrong please rerequest review.
Attachment #8402146 -
Flags: review?(khuey)
Comment 24•10 years ago
|
||
just a heads up: NFC functionality has been broken in master and 2.1 ever since since bug 1069188 landed: getNfcTag expects a session token, but it is no longer being provided via nfc-ndef-discovered, so getting the tag object is impossible right now.
Assignee | ||
Comment 25•10 years ago
|
||
Hi Krzysztof, Could you please respond to the concern raised in the comment#24 please, since you have dealing with these changes now ? Thanks
Flags: needinfo?(kmioduszewski)
Reporter | ||
Comment 26•10 years ago
|
||
getNFCTag should be only used by System app. To get a tag object should be done in Bug 991970.
Flags: needinfo?(kmioduszewski)
Comment 27•10 years ago
|
||
(In reply to iacobs from comment #24) > just a heads up: NFC functionality has been broken in master and 2.1 ever > since since bug 1069188 landed: getNfcTag expects a session token, but it is > no longer being provided via nfc-ndef-discovered, so getting the tag object > is impossible right now. Bug 1069188 has landed only on master, there were no changes introduced in 2.1. I'm not sure if you have any particular use case in mind, but unless you're not planning to write something onto NFC tag you don't need MozNFCTag object for now. Whole contents of a valid NDEF Message is attached to nfc-ndef-discovered (or different nfc originated activity), see here [1]. BTW, If you do plan to write something on NFC tag you're app needs to be certified. [1] - https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_manager.js#L405
Comment 28•10 years ago
|
||
(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #27) > Bug 1069188 has landed only on master, there were no changes introduced in > 2.1. Mkay, I'll rebuild for 2.1 then and check why it doesn't work then; I remember moving to master because the thing stopped working at some point. > I'm not sure if you have any particular use case in mind, but unless > you're not planning to write something onto NFC tag you don't need MozNFCTag > object for now. Well, tags are blank when you first get them (I got some of these: http://www.rfidshop.com/45mm-label-tag-mifare-ultralightnfc-393-p.asp - they are NDEF formatable); my app needs to put some NDEF info into them (basically an UUID), so it can retrieve it later. > Whole contents of a valid NDEF Message is attached to > nfc-ndef-discovered (or different nfc originated activity), see here [1]. I know; however, I still need to write, so... > BTW, If you do plan to write something on NFC tag you're app needs to be > certified. The app needs to be certified for any kind of NFC access from what I could see back when I wrote it, even for activities, so I wasn't hoping I'd ever put it in the app store anyway (I'll refrain from commenting on this because I'm not familiar with the thinking process that lead to it and I'd come off as abrasive / impolite). Can't wait for proper events, to be honest (the ontagfound / ontaglost thing mentioned above), the current ways are beyond clunky...
Comment 29•10 years ago
|
||
I haven't been checking tags recently (the main focus turned to P2P user stories that don't involve tags since that was generating the most interest) but there should be some kind of "empty" NDEF message generated by NFC manager if it finds a completely empty, NDEF-compatible tag. Create a message handler for that in the (certified) app to get the empty tag as well as the specific self-selected mime-type filter for your app. Any ambiguities will trigger a application picker, so be careful with empty. There are bugs associated with making the APIs more W3C complaint for the purposes of privilaged permission usage (marketplace). It's been a bit of an uphill battle to find the best way to resolve for the available platform APIs for processes mapping to Web APIs (for apps). Bug 991970, parent is Bug 1042851.
Comment 30•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_manager.js#L175 nfc-ndef-discovered is only fired when something that has NDEF information in it, which kinda makes sense; my app associates photos with tags (the photo is named with the tag id and is recalled next time you scan), and when formatting it just writes a text NDEF message with UUID4 in the id field and empty payload (I think there is only room for 5 or 6 characters on my 512bit tags, after the UUID and the NDEF overhead - they are made for things like smart posters).
Reporter | ||
Comment 31•10 years ago
|
||
Now we will implement ontagfound/onpeerfound, and in these callbacks we will provide MozNFCTag and MozNFCPeer so sessionToken can be removed. I'll make this bug WONTFIX, also remove the blocking of Bug 991970.
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•