988469 - (loop_msisdn_verific) MSISDN verification API for privileged apps

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Maria Angeles Oteo (:oteo)]

Several application authors want to securely verify the user's phone number, and do that in a way that requires minimal effort for the user. In
particular communication applications has this request as a top-features. This would be extremely useful for our Loop Client too.

Comment 2

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

Previous discussion is in Bug 832215.

Comment 3

[Ivan Tsay (:ITsay)]

Hi Ken,

Could you help to check if this one is under RIL team? WebRTC Loop client needs this feature in v2.0. Moreover, if this one is the same as bug 832215, we can use either one for the tracking.

Comment 4

[Ken Chang[:kenkai]]

Ivan, can you please clarify the scope of this bug? If the request is only to ask to have a webapi for MSISDN, we can handle it. But if it seems to be talking about an authentication mechanism or flow for APP, I think that it is out of RIL's scopes. And as I said in bug 832215, because some sim cards may not have MSISDN, I wonder if it is a good method to use MSISDN as a key for this authentication mechanism. Is it possible to use iccid?

Comment 5

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

Hi everyone,

There's been various threads on this topic, both inside of and outside of bugzilla. So I wanted to put some comments in a central place to get people up to speed.

This should be an API focused on getting the user's MSISDN, not an API for sending/receiving "silent" SMSs. I.e. rather than an API for sending silent SMSs, we should have an API which enables the app developer to get the user's phone number.

But there's some important requirements.

1. Protect against end-user modifying the device.

App developers can't trust the device. I.e. if we have an API like "x = navigator.getUserMSISDN()" then the developer can't rely on that the returned value is accurate.

I.e. a payment provider wouldn't be able to simply then ask the carrier to charge money to x. And a chat client like WhatsApp wouldn't be able simply send messages intended for x to that device.

In both cases they would run the risk that the user roots his device and installs a hacked version of Gecko which returns a user controlled arbitrary MSISDN. In that case the user could charge someone else for his purchases or listen in on someone else's discussions.

Remember that apps want this to be the *only* source of authentication. I.e. no passwords etc.

What we need is an API like "x = navigator.getUserMSISDNToken()", but asynchronous. This API would return a token which represents the user's phone number. The developer would then send this token to their app server and the app server would then contact an "authentication server". The authentication server can then verify that the token is a valid token, as well as return the actual MSISDN directly to the app server.

Note that the token is secret and needs to be domain/origin specific. This way, even though an application developer can get the tokens for other users, the developer can't then use this token to impersonate those users to other websites that also use this API.

2. Protect the user's privacy.

I don't see a reason that we couldn't put up a dialog saying "X wants to get your phone number for identification purposes. Are you ok with that? [Yes] [No]".

We should of course word-smith the text to make it clear that the website can't place phonecalls etc. And that the process is fully automatic and easy to use. Or whatever else we want to include or not include.

In this dialog we can also let the user choose which phone number to return on multi-sim devices.

Note that this would still be dramatically simpler than other authentication flows. In most scenarios the user would simply need to click a single "ok" button in order to log in to an app or website. No usernames and no passwords and no security codes from the user's point of view.

3. Protect against hacked gecko processes.

One of the problems with the "x = navigator.getUserMSISDNToken()" approach is that if a website manages to hack a child process it can impersonate other websites. I.e. it can make it look like the function call is coming from google.com and then get the user's token for google.com. It could then impersonate the user to google and for example listen in on a google run chat client.

One way to solve this is to only make the getUserMSISDNToken API available to special processes where we make sure to only run pages from a single domain/origin. However running separate processes is expensive.

Another solution would be to make the API work as follows:
1. Webpage calls navigator.getUserMSISDNToken()
2. FirefoxOS returns a token after checking that the user.
3. Webpage sends token to application server.
4. Application connects to authentication server and provides token and a "callback URL".
5. Authentication server checks that the "callback URL" is a https URL from the domain that matches the token.
6. Authentication server makes a https connection to the server in the callback URL.
7. The authentication server uses the normal TLS certificate to verify that the connection is indeed to the correct domain.
8. The authentication server sends the users MSISDN to the callback URL.

This way we can be sure that the MSISDN is sent only to the domain/app that was asked about in the dialog in step 2. The app developer can be sure that the number is correct since it's received from the authentication server, which the user does not have control over. And the authentication server can be sure that only the domain/app that should receive the MSISDN receives it, even if gecko processes were hacked.

----

A few additional thoughts on the implementation strategy for getUserMSISDNToken:

When getUserMSISDNToken is called, FirefoxOS should connect to the authentication server in order to have it verify the users MSISDN. There are at least three ways that the authentication server can verify the user's MSISDN:
1. Make a http request to the authentication server over the device's mobile data connection such that the carrier injects a header which contains a token which can be used to verify the users MSISDN.
2. Do the "silent SMS" messages between the authentication server and the device.
3. Place phone calls between the authentication server and the device.

We should try to let the authentication server make the decision which method to use. That way we can migrate between the different solutions as over time. For example if carriers are able to provide the 1 solution, or if they are able to provide zero-rated SMSs for the 2 solution.

One way to do this would be to have the device to an initial request to the authentication server and provide the user's current MCC/MNC (and phone number read from the SIM when possible), and then have the authentication server return a response which indicates which authentication mechanism to use. We'd likely also want to send data indicating if the user is currently roaming or not.

Note that technically we can do 1 even when the user is on a wifi connection by making a single request over the mobile data connection. This would not mean that we have to completely disable the wifi connection for a period of time. Instead we could send a single http request over the mobile connection only from the getUserMSISDNToken implementation, while letting all other network requests go over the wifi connection.

The getUserMSISDNToken API can also ensure that we only verify the users phone number when needed. Once the authentication server has authenticated the user's phone number we can store a cookie which is used next time we ask the authentication server for a token. This way the authentication server wouldn't need to reauthenticate the user next time getUserMSISDNToken is called. This cookie would of course be different from the tokens returned from getUserMSISDNToken and would never need to be exposed to anyone other than the authentication server.

The authentication server could of course choose to expire this cookie after a certain time as it wants to.

Another thing that we should let getUserMSISDNToken do is to automatically deal with the user swapping SIM cards. So the cookie that we store would be per-SIM card. So if the user swaps sim card we wouldn't send the previously stored cookie when getUserMSISDNToken contacts the authentication server. But if the user changes back to a previous SIM card, we can again use the cookie that was generated with that SIM card.

All of this complexity would be hidden from application authors and web developers. They would simply call getUserMSISDNToken and wait to get a token which they can use when talking to the authentication server.

Comment 6

[Chris Karlof [:ckarlof]]

Hi Jonas, I'm the lead for Firefox Accounts and other related authentication services in Mozilla cloud services.

Last week, I put together a straw man proposal for a MSISDN verification API using SMS:

https://github.com/mozilla/fxa-auth-server/wiki/API-extensions-for-supporting-MSISDN-verification-in-FxA-auth-server

I think the above server API is pretty well aligned with the requirements you've laid out, but it's still early and nothing is set in stone. FWIW, it was designed with the use case you're discussing here in mind. 

To relate our different language, your "userMSISDNToken" would be a "BrowserID assertion about the user's MSISDN scoped to a particular audience" in my proposal.

Some notes:

In terms of "protecting against hacked gecko processes", I would argue for the "smart client side architecture approach" you mention rather than complicating the verification protocol. IMO, this is a client-side security issue, and the alternative you mention above (callback URLs and TLS certificate checking) is externalizing the costs on the application (server) developers too much for my taste.  

> The getUserMSISDNToken API can also ensure that we only verify the users phone number when needed. Once the authentication server has authenticated the user's phone number we can store a cookie which is used next time we ask the authentication server for a token. This way the authentication server wouldn't need to re-authenticate the user next time getUserMSISDNToken is called.

