Closed Bug 1088105 Opened 6 years ago Closed 6 years ago

You're able to authenticate with a verified phone number even if you don't have the corresponding SIM card anymore

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
b2g-v2.0 --- affected
b2g-v2.1 --- affected

People

(Reporter: jlorenzo, Assigned: rhubscher)

References

Details

(Whiteboard: [2.1-bug-bash] )

Attachments

(1 file)

Build Information
Gaia-Rev        1e48e3e40e0780c0cd07a3457e5fe2efeeb542d1
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/09fb60a37850
Build-ID        20141023001201
Version         34.0
Device-Name     flame
Base image:     v188


Steps to Reproduce
1. Authenticate to Firefox Hello with your phone number
2. Log out
3. Turn off your device and switch you SIM card to another one
4. Turn on the device
5. Open Firefox Hello and select "Use phone number"
6. Tap your previous phone number


Expected Results
This phone number might not belong to you anymore, so it shouldn't be present in the list.

Actual Results
You're logged in.

Reproduction Frequency: 2/2
Whiteboard: [2.1-FC-bug-bash] → [2.1-bug-bash]
[Blocking Requested - why for this release]:

Does this affect 2.0?
Blocks: Loopmov_1_1
blocking-b2g: --- → 2.1?
Blocks: mobileid, 1036490
No longer blocks: Loopmov_1_1
Component: Gaia::Loop → DOM: Device Interfaces
Product: Firefox OS → Core
Flags: needinfo?(jlorenzo)
This is the currently designed behavior (at least for 2.0) as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1026999#c11. When you select log-out from Loop you are logging-out from the Loop Service, not from Mobile ID (similarly to what happens with FxAccounts). If you want to log-out totally from MobileID you need to deny that permission to the app.

Adding ni to Fernando as he owns that code and can provide a more detailed explanation and/or whether there are changes planned after 2.0.
Flags: needinfo?(ferjmoreno)
Per comment 2.
Flags: needinfo?(jlorenzo)
Per comment #2, clearing flag. Re-nom if not.
blocking-b2g: 2.1? → ---
As mentioned by María Ángeles on comment 2, this is allowed by design.

We allow users to keep using an already verified phone number if its corresponding SIM is not present in the device anymore the same way that we allow users to verify external phone numbers, understanding "external phone number" as a phone number that doesn't belong to any of the inserted SIM cards (if any) when the verification process is done. Note that this will always be the case for desktop once we support this API on Firefox.

The problem with this feature as you mentioned in the bug description is that the phone number might not belong to the user anymore and so we might face issues like users using an identity that don't belong to them anymore. This does not seem like a big deal for the Loop use case, but it might be a bigger issue for carrier billing dependent apps for example.

We briefly discussed this scenario at bug 1022678 but we didn't really fixed it. I am adding some of the folks involved in that discussion in CC.
Flags: needinfo?(ferjmoreno)
My understanding is that payment providers also set a cookie after they do phone number verification. The cookie has some expiration, but until that hits the provider might be charging the user even though the user no longer has the SIM card.

I think there are two things we could do here:

* Make the server side service return not just the user's phone number, but also the date when it was last
  verified. We could of course return this information on the client side as well, though client side
  data can never be trusted.
* Enable the webpage callers to indicate that they want the number re-verified. Though we would have to
  figure out a way that this doesn't cause the user to get charged for a bunch of extra SMSs. Right now
  the best thing I can think of is to display a dialog to the user if we're verifying "too often" using
  the SMS flow.
Thanks for the feedback Jonas.

(In reply to Jonas Sicking (:sicking) from comment #6)
> My understanding is that payment providers also set a cookie after they do
> phone number verification. The cookie has some expiration, but until that
> hits the provider might be charging the user even though the user no longer
> has the SIM card.
> 

Yes, this cookie is to avoid repeating the verification process every time a payment is requested. IIRC that's the current case for WebPay and Bango. But this cookie should be removed if the payment provider uses the Mobile ID API which already saves verified numbers.

> I think there are two things we could do here:
> 
> * Make the server side service return not just the user's phone number, but
> also the date when it was last
>   verified. We could of course return this information on the client side as
> well, though client side
>   data can never be trusted.

Yes, this would be useful. Maybe we can add this field to the assertion. Rémy, what do you think?

> * Enable the webpage callers to indicate that they want the number
> re-verified.

This sounds good to me. We can add a "forceVerification" flag to [1].

> Though we would have to
>   figure out a way that this doesn't cause the user to get charged for a
> bunch of extra SMSs. Right now
>   the best thing I can think of is to display a dialog to the user if we're
> verifying "too often" using
>   the SMS flow.

Ideally, we should start thinking about alternative verification methods that won't cost any money to the user and to Mozilla (network auth, telephony auth).

In this case we would already know the phone number to re-verify, so we would need to use the SMS MT only flow which shouldn't cost any money to the user (receiving SMS is usually free). But we still need to send one SMS which costs money to Mozilla.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Navigator.webidl#166
Flags: needinfo?(rhubscher)
> Maybe we can add this field to the assertion. Rémy, what do you think?

I agree that it would be useful to return the last verification date to the user.
Also I am not sure if we want to put that inside the assertion. I would prefer to return this information with the certificate.

    {
      "cert": certificate,
      "lastUpdated": certificateData.lastUpdatedAt,
      "lastVerified": certificateData.creationDate
    }

Whould that works for you guys?
Flags: needinfo?(rhubscher)
The good news is that we already have the needed information in the database, so it would work even from existing certificate: https://github.com/mozilla-services/msisdn-gateway/blob/master/msisdn-gateway/routes/sms.js#L321
We spoke this over IRC and it seems that we were already thinking about returning this value within the certificate (so it would also go within the assertion) [1]. However, it seems that the code is not reflecting what the documentation says [2]. We are adding a 'lastAuthAt' parameter with Date.now() value, but that's not what we need in this case. What we want is to add the 'lastVerifiedAt' parameter with the date of the last time this MSISDN was verified for that session token.

[1] https://github.com/mozilla-services/msisdn-gateway/blob/master/API.md#response-5 
[2] https://github.com/mozilla-services/msisdn-gateway/blob/master/msisdn-gateway/utils.js#L74
Attached file Link to GitHub PR.
Attachment #8518883 - Flags: review?(tarek)
Attachment #8518883 - Flags: feedback?(ferjmoreno)
> We spoke this over IRC and it seems that we were already thinking about returning this value within the certificate (so it would also go within the assertion) [1]. However, it seems that the code is not reflecting what the documentation says [2]. We are adding a 'lastAuthAt' parameter with Date.now() value, but that's not what we need in this case. What we want is to add the 'lastVerifiedAt' parameter with the date of the last time this MSISDN was verified for that session token.

Agreed :ferjm, we included lastVerifiedAt in the API design to address exactly this situation.
Comment on attachment 8518883 [details]
Link to GitHub PR.

Thanks Rémy! LGTM
Attachment #8518883 - Flags: feedback?(ferjmoreno) → feedback+
Comment on attachment 8518883 [details]
Link to GitHub PR.

LGTM. Will we have a test on the client side to make sure this scenario is exercised ?
Attachment #8518883 - Flags: review?(tarek) → review+
https://github.com/mozilla-services/msisdn-gateway/commit/626bfb2af78537837d4ff7583cc352e6033f2bc5
Assignee: nobody → rhubscher
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.