788191 - MobileConnection: expose MSISDN and SPN

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, enhancement, P2)

Description

[:gerard-majax]

Attachment: Exposing SPN and MSISDN through iccInfo

Following both bug 782603 and bug 787967, we can now display some more informations to the user about his SIM card:
 - SPN, Service Provider Name, i.e., name of the carrier
 - MSISDN, Mobile Station ISDN Number, i.e., user's phone number

Comment 1

[Philipp von Weitershausen [:philikon]]

Comment on attachment 658127 [details] [diff] [review]
Exposing SPN and MSISDN through iccInfo

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

This looks ok to me, but I don't think I'm up to date on the discussion on exposing user-identifiable stuff like the MSISDN to content. Marshall, Beatriz, do you guy shave any concerns about exposing the MSISDN to (privileged) content?

Comment 2

[Marshall Culpepper [:marshall_law]]

Comment on attachment 658127 [details] [diff] [review]
Exposing SPN and MSISDN through iccInfo

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

(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> Marshall, Beatriz, do you guy shave any concerns about exposing the MSISDN to
> (privileged) content?
I think this should be fine, but we should probably mark this for security review just to be on the safe side.

This is a good start, but it needs some work :) r- just because the logic doesn't look fully fleshed out yet.

Also, all new patches now require tests -- so you will need to make sure that both |spn| and |msisdn| have coverage, preferably in dom/network/tests/marionette/test_mobile_iccinfo.js

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +362,5 @@
> +
> +  /**
> +   * Service Provider Name (SPN) of the subscriber's home network.
> +   */
> +  readonly attribute DOMString spn;

A have a quick API question related to your roaming heuristic here:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=787967&attachment=658277

Do we know for a certainty that the SPN will exactly equal either the operator's long or short name when not roaming? Isn't there still a possibility that both of these names will be empty (and then you only have MCC / MNC to compare?)

::: dom/system/gonk/RILContentHelper.js
@@ +216,5 @@
>    updateICCInfo: function updateICCInfo(srcInfo, destInfo) {
>      destInfo.mcc = srcInfo.mcc;
>      destInfo.mnc = srcInfo.mnc;
> +    destInfo.spn = srcInfo.spn;
> +    destInfo.msisdn = srcInfo.msisdn;

You will also need to check when these two new fields change in RadioInterfaceLayer.js. See handleICCInfoChange around line 1020:

http://dxr.mozilla.org/mozilla-central/dom/system/gonk/RadioInterfaceLayer.js.html#l1020

Comment 3

[:gerard-majax]

(In reply to Marshall Culpepper [:marshall_law] from comment #2)
> Comment on attachment 658127 [details] [diff] [review]
> Exposing SPN and MSISDN through iccInfo
> 
> Review of attachment 658127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Philipp von Weitershausen [:philikon] from comment #1)
> > Marshall, Beatriz, do you guy shave any concerns about exposing the MSISDN to
> > (privileged) content?
> I think this should be fine, but we should probably mark this for security
> review just to be on the safe side.
> 
> This is a good start, but it needs some work :) r- just because the logic
> doesn't look fully fleshed out yet.
> 
> Also, all new patches now require tests -- so you will need to make sure
> that both |spn| and |msisdn| have coverage, preferably in
> dom/network/tests/marionette/test_mobile_iccinfo.js
> 
> ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> @@ +362,5 @@
> > +
> > +  /**
> > +   * Service Provider Name (SPN) of the subscriber's home network.
> > +   */
> > +  readonly attribute DOMString spn;
> 
> A have a quick API question related to your roaming heuristic here:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=787967&attachment=658277
> 
> Do we know for a certainty that the SPN will exactly equal either the
> operator's long or short name when not roaming? Isn't there still a
> possibility that both of these names will be empty (and then you only have
> MCC / MNC to compare?)

I don't know, but I did not pulled it out of nowhere, it's the way it's handled in Android.
According to http://code.google.com/p/android/issues/detail?id=4590 it might happen to have an empty SPN. And you can also find https://android-review.googlesource.com/#/c/12666/ and http://code.google.com/p/android/issues/detail?id=3499

If anyone has an idea here ...

> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +216,5 @@
> >    updateICCInfo: function updateICCInfo(srcInfo, destInfo) {
> >      destInfo.mcc = srcInfo.mcc;
> >      destInfo.mnc = srcInfo.mnc;
> > +    destInfo.spn = srcInfo.spn;
> > +    destInfo.msisdn = srcInfo.msisdn;
> 
> You will also need to check when these two new fields change in
> RadioInterfaceLayer.js. See handleICCInfoChange around line 1020:
> 
> http://dxr.mozilla.org/mozilla-central/dom/system/gonk/RadioInterfaceLayer.
> js.html#l1020

Comment 4

[:gerard-majax]

Attachment: Exposing SPN and MSISDN through iccInfo v2

Addressing suggestions:
 - added marionnette tests on spn and msisdn
 - added checks in updateICCInfo()

Comment 5

[Beatriz Rodríguez [:brg]]

(In reply to Alexandre LISSY :gerard-majax from comment #3)
> (In reply to Marshall Culpepper [:marshall_law] from comment #2)
> > 
> > (In reply to Philipp von Weitershausen [:philikon] from comment #1)
> > > Marshall, Beatriz, do you guy shave any concerns about exposing the MSISDN to
> > > (privileged) content?
> > I think this should be fine, but we should probably mark this for security
> > review just to be on the safe side.

I see no issue about exposing the MSISDN but I guess it is great if you perform any security review. On the other hand, I would like to emphasize that most of the SIM cards will not include this field, I mean it could be empty that is why I do not understand why you want to expose it.

Comment 6

[:gerard-majax]

Attachment: Exposing SPN and MSISDN through iccInfo v3

Fixing an issue in handleICCInfoChange that made the event not fired.

Comment 7

[Fabien Cazenave [:kaze]]

(In reply to Beatriz Rodríguez [:brg] from comment #5)
> I see no issue about exposing the MSISDN but I guess it is great if you
> perform any security review. On the other hand, I would like to emphasize
> that most of the SIM cards will not include this field, I mean it could be
> empty that is why I do not understand why you want to expose it.

It might be empty but it’s nice to expose nonetheless. That’s the kind of information I’d like to have when available.

Comment 8

[:gerard-majax]

ping?

Comment 9

[:gerard-majax]

Attachment: Exposing SPN and MSISDN through iccInfo v4

Updating the code against latest master.

Comment 10

[Marshall Culpepper [:marshall_law]]

Comment on attachment 661521 [details] [diff] [review]
Exposing SPN and MSISDN through iccInfo v4

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

Good work! Were you able to run your marionette test to verify the new assertions are working?

Comment 11

[:gerard-majax]

Sadly, I don't know how to run marionette and my environment is configured for my Nexus S phone, and right now I don't have the time to do a new config.sh to be able to test it. I'll have a lifetime thinking for anyone who could check it for me :)

Comment 12

[Philipp von Weitershausen [:philikon]]

I just ran tests with your patch applied:

======================================================================
FAIL: /home/philipp/dev/mc.hg/dom/network/tests/marionette/test_mobile_iccinfo.js, runTest (marionette_test.MarionetteJSTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/philipp/dev/mc.hg/testing/marionette/client/marionette/marionette_test.py", line 208, in runTest
    '%d tests failed:\n%s' % (results['failed'], '\n'.join(fails)))
AssertionError: 2 tests failed:
TEST-UNEXPECTED-FAIL | got null, expected Android | got false, expected true
TEST-UNEXPECTED-FAIL | got 15555215554, expected 15555218135 | got false, expected true

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to Alexandre LISSY :gerard-majax from comment #11)
> Sadly, I don't know how to run marionette and my environment is configured
> for my Nexus S phone, and right now I don't have the time to do a new
> config.sh to be able to test it.

I suggest doing a separate clone of B2G for the emulator. Leave it running over night. It's a well-worth investment.

Comment 14

[:gerard-majax]

Attachment: Exposing SPN and MSISDN through iccInfo v5

Correcting the MSISDN of the emulator. Currently, as long as bug 788191 is not landed, the spn test will fail.

Comment 15

[:gerard-majax]

It's bug 787967 sorry. It has landed, if someone can relaunch the marionnette test ...

Comment 16

[Philipp von Weitershausen [:philikon]]

Comment on attachment 662859 [details] [diff] [review]
Exposing SPN and MSISDN through iccInfo v5

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1085,5 @@
>                           oldIcc.iccid != message.iccid ||
>                           oldIcc.mcc != message.mcc || 
> +                         oldIcc.mnc != message.mnc ||
> +			 oldIcc.spn != message.spn ||
> +			 oldIcc.msisdn != message.msisdn;

Please don't use tabs to indent.

Comment 17

[:gerard-majax]

Attachment: Exposing SPN and MSISDN through iccInfo v6

Addressing tab comment

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 666245 [details] [diff] [review]
Exposing SPN and MSISDN through iccInfo v6

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

1. That's not a hg patch and the format of the summary is wrong.
2. The marionette test fails actually.

::: dom/network/tests/marionette/test_mobile_iccinfo.js
@@ +17,4 @@
>  // See it here {B2G_HOME}/external/qemu/telephony/android_modem.c#L2465.
>  is(connection.iccInfo.mcc, 310);
>  is(connection.iccInfo.mnc, 260);
> +is(connection.iccInfo.spn, "Android");

MARIONETTE TEST RESULT:TEST-UNEXPECTED-FAIL | undefined - got null, expected Android

Comment 19

[:gerard-majax]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> Comment on attachment 666245 [details] [diff] [review]
> Exposing SPN and MSISDN through iccInfo v6
> 
> Review of attachment 666245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1. That's not a hg patch and the format of the summary is wrong.

Well according to https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_do_I_check_stuff_in.3F I'm following git patch style, so what's wrong ?

> 2. The marionette test fails actually.
> 
> ::: dom/network/tests/marionette/test_mobile_iccinfo.js
> @@ +17,4 @@
> >  // See it here {B2G_HOME}/external/qemu/telephony/android_modem.c#L2465.
> >  is(connection.iccInfo.mcc, 310);
> >  is(connection.iccInfo.mnc, 260);
> > +is(connection.iccInfo.spn, "Android");
> 
> MARIONETTE TEST RESULT:TEST-UNEXPECTED-FAIL | undefined - got null, expected
> Android

Okay, might be my fault here, looks like I used the wrong SPN,
https://github.com/mozilla-b2g/platform_external_qemu/blob/master/telephony/sim_card.c#L290

But I don't understand why the emulator returns undefined for this.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Alexandre LISSY :gerard-majax from comment #19)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> > 1. That's not a hg patch and the format of the summary is wrong.
> 
> Well according to
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_do_I_check_stuff_in.3F I'm following git patch style, so
> what's wrong ?

You omitted that patches are generated from HG in that page. And, the format for commit summary should be "Bug 12345: What have I done, r=xxx". For now, you don't need ",r=xxx" because the patch has not passed review yet.

> > 2. The marionette test fails actually.
> > > +is(connection.iccInfo.spn, "Android");
> > 
> > MARIONETTE TEST RESULT:TEST-UNEXPECTED-FAIL | undefined - got null, expected
> > Android
> 
> Okay, might be my fault here, looks like I used the wrong SPN,
> https://github.com/mozilla-b2g/platform_external_qemu/blob/master/telephony/
> sim_card.c#L290

That's CPHS(Common PCN Handset Specification), which is not supported yet.

> But I don't understand why the emulator returns undefined for this.

Function `asimcard_io` in same file has:

    { "+CRSM=192,28486,0,0,15", "+CRSM: 148,4" },

, which means for every GET_RESPONSE(192) restricted SIM I/O(CRSM) request for EFspn(FileId: 0x6F46, or 28486 in decimal), answer SW1 = 0x94(148 in decimal) and SW2 = 4, that is "FileId not found".

I had a few investigations into Android's implementation and we'll need CPHS SPN support for correct SPN display/matching. But for now, I think we can just rewrite the SIM I/O answer with a valid one instead.

Comment 21

[:gerard-majax]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #20)
> (In reply to Alexandre LISSY :gerard-majax from comment #19)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> > > 1. That's not a hg patch and the format of the summary is wrong.
> > 
> > Well according to
> > https://developer.mozilla.org/en-US/docs/
> > Mercurial_FAQ#How_do_I_check_stuff_in.3F I'm following git patch style, so
> > what's wrong ?
> 
> You omitted that patches are generated from HG in that page. And, the format
> for commit summary should be "Bug 12345: What have I done, r=xxx". For now,
> you don't need ",r=xxx" because the patch has not passed review yet.
> 
> > > 2. The marionette test fails actually.
> > > > +is(connection.iccInfo.spn, "Android");
> > > 
> > > MARIONETTE TEST RESULT:TEST-UNEXPECTED-FAIL | undefined - got null, expected
> > > Android
> > 
> > Okay, might be my fault here, looks like I used the wrong SPN,
> > https://github.com/mozilla-b2g/platform_external_qemu/blob/master/telephony/
> > sim_card.c#L290
> 
> That's CPHS(Common PCN Handset Specification), which is not supported yet.
> 
> > But I don't understand why the emulator returns undefined for this.
> 
> Function `asimcard_io` in same file has:
> 
>     { "+CRSM=192,28486,0,0,15", "+CRSM: 148,4" },
> 
> , which means for every GET_RESPONSE(192) restricted SIM I/O(CRSM) request
> for EFspn(FileId: 0x6F46, or 28486 in decimal), answer SW1 = 0x94(148 in
> decimal) and SW2 = 4, that is "FileId not found".
> 
> I had a few investigations into Android's implementation and we'll need CPHS
> SPN support for correct SPN display/matching. But for now, I think we can
> just rewrite the SIM I/O answer with a valid one instead.

So my code is not compliant ? I don't have the time anymore to rewrite it :(

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Alexandre LISSY :gerard-majax from comment #21)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #20)
> > I had a few investigations into Android's implementation and we'll need CPHS
> > SPN support for correct SPN display/matching. But for now, I think we can
> > just rewrite the SIM I/O answer with a valid one instead.
> 
> So my code is not compliant ? I don't have the time anymore to rewrite it :(

Here it is :)
https://github.com/mozilla-b2g/platform_external_qemu/pull/6

Yoshi, could you help review that merge request?

Comment 23

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #22)
> (In reply to Alexandre LISSY :gerard-majax from comment #21)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #20)
> > > I had a few investigations into Android's implementation and we'll need CPHS
> > > SPN support for correct SPN display/matching. But for now, I think we can
> > > just rewrite the SIM I/O answer with a valid one instead.
> > 
> > So my code is not compliant ? I don't have the time anymore to rewrite it :(
> 
> Here it is :)
> https://github.com/mozilla-b2g/platform_external_qemu/pull/6
> 
> Yoshi, could you help review that merge request?

looks good to me.

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 666245 [details] [diff] [review]
Exposing SPN and MSISDN through iccInfo v6

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

Re-verified with latest Gecko & Gaia again. Request for super review because this patch touches DOM interface and haven't been super reviewed.

Comment 25

[Fabien Cazenave [:kaze]]

This is a dependency of bug 800951, which is blocking-basecamp+. Nominating.

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Exposing SPN and MSISDN through iccInfo v7

re-made in hg format

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43c585774a13

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43c585774a13

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f0a3de05e4b5