We should also consider the "freshness" of the MSISDN verification, i.e., when the user's MSISDN was last verified. Applications might have different requirements for this. 

> 1. Make a http request to the authentication server over the device's mobile data connection such that the carrier injects a header which contains a token which can be used to verify the users MSISDN.
> 3. Place phone calls between the authentication server and the device.

We haven't considered these yet, but they're interesting. 

Overall, thanks for putting this together!

Comment 7

[Ian Bicking (:ianbicking)]

As an implementation note, mozPaymentProvider contains some SMS related functionality: https://wiki.mozilla.org/WebAPI/WebPaymentProvider#Silent_SMS

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Thanks Jonas! This is great.

(In reply to Jonas Sicking (:sicking) from comment #5)
> This should be an API focused on getting the user's MSISDN, not an API for
> sending/receiving "silent" SMSs. I.e. rather than an API for sending silent
> SMSs, we should have an API which enables the app developer to get the
> user's phone number.

I am not sure if you already considered this in your proposal, but I would say "an API for getting the user's MSISDN or verifying a provided MSISDN". For those cases where it is not possible to get the user's MSISDN (no operator support, roaming, no available short code for SMS MO, etc.), it would be great if we still allow devs to get a verification of a user provided MSISDN.

> 3. Protect against hacked gecko processes.
> 
> One of the problems with the "x = navigator.getUserMSISDNToken()" approach
> is that if a website manages to hack a child process it can impersonate
> other websites. I.e. it can make it look like the function call is coming
> from google.com and then get the user's token for google.com. It could then
> impersonate the user to google and for example listen in on a google run
> chat client.
> 
> One way to solve this is to only make the getUserMSISDNToken API available
> to special processes where we make sure to only run pages from a single
> domain/origin. However running separate processes is expensive.
> 
> Another solution would be to make the API work as follows:
> 1. Webpage calls navigator.getUserMSISDNToken()
> 2. FirefoxOS returns a token after checking that the user.
> 3. Webpage sends token to application server.
> 4. Application connects to authentication server and provides token and a
> "callback URL".
> 5. Authentication server checks that the "callback URL" is a https URL from
> the domain that matches the token.
> 6. Authentication server makes a https connection to the server in the
> callback URL.
> 7. The authentication server uses the normal TLS certificate to verify that
> the connection is indeed to the correct domain.
> 8. The authentication server sends the users MSISDN to the callback URL.
> 
> This way we can be sure that the MSISDN is sent only to the domain/app that
> was asked about in the dialog in step 2. The app developer can be sure that
> the number is correct since it's received from the authentication server,
> which the user does not have control over. And the authentication server can
> be sure that only the domain/app that should receive the MSISDN receives it,
> even if gecko processes were hacked.
> 

How do we solve this for Firefox Accounts?

I tend to agree with Chris about solving this issue in the client rather than in the server if possible.

> The getUserMSISDNToken API can also ensure that we only verify the users
> phone number when needed. Once the authentication server has authenticated
> the user's phone number we can store a cookie which is used next time we ask
> the authentication server for a token. This way the authentication server
> wouldn't need to reauthenticate the user next time getUserMSISDNToken is
> called. This cookie would of course be different from the tokens returned
> from getUserMSISDNToken and would never need to be exposed to anyone other
> than the authentication server.

I guess this would be a browser ID certificate.

> 
> The authentication server could of course choose to expire this cookie after
> a certain time as it wants to.
>

Indeed. We'd need something similar to forceAuth in mozId.

Comment 9

[Chris Karlof [:ckarlof]]

:ferjm, are you leading the implementation of this in FxOS? If so, what's the timeline?

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Chris Karlof [:ckarlof] from comment #9)
> :ferjm, are you leading the implementation of this in FxOS? If so, what's
> the timeline?

Yes. Since this is a requirement for Loop, I'll target FxOS 2.0.

Comment 11

[Chris Karlof [:ckarlof]]

Jonas laid out a few implementation strategies (header injection, SMS, phone calls). Do we have a decision on which of these FxOS 2.0 will support, and if the SMS approach is supported, is the below API still favorable?

https://github.com/mozilla/fxa-auth-server/wiki/API-extensions-for-supporting-MSISDN-verification-in-FxA-auth-server

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

I've done some modifications to the API at https://github.com/ferjm/msisdn-verification based on Jonas' proposal. Basically we delegate to the server the decision of which kind of authentication mechanism to use based on a few network details (MCC, MNC and roaming) given by the client.

I also had a brief chat with Jonas about this and we both agreed that for FxOS 2.0 we should probably only focus on the SMS verification mechanism. We already have some of the required pieces in the platform, so it shouldn't be that hard to implement.

Comment 13

[Tarek Ziadé (:tarek)]

Note that we're working on a prototype of Chris' API at https://github.com/mozilla-services/msisdn-gateway
for the server-side

It could be interesting to chat about it sometimes this week

Comment 14

[Chris Karlof [:ckarlof]]

> I've done some modifications to the API at https://github.com/ferjm/msisdn-verification

It would be nice to settle on place to host and iterate the server verification API. I propose we:

1) Move https://github.com/ferjm/msisdn-verification to https://github.com/mozilla-services/msisdn-gateway
2) Mark https://github.com/mozilla/fxa-auth-server/wiki/API-extensions-for-supporting-MSISDN-verification-in-FxA-auth-server as obsolete

Comment 15

[Rémy Hubscher (:natim)]

This looks great, I will put the new API documentation inside the msisdn-gateway repository.

The main change I see here is that we separate the registration and verification step. (To allow multiple verification procedure). This is great.

Just a little interrogation about the difference between /sms/verify and /sms/resend_code the two endpoint seams to generate a code, store it and send it using a SMS Gateway provider. Maybe we could merge them?

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Tarek Ziadé (:tarek) from comment #13)
> Note that we're working on a prototype of Chris' API at
> https://github.com/mozilla-services/msisdn-gateway
> for the server-side
> 

Oh, good to know. Thanks!

> It could be interesting to chat about it sometimes this week

Absolutely. I'm available to chat as soon as you can :), just ping me on IRC or send me a meeting invite.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Chris Karlof [:ckarlof] from comment #14)
> > I've done some modifications to the API at https://github.com/ferjm/msisdn-verification
> 
> It would be nice to settle on place to host and iterate the server
> verification API. I propose we:
> 
> 1) Move https://github.com/ferjm/msisdn-verification to
> https://github.com/mozilla-services/msisdn-gateway
> 2) Mark
> https://github.com/mozilla/fxa-auth-server/wiki/API-extensions-for-
> supporting-MSISDN-verification-in-FxA-auth-server as obsolete

Sounds good to me. I added the modifications to a side repo as I didn't want to directly modify your proposal without getting feedback first.

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Rémy Hubscher (:natim) from comment #15)
> This looks great, I will put the new API documentation inside the
> msisdn-gateway repository.
> 

Thanks!

> The main change I see here is that we separate the registration and
> verification step. (To allow multiple verification procedure). This is great.
> 
> Just a little interrogation about the difference between /sms/verify and
> /sms/resend_code the two endpoint seams to generate a code, store it and
> send it using a SMS Gateway provider. Maybe we could merge them?

As mentioned on IRC, if you think that /sms/verify and /sms/resend_code do exactly the same, we can get rid of /sms/resend_code. I just left it that way because we might want to do different things in the server even if the side effect for the client is the same. I guess that depends on how much state do we want to save in the server. We might not want to allow the client to request resend_code more than once for example.

Comment 19

[Tarek Ziadé (:tarek)]

> Absolutely. I'm available to chat as soon as you can :), just ping me on IRC or send me a meeting invite.

I guess that kinda happened via Rémy :)

This week ALexis and Rémy are not working but I am around in case there's anything you want to talk about / work on https://github.com/mozilla-services/msisdn-gateway - it has been deployed on AWS so anyone can play with it - I can update it in case we want to change something.

