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

RESOLVED FIXED in 2.2 S1 (5dec)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: allstars.chh, Assigned: allstars.chh)

Tracking

({dev-doc-complete})

unspecified
2.2 S1 (5dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=5])

Attachments

(3 attachments, 8 obsolete attachments)

46 bytes, text/x-github-pull-request
alive
: review+
dkuo
: review+
dkuo
: review+
arthurcc
: review+
arcturus
: review+
Details | Review
8.09 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
6.88 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
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)
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: nobody → allstars.chh
Assignee: allstars.chh → nobody
Assignee: nobody → allstars.chh
Attachment #8517884 - Attachment is obsolete: true
Attachment #8517887 - Flags: review?(dlee) → review+
Attachment #8517885 - Flags: review?(dlee) → review+
Summary: B2G NFC: merge nfc-read and nfc-write permissions into nfc → B2G NFC: Add a privileged nfc permission
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.
Attachment #8517887 - Attachment is obsolete: true
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)
Attachment #8518766 - Flags: review?(dlee) → review+
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+
Flagging Paul to check the permission level. You'll also need a DOM peer to r+ the webidl changes.
Dimi's comments addressed.
Attachment #8518767 - Attachment is obsolete: true
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+
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)
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)
Attachment #8518766 - Flags: review?(bugs) → review+
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.
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.
Duplicate of this bug: 1094147
(In reply to Yoshi Huang[:allstars.chh] from comment #16)
> I'll enumerate the detail API using privileged permission in another bug,
Bug 1082475,
Attachment #8518766 - Attachment is obsolete: true
Attachment #8518766 - Flags: feedback?(ptheriault)
Attachment #8528790 - Flags: review?(dlee)
Attachment #8519652 - Attachment is obsolete: true
Attachment #8528791 - Flags: review?(dlee)
Attachment #8528790 - Flags: review?(dlee) → review+
Attachment #8528791 - Flags: review?(dlee) → review+
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)
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.
Attachment #8528790 - Flags: review?(bugs) → review+
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 on attachment 8529464 [details] [review]
Gaia Pull Request

r=me on the settings part.
Attachment #8529464 - Flags: review?(arthur.chen) → review+
add nfc-share permission check in notifyPeerLost.
Attachment #8528790 - Attachment is obsolete: true
Attachment #8529540 - Flags: review+
Attachment #8529464 - Flags: review?(alive) → review+
Comment on attachment 8529464 [details] [review]
Gaia Pull Request

Change in comms looking trivial.

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

w000ts, fogot the r+ :)
Attachment #8529464 - Flags: review?(francisco) → review+
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+
Whiteboard: [p=5]
Target Milestone: --- → 2.2 S1 (5dec)
(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)
Flags: needinfo?(stephouillon)
You need to log in before you can comment on or make changes to this bug.