507433 - Update client to use the weave 0.5 server

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Toby Elliott [:telliott]]

Documentation for the 0.5 API is at https://wiki.mozilla.org/Labs/Weave/0.5/API

The two major changes are:

1) New URL semantics, with a functional-group name (storage and info) after the user id. These should map 1-1 with the old urls. 

2) /username/info/collections now returns a hash with all the users collections and their last modified dates, removing the need for a call to each of them. 

The function to do quotas and quota use is also available if we want to pack that into this release

Comment 1

[Anant Narayanan [:anant]]

(In reply to comment #0)
> The function to do quotas and quota use is also available if we want to pack
> that into this release

We're sending quota information via X_WEAVE_ALERT, right? We're adding client-support for that in bug 478330.

Comment 2

[Toby Elliott [:telliott]]

Hmm. X-Weave-Alert should be used for actual alerts. This is not a call we want to be making constantly, as it's not that cheap. (working on making it cheap)

Dan was talking about call-on-startup.

Comment 3

[Toby Elliott [:telliott]]

Note that going near or over quota will cause an X-Weave-Alert, but that'sa  separate issue.

Comment 4

[Anant Narayanan [:anant]]

Attachment: Use 0.5 API paths and new user API

This patch updates the client to use the new user API as documented on https://wiki.mozilla.org/Labs/Weave/User/API. It also changes paths to use the new structure as defined in the 0.5 storage API (https://wiki.mozilla.org/Labs/Weave/0.5/API).

Note that after applying this patch, you will only be able to use Weave if you manually set extensions.weave.serverURL and tmpServerURL to https://weave-dev.services.mozilla.com/ because the new APIs aren't available on https://auth.services.mozilla.com/

Comment 5

[Ed Lee :Mardak]

Wipe server will probably need to ask for info/collections as it iterates over each collection name to wipe them.

Comment 6

[Ed Lee :Mardak]

Comment on attachment 395682 [details] [diff] [review]
Use 0.5 API paths and new user API

wipeServer needs to use info/collections and iterate over the key names

baseURL points to auth.services.mozilla.com so cluster lookup is wrong:
https://auth.services.mozilla.com/user/1/EdTest3/node/weave

should be

https://services.mozilla.com/user/1/EdTest3/node/weave

And even then, some reason the node lookup returns a string with quotes.. unsure if it's a server bug.

POST response doesn't have a modified value in the return hash.. I believe we're supposed to use the X-Weave-Timestamp.

I have some local patches for some of these.. I'll put them on a repo.

Not sure what to do with baseURL.. is it supposed to be auth.services.mozilla.com ? Should we use tmpServerURL?

Comment 7

[Ed Lee :Mardak]

Attachment: v2

Attached is a super patch of the base + additional changes here:
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-0.5-api/

I haven't tested change password/passphrase or new user creation yet because user API servers are in flux.

Anant, did you test those earlier? Maybe weave-dev?

Comment 8

[Anant Narayanan [:anant]]

(In reply to comment #7)
> Anant, did you test those earlier? Maybe weave-dev?

I tested my earlier changes against weave-dev.s.m.c and they worked fine.

Comment 9

[Ed Lee :Mardak]

Creating an account works.. kinda, but it's not usable for several minutes. Bug 512608.

For weave-dev, I wasn't able to create an account because it checks against services.mozilla.com to see if the account is available. And if I choose an account that is free on services.mozilla.com, it ends up creating the account there and not on weave-dev.

Anant, am I doing something wrong for creating an account?

Comment 10

[Dan Mills [:thunder]]

Comment on attachment 396628 [details] [diff] [review]
v2

>-        if (up.data.modified > this.lastSync)
>-          this.lastSync = up.data.modified;
>+
>+        // Record the modified time of the upload
>+        let modified = up._lastChannel.getResponseHeader("X-Weave-Timestamp");
>+        if (modified > this.lastSync)
>+          this.lastSync = modified;

Would prefer not to use _lastChannel.  Can we look into adding the headers to the return value of get(), and use that?  Or maybe even put the associated channel there and use ret.lastChannel.

>-    let res = new Resource(this.baseURL + "api/register/chknode/" + username);
>+    let res = new Resource(this.baseURL + "user/1/" + username + "/node/weave");

This seems wrong - the 'user/' prefix is not required, only part of our setup.  I believe the user API URL needs its own pref.

This path ("user/1/") appears several times in the patch.

>+          // The server gives JSON of a string literal, but JS doesn't allow it
>+          res.data = res.data.replace(/"/g, "");
>           return "https://" + res.data + "/";

Hm, is the server returning what we want here?  Because now would be the time to make it so.  Let's work it out, it probably shouldn't be returning what it is.

Maybe a JSON hash, so we can add more data later if we need to, without breaking backwards compat?

>       // No exceptions must have meant it was successful
>       this._log.info("Account created: " + ret.response);
>       return ret;
>-    }
>-    catch(ex) {
>+    } catch(ex) {
>       this._log.warn("Failed to create account: " + ex);
>       let status = ex.request.responseStatus;
>       switch (status) {
>         case 400:
>           ret.error = this._errorStr(status);
>           break;
>         case 417:
>           ret.error = "captcha-incorrect";

I didn't check, but does the new user creation API return the same error codes?  This patch didn't change those.

Comment 11

[Anant Narayanan [:anant]]

(In reply to comment #10)
> Would prefer not to use _lastChannel.  Can we look into adding the headers to
> the return value of get(), and use that?  Or maybe even put the associated
> channel there and use ret.lastChannel.

That patch for bug 478330 already does that. We use it to retrieve the value of the X-Weave-Alert header.

Comment 12

[Ed Lee :Mardak]

(In reply to comment #10)
> >+          // The server gives JSON of a string literal, but JS doesn't allow it
> >+          res.data = res.data.replace(/"/g, "");
> >           return "https://" + res.data + "/";
> Hm, is the server returning what we want here?  Because now would be the time
> to make it so.
The server is responding with something like JSON.stringify("sj-weave01"). But our implementation of JSON doesn't allow literals -- only objects (including arrays). It's consistent with the other responses coming from the server, but I suppose a more complex structure/hash could work too.

> I didn't check, but does the new user creation API return the same error codes?
Hrmm... apparently not?

I get JSON strings like:
"Bad password"

... but apparently there's not much checking going on?

I just did a PUT to https://services.mozilla.com/user/1/EdTest9%3d9

and if you check that url, it responds with 1 now.

I didn't provide a captcha or secret...

Comment 13

[Ed Lee :Mardak]

(Not to mention a bogus uid)

Comment 14

[Toby Elliott [:telliott]]

I had captcha turned off for testing; it's back on now.

Stupid error in the username regex allowed that to slip through. It's fixed.

Comment 15

[Toby Elliott [:telliott]]

(In reply to comment #10)

> I didn't check, but does the new user creation API return the same error codes?


Pretty much. It conforms to https://wiki.mozilla.org/Labs/Weave/0.3/ResponseCodes

"Bad Password" corresponds to a password strength check failure, and doesn't have an associated code. I'm swapping it to "9"

Comment 16

[Dan Mills [:thunder]]

(In reply to comment #12)
> The server is responding with something like JSON.stringify("sj-weave01"). But
> our implementation of JSON doesn't allow literals -- only objects (including
> arrays). It's consistent with the other responses coming from the server, but I
> suppose a more complex structure/hash could work too.

Literals are not actually valid JSON on their own.

I suggested returning an object because it'd be extensible, but it's not absolutely required.

(In reply to comment #15)
> (In reply to comment #10)
> 
> > I didn't check, but does the new user creation API return the same error codes?
> 
> 
> Pretty much. It conforms to
> https://wiki.mozilla.org/Labs/Weave/0.3/ResponseCodes
> 
> "Bad Password" corresponds to a password strength check failure, and doesn't
> have an associated code. I'm swapping it to "9"

https://wiki.mozilla.org/Labs/Weave/ServerAPI

Is what we had implemented.  Please update the docs and mark deprecated stuff as such.

Comment 17

[Toby Elliott [:telliott]]

> Is what we had implemented.  Please update the docs and mark deprecated stuff
> as such.

Once this goes live, we can deprecate the entire page and point to the new API

Comment 18

[Dan Mills [:thunder]]

Attachment: v3

* adds another pref for the "misc" api
* makes base/misc url prefs default to auth.smc/{user,misc}/
* fixes about:config captcha path

Comment 19

[Ed Lee :Mardak]

Attachment: v2 -> v3 interdiff

Comment 20

[Ed Lee :Mardak]

Comment on attachment 396859 [details] [diff] [review]
v3

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-0.5-api/rev/9f025d97854b
Adds another pref for the "misc" api, makes base/misc url prefs default to auth.smc/{user,misc}/, fixes about:weave captcha path. r=Mardak

Comment 21

[Ed Lee :Mardak]

Resolve conflicts from bug 511746:
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/weave-0.5-api/rev/44c71cbbf770

Comment 22

[Dan Mills [:thunder]]

Attachment: additional patch to fix error code parsing

the 0.5 server has new error codes, this patch updates service.js to parse them

Comment 23

[Ed Lee :Mardak]

Landed weave-0.5-api branch to weave:
http://hg.mozilla.org/labs/weave/rev/808c4a993716

All related changesets:
http://hg.mozilla.org/labs/weave/pushloghtml?changeset=808c4a993716

Comment 24

[Tony Chung [:tchung]]

Verified fix on weave 1.3b5.  server is now up to 1.3