Closed
Bug 1023266
Opened 10 years ago
Closed 10 years ago
Make the Mobile ID API privileged
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Firefox OS Graveyard
Gaia::Settings
Tracking
(feature-b2g:2.0, 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)
46 bytes,
text/x-github-pull-request
|
arthurcc
:
review+
bajaj
:
approval-gaia-v2.0+
|
Details | Review |
3.92 KB,
patch
|
ehsan.akhgari
:
review+
smaug
:
review+
ehsan.akhgari
:
superreview+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
4.08 KB,
patch
|
jedp
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
We missed that detail in bug 988469...
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ferjmoreno
Comment 2•10 years ago
|
||
Is it only available to certified apps right now?
Flags: needinfo?(ferjmoreno)
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8438655 -
Flags: review?(jonas)
Flags: needinfo?(ferjmoreno)
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Attachment #8438655 -
Flags: review?(jonas)
Assignee | ||
Updated•10 years ago
|
Attachment #8438655 -
Flags: review?(jonas)
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
(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
Updated•10 years ago
|
Whiteboard: ft:loop
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8442175 -
Flags: review?(arthur.chen)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8438655 -
Attachment is obsolete: true
Attachment #8438655 -
Flags: review?(jonas)
Attachment #8442178 -
Flags: review?(jonas)
Assignee | ||
Comment 10•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8442175 -
Flags: review?(arthur.chen) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Thanks Arthur! The Gaia side can land without the Gecko one. https://github.com/mozilla-b2g/gaia/commit/8ed871a402eb6a7b47eef033038a86c2bd7fbc20
Keywords: late-l10n
Comment 13•10 years ago
|
||
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)
Comment 14•10 years ago
|
||
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?
Comment 17•10 years ago
|
||
(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?
Comment 18•10 years ago
|
||
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.
Assignee | ||
Comment 19•10 years ago
|
||
(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)
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
(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!
Assignee | ||
Updated•10 years ago
|
Attachment #8442178 -
Flags: review?(ehsan)
Comment 22•10 years ago
|
||
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)
Comment 23•10 years ago
|
||
(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)
Assignee | ||
Comment 24•10 years ago
|
||
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)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8444595 -
Flags: review?(jparsons)
Comment 26•10 years ago
|
||
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+
Assignee | ||
Comment 27•10 years ago
|
||
Comment on attachment 8444591 [details] [diff] [review] Part 1: Add "mobileid" permission Thanks Ehsan!
Attachment #8444591 -
Flags: superreview?(jonas)
Comment 28•10 years ago
|
||
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+
Comment 29•10 years ago
|
||
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.
Comment 30•10 years ago
|
||
(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)
Assignee | ||
Updated•10 years ago
|
Component: DOM: Device Interfaces → Gaia::Settings
Product: Core → Firefox OS
Assignee | ||
Comment 31•10 years ago
|
||
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?
Updated•10 years ago
|
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)
Comment 33•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8444591 -
Flags: superreview?(ehsan) → superreview+
Comment 34•10 years ago
|
||
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.
Comment 35•10 years ago
|
||
(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)
Comment 36•10 years ago
|
||
v2.0: https://github.com/mozilla-b2g/gaia/commit/ea8e11682811c3084160831bfae05d52a09c8afa
Flags: needinfo?(ryanvm)
Comment 37•10 years ago
|
||
Also pushed that string out to localizers now.
Comment 38•10 years ago
|
||
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+
Assignee | ||
Comment 39•10 years ago
|
||
Thanks Olli! https://hg.mozilla.org/integration/b2g-inbound/rev/f106e9dd913e https://hg.mozilla.org/integration/b2g-inbound/rev/2c57207404b1
Comment 40•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f106e9dd913e https://hg.mozilla.org/mozilla-central/rev/2c57207404b1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 41•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Attachment #8444595 -
Flags: approval-mozilla-aurora?
Comment 42•10 years ago
|
||
(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 43•10 years ago
|
||
Comment on attachment 8444591 [details] [diff] [review] Part 1: Add "mobileid" permission Aurora approval granted.
Attachment #8444591 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8444595 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 44•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c0abc527018 https://hg.mozilla.org/releases/mozilla-aurora/rev/d1302463099e
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
status-firefox31:
--- → wontfix
status-firefox32:
--- → fixed
status-firefox33:
--- → fixed
Target Milestone: --- → 2.0 S5 (4july)
You need to log in
before you can comment on or make changes to this bug.
Description
•