1120001 - direct calls incorrectly say "caller unavailable" instead of "something went wrong" sometimes

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This is confusing to the user, since they may need to ask for debugging help if things are going wrong for them.  It also messes up our metrics.

The problem is that right now, if you try to call an email address or phone number that doesn't have an account, the server REST API returns errno 107, which is INVALID_PARAMETERS, which can include other problems as well.

We need to create a separate error code for at least this case (USER_UNAVAILABLE?), and then implement both client and server support for it.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

At the same time, I suspect it'll make sense to kill the "user-unknown" websocket termination reason documented at <https://docs.services.mozilla.com/loop/apis.html#termination-reasons>, both in the client validator and documention.  We know the problem in advance of setting up the websocket, and the server doesn't use that reason at all.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Adam, do these things seem like a reasonable approach to you?

* get rid of the unused websocket code
* split out a separate rest errno for user unavailable
* use the name USER_UNAVAILABLE rather than USER_UNKNOWN based on the speculation that it's since it's more opaque, at least unsophisticated spammers would be less likely to consider using it to test for the existence for accounts

Comment 3

[Adam Roach [:abr]]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #2)
> Adam, do these things seem like a reasonable approach to you?
> 
> * get rid of the unused websocket code

I think that's okay.

> * split out a separate rest errno for user unavailable

Sure. Do you know where the authoritative database for these errnos is, by the way? The numbering on them seems to be skipping around as if to avoid codes used in other services.

> * use the name USER_UNAVAILABLE rather than USER_UNKNOWN based on the
> speculation that it's since it's more opaque, at least unsophisticated
> spammers would be less likely to consider using it to test for the existence
> for accounts

That seems to be of extremely marginal value, but I don't object.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Adam Roach [:abr] from comment #3)

> > * split out a separate rest errno for user unavailable
> 
> Sure. Do you know where the authoritative database for these errnos is, by
> the way? The numbering on them seems to be skipping around as if to avoid
> codes used in other services.

AFAIK, the authoritative source is at

https://github.com/mozilla-services/loop-server/blob/master/loop/errno.json

I don't know why they skip around the way they do.  I was figuring I'd propose this be 122, based on not knowing any better.

> > * use the name USER_UNAVAILABLE rather than USER_UNKNOWN based on the
> > speculation that it's since it's more opaque, at least unsophisticated
> > spammers would be less likely to consider using it to test for the existence
> > for accounts
> 
> That seems to be of extremely marginal value, but I don't object.

Agreed.  It also makes debugging harder, by making the error message less specific.  Unless someone else has a strong opinion on it, jaws and I will discuss tomorrow and pick one of the two for our patch.  :-)

Comment 5

[Adam Roach [:abr]]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #4)
> I don't know why they skip around the way they do.  I was figuring I'd
> propose this be 122, based on not knowing any better.

There appears to be semantic commonality with these, for the most part:

https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format

There's clearly an issue here to be resolved, and I've opened Bug 1120671 to deal with that.

In any case, I don't think we should block *this* bug on figuring out this question. I don't find any claims on 122, so that seems to be safe for this particular case. I'd just like to have a consistent allocation behavior for future codes.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

[Tracking Requested - why for this release]:

Since we're trying to get our metrics cleaner sooner rather than later, this may be worth uplifting to 36.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #4)
> Agreed.  It also makes debugging harder, by making the error message less
> specific.  Unless someone else has a strong opinion on it, jaws and I will
> discuss tomorrow and pick one of the two for our patch.  :-)

We went with USER_UNKNOWN. We can use server-side rate limiting to prevent people from scraping the email addresses of all Loop users.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Server change [landed]: Split out USER_UNAVAILABLE from INVALID_PARAMETERS so clients can show better error messages

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Here's a server patch that, as it turned out, went with USER_UNAVAILABLE, because that actually turns to be the easiest thing to correctly implement server side (ie represents both "user unknown" and "user not logged in" cases).

We'll put in a PR.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Server-side pull-request: https://github.com/mozilla-services/loop-server/pull/287

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: client-patch: direct calls sometime say 'unavailable' rather than 'something went wrong'; patch=jaws,dmose

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8548398 [details] [diff] [review]
client-patch: direct calls sometime say 'unavailable' rather than 'something went wrong'; patch=jaws,dmose

Patch paired & reviewed by jaws & dmose.  Note that unless we land the client patch and deploy the server patch at the same, there will be some short period where Nightly 38 is slightly broken.

Our suggestion is to wait until the server deploy happens, and land this on Nightly that day.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Furthermore, 37 will be broken for the same reason until the patch is uplifted.

Comment 14

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #12)
> Patch paired & reviewed by jaws & dmose.  Note that unless we land the
> client patch and deploy the server patch at the same, there will be some
> short period where Nightly 38 is slightly broken.
> 
> Our suggestion is to wait until the server deploy happens, and land this on
> Nightly that day.

