1055477 - B2G NFC: Add 'CheckPermission' and AvailableIn in NFC WebIDLs.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

Right now the function Navigator::HasNfcSupport will also check permissions, however we should do the permission check in the WebIDL part.

Also add AvaialbleIn for these NFC WebIDLs.

Comment 1

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

Attachment: Part 1: Add CheckPermission

Comment 2

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

Attachment: Part 2: Add AvailableIn

Comment 3

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

Comment on attachment 8475090 [details] [diff] [review]
Part 1: Add CheckPermission

!!win is a bit odd, and CheckPermissions in BindingUtils.cpp does similar check
already. So you could just return true there.

Comment 4

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

Comment on attachment 8475091 [details] [diff] [review]
Part 2: Add AvailableIn

.webidl changes are fine to me, but I think ehsan or sicking should give the
opinion why nfc stuff needs to be for certifiedapps only

Comment 5

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

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8475091 [details] [diff] [review]
> Part 2: Add AvailableIn
> 
> .webidl changes are fine to me, but I think ehsan or sicking should give the
> opinion why nfc stuff needs to be for certifiedapps only

Hi smaug
Actually I am not sure when to use AvailableIn.

"nfc" permission is already certified now.
http://lxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#326

It seems to me CheckPermission is already enough.
If AvailableIn is not mandatory I'll remove it and land only Part 1 patch.

Comment 6

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

Comment on attachment 8475091 [details] [diff] [review]
Part 2: Add AvailableIn

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

Hi Ehsan
Can you help to review this patch for me?
Right now NFC is certified API, so I add AvailableIn in these WebIDLs.

This came to my mind base on your review on Bug 879861 (NFC Secure Element Support ) and Bug 951976 (API for Resource Statistics), you suggested to add AvailableIn on these WebIDLS.

However there's one thing I am not quite sure.
These WebIDLs already have CheckPermissions attribute, and why should we need to add "AvailableIn" to these?

For example, nfc-manager or resource-manager is already a certified permission, so CheckPermissions="resource-manager" implied the caller is already a certified app. Do we still need to add AvailableIn="CertifiedApps" in this case?

Thank you

Comment 7

[(no longer active)]

(In reply to Yoshi Huang[:allstars.chh] from comment #6)
> Comment on attachment 8475091 [details] [diff] [review]
> Part 2: Add AvailableIn
> 
> Review of attachment 8475091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Ehsan
> Can you help to review this patch for me?
> Right now NFC is certified API, so I add AvailableIn in these WebIDLs.
> 
> This came to my mind base on your review on Bug 879861 (NFC Secure Element
> Support ) and Bug 951976 (API for Resource Statistics), you suggested to add
> AvailableIn on these WebIDLS.
> 
> However there's one thing I am not quite sure.
> These WebIDLs already have CheckPermissions attribute, and why should we
> need to add "AvailableIn" to these?
> 
> For example, nfc-manager or resource-manager is already a certified
> permission, so CheckPermissions="resource-manager" implied the caller is
> already a certified app. Do we still need to add AvailableIn="CertifiedApps"
> in this case?

Our permissions table is the ultimate source of truth when it comes to what kinds of apps can request what types of permissions.  So for example, as you suggest, if the resource-manager permission can only be granted to the certified apps, adding the [AvailableIn=CertifiedApps] on top of [CheckPermissions="resource-manager"] won't change where the API is exposed.  It would, however, make it *much* easier to read the IDL and figure out where the API is exposed.  It also makes exposing the API to let's say privileged apps require a WebIDL change, which means it will require a DOM peer review.  Both of these are great results, and I would like to make more APIs use AvailableIn for this reason.  If you know of more, please keep these kinds of patches coming.  :-)  Much appreciated.

Comment 8

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> (In reply to Yoshi Huang[:allstars.chh] from comment #6)
Thanks for the detail explaination. I got it now.
Thank you.

Comment 9

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

Attachment: Part 3: Update MozNFCPeerEvent and mochitests.

Hi, Smaug
I found I forgot to update MozNFCPeerEvent.webidl, and upload another patch for that. Also update test_interfaces.html.

Could you help to review this for me ?

Thanks

Comment 10

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

Try https://tbpl.mozilla.org/?tree=Try&rev=b9d6bf58c006

Comment 11

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

Comment on attachment 8475090 [details] [diff] [review]
Part 1: Add CheckPermission

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

::: dom/base/Navigator.cpp
@@ +2314,5 @@
>    if (!contentHelper) {
>      return false;
>    }
>  
> +  return !!win;

if return true directly is okay,
I'll update the patch to

nsCOMPtr<nsISupports> contentHelper =  do_GetService("@mozilla.org/nfc/content-helper;1");
return !!contentHelper;

Comment 12

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

Oh, sure that is fine.

Comment 13

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

Comment on attachment 8476591 [details] [diff] [review]
Part 3: Update MozNFCPeerEvent and mochitests.

Hmm, we don't have a way to test AvailableIn stuff in test_interfaces :/

Comment 14

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

Attachment: Part 1. Add CheckPermission v2.

comment addressed.

Comment 15

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

https://hg.mozilla.org/integration/b2g-inbound/rev/8507f750761a
https://hg.mozilla.org/integration/b2g-inbound/rev/7e28bef4ef4f
https://hg.mozilla.org/integration/b2g-inbound/rev/48d86bbd3cc6

Comment 16

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

forgot to paste try : https://tbpl.mozilla.org/?tree=Try&rev=970d0cbf6ab3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8507f750761a
https://hg.mozilla.org/mozilla-central/rev/7e28bef4ef4f
https://hg.mozilla.org/mozilla-central/rev/48d86bbd3cc6