1082475 - B2G NFC: move 'nfc' permission to privilege.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

In Bug 1048676 we're going to merge nfc-read and nfc-write permissions into 'nfc'.
In this bug we'd change 'nfc' to privilage level.

Comment 1

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

We have Bug 1094147 for privilege permission, close this as WONTFIX.

Comment 2

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

Attachment: Patch.

Comment 3

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

Comment on attachment 8528793 [details] [diff] [review]
Patch.

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

Hi,
Right now the following APIs will use nfc permission

- MozNFC
* ontagfound
* ontaglost
* onpeerfound
* onpeerlost

- MozNFCTag
* readNDEF
* writeNDEF
* some other attributes.

- MozNFCPeer
* sendNDEF
* some other attributes.

- MozNFCTagEvent (will be passed in ontagfound)
- MozNFCPeerEvent (will be passed in onpeerfound/onpeerready)

And we plan to open these API to privilege apps.

Thanks

Comment 4

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

Comment on attachment 8528793 [details] [diff] [review]
Patch.

Why ALLOW and why not PROMPT?

Comment 5

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

Comment on attachment 8528793 [details] [diff] [review]
Patch.

Isn't nfc in a way similar to camera, which has prompt.
(camera "reads" visual data of the surroundings, nfc reads data from something nearby).

Feel free to explain why ALLOW should be used, and re-ask review.

But if ptheriault and jonas disagree with me, and think that ALLOW is fine, then the patch is ok.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think NFC is more like TCPSocket and systemXHR.

I.e. it's very hard to explain to the user what granting NFC access means. What risks are involved in granting it, what risks the user does not have to worry about, and what functionality will break by not granting it.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8528793 [details] [diff] [review]
Patch.

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

Hi Yoshi,

I'm happy to generally defer to you here. I.e. as long as you think we should expose this API and keep it stable, then I'm fine with it.

The only thing that I wanted to make sure on top of this was that none of these functions can trigger the shrinking UI, which seems like that's the case. So sounds good to me.

Comment 8

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

(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8528793 [details] [diff] [review]
> Isn't nfc in a way similar to camera, which has prompt.
> (camera "reads" visual data of the surroundings, nfc reads data from
> something nearby).
>

Hi, smaug
Thanks for your comment.

Sorry I didn't explain why I use ALLOW here.

If you mean when we turn on NFC, we'd also prompt like camera case.
Right now turning on NFC needs 'nfc-manager' permission, which is still certified.
And should not be related to this bug.

And one important feature of NFC is it can only be operated within 3~5 cm, the range is so
close so I think it also involves some trust from the user.

For example,
when you take public transport system, you just tap your transportation card on
the reader, you don't have to type your PIN code on that reader.
Now image that your transportation card is your phone, you tab the phone on the reader,
and it immediately charges you, just like using a card, you don't have to do any user interactions. You just get a notification that you have been charged. And this works the same way even when your phone is out of battery.

And for the P2P Sharing case, 
for sending side, it's like the above case, user takes the phone to some other
device or tag with 3~5 cm, this already has some degree of trust by the user.

And for the receiving side,
unlike bluetooh, the receiving side has to confirm
he wants to receive this file, once the user grants the request the transfer will start.
For NFC case, once the two devices have established the physical link with each other, any side
could send data (NDEF) to the other side, without any acknowlegement from the other side.

We should let applications to choose its own UX, whether to inform user or not.
But if we implement this NFC API with some permission dialog, this may have some 
UX influence on those apps.

Do this make sense to you?

Thanks

Comment 9

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

Comment on attachment 8528793 [details] [diff] [review]
Patch.

r? to smaug again base on my comment in Comment 8.

Comment 10

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

(In reply to Jonas Sicking (:sicking) from comment #7)
> The only thing that I wanted to make sure on top of this was that none of
> these functions can trigger the shrinking UI, which seems like that's the
> case. So sounds good to me.
Hi Jonas

I think you mean these APIs cannot trigger Shrinking UI and those DOM callbacks (ontagfound, ontaglost, onpeerfound, onpeerlost) will NOT be triggered by Shrinkging UI.

If so, YES.

Because once app get *onpeerfound* callback, it could trigger its own Sharing UI or default Shrinking UI at its own will.
That's the freedom we'd like to deliever to the developers.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Yoshi Huang[:allstars.chh] from comment #10)
> I think you mean these APIs cannot trigger Shrinking UI and those DOM
> callbacks (ontagfound, ontaglost, onpeerfound, onpeerlost) will NOT be
> triggered by Shrinkging UI.
> 
> If so, YES.

Yes, that's what I meant! Sorry I was unclear.

> Because once app get *onpeerfound* callback, it could trigger its own
> Sharing UI

Absolutely. I fully agree.

> or default Shrinking UI at its own will.

Are you saying that there is a separate API for apps to trigger our shrinking-ui implementation? I would prefer not to do that and instead give them a JS library which implements a shrinking UI that looks like ours. Would that be ok?

Comment 12

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

(In reply to Jonas Sicking (:sicking) from comment #11)
> > or default Shrinking UI at its own will.
> 
> Are you saying that there is a separate API for apps to trigger our
> shrinking-ui implementation? 
No

> I would prefer not to do that and instead give
> them a JS library which implements a shrinking UI that looks like ours.
Yeah, I mean the app could import shrinking_ui.js, which is in our share JS library in Gaia,
and the app could launch the shrinking_ui.js in onpeerfound to have the same UX with our apps.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Awesome, that sounds great to me.

Comment 14

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

(In reply to Yoshi Huang[:allstars.chh] from comment #8)
> If you mean when we turn on NFC, we'd also prompt like camera case.
I didn't mean that. turning NFC on should be certified.

I meant reading data when nfc is enabled.



> We should let applications to choose its own UX, whether to inform user or
> not.
> But if we implement this NFC API with some permission dialog, this may have
> some 
> UX influence on those apps.
Indeed.


But ok, r+, I think I can live with this.

Comment 15

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

Comment on attachment 8528793 [details] [diff] [review]
Patch.

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

As we discussed, this looks fine and we'll continue the sec-review once this is landed.

Comment 16

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

https://hg.mozilla.org/integration/b2g-inbound/rev/349ab6e6522d

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/349ab6e6522d