Add a "forceIdSelection" to navigator.getMobileIdAssertion

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

({dev-doc-needed})

unspecified
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: ft:loop)

Attachments

(3 attachments)

In order to give the developer more flexibility to request a different mobile id we should add something like a "forceIdSelection" argument to "navigator.getMobileIdAssertion".
Assignee: nobody → ferjmoreno
Whiteboard: ft:loop
This bug will allow apps to provide a "logout" or "change mobile id" feature. Without this fix an user will be stuck with a single mobile identity per app and this mobile identity might not be valid anymore. We need a way for users to change the mobile identity that they shared with the apps.

In the future (2.1 probably) we will probably want a settings menu where the user can control her mobile identities and their consumers, but until then, we have to relay on app authors to provide this functionality. Loop for instance is waiting for this fix to implement this "change mobile id" functionality.
blocking-b2g: --- → 2.0?
This reads off as a feature, not a bug, since this is adding something to an existing API. If this is required for loop, then I think the feature-b2g flag should be set here.

Maria - Can you weigh in on whether this is needed & set the flag accordingly?
blocking-b2g: 2.0? → ---
Flags: needinfo?(oteo)
(In reply to Jason Smith [:jsmith] from comment #2)
> This reads off as a feature, not a bug, since this is adding something to an
> existing API. If this is required for loop, then I think the feature-b2g
> flag should be set here.
> 
> Maria - Can you weigh in on whether this is needed & set the flag
> accordingly?

It's needed for Loop MVP but perhaps it's more accurate set it with flag feature-b2g.
Thanks Jason!
feature-b2g: --- → 2.0
Flags: needinfo?(oteo)
Attachment #8445066 - Flags: review?(jonas)
Attachment #8445066 - Attachment filename: bug1022193.forceIdSelection.patch → Part 1: WebIDL
Attachment #8445066 - Attachment description: bug1022193.forceIdSelection.patch → Part 1: WebIDL
Attachment #8445066 - Attachment filename: Part 1: WebIDL → bug1022193.forceIdSelection.patch
Posted patch Part 2: DOMSplinter Review
Attachment #8445068 - Flags: review?(jonas)
I'll be adding the tests for this changes to bug 1022181
Attachment #8445069 - Flags: review?(jparsons)
Comment on attachment 8445066 [details] [diff] [review]
Part 1: WebIDL

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

r=me with that fixed.

::: dom/webidl/Navigator.webidl
@@ +154,5 @@
>  #ifdef MOZ_B2G
>  [NoInterfaceObject]
>  interface NavigatorMobileId {
>      [Throws, NewObject, Func="Navigator::HasMobileIdSupport"]
> +    Promise getMobileIdAssertion(optional boolean forceIdSelection);

Could you change this to take a dictionary instead? Boolean arguments tend to make the code hard to read since it's not obvious what "true" or "false" means, especially as more arguments are added.

Also, using a dictionary makes it easier to add other optional arguments in the future.

So something like getMobileIdAssertion({ forceSelection: true }) would be great. (The 'id' in the argument name seems redundant)
Attachment #8445066 - Flags: review?(jonas) → review+
Comment on attachment 8445068 [details] [diff] [review]
Part 2: DOM

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

::: dom/base/Navigator.cpp
@@ +1606,5 @@
>      return nullptr;
>    }
>  
> +  const bool forceIdSelection = aForceIdSelection.WasPassed() &&
> +                                aForceIdSelection.Value() || false;

Just set the default to be 'false' in the webidl. That way you can use a normal bool in the code.

I.e.

dictionary MobileIdAssertionArgs {
  bool forceSelection = false;
}
Attachment #8445068 - Flags: review?(jonas) → review+
Comment on attachment 8445069 [details] [diff] [review]
Part 3: MobileIdentityManager

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

Looks reasonable to me!  Thanks, ferjm

::: services/mobileid/MobileIdentityManager.jsm
@@ +88,5 @@
>      let promiseId = msg.promiseId;
>      this.messageManagers[promiseId] = aMessage.target;
>  
>      this.getMobileIdAssertion(aMessage.principal, promiseId,
> +                              msg.forceIdSelection);

Looks like msg.msisdn and msg.prompt are no longer used, but just to future-proof against the need for other params, would it be safer to make this last argument an options dictionary?
Attachment #8445069 - Flags: review?(jparsons) → review+
Thanks Jonas and Jed!

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #9)
> Comment on attachment 8445069 [details] [diff] [review]
> Part 3: MobileIdentityManager
> 
> Review of attachment 8445069 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable to me!  Thanks, ferjm
> 
> ::: services/mobileid/MobileIdentityManager.jsm
> @@ +88,5 @@
> >      let promiseId = msg.promiseId;
> >      this.messageManagers[promiseId] = aMessage.target;
> >  
> >      this.getMobileIdAssertion(aMessage.principal, promiseId,
> > +                              msg.forceIdSelection);
> 
> Looks like msg.msisdn and msg.prompt are no longer used, but just to
> future-proof against the need for other params, would it be safer to make
> this last argument an options dictionary?

Yeah, msg.msisdn and msg.prompt were just a leftover from previous code that were not used. I added the dictionary as you both suggested.

https://hg.mozilla.org/integration/b2g-inbound/rev/2a62206bd112
https://hg.mozilla.org/integration/b2g-inbound/rev/f1f79be526d8
https://hg.mozilla.org/integration/b2g-inbound/rev/d7853790a8b2
Comment on attachment 8445066 [details] [diff] [review]
Part 1: WebIDL

Approval Request Comment

[Feature/regressing bug #]: Allows devs to provide the option to users to change the mobile identity that they are sharing with 3rd party apps.

[User impact if declined]: The user won't be able to change a previous decision of which mobile identity she wants to share with an specific app and so she will be tied to it even if this identity is no longer valid, affecting the functionality where the mobile identity is used in the app.

[Describe test coverage new/current, TBPL]: Several local tests. Unit tests are being added to bug 1022181

[Risks and why]: Very low risk. The code is quite self contained and the only consumer of this API is Loop which is being developed by the same team as this API.

[String/UUID change made/needed]: dom/mobileid/interfaces/nsIMobileIdentityService.idl uuid changed
Attachment #8445066 - Flags: approval-mozilla-aurora?
Attachment #8445068 - Flags: approval-mozilla-aurora?
Attachment #8445069 - Flags: approval-mozilla-aurora?
Blocks: 1026999
Just to clarify: This needs tests (Bug 1022181) for this to be uplifted.
FYI, I poked bug 1022181 about creating the tests to support this bug.
I just pushed the tests at bug 1022181 to b2g-inbound. Since the patches at bug 1022181 are including tests for the change introduced in this bug, I am changing the dependency, so the uplift can be done in the correct order.

The correct order of uplift to 2.0 should be:

bug 1022193
bug 1026999
bug 1022181
No longer depends on: 1022181
Comment on attachment 8445066 [details] [diff] [review]
Part 1: WebIDL

Aurora+

I will review and mark the dependent bugs that you listed as well.
Attachment #8445066 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8445068 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8445069 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.