1109178 - Thunderbird OAuth implementation does not work with Evernote

Status: RESOLVED FIXED

(Thunderbird :: FileLink, defect)

Description

[Mike Kaply [:mkaply]]

I was attempting to use oauth.jsm and found a couple bugs.

1. oauth_token is passed even if it is empty (intial request). This breaks Evernote because Evernote uses the same oauth entrypoint for all requests and uses the presence of this parameter to determine if it is an initial request or not. oauth_token should not be sent if empty.

2. http://mxr.mozilla.org/comm-central/source/mail/base/modules/oauth.jsm#194 is incorrect. It's checking for data.oauth_token_secret, but there is no oauth_token_secret if you are using PLAINTEXT auth.

Comment 1

[Mike Kaply [:mkaply]]

Also, if authorizeMethod or requestCredentialsMethod isn't specified, defaults are used.

Again, this breaks evernote which uses one oAuth endpoint.

You should be required to specify the individual endpoints, or there should be a way to specify empty endpoints. Right now null is treated the same as "".

Comment 2

[Mike Kaply [:mkaply]]

Also, success callback after connection should return the response (excluding token and token_secret) because some services (like evernote) return data back after the authentication.

Comment 3

[Patrick Cloke [:clokep]]

The OAUTH implementation is certainly a work in progress, unfortunately each implementation is slightly different (as you're seeing with Evernote). I've CCed Philipp, who (with me) seem to be the current maintainers of this code.

Comment 4

[Mike Kaply [:mkaply]]

I have fixes for some of these and I will post.

Comment 5

[Patrick Cloke [:clokep]]

Great! Thanks. Feel free to r? me.

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Mike Kaply [:mkaply] from comment #4)
> I have fixes for some of these and I will post.

Mike, we're close to release but we still might be able to sneak them in

Comment 7

[Mike Kaply [:mkaply]]

Attachment: Fixes for my issues

Here's a patch that fixes the issues I found.

It shouldn't change any existing behavior.

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8596237 [details] [diff] [review]
Fixes for my issues

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

::: mail/base/modules/oauth.jsm
@@ +45,5 @@
> +  if (typeof aRequestCredentialsMethod === "undefined") {
> +    this.requestCredentialsMethod = "oauth/access_token";
> +  } else {
> +    this.requestCredentialsMethod = aRequestCredentialsMethod
> +  }

Why this change? If we are going to do this doesn't it make more sense to do (e.g.):
if (aTempCredentialsMethod === undefined) {

@@ +306,5 @@
>  
>      this.token = result.oauth_token;
>      this.tokenSecret = result.oauth_token_secret;
> +    delete(result.oauth_token);
> +    delete(result.oauth_token_secret);

I'm not against deleting this from the result, but I'm not convinced it's necessary either.

Comment 9

[Mike Kaply [:mkaply]]

(In reply to Patrick Cloke [:clokep] from comment #8)
> Comment on attachment 8596237 [details] [diff] [review]
> Fixes for my issues
> 
> Review of attachment 8596237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/modules/oauth.jsm
> @@ +45,5 @@
> > +  if (typeof aRequestCredentialsMethod === "undefined") {
> > +    this.requestCredentialsMethod = "oauth/access_token";
> > +  } else {
> > +    this.requestCredentialsMethod = aRequestCredentialsMethod
> > +  }
> 
> Why this change? If we are going to do this doesn't it make more sense to do
> (e.g.):
> if (aTempCredentialsMethod === undefined) {

This change is because there's currently no way to say "This API doesn't need an additional requestCredentialsMethod".

If you pass empty string (which means don't add anything), it's interpreted as null, so you get "oauth/access_token" appended.

So there needs to be a way to use oauth/access_token when you don't define anything, but don't use it if you specify empty string.

And yes, there's probably a better way to do it. Wasn't sure which was the best way.

Is === undefined a better option for detecting unpassed parameters?

> 
> @@ +306,5 @@
> >  
> >      this.token = result.oauth_token;
> >      this.tokenSecret = result.oauth_token_secret;
> > +    delete(result.oauth_token);
> > +    delete(result.oauth_token_secret);
> 
> I'm not against deleting this from the result, but I'm not convinced it's
> necessary either.

Yeah, I realized that after I did the patch. They aren't really secret to the consumer.

Comment 10

[Mike Kaply [:mkaply]]

Attachment: Address review comments

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8596856 [details] [diff] [review]
Address review comments

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

This looks fine to me!

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Why not use js default parameters?


function OAuth(aDisplayName, aBaseUri, aAuthUri, aAuthToken, aAuthTokenSecret,
               aAppKey, aAppSecret, aSignatureMethod="HMAC-SHA1",
               aTempCredentialsMethod="oauth/request_token",
               aAuthorizeMethod="oauth/authorize",
               aRequestCredentialsMethod="oauth/access_token") { ...

Comment 13

[Florian Quèze [:florian]]

(In reply to Philipp Kewisch [:Fallen] from comment #12)
> Why not use js default parameters?
> 
> 
> function OAuth(aDisplayName, aBaseUri, aAuthUri, aAuthToken,
> aAuthTokenSecret,
>                aAppKey, aAppSecret, aSignatureMethod="HMAC-SHA1",
>                aTempCredentialsMethod="oauth/request_token",
>                aAuthorizeMethod="oauth/authorize",
>                aRequestCredentialsMethod="oauth/access_token") { ...

Exactly what I was about to suggest! :-)

Comment 14

[Mike Kaply [:mkaply]]

> Why not use js default parameters?

I didn't even know they existed. New patch coming.

Comment 15

[Mike Kaply [:mkaply]]

I don't think we should use it for signature method, otherwise you would have to specify the signature method if you wanted to redefine the credentials.

So I would keep the old way for HMAC_SHA1 (so null defaults to that) and use default parameters for the rest.

Comment 16

[Mike Kaply [:mkaply]]

Attachment: 1109178.diff

One more time.

Comment 17

[Patrick Cloke [:clokep]]

Comment on attachment 8597237 [details] [diff] [review]
1109178.diff

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

I totally blanked on the default parameters, sorry about that!

r+ with a few nits fixed.

::: mail/base/modules/oauth.jsm
@@ +20,5 @@
>  function OAuth(aDisplayName, aBaseUri, aAuthUri, aAuthToken, aAuthTokenSecret,
> +               aAppKey, aAppSecret, aSignatureMethod,
> +               aTempCredentialsMethod="oauth/request_token",
> +               aAuthorizeMethod="oauth/authorize",
> +               aRequestCredentialsMethod="oauth/access_token")

Nit: Spaces around =.

@@ +35,5 @@
>  
>    this.signatureMethod = aSignatureMethod || "HMAC-SHA1";
> +  this.tempCredentialsMethod = aTempCredentialsMethod
> +  this.authorizeMethod = aAuthorizeMethod
> +  this.requestCredentialsMethod = aRequestCredentialsMethod

Nit: Add a semi-colon to the end of each of these lines before check-in.

@@ +107,5 @@
>      ]);
> +    if (this.token) {
> +      params = params.concat([
> +        ["oauth_token", this.token],
> +      ]);

This can just be params.push(["oauth_token, this.token]) since there's only one value being added.

Comment 18

[Mike Kaply [:mkaply]]

Forgotten semicolons? It's amateur hour over here.

So what are the checkin rules here?

Is there a comm-central-inbound?

Comment 19

[Patrick Cloke [:clokep]]

Commit to comm-central directly if it is open after starring things on treeherder [1], otherwise mark checkin-needed (but upload a new patch with the nits fixed). Looks like stuff is pretty unstarred / red right now, so maybe checkin-needed might be your best bet?

[1] https://treeherder.mozilla.org/#/jobs?repo=comm-central

Comment 20

[Mike Kaply [:mkaply]]

Attachment: Final patch for checkin

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 8597307 [details] [diff] [review]
Final patch for checkin

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

Carrying over r+ from :clokep

Comment 22

[Josiah Bruner [:jsbruner]]

Fixed -> https://hg.mozilla.org/comm-central/rev/6b81e33574d0

Comment 23

[Kent James (:rkent)]

Comment on attachment 8597307 [details] [diff] [review]
Final patch for checkin

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Could somebody who is familiar with the issues here fill in some of the approval form for targeting tb-38? I'll consider this for beta 4 or beta 5.

Comment 24

[Mike Kaply [:mkaply]]

This patch is very low risk because it doesn't change existing behavior.

It will allow the oauth module to be used for other services (evernote) by addons.

Comment 25

[Kent James (:rkent)]

Comment on attachment 8597307 [details] [diff] [review]
Final patch for checkin

http://hg.mozilla.org/releases/comm-aurora/rev/ef2dc070e814
http://hg.mozilla.org/releases/comm-beta/rev/891622c58ed2