Why do we need to wait to land the client patch? It is a new error code we're detecting, and if we don't see it the old behavior is the same.

I would say that we only need to wait until the server patch is reviewed and landed in the server tree before we land this patch - we don't need to wait to deploy.

I think the same goes for 37 - we could uplift as soon as we're ready.

Comment 15

[Rémy Hubscher (:natim)]

Maria can you verify that this server change is ok for the Firefox OS client?

https://github.com/mozilla-services/loop-server/pull/287

Comment 16

[Maria Angeles Oteo (:oteo)]

In theory, the change should not affect how FxOS Loop client works as we are not checking the error code and just offering the user the option to share a URL in case the REST call fails. However, I'd like to give it a try, is this already deployed in Development?

Comment 17

[Rémy Hubscher (:natim)]

https://github.com/mozilla-services/loop-server/commit/fa04dcf1c0ddb2df006f71306e68d90644084652

Comment 18

[Rémy Hubscher (:natim)]

Maria — I merged and updated the dev environment.

Comment 19

[Maria Angeles Oteo (:oteo)]

Remy, I've tested pointed to Development Server with 1.1 Loop Mobile version and the master branch (future 1.1.1 based on Rooms) and all is working as expected.

You can remove the patch from the Development Server and thanks for asking!

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mark Banner (:standard8) from comment #14)
> (In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment
> #12)
> > Patch paired & reviewed by jaws & dmose.  Note that unless we land the
> > client patch and deploy the server patch at the same, there will be some
> > short period where Nightly 38 is slightly broken.
> > 
> > Our suggestion is to wait until the server deploy happens, and land this on
> > Nightly that day.
> 
> Why do we need to wait to land the client patch? It is a new error code
> we're detecting, and if we don't see it the old behavior is the same.

Yeah, it's not the biggest of issues. But if this lands before the server code is deployed, the behavior will regress back to the point of generic error messages until the server is deployed. Dan and I talked about the timing of landing this, and there really is no good time due to the separate deploy/release cycles between Firefox desktop and the server. At some point, one of the two will be out of sync with the other in relation to these patches.

The goal with trying to sync the landings is just to reduce that period.

> I would say that we only need to wait until the server patch is reviewed and
> landed in the server tree before we land this patch - we don't need to wait
> to deploy.
> 
> I think the same goes for 37 - we could uplift as soon as we're ready.

Yeah, that would be a fine approach as well.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Jared and I just discussed, and we're going to go ahead and land this now, and request uplift after it's been on m-c for a day or two...

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8548398 [details] [diff] [review]
client-patch: direct calls sometime say 'unavailable' rather than 'something went wrong'; patch=jaws,dmose

https://hg.mozilla.org/integration/fx-team/rev/a23d5b6b7278

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a23d5b6b7278

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Dan, do you want to uplift this patch to 36?

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Sylvestre: yes, but it would require uplifting at least the two strings in 

https://bugzilla.mozilla.org/attachment.cgi?id=8546881&action=diff#a/browser/locales/en-US/chrome/browser/loop/loop.properties_sec2

in bug 1118061 (I actually would like to uplift that entire bug, because it helps users avoid perceiving Hello as buggy in ways that it is not).

What are your thoughts here?

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Pike, Francesco: could you help here? thanks

Comment 27

[Francesco Lodolo [:flod]]

Bug 1118061 is already on mozilla-aurora, with the 2 strings
http://hg.mozilla.org/releases/mozilla-aurora/log/486b783c09b6/browser/locales/en-US/chrome/browser/loop/loop.properties

What am I missing?

Comment 28

[Francesco Lodolo [:flod]]

OK, found out what I missed right after hitting submit: fx36=mozilla-beta. Reopening NI for pike too based on that.

To be honest I wouldn't add new strings in mozilla-beta. A lot of locales won't have the chance to translate these strings, which means displaying the error message in English. And displaying a localized generic error message seems preferable than displaying one specific but in a language users might not understand.

Comment 29

[Axel Hecht [:Pike]]

I don't think we should uplift strings for this. We might want to create a branch specific patch to absorb the new error code in a backwards compatible way, though.

Not sure how users on release are going to experience this?

Comment 30

[Rémy Hubscher (:natim)]

We forgot about updating the documentation.

Comment 31

[Rémy Hubscher (:natim)]

Attachment: Update documentation

Comment 32

[Sylvestre Ledru [:Sylvestre]]

L10N folks are pretty clear on this. This will wait for 37!

Comment 33

[Alexis Metaireau (:alexis)]

Comment on attachment 8550897 [details] [review]
Update documentation

https://github.com/mozilla-services/docs/commit/46c69e17d94f1ca888c84129c4344314d40f2f54

Comment 34

[Alexis Metaireau (:alexis)]

I just landed server code.