Do not hesitate to ping me. I am 'tarek' on IRC / Paris timezone

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Thanks for letting me know! Actually, I proposed a few more changes to the server this morning https://github.com/mozilla-services/msisdn-gateway/pull/21 so I'll ping you about them on IRC tomorrow.

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: WebIDL

Jonas, this WebIDL exposes a getMobileIdAssertion() in the navigator object.

Forget about my comment regarding the need of another function to enter the verification code. All this user interaction will be done through the chrome UI, so I think we don't need any extra functions for this API.

Also, please ignore the comment with the wiki link as that page does not exists yet.

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM implementation (early WIP)

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Attachment: High level overview flow

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API (WIP)

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Make FxA specific stuff generic

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Mobile ID service (WIP)

Comment 27

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

Comment on attachment 8414623 [details] [diff] [review]
Part 0: WebIDL

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

The API looks good.

The only time that this throws is if we run out of memory creating the Promise, right? For all other types of errors we should simply reject the promise rather than throw. I'm not actually sure that you need a [Throws] to handle the out-of-memory thing.

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API (WIP)

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G UI Glue (WIP)

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: Mobile ID Service (WIP)

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API (WIP)

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Make FxA specific stuff generic

Comment 33

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G UI Glue (WIP)

Comment 34

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: Mobile ID core (WIP)

Comment 35

[Jason Smith [:jsmith]]

This isn't a vendor bug to my understanding, it's a gecko WebAPI.

Comment 36

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8427846 [details] [diff] [review]
Part 2: Make FxA specific stuff generic

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

::: services/common/hawkclient.js
@@ +177,5 @@
>        let status = restResponse.status;
>  
>        log.debug("(Response) " + path + ": code: " + status +
>                  " - Status text: " + restResponse.statusText);
> +      log.trace("Response text: " + restResponse.body);

I think we still want the pii check

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Par 1: DOM API

Jonas, this patch still needs some work + tests but I'd love to hear some feedback from you in the mean time. Regarding your comment about [Throws] in the webidl from comment 27, I believe that we still need to check if we have a window and a docshell, but if you think that is not needed, I'll remove that check and the [Throws] and reject the promise in other cases.
Thanks in advance for your feedback!

Comment 38

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G UI Glue

Fabrice, as the DOM part, this patch still needs some tests and I'd like to test the whole flow when it is completed before asking for your review, but I'd like to have your feedback in the meantime.
Thanks!

Comment 39

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: Mobile ID core (WIP)

Mark and Jed,

This patch contains the core part of the MobileID service. I've been able to successfully test the SMS MT flow by running the server locally and manually sending the verification code from other device since the server (https://github.com/mozilla-services/msisdn-gateway) is not ready yet. I still need to work on the SMS MO+MT flow, some edge cases and a whole bunch of tests, but it would be great if you could start giving me feedback about this.

Thanks in advance!

Comment 40

[Fernando Jiménez Moreno [:ferjm]]

Attachment: High level overview flow

Updated flow

Comment 41

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

Comment on attachment 8431758 [details]
High level overview flow

Should the second box say "Is there an assertion for this origin" though? Or is "certificate" the right word?

Is the reason we can have a certificate/assertion for an origin, but still not have a permission, that we generated the certificate/assertion for the same origin but for another app?

If the ICC has changed, shouldn't that send us to the top blue box on the right side? I.e. can't a changed ICC trigger a MO+MT flow?

Comment 42

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

Comment on attachment 8431750 [details] [diff] [review]
Par 1: DOM API

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

::: dom/mobileid/src/MobileIdentity.js
@@ +32,5 @@
> +    log.debug("getMobileIdAssertion");
> +
> +    if (!this.init) {
> +      this.initDOMRequestHelper(aWindow, IPC_MSG_NAMES);
> +      this.init = true;

Do you use this?

@@ +35,5 @@
> +      this.initDOMRequestHelper(aWindow, IPC_MSG_NAMES);
> +      this.init = true;
> +    }
> +
> +    this._window = aWindow;

Services are global to a process, so they will be shared across many windows. So you you can't do this. Stick the window in the promise-resolver or some such.

Comment 43

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8431756 [details] [diff] [review]
Part 4: Mobile ID core (WIP)

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

I really don't have enough b2g context for my feedback to be valuable, so I just picked on a few trivial/easy things :)

Jed, do you think we should also ask warner for feedback?

::: b2g/chrome/content/shell.js
@@ +29,5 @@
>  Cu.import('resource://gre/modules/FxAccountsMgmtService.jsm');
>  #endif
>  
>  Cu.import('resource://gre/modules/DownloadsAPI.jsm');
> +Cu.import('resource://gre/modules/MobileIdentityManager.jsm');

I don't know much about b2g, but it seems possible this should be a lazyModuleGetter?

