1022193 - Add a "forceIdSelection" to navigator.getMobileIdAssertion

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

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".

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 2

[Jason Smith [:jsmith]]

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?

Comment 3

[Maria Angeles Oteo (: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!

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: WebIDL

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: DOM

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: MobileIdentityManager

I'll be adding the tests for this changes to bug 1022181

Comment 7

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

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)

Comment 8

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

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;
}

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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?

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

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 11

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a62206bd112
https://hg.mozilla.org/mozilla-central/rev/f1f79be526d8
https://hg.mozilla.org/mozilla-central/rev/d7853790a8b2

Comment 13

[Maire Reavy [:mreavy]]

Just to clarify: This needs tests (Bug 1022181) for this to be uplifted.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

FYI, I poked bug 1022181 about creating the tests to support this bug.

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8445066 [details] [diff] [review]
Part 1: WebIDL

Aurora+

I will review and mark the dependent bugs that you listed as well.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/185024f210a6
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f0e02fcef85
https://hg.mozilla.org/releases/mozilla-aurora/rev/778cae6edf6d