579355 - Do not return 'null' as storage node for non-existent users

Status: RESOLVED FIXED

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Right now node/weave returns 'null' for both non-existent and throttled users. That means if somebody fat-fingers their username when setting up a new client, the client will happily accept that and just tell them that sync will happen shortly...

We should return 'null' only for throttled users and continue to return "No Location" or whatever it was for non-existent users.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: patch against reg-server-secure

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: patch against reg-server-secure (v2)

Actually, we want to return a 404 when the user doesn't exist so the client knows it's an invalid user name.

Comment 3

[Philipp von Weitershausen [:philikon]]

Attachment: patch against reg-server

Here's a patch against reg-server.

Comment 4

[Les Orchard [:lorchard]]

Looks like this needs patching in both reg-server and reg-server-secure. I took a stab here with two patches:

http://hg.mozilla.org/users/lorchard_mozilla.com/weaveserver-registration-admin-patches/file/e92b06ee45ee/bug-579355
http://hg.mozilla.org/users/lorchard_mozilla.com/weaveserver-registration-patches/file/1df8f53ae4b3/bug-579355

This reverts to the 404 / No location behavior where a username doesn't exist

Comment 5

[Les Orchard [:lorchard]]

Oh, and heh: I just saw philikon's 2 new patches, so you can probably just ignore me.

Comment 6

[Toby Elliott [:telliott]]

ugh. This is why I didn't want to be rushed into this fix yesterday. I'll throw something in.

Comment 7

[Toby Elliott [:telliott]]

Attachment: Patch to frontend server to differ between non-user and pending user

I think this is the cleanest way to do it.

Comment 8

[Les Orchard [:lorchard]]

Comment on attachment 457877 [details] [diff] [review]
Patch to frontend server to differ between non-user and pending user

Looks good to me - thinking more about it, frontend change should be sufficient without a tweak to the secure backend.

Comment 9

[Ed Lee :Mardak]

Where was this fixed? Are we marking bugs as fixed when code is checked in and verified when it's pushed?

I believe some websites mark as fixed when pushed to production and verified stays as something for QA to verify. I believe various tags are set in the whiteboard for the next tagged production push, and doing a bugzilla query for that tag makes it easy to mark all as fixed.

Comment 10

[Toby Elliott [:telliott]]

We have traditionally been following a model where fixed means that the fix is checked in and verified means verified on stage. If we want to change that, someone needs to come up with an official plan.

Comment 11

[Ed Lee :Mardak]

http://hg.mozilla.org/services/reg-server/rev/b13257779749

Comment 12

[Prof_NARF]

Hi,

I think there is something wrong with that fix.

First, shouldn't it be report_problem("No location", 404); instead of report_problem(404, "No location");

Second, right now this function returns "null", if the user exists and mysql is used for authentification (since get_user_location() of mysql.php always returns false). Is that correct behavior? In my setup this leads to a problem on the client side (Firefox Sync 1.4.2 on FF 3.6.8). If GET https://server/user/1.0/user/node/weave is called and success 200 "null" is returned, the client just stops. No syncing takes place. The "Sync now" option is grayed out.

I don't exactly know what the intendet behavior should be like, but I got it working by replacing the line
if ($location === false || $location == "null") {
	if (!$authdb->user_exists())
		report_problem("No location", 404);
	exit("null");
}
with
if ($location === false || $location == "null") {
	if (!$authdb->user_exists())
		report_problem("No location", 404);
	else
		report_problem(WEAVE_ERROR_FUNCTION_NOT_SUPPORTED, 404);
}

Since in a single box setup the location functionality is not needed, right?

It took me hours to figure that out. I hope this helps.

Best regards,
Prof_NARF

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to comment #12)
> I think there is something wrong with that fix.
> 
> First, shouldn't it be report_problem("No location", 404); instead of
> report_problem(404, "No location");

You're right, though Toby fixed it already:
http://hg.mozilla.org/services/reg-server/rev/edc839afa2e0

> Second, right now this function returns "null", if the user exists and mysql is
> used for authentification (since get_user_location() of mysql.php always
> returns false). Is that correct behavior? In my setup this leads to a problem
> on the client side (Firefox Sync 1.4.2 on FF 3.6.8). If GET
> https://server/user/1.0/user/node/weave is called and success 200 "null" is
> returned, the client just stops. No syncing takes place. The "Sync now" option
> is grayed out.

The server should return "null" whenever there isn't a storage node assigned to a user (but the user exists). This can be used to throttle sync for newly signed up users. It seems like the MySQL connector should probably be fixed to support that use case.

Comment 14

[Prof_NARF]

Thank you for your very quick reply.

But then the supplied test of the registration server seems to be broken. It expects "No location" for an existing user:
output_test ("Get user node", $test == json_encode('No location'), $test);

Comment 15

[Toby Elliott [:telliott]]

Ugly. I've found the appropriate way to force "No location" from the mysql client and it's pushed. Updating to tip should solve your problem.

This is, incidentally, confirmation that the whole "null/No location" spec question was supposed to be "No location" for everything. I have no idea where the null came from.

Comment 16

[Philipp von Weitershausen [:philikon]]

null is a special value to the client. It means gracefully delay sync until a node is available without failing.

Comment 17

[Toby Elliott [:telliott]]

yeah. Unfortunately, putting that in breaks all the clients trying to talk to servers that don't use node allocation :/