::: services/mobileid/MobileIdentityCertStore.jsm
@@ +77,5 @@
> +  getByMsisdn: function(aMsisdn) {
> +    log.debug("getByMsisdn " + aMsisdn);
> +    let deferred = Promise.defer();
> +    if (!aMsisdn) {
> +      deferred.resolve(null);

minor, but you could just return Promise.resolve() here, and init the deferred only past that block - and ditto a few other places in this file

::: services/mobileid/MobileIdentityManager.jsm
@@ +182,5 @@
> +        log.debug("onVerificationResult ");
> +        if (!verificationResult || !verificationResult.certificate ||
> +            !verificationResult.keyPair) {
> +          this.activeVerificationDeferred.reject(
> +            new Error(ERROR_INTERNAL_INVALID_VERIFICATION_RESULT)

it looks like you want to return here (and below) after rejecting the promise?

@@ +204,5 @@
> +          );
> +        }
> +
> +        // Store the new certificate.
> +        this.certStore.put(this.activeVerificationFlow.iccId,

I think rather than nesting this you should chain - so something like:

this.activeVerificationFlow.doVerification().then(
  verificationResult => {
    ....
    return this.certStore.put(this.activeVerificationFlow.iccId,
                              ...); 
  }
).then(
  () => this.activeVerificationDeferred.resolve(verificationResult);
).then(
  null,
  err => {
});

Also note the very final promise rejection handler, which should catch all unexpected errors - the end of the chain should always be a .then(null, err) - ending the chain with .then(goodHandler, badHandler) will not catch errors in goodHandler.

Finally, the use of/lifetime of this.activeVerificationDeferred looks a little suspect (eg, it's not clear at a quick glance how we know that promise isn't already resolved/rejected here), but that might just be due to me not looking hard enough :)

@@ +272,5 @@
> +      origin: aOrigin
> +    }
> +    this.activeVerificationFlow = new MobileIdentitySmsMtVerificationFlow(
> +      toVerify.msisdn,
> +      serviceId == undefined, // external

maybe you want === here?  IIRC, null will compare equal to undefined.  OTOH, if that's fine, then maybe just !!serviceId?

@@ +501,5 @@
> +        // Or we might already have a certificate for the selected phone
> +        // number and so we do the same: update the certificate store with the
> +        // new origin and return the certificate.
> +        return this.certStore.getByMsisdn(promptResult.msisdn)
> +        .then(

similarly here - you can probably chain rather than next

@@ +528,5 @@
> +    jwcrypto.generateAssertion(aCert.certificate, aCert.keyPair, null,
> +                               options, (error, signed) => {
> +      if (error) {
> +        log.error("Error generating assertion " + err);
> +        deferred.reject(error);

missing a return here, or you will both reject and resolve on error

::: services/mobileid/MobileIdentitySmsMtVerificationFlow.jsm
@@ +225,5 @@
> +        smsService.removeSilentNumber(this.observedSilentNumber);
> +        Services.obs.removeObserver(this.onSilentSms,
> +                                    SILENT_SMS_RECEIVED_TOPIC);
> +      } catch (e) {
> +        log.warn(e);

if this catch is really for removeSilentNumber and there is a reasonable chance it might fail, it looks like you will leak the observer.

::: services/mobileid/MobileIdentityUIGlue.manifest
@@ +1,2 @@
> +component {74bd1c90-dfa7-11e3-8b68-0800200c9a66} MobileIdentityUIGluePromptResult.js
> +contract @mozilla.org/services/mobileid-ui-glue-prompt-result;1 {74bd1c90-dfa7-11e3-8b68-0800200c9a66}

A little surprised JSMs aren't good enough here, but that's almost my lack of b2g context.

::: services/mobileid/MobileIdentityVerificationFlow.jsm
@@ +48,5 @@
> +                                    ASSERTION_LIFETIME)
> +      .then(
> +        (result) => {
> +          if (!result || !result.cert) {
> +            throw new Error("Wrong verification code");

throwing here looks a little suspect - I'd expect a rejection

Comment 44

[Fernando Jiménez Moreno [:ferjm]]

Thanks for your feedback Jonas!

(In reply to Jonas Sicking (:sicking) from comment #41)
> Comment on attachment 8431758 [details]
> High level overview flow
> 
> Should the second box say "Is there an assertion for this origin" though? Or
> is "certificate" the right word?
> 

Errmm... I should have updated the diagram after writing the code...

Actually it should say "credentials". Assertions are generated with the certificate that is gotten from https://github.com/mozilla-services/msisdn-gateway/blob/master/API.md#post-v1certificatesign but we don't save to disk these certificates (they have a very short live so we only store them in memory until they expire). We only store in disk the HAWK credentials that allow us to get new signed certificates to generate new assertions.

> Is the reason we can have a certificate/assertion for an origin, but still
> not have a permission, that we generated the certificate/assertion for the
> same origin but for another app?
> 

Yes, we can have credentials stored for an specific MSISDN but we require specific permission per origin to share the assertions with 3rd party requesters. Also, the user should be able to remove these permissions at some point (probably through the Settings app), but we won't be implementing that for 2.0.

> If the ICC has changed, shouldn't that send us to the top blue box on the
> right side? I.e. can't a changed ICC trigger a MO+MT flow?

Yes, that's actually what the code does. I'll update the diagram.

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jonas Sicking (:sicking) from comment #42)
> Comment on attachment 8431750 [details] [diff] [review]
> Par 1: DOM API
> 
> Review of attachment 8431750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileid/src/MobileIdentity.js
> @@ +32,5 @@
> > +    log.debug("getMobileIdAssertion");
> > +
> > +    if (!this.init) {
> > +      this.initDOMRequestHelper(aWindow, IPC_MSG_NAMES);
> > +      this.init = true;
> 
> Do you use this?
> 

Yes, I added that to avoid adding the IPC message listeners and all the other stuff that happens in the DOMRequestHelper initialization process in consecutive calls to the API.

> @@ +35,5 @@
> > +      this.initDOMRequestHelper(aWindow, IPC_MSG_NAMES);
> > +      this.init = true;
> > +    }
> > +
> > +    this._window = aWindow;
> 
> Services are global to a process, so they will be shared across many
> windows. So you you can't do this. Stick the window in the promise-resolver
> or some such.

I am not sure that I understand this comment.

Actually, you are right that I should not save the window at that point, but only because I don't need to do it there since it is already being saved by DOMRequestIPCHelper [1]. Other than that, saving the window instance for later usage is an extended pattern implemented in several WebAPI since many WebAPI implementations inherit from DOMRequestIPCHelper ([2][3][4] for example). Is that what is not correct?

[1] https://mxr.mozilla.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm#154
[2] https://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js#462
[3] https://mxr.mozilla.org/mozilla-central/source/dom/network/src/NetworkStatsManager.js#404
[4] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#230

Comment 46

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Mark Hammond [:markh] from comment #43)
> Comment on attachment 8431756 [details] [diff] [review]
> Part 4: Mobile ID core (WIP)
> 
> Review of attachment 8431756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really don't have enough b2g context for my feedback to be valuable, so I
> just picked on a few trivial/easy things :)
> 

Thanks Mark! I'll update the patch addressing your feedback and more stuff probably tonight.

Comment 47

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

(In reply to Fernando Jiménez Moreno [:ferjm] (work week, not reading bugmail) from comment #45)
> > Services are global to a process, so they will be shared across many
> > windows. So you you can't do this. Stick the window in the promise-resolver
> > or some such.
> 
> I am not sure that I understand this comment.

Services are global for a process. I.e. any time a Gecko process uses a service, it will get the exact same object instance. The do_GetService code ensures this.

Since we use the same process to render multiple different windows, potentially from multiple different origins, it means that JS code from all of those windows will end up calling into the same service.

If you store the window in the service, it will have two effects:

* The window that last used the service will be held alive until another window uses the service. This is effectively a leak.
* When window A calls into the API, you might end up using a different window B to create the DOMError object return to window A. Wrappers ensure that this isn't a security problem, but it will result in A not being able to use the DOMError object.

Comment 48

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: WebIDL

Comment 49

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API

Thanks for the detailed explanation Jonas.

I've linked the window instance to the promise ID locally in the MobileIdentity service and filed a follow up bug (bug 1020582) to store it in DOMRequestIPCHelper instead. I prefer to do that in a follow up bug because it affects several APIs.

Comment 50

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Remove specific FxA stuff from HAWK client

Mark, I added the logPII thing back, but made it specific for the HAWK client.

Comment 51

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: B2G UI Glue

Vivien, could you take a look at this patch, please? Fabrice already gave his feedback+ but he is away until June 17th.

The Gaia counter part of this code is bug 1003330.

Thanks!

Comment 52

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: Mobile ID core

Mark, Jed, I did a few changes based on Mark's previous feedback (thanks!) and other pending stuff like the MO+MT flow.

The server is mostly ready so I was able to successfully test both authentication flows (\o/).

I am aware that this patch is quite big, so let me know if I can clarify any details about it or if there is any way that I can help you understand the code in a better way, please. I can also split the patch in different patches if that helps your reviews.

Thanks in advance for your time and reviews!

Comment 53

[Fernando Jiménez Moreno [:ferjm]]

Working on the tests now.

Comment 54

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

Comment on attachment 8434429 [details] [diff] [review]
Part 1: DOM API

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

Looks good with the below fixes!

::: dom/mobileid/src/MobileIdentity.js
@@ +28,5 @@
> +MobileIdentityService.prototype = {
> +  __proto__: DOMRequestIpcHelper.prototype,
> +
> +  // TODO: this should be handled by DOMRequestIpcHelper. Bug 1020582
> +  _windows: null,

Just do "_windows: {}," here instead.

@@ +47,5 @@
> +        });
> +
> +        if (!this._windows) {
> +          this._windows = {};
> +        }

And remove this

@@ +90,5 @@
> +        promise.reject(new _window.DOMError(msg.error || ERROR_UNKNOWN));
> +        break;
> +    }
> +
> +    delete this._windows[promiseId];

Move this to right after you grab the window. Just in case someone adds code that ads early-returns later.

Comment 55

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8434434 [details] [diff] [review]
Part 3: B2G UI Glue

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

r+ with nits.

::: b2g/components/ContentRequestHelper.jsm
@@ +18,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "SystemAppProxy",
> +                                  "resource://gre/modules/SystemAppProxy.jsm");
> +
> +function debug(msg) {
> +  dump("ContentRequestHelper ** " + msg + "\n");

Let's // this line before landing.

@@ +51,5 @@
> +      } else {
> +        deferred.resolve(msg.result);
> +      }
> +      SystemAppProxy.removeEventListener(aContentEventName,
> +                                         onContentEvent);

Let's move the removal right after the start of the function. First this is usually cleaner, and that would also factorize this code that you do in any cases IIUC.

::: b2g/components/MobileIdentityUIGlue.js
@@ +42,5 @@
> +    return this._oncancel;
> +  },
> +
> +  set oncancel(aCallback) {
> +    this._oncancel = aCallback;

Instead of changing the shape of the object on the fly, let's add a |_oncancel: null,| above the first getter.

@@ +50,5 @@
> +    return this._onresendcode;
> +  },
> +
> +  set onresendcode(aCallback) {
> +    this._onresendcode = aCallback;

Same comment than |_oncancel|

@@ +57,5 @@
> +  startFlow: function(aManifestURL, aIccInfo) {
> +    let phoneNumberInfo;
> +    if (aIccInfo) {
> +      phoneNumberInfo = [];
> +      for (var i = 0; i < aIccInfo.length; i++) {

var iccInfo = aIccInfo[i]; and then use that.

@@ +72,5 @@
> +
> +    return this.contentRequest(CONTENT_EVENT,
> +                               CHROME_EVENT,
> +                               "onpermissionrequest",
> +                               { phoneNumberInfo: phoneNumberInfo,

Do you want to send |undefined| here or an empty array ?

@@ +76,5 @@
> +                               { phoneNumberInfo: phoneNumberInfo,
> +                                 manifestURL: aManifestURL })
> +    .then(
> +      (result) => {
> +        log.debug("Content request result " + JSON.stringify(result));

Remove this debug line.

@@ +79,5 @@
> +      (result) => {
> +        log.debug("Content request result " + JSON.stringify(result));
> +        if (!result || !result.phoneNumber && !result.serviceId) {
> +          throw new Error("Invalid prompt result");
> +        }

nit: add a line break

@@ +100,5 @@
> +                                 verificationTimeout: aTimeout,
> +                                 verificationTimeoutLeft: aTimeLeft })
> +    .then(
> +      (result) => {
> +        log.debug("Content request result " + JSON.stringify(result));

Remove this debug.

@@ +102,5 @@
> +    .then(
> +      (result) => {
> +        log.debug("Content request result " + JSON.stringify(result));
> +        if (!result || !result.verificationCode) {
> +          throw new Error("Invalid prompt result");

The output of this error and the one for the previous method are exactly the same. Maybe you want a slightly different message ?

@@ +103,5 @@
> +      (result) => {
> +        log.debug("Content request result " + JSON.stringify(result));
> +        if (!result || !result.verificationCode) {
> +          throw new Error("Invalid prompt result");
> +        }

nit: add a line break

@@ +106,5 @@
> +          throw new Error("Invalid prompt result");
> +        }
> +        return result.verificationCode;
> +      }
> +    );

I don't know if this is the usual indentation for promise but it looks ugly :)

@@ +110,5 @@
> +    );
> +  },
> +
> +  error: function(aError) {
> +    log.error("UI error " + aError);

Where does |log| comes from ? I would like to make sure this stuff is turned off for production code.

@@ +130,5 @@
> +    if (!msg) {
> +      log.warning("Got invalid event");
> +      return;
> +    }
> +    log.debug("Got content event " + JSON.stringify(msg));

Not sure you want to let this even if the ouput is turned off. JSON.stringify is expensive and we got some issues in the past with debug code and it. :)

@@ +140,5 @@
> +      case 'resendcode':
> +        this.onresendcode();
> +        break;
> +      default:
> +        log.warning("Invalid event name");

nit: add a break.

Comment 56

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

Comment on attachment 8434438 [details] [diff] [review]
Part 4: Mobile ID core

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

Fernando, this is beautiful work.  Reading it for the first time now, I can't say I grasp fully the intricacies of the state machine, and all the edge cases that are involved in the sms flows.  That said, the logic of the patch is clear and the code is solid.  I have only a handful of questions.  r=me.

::: b2g/app/b2g.js
@@ +969,5 @@
>  pref("services.sync.fxaccounts.enabled", true);
>  pref("identity.fxaccounts.enabled", true);
>  #endif
>  
> +pref("services.mobileid.server.uri", "http://msisdn.dev.mozaws.net");

Do we ultimately want a check to ensure that the server.uri is using https?

::: services/mobileid/MobileIdentityCredentialsStore.jsm
@@ +91,5 @@
> +        cursorReq.onerror = function(aEvent) {
> +          log.error(aEvent.target.error);
> +          deferred.reject(ERROR_INTERNAL_DB_ERROR);
> +        };
> +      }

Do you want to add a failureCb for newTxn?  Maybe add the final two callback args: 'null, promise.reject'?

@@ +134,5 @@
> +        index.get(aValue).onsuccess = function(aEvent) {
> +          aTxn.result = aEvent.target.result;
> +        };
> +      },
> +      function(result) {

Slight stylistic inconsistency - you have simply 'deferred.resolve', with no explicit argument, for the successCb for getByMsisdn.

::: services/mobileid/MobileIdentityManager.jsm
@@ +69,5 @@
> +    Services.obs.addObserver(this, "xpcom-shutdown", false);
> +    ppmm.addMessageListener(GET_ASSERTION_IPC_MSG, this);
> +    this.messageManagers = {};
> +    this.keyPairs = {};
> +    this.certificates = {};

Is it necessary to store these more durably (e.g., in indexedDB), in case the phone is restarted?

@@ +399,5 @@
> +        aToVerify.verificationDetails.mtSender,
> +        // TODO: Just for testing purposes with a spanish sim until
> +        // https://github.com/mozilla-services/msisdn-gateway/issues/73
> +        // is fixed.
> +        "[phone]", //aToVerify.verificationDetails.moVerifier,

Looks like issue 73 was just closed.  Don't forget to change me!

@@ +413,5 @@
> +      this.activeVerificationFlow = new MobileIdentitySmsMtVerificationFlow(
> +        aOrigin,
> +        aToVerify.msisdn,
> +        aToVerify.iccId,
> +        aToVerify.serviceId === undefined, // external

I don't fully understand what "external" means here.  Could you add a fuller comment to clarify, either here or in MobileIdentityUIGlueCommon.jsm?

[edit: aha! I see the comment now in MobileIdentitySmsVerificationFlow.jsm.  That's very helpful.  It could still be duplicated in MobileIdentityUIGlueCommon.jsm, I think.]

@@ +661,5 @@
> +      (keyPair) => {
> +        log.debug("keyPair " + keyPair.serializedPublicKey);
> +        let options = {
> +          duration: ASSERTION_LIFETIME,
> +          now: this.client.hawk.now()

You can add 'localtimeOffsetMsec: this.client.hawk.localtimeOffsetMsec' to these options, to account for skew in the local clock vis-a-vis the server.

@@ +769,5 @@
> +
> +        this.ui.verified();
> +
> +        let mm = this.messageManagers[aPromiseId];
> +        mm.sendAsyncMessage("MobileId:GetAssertion:Return:OK", {

Yay!  I feel triumphant arriving at this line.  This module performs some tricky and complex processes, and is never anything short of clear, legible, and beautiful throughout.

::: services/mobileid/MobileIdentitySmsMtVerificationFlow.jsm
@@ +66,5 @@
> +    }
> +#endif
> +    return this.client.smsMtVerify(this.sessionToken,
> +                                   this.verificationOptions.msisdn,
> +                                   this.verificationOptions.external);

How would this actually be used on desktop or other non-b2g firefox?  Does this want to be included in the #ifdef block?  (I'm sure I'm missing something.)

::: services/mobileid/MobileIdentitySmsVerificationFlow.jsm
@@ +69,5 @@
> +#endif
> +  },
> +
> +#ifdef MOZ_B2G_RIL
> +  onSilentSms: function(aSubject, aTopic, aData) {

It looks like this could be called, however unlikely, from MobileIdentitySmsMoMtVerificationFlow in a non-b2g context.  Would it be better to put the #ifdef inside the function declaration so it's a no-op on other platforms?

::: services/mobileid/interfaces/nsIMobileIdentityUIGlue.idl
@@ +7,5 @@
> +[scriptable, uuid(5759e799-63eb-4d06-a03d-ae797fda3882)]
> +interface nsIMobileIdentityUIGlue : nsISupports
> +{
> +  /**
> +  * Request the creation of a Mobile ID UI flow.

Note to self: Always jump to the idl files first when reviewing huge patches.  They are a good guide :)

Comment 57

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8434438 [details] [diff] [review]
Part 4: Mobile ID core

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

The lack of tests is troubling - can you please file a p1 bug to add some?

I believe there are a number of errors here, but they are relatively minor, and the general code is in good shape as Jed says.  Given the tight deadlines here, r=me with these comments all addressed (although ones where I say "consider" or am just making a minor comment are optional and at your discretion)

::: services/mobileid/MobileIdentityClient.jsm
@@ +34,5 @@
> +  register: function() {
> +    return this._request(REGISTER, "POST", null, {});
> +  },
> +
> +  smsMtVerify: function(aSessionToken, aMsisdn, aShortVerificationCode) {

consider declaring aShortVerificationCode=false, to avoid the '|| false' later and to make it clear the param is optional.  Also, consider renaming the variable to something that reads like a boolean - eg aWantShortCode?  At first glance I assumed this variable was holding a code, so seeing '|| false' in the body was confusing (eg, verifyCode takes aVerificationCode, which I assume is *not* a boolean even though the names are very similar)

@@ +83,5 @@
> +   *          id: the Hawk id (from the first 32 bytes derived)
> +   *          key: the Hawk key (from bytes 32 to 64)
> +   *        }
> +   */
> +  _deriveHawkCredentials: function(aSessionToken) {

I've seen this cargo-culted a few times now - it seems to be screaming to be part of hawkclient itself (which you should feel free to do, but also feel free to ignore ;)

@@ +129,5 @@
> +      },
> +
> +      (error) => {
> +        log.error("MobileIdentityClient -> Error " + JSON.stringify(error));
> +        if (!error.code && !error.errno) {

this seems a little strange - it looks like you are expecting an error object with .code and .errno attributes and if so, reject directly with that error.  But if not, you reject with a simple string value.  ie, I'd be expecting something like: error = {code: SERVER_ERROR_NO_RESPONSE} or something, so the consumer of the promise reject always gets something with a consistent "shape".  I'm not too bothered by this though.

::: services/mobileid/MobileIdentityManager.jsm
@@ +207,5 @@
> +      }
> +    )
> +    .then(
> +      (result) => {
> +        log.debug("Discover result " + JSON.stringify(result));

The logging module has recently grown a new feature - you could write this as:

  log.debug("Discover result ${}", result)
or, say:
  log.debug("Discover result ${result}", {result: result});

which will still write the JSON repr of result, but the JSON.stringify will only happen if the log level is debug - ie, in most cases, it will not happen.  Bug 966674 for more info and at your discretion.

@@ +226,5 @@
> +    // We try to get if we already have credentials for any of the inserted
> +    // SIM cards if any is available and we try to get the possible
> +    // verification mechanisms for these SIM cards.
> +    // All this information will be stored in iccInfo.
> +    let deferred = Promise.defer();

I think it would be better to write this like some of the others - ie:

  if (!this.iccInfo || !this.iccInfo.length) {
    return Promise.resolve();
  }
  let deferred = Promise.defer();
  ...

@@ +233,5 @@
> +      return deferred.promise;
> +    }
> +
> +    for (let i = 0; i < this.iccInfo.length; i++) {
> +      return this.getVerificationOptionsForIcc(i)

this looks wrong to me - doesn't the return mean that i never goes > 0?  Even without the return I'm not sure it is correct - if we fire off |this.iccInfo.length| promises, it seems possible the final one will resolve before earlier ones (and I assume the intent is to resolve the returned promise only when all are resolved).

I think this could be written as:
  let promises = [];
  for (let i = 0; i < this.iccInfo.length; i++) {
    promises.push(this.getVerificationOptionsForIcc(i));
  }
  return Promise.all(promises);
(or even fancier using map etc ;)

Assuming I'm not mising something obvious, please fix this.

@@ +320,5 @@
> +   ********************************************************/
> +
> +  hasPermission: function(aPrincipal) {
> +    let permission = permissionManager.testPermissionFromPrincipal(aPrincipal,
> +                                                                MOBILEID_PERM);

this line isn't aligned correctly (I'd prefer to see it extend just past 80 chars if necessary; YMMV)

@@ +372,5 @@
> +          );
> +        }
> +        this.resolveVerification(verificationResult);
> +      },
> +      (reason) => {

This rejection handler should be at the end of the chain, so it will handle (unlikely) errors in the resolution handler - ie:

this.activeVerificationFlow.doVerification()
.then(
  (verificationResult) => { ...
  })
.then(null, reason => {
});

@@ +445,5 @@
> +    // inserted SIMs known phone numbers.
> +    if (aUserSelection.msisdn && this.iccInfo) {
> +      for (let i = 0; i < this.iccInfo.length; i++) {
> +        if (aUserSelection.msisdn == this.iccInfo[i].msisdn) {
> +          serviceId = i;

can we break after this?

@@ +452,5 @@
> +    }
> +
> +    let toVerify = {};
> +
> +    if (serviceId) {

can't the loop above end up with serviceId being 0 and valid, but causing us to not enter this block?  It looks like this condition should be !=== undefined?  This looks like an error that must be fixed.

@@ +556,5 @@
> +        // if it is not notify the UI about the error and allow the user to
> +        // retry.
> +        if (msisdn && mcc &&
> +            !PhoneNumberUtils.parse(msisdn) &&
> +            !PhoneNumberUtils.parseWithMCC(msisdn, mcc)) {

this looks a little strange - isn't one of the .parse functions failing enough to be treated as an error, or do both really need to fail to be considered invalid?

@@ +603,5 @@
> +      }
> +    )
> +    .then(
> +      (promptResult) => {
> +        log.debug("promptResult " + JSON.stringify(promptResult));

see previous comment about how the logging module can avoid the stringify if necessary

@@ +776,5 @@
> +        });
> +      }
> +    )
> +    .then(
> +      () => {},

null here should be fine

::: services/mobileid/MobileIdentitySmsMoMtVerificationFlow.jsm
@@ +77,5 @@
> +      smsService.addSilentNumber(this.observedSilentNumber);
> +      Services.obs.addObserver(this.onSilentSms.bind(this),
> +                               SILENT_SMS_RECEIVED_TOPIC,
> +                               false);
> +      log.debug("Observing messages from " + this.observedSilentNumber);

See later comments - adding the result of .bind() is wrong as you can't remove it, and the catch is suspect as it implies some logic error if anything throws.  It also seems error prone and hard to maintain to have one module addObserver and a different removeObserver, but there might be a good reason I don't understand - so please consider seeing if this can be changed (eg, I wonder if this function could end with:

return deferred.promise.then(
  result => {
    removeObserver(...);
    return result;
  }, err => {
    removeObserver(...);
    throw err;
  }
);
?

@@ +89,5 @@
> +    // The MO SMS body that the server expects contains the API endpoint for
> +    // the MO verify request and the HAWK ID parameter derived via HKDF from
> +    // the session token. These parameters should go unnamed and space limited.
> +
> +     let body = SMS_MO_MT_VERIFY + " " +

wrong indentation on this line.

::: services/mobileid/MobileIdentitySmsMtVerificationFlow.jsm
@@ +56,5 @@
> +#ifdef MOZ_B2G_RIL
> +    this.observedSilentNumber = this.verificationOptions.mtSender;
> +    try {
> +      smsService.addSilentNumber(this.observedSilentNumber);
> +      Services.obs.addObserver(this.onSilentSms.bind(this),

see other comments - adding the result of .bind() is almost certainly wrong - you can't remove it.  Please fix this as described elsewhere.

@@ +61,5 @@
> +                               SILENT_SMS_RECEIVED_TOPIC,
> +                               false);
> +      log.debug("Observing messages from " + this.observedSilentNumber);
> +    } catch (e) {
> +      log.warn(e);

this catch is also suspect - unless you are just catching failures from .addSilentNumber, in which case the addObserver call should be outside the block.

::: services/mobileid/MobileIdentitySmsVerificationFlow.jsm
@@ +61,5 @@
> +
> +      try {
> +        Services.obs.removeObserver(this.onSilentSms,
> +                                    SILENT_SMS_RECEIVED_TOPIC);
> +      } catch (e) {

I'm not that keen on these error handlers.  Failure to remove an observer would imply a logic error that needs to be fixed rather than ignored.  In this case, I believe it will fail because you are adding this.onSilentSms.bind(this) - which is different than what is being removed here.  Note you also can't simply remove this.onSilentSms.bind(this) - each bind returns a new object.

I think the best answer is simply to define onSilentSms as : (aSubject, aTopic, aData) => {...} and you should be then able to add and remove it without binding it.

So please remove the catch() handler and verify things work as expected, and if not, work out what has gone wrong to make it fail (eg, trying to remove twice, remove before adding, etc)

::: services/mobileid/MobileIdentityVerificationFlow.jsm
@@ +53,5 @@
> +                                  VERIFICATIONCODE_TIMEOUT,
> +                                  this.timer.TYPE_ONE_SHOT);
> +    }
> +
> +    this.verificationCodeDeferred = Promise.defer();

does this need to be created even if you reject due to !this.verifyStrategy?

@@ +78,5 @@
> +                                         timeLeft)
> +          .then(
> +            (verificationCode) => {
> +              if (!verificationCode) {
> +                return Promise.reject(ERROR_INTERNAL_INVALID_PROMPT_RESULT);

it looks like this should be rejecting this.verificationCodeDeferred - I don't think this rejection will end up rejecting the returned promise otherwise.

::: services/mobileid/interfaces/nsIMobileIdentityUIGlue.idl
@@ +21,5 @@
> +  * @iccInfo array of objects containing the information about the
> +  *          SIM cards available in the device and that can be used for the
> +  *          phone number verification and share process.
> +  *
> +  * Returns a Promise. An instance of nsIMobileIdentityUIGluePromptResult will

It might also be nice to mention what the rejection value will be for promise returning functions - I'm *guessing* it might be a simple string that is then passed to the .error() method?

(I took Jed's advice and started with this file, so some of this might be clearer when I get to the code, but it's probably worth having the IDL cover these)

@@ +27,5 @@
> +   */
> +  jsval startFlow(in DOMString manifestURL, in jsval iccInfo);
> +
> +  /**
> +   * The verification code prompt request to the user the introduction of a

This comment needs rewording - IIUC, something like "Will prompt the user to enter a code used to verify a phone number.  This will only be called if an external phone number is selected in startFlow()."

@@ +34,5 @@
> +   * a previous step.
> +   *
> +   * @retries number of retries left to validate a verification code.
> +   * @timeout TTL of the verification code in miliseconds.
> +   * @timeLeft miliseconds left before the verification code expires.

The comments here don't seem clear to me.  I'd have expected TTL to be equivalent to expiry (ie, it is expired after TTL elapses), so I'd have though a single millisecond value indicating when the code expires would be enough.

Comment 58

[Fernando Jiménez Moreno [:ferjm]]

Thanks Vivien!

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #55)
> @@ +106,5 @@
> > +          throw new Error("Invalid prompt result");
> > +        }
> > +        return result.verificationCode;
> > +      }
> > +    );
> 
> I don't know if this is the usual indentation for promise but it looks ugly
> :)
> 

Heh, I've seen it like this all over Gecko, so I followed the same style :)

> @@ +110,5 @@
> > +    );
> > +  },
> > +
> > +  error: function(aError) {
> > +    log.error("UI error " + aError);
> 
> Where does |log| comes from ? I would like to make sure this stuff is turned
> off for production code.

|log| comes from MobileIdentityCommon.jsm and it is behind a pref that defines the logging level. By default we log only errors.

> 
> @@ +130,5 @@
> > +    if (!msg) {
> > +      log.warning("Got invalid event");
> > +      return;
> > +    }
> > +    log.debug("Got content event " + JSON.stringify(msg));
> 
> Not sure you want to let this even if the ouput is turned off.
> JSON.stringify is expensive and we got some issues in the past with debug
> code and it. :)
> 

I've changed it for |log.debug("Got content event ${}", msg);| which will only JSON.stringify the object if the log level is debug (thanks to Mark for the suggestion).

Comment 59

[Fernando Jiménez Moreno [:ferjm]]

Thanks Jed!

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #56)
> 
> ::: b2g/app/b2g.js
> @@ +969,5 @@
> >  pref("services.sync.fxaccounts.enabled", true);
> >  pref("identity.fxaccounts.enabled", true);
> >  #endif
> >  
> > +pref("services.mobileid.server.uri", "http://msisdn.dev.mozaws.net");
> 
> Do we ultimately want a check to ensure that the server.uri is using https?

Thanks for the suggestion. I've added the check in MobileIdentityClient and a preference "services.mobileid.forcehttp". I'll land this with the pref off and will pref it on when the server is https. Bug 1021595

> ::: services/mobileid/MobileIdentityManager.jsm
> @@ +69,5 @@
> > +    Services.obs.addObserver(this, "xpcom-shutdown", false);
> > +    ppmm.addMessageListener(GET_ASSERTION_IPC_MSG, this);
> > +    this.messageManagers = {};
> > +    this.keyPairs = {};
> > +    this.certificates = {};
> 
> Is it necessary to store these more durably (e.g., in indexedDB), in case
> the phone is restarted?
> 

That's a good point, but I am not sure that storing the key pair is possible in indexedDB since it's a pretty complex object. I filed bug 1021605 for this.

> 
> @@ +769,5 @@
> > +
> > +        this.ui.verified();
> > +
> > +        let mm = this.messageManagers[aPromiseId];
> > +        mm.sendAsyncMessage("MobileId:GetAssertion:Return:OK", {
> 
> Yay!  I feel triumphant arriving at this line.  This module performs some
> tricky and complex processes, and is never anything short of clear, legible,
> and beautiful throughout.

:)

> 
> ::: services/mobileid/MobileIdentitySmsMtVerificationFlow.jsm
> @@ +66,5 @@
> > +    }
> > +#endif
> > +    return this.client.smsMtVerify(this.sessionToken,
> > +                                   this.verificationOptions.msisdn,
> > +                                   this.verificationOptions.external);
> 
> How would this actually be used on desktop or other non-b2g firefox?  Does
> this want to be included in the #ifdef block?  (I'm sure I'm missing
> something.)

The smsMtVerify is just telling the server to send us an SMS to the given MSISDN. In Firefox Desktop we won't be receiving the SMS in Firefox itself (that's why the silent SMS code is included within the #ifdef block) however we can still receive the SMS in another mobile phone and provide the verification code manually.

All this patch is thought to allow Desktop and Android to also expose the mobile ID functionality, we would just need the UI Glue bits implementing nsIMobileIdentityUIGlue in Desktop and Android to make it work. (Which reminds me that I need to disable the MO+MT if MOZ_B2G_RIL is not defined :) )

> 
> ::: services/mobileid/MobileIdentitySmsVerificationFlow.jsm
> @@ +69,5 @@
> > +#endif
> > +  },
> > +
> > +#ifdef MOZ_B2G_RIL
> > +  onSilentSms: function(aSubject, aTopic, aData) {
> 
> It looks like this could be called, however unlikely, from
> MobileIdentitySmsMoMtVerificationFlow in a non-b2g context.  Would it be
> better to put the #ifdef inside the function declaration so it's a no-op on
> other platforms?
> 

That won't happen if we allow the MO+MT flow only on B2G. We can allow it later for Android too.

Comment 60

[Fernando Jiménez Moreno [:ferjm]]

Thanks for the awesome review Mark!

(In reply to Mark Hammond [:markh] from comment #57)
> Comment on attachment 8434438 [details] [diff] [review]
> Part 4: Mobile ID core
> 
> Review of attachment 8434438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The lack of tests is troubling - can you please file a p1 bug to add some?
> 
> I believe there are a number of errors here, but they are relatively minor,
> and the general code is in good shape as Jed says.  Given the tight
> deadlines here, r=me with these comments all addressed (although ones where
> I say "consider" or am just making a minor comment are optional and at your
> discretion)

Actually, I have a few half baked tests that I was planning to add to this bug, but I might be out of time for getting them reviewed before the 2.0 branch, so I'll add them in a follow-up bug as you suggest. Tests are easier to uplift.

> @@ +83,5 @@
> > +   *          id: the Hawk id (from the first 32 bytes derived)
> > +   *          key: the Hawk key (from bytes 32 to 64)
> > +   *        }
> > +   */
> > +  _deriveHawkCredentials: function(aSessionToken) {
> 
> I've seen this cargo-culted a few times now - it seems to be screaming to be
> part of hawkclient itself (which you should feel free to do, but also feel
> free to ignore ;)
>

I added it to hawkclient in the previous version of Part 2 but I ended up removing it because the derivation functions are different enough between services (FxA, MobileID) that I didn't see the point for a common helper. Even if this is not part of HAWK itself, it would be great if Mozilla services could use the same credentials derivation function. I'll check with the server side folks.

> @@ +129,5 @@
> > +      },
> > +
> > +      (error) => {
> > +        log.error("MobileIdentityClient -> Error " + JSON.stringify(error));
> > +        if (!error.code && !error.errno) {
> 
> this seems a little strange - it looks like you are expecting an error
> object with .code and .errno attributes and if so, reject directly with that
> error.  But if not, you reject with a simple string value.  ie, I'd be
> expecting something like: error = {code: SERVER_ERROR_NO_RESPONSE} or
> something, so the consumer of the promise reject always gets something with
> a consistent "shape".  I'm not too bothered by this though.
> 

I was waiting for the server to define the errors. I am changing this to parse the error code and errno and always return a single string. In the end, the only one that needs to know about server error codes is the REST client.

> ::: services/mobileid/MobileIdentitySmsVerificationFlow.jsm
> @@ +61,5 @@
> > +
> > +      try {
> > +        Services.obs.removeObserver(this.onSilentSms,
> > +                                    SILENT_SMS_RECEIVED_TOPIC);
> > +      } catch (e) {
> 
> I'm not that keen on these error handlers.  Failure to remove an observer
> would imply a logic error that needs to be fixed rather than ignored.  In
> this case, I believe it will fail because you are adding
> this.onSilentSms.bind(this) - which is different than what is being removed
> here.  Note you also can't simply remove this.onSilentSms.bind(this) - each
> bind returns a new object.
> 
> I think the best answer is simply to define onSilentSms as : (aSubject,
> aTopic, aData) => {...} and you should be then able to add and remove it
> without binding it.
> 
> So please remove the catch() handler and verify things work as expected, and
> if not, work out what has gone wrong to make it fail (eg, trying to remove
> twice, remove before adding, etc)
> 

Yeah, the bind wasn't helping here. I removed it, placed onSilentSms in the same scope and removed the try/catch for the removeObserver. Thanks!

Comment 61

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ffa3043bd8eb
https://hg.mozilla.org/integration/b2g-inbound/rev/52bbad182cb3
https://hg.mozilla.org/integration/b2g-inbound/rev/fe1432e7b895
https://hg.mozilla.org/integration/b2g-inbound/rev/8a38d3372bf2
https://hg.mozilla.org/integration/b2g-inbound/rev/e3dc24f42573

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ffa3043bd8eb
https://hg.mozilla.org/mozilla-central/rev/52bbad182cb3
https://hg.mozilla.org/mozilla-central/rev/fe1432e7b895
https://hg.mozilla.org/mozilla-central/rev/8a38d3372bf2
https://hg.mozilla.org/mozilla-central/rev/e3dc24f42573

Comment 63

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

Is there any documentation for what the webpage exposed API looks like?

The API between the device and the mozilla verification server seems documented at [1] (is that the right URL?), but it'd be good to have someplace to point people to so that they can understand what this looks like from a web developer point of view.

This would both include the JS API, and the API exposed by the mozilla verification server towards the application server.

[1] https://github.com/mozilla-services/msisdn-gateway

Comment 64

[Rémy Hubscher (:natim)]

There is no JS SDK yet as far as I know.

I think it could basically be some direct calls to the server

> $.post(msisdnHost+"/path", data)

 maybe :ferjm as more input on client side.

Comment 65

[Frederik Braun [:freddy]]

in JS you can do navigator.getMobileIdAssertion() which returns a promise.
I think Jonas referred to that one.

Considering this API is still young, can we change what the promise will fulfill to? Currently, it responds with a text that is multiple urlsafe-base64 blobs joined with a dot (e.g. base64blob1.base64blob2...).
I understand that the exact blob is still required to perform a correct signature validation.

Comment 66

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

Unless we expect developers to actually read data out of the assertion directly, I don't think that we need to define or document what it contains.

What we do need to define and document is how you get the assertion, and how you use that assertion to get a phone number. I.e. we need to define and document what the web-exposed JS API looks like, as well as the developer-exposed HTTP API from the mozilla server.

Comment 67

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jonas Sicking (:sicking) from comment #63)
> Is there any documentation for what the webpage exposed API looks like?
> 
> The API between the device and the mozilla verification server seems
> documented at [1] (is that the right URL?), but it'd be good to have
> someplace to point people to so that they can understand what this looks
> like from a web developer point of view.
> 
> This would both include the JS API, and the API exposed by the mozilla
> verification server towards the application server.
> 
> [1] https://github.com/mozilla-services/msisdn-gateway

Unfortunately there is no API documentation yet. I didn't have the time to work on it. I'll do it as soon as I can.

(In reply to Jonas Sicking (:sicking) from comment #66)
> Unless we expect developers to actually read data out of the assertion
> directly, I don't think that we need to define or document what it contains.
> 
> What we do need to define and document is how you get the assertion, and how
> you use that assertion to get a phone number. I.e. we need to define and
> document what the web-exposed JS API looks like, as well as the
> developer-exposed HTTP API from the mozilla server.

Yes, we need to document the DOM API and how developers can verify the given assertion.

Comment 68

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

Comment on attachment 8434438 [details] [diff] [review]
Part 4: Mobile ID core

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

::: services/mobileid/MobileIdentityManager.jsm
@@ +123,5 @@
> +      if (!info) {
> +        log.warn("No ICC info");
> +        continue;
> +      }
> +      this._iccInfo.push({

The _iccInfo[] is supposed to be an array that every element has the array index equals to its clientId. However, as you may see, there are several continue statement that "skips" unavailable ICC entries. So, you may actually have:

  [0: <iccInfo of client 1>, 1: <iccInfo of client 3>, ...]

And then I see:

  this.iccInfo[aServiceId].credentials = creds;

I haven't trace every line of this jsm but this is really terrifying judging from the term 'credentials' used here.

Besides, I don't see any event listener registered for either mobileconnection or icc event. The operater, roaming state, icc card lock, ... may vary all the time, but there is just no event listener at all.

Comment 69

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

Fernando, can you file a follow-up for these?

Comment 70

[Fernando Jiménez Moreno [:ferjm]]

You are absolutely right. Thanks for the catch Vicamo!

bug 1051746
bug 1051749

Comment 71

[Martijn Wargers (dead)]

https://moztrap.mozilla.org/manage/case/14513/