Closed Bug 1109178 Opened 5 years ago Closed 5 years ago

Thunderbird OAuth implementation does not work with Evernote

Categories

(Thunderbird :: FileLink, defect)

defect
Not set

Tracking

(thunderbird38 fixed, thunderbird39 fixed, thunderbird40 fixed)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 --- fixed
thunderbird39 --- fixed
thunderbird40 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 793749
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 "".
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.
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.
Summary: Bugs in the Thunderbird OAuth implementation → Thunderbird OAuth implementation does not work with Evernote
I have fixes for some of these and I will post.
Great! Thanks. Feel free to r? me.
(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
Flags: needinfo?(mozilla)
Attached patch Fixes for my issues (obsolete) — Splinter Review
Here's a patch that fixes the issues I found.

It shouldn't change any existing behavior.
Flags: needinfo?(mozilla)
Attachment #8596237 - Flags: review?(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.
Attachment #8596237 - Flags: review?(clokep) → review-
(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.
Attached patch Address review comments (obsolete) — Splinter Review
Attachment #8596237 - Attachment is obsolete: true
Attachment #8596856 - Flags: review?(clokep)
Comment on attachment 8596856 [details] [diff] [review]
Address review comments

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

This looks fine to me!
Attachment #8596856 - Flags: review?(clokep) → review+
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") { ...
(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! :-)
> Why not use js default parameters?

I didn't even know they existed. New patch coming.
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.
Attached patch 1109178.diff (obsolete) — Splinter Review
One more time.
Assignee: nobody → mozilla
Attachment #8596856 - Attachment is obsolete: true
Attachment #8597237 - Flags: review?(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.
Attachment #8597237 - Flags: review?(clokep) → review+
Forgotten semicolons? It's amateur hour over here.

So what are the checkin rules here?

Is there a comm-central-inbound?
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
Attachment #8597237 - Attachment is obsolete: true
Comment on attachment 8597307 [details] [diff] [review]
Final patch for checkin

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

Carrying over r+ from :clokep
Attachment #8597307 - Flags: review+
Keywords: checkin-needed
Fixed -> https://hg.mozilla.org/comm-central/rev/6b81e33574d0
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
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.
Attachment #8597307 - Flags: approval-comm-beta?
Attachment #8597307 - Flags: approval-comm-aurora?
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 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
Attachment #8597307 - Flags: approval-comm-beta?
Attachment #8597307 - Flags: approval-comm-beta+
Attachment #8597307 - Flags: approval-comm-aurora?
Attachment #8597307 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.