1048676 - B2G NFC: Add "nfc" and "nfc-share" permissions.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

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)

Comment 1

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

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.

Comment 2

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

Attachment: Part 1: WebIDL and DOM change

Comment 3

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

Attachment: Part 2: Nfc.js and test case change

Comment 4

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

Attachment: Part 1: WebIDL and DOM change.

Comment 5

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

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.

Comment 6

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

Attachment: Part 1: WebIDL and DOM change. v2.

Comment 7

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

Attachment: Part 2: Nfc.js and test case change v2.

Comment 8

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

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.

Comment 9

[Dimi Lee [:dimi]]

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

Comment 10

[[:fabrice] Fabrice Desré]

Flagging Paul to check the permission level. You'll also need a DOM peer to r+ the webidl changes.

Comment 11

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

Attachment: Part 2: Nfc.js and test case change v3.

Dimi's comments addressed.

Comment 12

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

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.

Comment 13

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

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

Comment 14

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

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.

Comment 15

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

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'.

Comment 16

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

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.

Comment 18

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

(In reply to Yoshi Huang[:allstars.chh] from comment #16)
> I'll enumerate the detail API using privileged permission in another bug,
Bug 1082475,

Comment 19

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

Attachment: Part 1: WebIDL and DOM change. v3.

Comment 20

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

Attachment: Part 2: Nfc.js and test case change v4.

Comment 21

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

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

Comment 22

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

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.

Comment 23

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

Attachment: Gaia Pull Request

Comment 24

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

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.

Comment 25

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8529464 [details] [review]
Gaia Pull Request

r=me on the settings part.

Comment 26

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

Attachment: Part 1: WebIDL and DOM change. v4.

add nfc-share permission check in notifyPeerLost.

Comment 27

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8529464 [details] [review]
Gaia Pull Request

Change in comms looking trivial.

Thanks!

Comment 28

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8529464 [details] [review]
Gaia Pull Request

w000ts, fogot the r+ :)

Comment 29

[Dominic Kuo [:dkuo]]

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.

Comment 30

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

Attachment: Part 2: Nfc.js and test case change v5

rebase

Comment 31

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

https://hg.mozilla.org/integration/b2g-inbound/rev/cee5af0a675a
https://hg.mozilla.org/integration/b2g-inbound/rev/8899e46bffff

Comment 32

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

Gaia: https://github.com/mozilla-b2g/gaia/commit/c3bf96d0db81056802ff544112738873a817827f

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cee5af0a675a
https://hg.mozilla.org/mozilla-central/rev/8899e46bffff

Comment 34

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

(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.

Comment 35

[Chris Mills [:cmills]]

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