Closed
Bug 1048676
Opened 7 years ago
Closed 6 years ago
B2G NFC: Add "nfc" and "nfc-share" permissions.
Categories
(Firefox OS Graveyard :: NFC, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.2 S1 (5dec)
People
(Reporter: allstars.chh, Assigned: allstars.chh)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [p=5])
Attachments
(3 files, 8 obsolete files)
Right now we use nfc-read, and nfc-write permissions, which are all certified. And these permissions are both used by MozNFCPeer and MozNFCTag, for example, nfc-write is used by MozNFCTag.writeNDEF and MozNFCPeer.sendNDEF. I'd like to proposal a new privilege permission 'nfc-tag' for NFC, and this permission is only used by NFCTag, so we could change MozNFCTag APIs to privilege more easily without waiting MozNFCPeer APIs are ready. Right now the APIs used by MozNFCTag are only - readNDEF - writeNDEF - getDetails - connect/close (Bug 963541)
Assignee | ||
Updated•7 years ago
|
Blocks: b2g-nfc-privilege
Assignee | ||
Comment 1•6 years ago
|
||
Given it will be too complicated to have two permissions doing the same things mostly at the same time, I'd change this bug to merge nfc-read/nfc-write permission.
Summary: B2G NFC: add new privilege permission for NFC → B2G NFC: merge nfc-read and nfc-write permissions into nfc
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → allstars.chh
Assignee | ||
Updated•6 years ago
|
Assignee: allstars.chh → nobody
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → allstars.chh
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #8517884 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8517887 -
Flags: review?(dlee)
Assignee | ||
Updated•6 years ago
|
Attachment #8517885 -
Flags: review?(dlee)
Updated•6 years ago
|
Attachment #8517887 -
Flags: review?(dlee) → review+
Updated•6 years ago
|
Attachment #8517885 -
Flags: review?(dlee) → review+
Assignee | ||
Updated•6 years ago
|
Summary: B2G NFC: merge nfc-read and nfc-write permissions into nfc → B2G NFC: Add a privileged nfc permission
Assignee | ||
Comment 5•6 years ago
|
||
After discussed with Dimi, we decide to merge nfc-read/nfc-write into nfc, and change nfc to privileged. Also for non-ready yet feature like ontagfound/lost/peerfound, I'll use nfc-manager permission for these. And once we're ready I'll file another bug to change these API to nfc permission.
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8517887 -
Attachment is obsolete: true
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #8517885 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8518766 -
Flags: review?(dlee)
Assignee | ||
Comment 8•6 years ago
|
||
Comment on attachment 8518767 [details] [diff] [review] Part 2: Nfc.js and test case change v2. Review of attachment 8518767 [details] [diff] [review]: ----------------------------------------------------------------- Update on Nfc.js. assertPermission for nfc and nfc-manager.
Attachment #8518767 -
Flags: review?(dlee)
Updated•6 years ago
|
Attachment #8518766 -
Flags: review?(dlee) → review+
Comment 9•6 years ago
|
||
Comment on attachment 8518767 [details] [diff] [review] Part 2: Nfc.js and test case change v2. Review of attachment 8518767 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/gonk/Nfc.js @@ +291,2 @@ > return null; > } Update the naming of NFC_IPC_READ_PERM_MSG_NAMES & NFC_IPC_WRITE_PERM_MSG_NAMES to reflect current change. "NFC:MakeReadOnly" should use nfc-manager permission check
Attachment #8518767 -
Flags: review?(dlee) → review+
Comment 10•6 years ago
|
||
Flagging Paul to check the permission level. You'll also need a DOM peer to r+ the webidl changes.
Assignee | ||
Comment 11•6 years ago
|
||
Dimi's comments addressed.
Attachment #8518767 -
Attachment is obsolete: true
Assignee | ||
Comment 12•6 years ago
|
||
Comment on attachment 8519652 [details] [diff] [review] Part 2: Nfc.js and test case change v3. Review of attachment 8519652 [details] [diff] [review]: ----------------------------------------------------------------- This patch has been r+ by Dimi before.
Attachment #8519652 -
Flags: review+
Assignee | ||
Comment 13•6 years ago
|
||
Comment on attachment 8518766 [details] [diff] [review] Part 1: WebIDL and DOM change. v2. Add r? to smaug for the WebIDL changed. We will change AvailableIn to PrivilegedApps in Bug 1091316
Attachment #8518766 -
Flags: review?(bugs)
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 8518766 [details] [diff] [review] Part 1: WebIDL and DOM change. v2. Review of attachment 8518766 [details] [diff] [review]: ----------------------------------------------------------------- Ask Paul's feedback per Fabrices's request. Also I've filed a bug for Sec-Review of NFC privileged API in Bug 1095417.
Attachment #8518766 -
Flags: feedback?(ptheriault)
Updated•6 years ago
|
Attachment #8518766 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 15•6 years ago
|
||
Since Jonas requests not to open Share API, I'll update these patches. My plan is to - merge 'nfc-read' and 'nfc-write' into 'nfc' permission, and make it privilegde. - add a certified 'nfc-share' permission, and this permission will protect 'onpeeready' and 'MozNFCPeer.sendFile'.
Summary: B2G NFC: Add a privileged nfc permission → B2G NFC: Add a privileged "nfc" and a certified "nfc-share" permissions.
Assignee | ||
Comment 16•6 years ago
|
||
I'll enumerate the detail API using privileged permission in another bug, so in this bug I will just merge nfc-read/nfc-write into nfc, and add a nfc-share.
Summary: B2G NFC: Add a privileged "nfc" and a certified "nfc-share" permissions. → B2G NFC: Add "nfc" and "nfc-share" permissions.
Assignee | ||
Comment 18•6 years ago
|
||
(In reply to Yoshi Huang[:allstars.chh] from comment #16) > I'll enumerate the detail API using privileged permission in another bug, Bug 1082475,
Assignee | ||
Comment 19•6 years ago
|
||
Attachment #8518766 -
Attachment is obsolete: true
Attachment #8518766 -
Flags: feedback?(ptheriault)
Attachment #8528790 -
Flags: review?(dlee)
Assignee | ||
Comment 20•6 years ago
|
||
Attachment #8519652 -
Attachment is obsolete: true
Attachment #8528791 -
Flags: review?(dlee)
Updated•6 years ago
|
Attachment #8528790 -
Flags: review?(dlee) → review+
Updated•6 years ago
|
Attachment #8528791 -
Flags: review?(dlee) → review+
Assignee | ||
Comment 21•6 years ago
|
||
Comment on attachment 8528790 [details] [diff] [review] Part 1: WebIDL and DOM change. v3. Hi, smaug Sorry for requesting r? again. Because Jonas requested that we shouldn't open NFC-Share API in [1], so I changed this patch. So in this version, I merge nfc-read/nfc-write into nfc, and also add another permission nfc-share. Only MozNFC.onpeeready, and MozNFCPeer.sendFile will use nfc-share, and nfc-share still remains certified permission. Also I move changing 'nfc' to privileged in Bug 1082475, since it maybe involve more reviews from different reviewer. Thanks [1] : https://bugzilla.mozilla.org/show_bug.cgi?id=1042851#c8
Attachment #8528790 -
Flags: review?(bugs)
Assignee | ||
Comment 22•6 years ago
|
||
Comment on attachment 8528790 [details] [diff] [review] Part 1: WebIDL and DOM change. v3. Review of attachment 8528790 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/nsNfc.js @@ +481,5 @@ > dump("this._window or this.__DOM_IMPL__ is a dead wrapper."); > return; > } > > + if (!this.checkPermissions(["nfc"])) { for onpeeready case, I think I also need to add 'nfc-share' permission, will address this in next version.
Updated•6 years ago
|
Attachment #8528790 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 23•6 years ago
|
||
Assignee | ||
Comment 24•6 years ago
|
||
Comment on attachment 8529464 [details] [review] Gaia Pull Request Hi, we'd like to open some NFC API to privileged, so we updated some permissions for NFC. We merged nfc-read/nfc-write into nfc, and this permission will be privileged in the near future. So for the original NFC Sharing functionality, we will replace it with nfc-share. @Alive, could you review the System app part? @David, could you review the Video app/Gallery app part? @Dominic, could you review the Music app part ? @Arthur, could you review the Settings app part? (Settings app only use nfc permission since it only uses 'navigator.mozNfc') @Francisco, could you review the Contacts app part ? Thanks.
Attachment #8529464 -
Flags: review?(francisco)
Attachment #8529464 -
Flags: review?(dkuo)
Attachment #8529464 -
Flags: review?(dflanagan)
Attachment #8529464 -
Flags: review?(arthur.chen)
Attachment #8529464 -
Flags: review?(alive)
Comment 25•6 years ago
|
||
Comment on attachment 8529464 [details] [review] Gaia Pull Request r=me on the settings part.
Attachment #8529464 -
Flags: review?(arthur.chen) → review+
Assignee | ||
Comment 26•6 years ago
|
||
add nfc-share permission check in notifyPeerLost.
Attachment #8528790 -
Attachment is obsolete: true
Attachment #8529540 -
Flags: review+
Updated•6 years ago
|
Attachment #8529464 -
Flags: review?(alive) → review+
Comment 27•6 years ago
|
||
Comment on attachment 8529464 [details] [review] Gaia Pull Request Change in comms looking trivial. Thanks!
Comment 28•6 years ago
|
||
Comment on attachment 8529464 [details] [review] Gaia Pull Request w000ts, fogot the r+ :)
Attachment #8529464 -
Flags: review?(francisco) → review+
Comment 29•6 years ago
|
||
Comment on attachment 8529464 [details] [review] Gaia Pull Request Trivial fix so looks good to me! I also r+ the gallery and video parts for David because he might be busy and the fixes are exactly the same as music's.
Attachment #8529464 -
Flags: review?(dkuo)
Attachment #8529464 -
Flags: review?(dflanagan)
Attachment #8529464 -
Flags: review+
Assignee | ||
Comment 30•6 years ago
|
||
rebase
Attachment #8528791 -
Attachment is obsolete: true
Attachment #8531470 -
Flags: review+
Assignee | ||
Comment 31•6 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/cee5af0a675a https://hg.mozilla.org/integration/b2g-inbound/rev/8899e46bffff
Assignee | ||
Updated•6 years ago
|
Whiteboard: [p=5]
Target Milestone: --- → 2.2 S1 (5dec)
Assignee | ||
Comment 32•6 years ago
|
||
Gaia: https://github.com/mozilla-b2g/gaia/commit/c3bf96d0db81056802ff544112738873a817827f
Comment 33•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cee5af0a675a https://hg.mozilla.org/mozilla-central/rev/8899e46bffff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Fabrice Desré [:fabrice] from comment #10) > Flagging Paul to check the permission level. You'll also need a DOM peer to > r+ the webidl changes. /me catches the invisible flag. Looks fine to me, but Stephanie is handling this review, so I'll let her comment.
Flags: needinfo?(stephouillon)
Updated•6 years ago
|
Keywords: dev-doc-needed
Updated•6 years ago
|
Flags: needinfo?(stephouillon)
Comment 35•6 years ago
|
||
I have updated the permissions information here: https://developer.mozilla.org/en-US/Apps/Build/App_permissions#nfc And here: https://developer.mozilla.org/en-US/docs/Web/API/NFC_API/Using_the_NFC_API#Permissions Also to confirm, this change is mentioned in the FxOS 2.2 release notes: https://developer.mozilla.org/en-US/Firefox_OS/Releases/2.2
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•