Closed Bug 1023266 Opened 10 years ago Closed 10 years ago

Make the Mobile ID API privileged

Categories

(Firefox OS Graveyard :: Gaia::Settings, defect)

defect
Not set
normal

Tracking

(feature-b2g:2.0, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S5 (4july)
feature-b2g 2.0
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: ferjm, Assigned: ferjm)

References

Details

(Keywords: late-l10n, Whiteboard: ft:loop)

Attachments

(3 files, 2 obsolete files)

We missed that detail in bug 988469...
Assignee: nobody → ferjmoreno
We need to have this in 2.0 for Loop so nominating
blocking-b2g: --- → 2.0?
Is it only available to certified apps right now?
Flags: needinfo?(ferjmoreno)
Given loop is privileged we need to fix this, and this looks like a feature miss or fallout.
blocking-b2g: 2.0? → ---
feature-b2g: --- → 2.0
Attached patch v1 (obsolete) — Splinter Review
Attachment #8438655 - Flags: review?(jonas)
Flags: needinfo?(ferjmoreno)
(In reply to Andrew Overholt [:overholt] from comment #2)
> Is it only available to certified apps right now?

No, it was exposed to any app by mistake.
Attachment #8438655 - Flags: review?(jonas)
Attachment #8438655 - Flags: review?(jonas)
Let's make sure we track that there are some pending changes to get "mobileid" defaults in PermissionsTable.jsm and a way to properly revoke.
(In reply to Frederik Braun [:freddyb] from comment #6)
> Let's make sure we track that there are some pending changes to get
> "mobileid" defaults in PermissionsTable.jsm and a way to properly revoke.

Bug 1026549
Whiteboard: ft:loop
Attached patch v1 (obsolete) — Splinter Review
Attachment #8438655 - Attachment is obsolete: true
Attachment #8438655 - Flags: review?(jonas)
Attachment #8442178 - Flags: review?(jonas)
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #7)
> (In reply to Frederik Braun [:freddyb] from comment #6)
> > Let's make sure we track that there are some pending changes to get
> > "mobileid" defaults in PermissionsTable.jsm and a way to properly revoke.
> 
> Bug 1026549

I finally decided to do it in this bug so we save one 2.0 nomination. Sorry for the noise.
Attachment #8442175 - Flags: review?(arthur.chen) → review+
Thanks Arthur!

The Gaia side can land without the Gecko one.

https://github.com/mozilla-b2g/gaia/commit/8ed871a402eb6a7b47eef033038a86c2bd7fbc20
Keywords: late-l10n
THe string freeze for 2.0 was 6/20, I do not think we can land this in 2.0 at this point. can you explain the user impact here ? We should try to pursue alternatives without string changes on this one.
Flags: needinfo?(oteo)
Flags: needinfo?(ferjmoreno)
First of all, let us apologize for not realizing this required late-l10n and not landing it on Friday despite having the r+. We are really sorry about that. 

I would like :ferjm to confirm, but what the gaia patch includes is a way to revoke the “Mobile identity” permission to an application the user has granted access before. The new string will be shown to identify this permission in the list of application permissions that is shown in the Settings Application (Settings -> App Permissions > “App Name”).

I can think in some solutions:

 1/ We don’t give users the possibility to revoke this permission
 2/ We don’t localize this permission (i.e. In all the languages “Mobile Identity” is shown for this permission)
 3/ We reuse the “Mobile identity” string that is already available in https://github.com/mozilla-b2g/gaia/blob/master/apps/system/mobile_id/locales/mobile_id.en-US.properties#L21 (not sure if this OK from a localization point of view). 

As said, I would like Fernando to confirm if my understanding is correct, but in the meantime, let me know what you think. Apologies again
Flags: needinfo?(oteo)
Comment on attachment 8442178 [details] [diff] [review]
v1

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

You should make sure that this gets hooked up to the hasFeature function so that the marketplace can detect that this function is available. Ehsan knows better how that works.

I also think that we have added functions to WebIDL which enables testing for permissions directly in WebIDL so that you don't need to write any code manually.
Attachment #8442178 - Flags: review?(jonas) → review?(ehsan)
Hmm.. Technically you shouldn't test for ALLOW/PROMPT, but rather if the permission was enumerated in the manifest. I.e. the user choosing DENY shouldn't make the function disappear.

What do we do elsewhere?
(In reply to Maria Angeles Oteo (:oteo) from comment #14)
>  3/ We reuse the “Mobile identity” string that is already available in
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/mobile_id/
> locales/mobile_id.en-US.properties#L21 (not sure if this OK from a
> localization point of view). 

How would you do that, from a technical point of view?
As I said, I am not an expert in l10n so was just suggesting possible ways forward. 

I didn’t know if the deadline was for asking for translations and in that case we could ask translators to use the same string for both situations. If the problem is that we cannot even add the string identifier (even if translation is the same than an old one), I can only think on two technical solutions:

 1/ Settings import MobileID locales (I know this is ugly)
 2/ The new string is copied from the existing one during build generation (I know this is even uglier)

If none of these options (asking translators for re-using the string or the technical ones) are feasible, the only thing we could do is not translating "Mobile Identifier" string in settings application.

But as said in comment 14 I would like Fernando to chime in.
(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #13)
> THe string freeze for 2.0 was 6/20, I do not think we can land this in 2.0
> at this point. can you explain the user impact here ? We should try to
> pursue alternatives without string changes on this one.

I thought that was what the late-10n keyword meant [1].

What I am missing here is asking review to :stas. Sorry about that.

In any case, not landing this Gaia patch means that the user won't be able to change a permission given to an app using the Mobile Identity API as it won't appear in the corresponding Settings menu.

[1] https://wiki.mozilla.org/L10n:B2G/Developers
Flags: needinfo?(ferjmoreno)
Comment on attachment 8442175 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20696

Stas, sorry for missing asking for your review for the l10n part of this patch before. We need to uplift this patch to 2.0, is that still possible after the string freeze?
Attachment #8442175 - Flags: review?(stas)
(In reply to Jonas Sicking (:sicking) from comment #15)
> Comment on attachment 8442178 [details] [diff] [review]
> v1
> 
> Review of attachment 8442178 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should make sure that this gets hooked up to the hasFeature function so
> that the marketplace can detect that this function is available. Ehsan knows
> better how that works.
> 

Ok, but that sounds like another bug, right?

> I also think that we have added functions to WebIDL which enables testing
> for permissions directly in WebIDL so that you don't need to write any code
> manually.

We have https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#CheckPermissions but it seems that it won't work with the PROMPT_ACTION permission.

(In reply to Jonas Sicking (:sicking) from comment #16)
> Hmm.. Technically you shouldn't test for ALLOW/PROMPT, but rather if the
> permission was enumerated in the manifest. I.e. the user choosing DENY
> shouldn't make the function disappear.
> 
> What do we do elsewhere?

That's true! I'll change that in the patch. Thanks!
Attachment #8442178 - Flags: review?(ehsan)
Comment on attachment 8442175 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20696

Cancelling the review request. That was back in 2012, when stas handled stuff, then I took over, and now I passed on the buck to release management.

I've removed the wiki page as there wasn't any currently useful doc on it and cleaned up the only link to it.

Technically, I don't think it's easy to pick up strings from one app in another, and visual context is often important. So even if the strings are the same in en-US, that might not be possible in a localization.

As for the path forward, I defer to release management
Attachment #8442175 - Flags: review?(stas)

(In reply to Axel Hecht [:Pike] from comment #22)

> Technically, I don't think it's easy to pick up strings from one app in
> another, and visual context is often important. So even if the strings are
> the same in en-US, that might not be possible in a localization.
> 

We know it's not that easy, but in this case, seems quite safe as both are title strings and are not combined with other strings. That's why I suggested this for 2.0, but as I said, I am not a l10n expert.

> As for the path forward, I defer to release management

Bhavana who can make that decision? I think all the possible options are in this bug
Flags: needinfo?(bbajaj)
Ehsan, this patch adds the "mobileid" permission and checks that the permission is listed in the manifest.
Attachment #8442178 - Attachment is obsolete: true
Attachment #8444591 - Flags: review?(ehsan)
Comment on attachment 8444591 [details] [diff] [review]
Part 1: Add "mobileid" permission

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

::: dom/webidl/Navigator.webidl
@@ +153,5 @@
>  
>  #ifdef MOZ_B2G
>  [NoInterfaceObject]
>  interface NavigatorMobileId {
> +    [Throws, NewObject, Func="Navigator::HasMobileIdSupport"]

Nit: please add a comment saying why you can't use [CheckPermissions] here.
Attachment #8444591 - Flags: review?(ehsan) → review+
Comment on attachment 8444591 [details] [diff] [review]
Part 1: Add "mobileid" permission

Thanks Ehsan!
Attachment #8444591 - Flags: superreview?(jonas)
Comment on attachment 8444595 [details] [diff] [review]
Part 2: Prompt when required based on permission

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

Looks good to me.  Thanks, Fernando!
Attachment #8444595 - Flags: review?(jparsons) → review+
The string freeze has been broken over the week-end, and it seems this is the last remaining piece that's adding new strings for 2.0. (Except maybe https://bugzilla.mozilla.org/show_bug.cgi?id=1017756 but that's another issue).

We need to (re)declare 2.0 as string frozen ASAP, if you can't land now, please provide an estimate.
(In reply to Maria Angeles Oteo (:oteo) from comment #23)
> 
> (In reply to Axel Hecht [:Pike] from comment #22)
> 
> > Technically, I don't think it's easy to pick up strings from one app in
> > another, and visual context is often important. So even if the strings are
> > the same in en-US, that might not be possible in a localization.
> > 
> 
> We know it's not that easy, but in this case, seems quite safe as both are
> title strings and are not combined with other strings. That's why I
> suggested this for 2.0, but as I said, I am not a l10n expert.
> 
> > As for the path forward, I defer to release management
> 
> Bhavana who can make that decision? I think all the possible options are in
> this bug

Maria, the gaia patch which has l10n changes looks ready, can you request approval-gaia-2.0 so this lands asap ?
Flags: needinfo?(bbajaj)
Component: DOM: Device Interfaces → Gaia::Settings
Product: Core → Firefox OS
Comment on attachment 8442175 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20696

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Not a regression, just a leftover from bug 988469
[User impact] if declined: The user won't be able to change an already given permission for a 3rd party to obtain the user's verified phone number.
[Testing completed]: Yes, locally.
[Risk to taking this patch] (and alternatives if risky): None
[String changes made]: Added one string to shared/locales/permissions/
Attachment #8442175 - Flags: approval-gaia-v2.0?
Attachment #8442175 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Comment on attachment 8444591 [details] [diff] [review]
Part 1: Add "mobileid" permission

Don't we have WebIDL annotations for this these days? And you should probably hook this up to the feature detection API. Ehsan can help.
Attachment #8444591 - Flags: superreview?(jonas) → superreview?(ehsan)
(In reply to Jonas Sicking (:sicking) from comment #32)
> Comment on attachment 8444591 [details] [diff] [review]
> Part 1: Add "mobileid" permission
> 
> Don't we have WebIDL annotations for this these days?

I asked Fernando about this on IRC, and the reason he didn't use [CheckPermissions] here is that maps to ALLOW_ACTION and not PROMPT_ACTION.  This is what I meant when I asked Fernando to add a comment for in comment 26, sorry for not being clear about that!

> And you should
> probably hook this up to the feature detection API. Ehsan can help.

Bug 1009645 has unfortunately not landed yet, so we don't yet have [FeatureDetectible].  But I'll add a comment there to remind Reuben to add that for this API as well, since this will probably land first.
Attachment #8444591 - Flags: superreview?(ehsan) → superreview+
Sorry if I missed something, but is there anything preventing the Gaia PR to land right now on 2.0? We really need 2.0 to be string frozen.
(In reply to Théo Chevalier [:tchevalier] from comment #34)
> Sorry if I missed something, but is there anything preventing the Gaia PR to
> land right now on 2.0? We really need 2.0 to be string frozen.

nope, let me flag ryan to help here as this is urgent.
Flags: needinfo?(ryanvm)
Also pushed that string out to localizers now.
Comment on attachment 8444591 [details] [diff] [review]
Part 1: Add "mobileid" permission

> Navigator::HasFeatureDetectionSupport(JSContext* /* unused */, JSObject* aGlobal)
> {
>   nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
>   return CheckPermission(win, "feature-detection");
> }
> 
>+#ifdef MOZ_B2G
>+/* static */
>+bool
>+Navigator::HasMobileIdSupport(JSContext* aCx, JSObject* aGlobal)
>+{
>+  JS::Rooted<JSObject*> global(aCx, aGlobal);
>+
>+  nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(global);
>+  if (!win) {
>+    return false;
>+  }
You could just nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
here, like the code in other methods.



>+  nsIPrincipal* principal = doc->NodePrincipal();
>+  if (!principal) {
>+    return false;
>+  }
NodePrincipal() is never null.
In general methods starting with Get* may return null in DOM.
Attachment #8444591 - Flags: review+
Comment on attachment 8444591 [details] [diff] [review]
Part 1: Add "mobileid" permission

Approval Request Comment
[Feature/regressing bug #]: Mobile Identity
[User impact if declined]: No user impact. This is a security issue. The API is currently open to any app.
[Describe test coverage new/current, TBPL]: Manual tests.
[Risks and why]: No risk. The only consumer of this API so far is Loop and it is being developed by the same team that develops this API. We've verified that this patches are working as expected and they shouldn't affect any other code.
[String/UUID change made/needed]: None
Attachment #8444591 - Flags: approval-mozilla-aurora?
Attachment #8444595 - Flags: approval-mozilla-aurora?
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #41)
> Comment on attachment 8444591 [details] [diff] [review]

> [User impact if declined]: No user impact. This is a security issue. The API
> is currently open to any app.
Note on this: This *is* a user impact. We could potentially expose personal data to untrusted third parties without this patch.
Comment on attachment 8444591 [details] [diff] [review]
Part 1: Add "mobileid" permission

Aurora approval granted.
Attachment #8444591 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8444595 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: