1044792 - Send MCC and MNC on POST /sms/mt/verify

Status: RESOLVED FIXED

(Cloud Services Graveyard :: MobileID, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

The rationale is at https://bugzilla.mozilla.org/show_bug.cgi?id=1042866

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

We need this change landed on 2.0 to allow the usage of Mobile ID on USA and Canada.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Sam, this simple change adds the MCC and MNC (this one only when we know it) values to the already existing /sms/mt/verify request [1]. We need this addition to let the server decide which phone number to use to send the verification codes to the client via SMS. So far the server was using a default number, but that doesn't work for US and Canada (bug 1042866) and wouldn't scale on an scenario where we need to use an specific SMS sender for a specific MCC/MNC pair (i.e, if we have a carrier providing free SMS for their customers).

Thanks for your review!

[1] https://github.com/mozilla-services/msisdn-gateway/blob/master/API.md#post-v1msisdnsmsmtverify

Comment 3

[Sam Penrose]

Comment on attachment 8463429 [details] [diff] [review]
v1

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

The code changes are clean, and fit with their contexts. The unit tests run and exercise at least part of the patch. r+ as-is.

Notes on the services/mobileid package: It's very helpful that it follows the pattern in services/fxaccounts. It could really use an explanation of the basic control flow. The names are a bit opaque and assume familiarity with the the caller and data types, for example the "MoMt" in "MobileIdentitySmsMoMtVerificationFlow", or the parameter names in this block:

+      this.activeVerificationFlow = new MobileIdentitySmsMtVerificationFlow({
+          origin: aOrigin,
+          msisdn: aToVerify.msisdn,
+          mcc: aToVerify.mcc,
+          mnc: aToVerify.mnc,
+          iccId: aToVerify.iccId,
+          external: aToVerify.serviceId === undefined,
+          mtSender: aToVerify.verificationDetails.mtSender
+        },

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Thanks Sam!

(In reply to Sam Penrose from comment #3)
> Comment on attachment 8463429 [details] [diff] [review]
> v1
> 
> Review of attachment 8463429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code changes are clean, and fit with their contexts. The unit tests run
> and exercise at least part of the patch. r+ as-is.
> 
> Notes on the services/mobileid package: It's very helpful that it follows
> the pattern in services/fxaccounts. It could really use an explanation of
> the basic control flow. The names are a bit opaque and assume familiarity
> with the the caller and data types, for example the "MoMt" in
> "MobileIdentitySmsMoMtVerificationFlow", or the parameter names in this
> block:
> 
> +      this.activeVerificationFlow = new
> MobileIdentitySmsMtVerificationFlow({
> +          origin: aOrigin,
> +          msisdn: aToVerify.msisdn,
> +          mcc: aToVerify.mcc,
> +          mnc: aToVerify.mnc,
> +          iccId: aToVerify.iccId,
> +          external: aToVerify.serviceId === undefined,
> +          mtSender: aToVerify.verificationDetails.mtSender
> +        },

Yes. I have a pending task about documenting all about the client implementation at https://wiki.mozilla.org/WebAPI/MobileIdentity#Implementation_details . I just need the time to find the time do it :)

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5af3884cea83

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5af3884cea83

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e09a13eef6ae