Closed Bug 1599054 Opened 11 months ago Closed 11 months ago

OAuth2.jsm should not send client_secret, if none is specified

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(thunderbird_esr68 fixed)

RESOLVED FIXED
Thunderbird 72.0
Tracking Status
thunderbird_esr68 --- fixed

People

(Reporter: TbSync, Assigned: TbSync)

Details

Attachments

(1 file, 4 obsolete files)

The current OAuth2.jsm is very much optimized for the Google v1 endpoints. Many things can be fixed for example for Office365 or the Google v2 endpoints by simply overiding internal values after an OAuth2 Object has been created (like authURI, tokenURI and completionURI).

One thing I cannot simply change is the inclusion of the client_secret, which is not optional with OAuth2.jsm. However Office365 does not require an client_secret in native apps and will actually not work if one is present.

https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-oauth2-auth-code-flow#request-an-access-token

TL;DR: You shouldn't use the application secret in a native app because client_secrets can't be reliably stored on devices

I thus would like to request to only send the client_secret, if one has been set.

Status: UNCONFIRMED → NEW
Component: Untriaged → General
Ever confirmed: true

So it does not get lost:

"Add code comment referring to rfc 6749 2.3.1. Client Password and how client_secret MAY be omitted."

Attached patch bug1599054.patch (obsolete) — Splinter Review
Assignee: nobody → john.bieling
Attachment #9112010 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9112010 [details] [diff] [review]
bug1599054.patch

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

::: mailnews/base/util/OAuth2.jsm
@@ +197,5 @@
>     */
>    requestAccessToken(aCode, aRefresh) {
>      // @see RFC 6749 section 4.1.3. Access Token Request
>      // @see RFC 6749 section 6. Refreshing an Access Token
> +    // @see RFC 6749 section 2.3.1. Client Password

I think this addition is not needed, as you quote the section later on where it's involved.

@@ +204,5 @@
>      data.append("client_id", this.consumerKey);
> +    if (this.consumerSecret !== null) {
> +      // Section 2.3.1. of RFC 6749 states, that empty secrets MAY be omitted
> +      // by the client. This OAuth implementation delegates this decission to
> +      // the user: If the secret is not set at all or set to null, it will not

"the caller" could be clearer then "the user"

Could you add jsdoc documentation to function OAuth2 and there describe this?
Status: NEW → ASSIGNED
Attached patch bug1599054.patch (obsolete) — Splinter Review
Attachment #9112010 - Attachment is obsolete: true
Attachment #9112010 - Flags: review?(mkmelin+mozilla)
Attached patch bug1599054.patch (obsolete) — Splinter Review

Minor spelling error.

Attachment #9112237 - Attachment is obsolete: true
Attachment #9112238 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9112238 [details] [diff] [review]
bug1599054.patch

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

Almost there... :)

::: mailnews/base/util/OAuth2.jsm
@@ +17,5 @@
>  var gConnecting = {};
>  
> +  /**
> +   * Constructor for the OAuth2 object.
> +   * @param {string} aBaseURI     - The base uri for authentication and token

nit: please just one space. Like 

   * @param {string} aBaseURI - The base uri .......

@@ +20,5 @@
> +   * Constructor for the OAuth2 object.
> +   * @param {string} aBaseURI     - The base uri for authentication and token
> +   *                                requests, oauth2/auth or oauth2/token will
> +   *                                be added for the actual requests.
> +   * @param {string} aScope       - The scope string as defined by the OAuth

@param {string} [aScope]

since it's optional

@@ +24,5 @@
> +   * @param {string} aScope       - The scope string as defined by the OAuth
> +   *                                service provider.
> +   * @param {string} aAppKey      - The appKey or client_id as defined by the
> +   *                                OAuth service provider.
> +   * @param {string} [aAppSecret] - The appSecret or client_secret as defined

[aAppSecret=null] since null is the default value. Then you don't need to describe the "If null or not specified"
Attachment #9112238 - Flags: review?(mkmelin+mozilla)

Scope is optional? Sure? How should that work with aAppKey not being optional behind it?

It's optional in regards to the RFC - https://tools.ietf.org/html/rfc6749#section-3.3.
Maybe you can also add that in the param comment: "as specified by RFC 6749 Section 3.3"

Internally Thunderbird will always have it set yes. But I think your use case was to use the module more widely, and then it's not required unless the OAuth2 provider requires/wants it.

Yes, found it in the spec just now. But for the javascript constructor we must make it required, but the caller may provide null to indicate that it should not be included, right? Just leaving the scope out (what [aScope] suggests to me) would map the aAppKey to the scope. If I am not missing the point here.

You do not want the JsDoc explenations alligned, but each with only a single space before and after the dash?

Ah, I see what you mean. It should indeed be (instead):

@param {?string} aScope - The scope as specified by RFC 6749 Section 3.3

You do not want the JsDoc explenations alligned, but each with only a single space before and after the dash?

Correct.

Attached patch bug1599054.patch (obsolete) — Splinter Review

As I am not a native speaker, wording of comments and fitting everything within 80 chars per line is a PITA for me. If this is still not how you like it, I kindly ask you to drop them as comments in this bug and I will adjust the patch.

Attachment #9112276 - Flags: review?(mkmelin+mozilla)
Attachment #9112238 - Attachment is obsolete: true
Comment on attachment 9112276 [details] [diff] [review]
bug1599054.patch

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

I notice the function is now wrongly indented. 
I'll attach a slight modification.
Attachment #9112276 - Flags: review?(mkmelin+mozilla)
Attached patch bug1599054.patchSplinter Review
Attachment #9112276 - Attachment is obsolete: true
Attachment #9112514 - Flags: review+

Now I know how you want the Js Docs to look like (and that makes it a lot easier) :-)

"falsy" is a typo - should be "false"? (I didn't examine the surrounding code)

Flags: needinfo?(john.bieling)

Magnus cut that short. In my original draft I wrote "if it resolves to false (null, empty string, ...)". We do mean "falsy".

This is important as the client_secret behaves differently, it must be null to be excluded, but if it is an empty string, it will be included.

Flags: needinfo?(john.bieling)

It's indeed intended to read falsy. Falsy is the term that covers null, undefined, "", 0 - stuff that evaluates to false in an if clause.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/8c2f8de77106
allow callers to ommit sending OAuth2 client_secret parameter. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Comment on attachment 9112514 [details] [diff] [review]
bug1599054.patch

[Approval Request Comment]
I like to have it in Beta and later in TB68.
Attachment #9112514 - Flags: approval-comm-beta?
Target Milestone: --- → Thunderbird 72.0
Comment on attachment 9112514 [details] [diff] [review]
bug1599054.patch

This is already in TB 72 which is now in beta. You can request ESR uplift later.
Attachment #9112514 - Flags: approval-comm-beta?
Attachment #9112514 - Flags: approval-comm-esr68?

Would have to be uplifted with bug 1597933 which is more scary.

(In reply to Magnus Melin [:mkmelin] from comment #21)

Would have to be uplifted with bug 1597933 which is more scary.

Both have been on beta for a month and a half, so there's that. But a more cautious approach is fine - I'd suggest doing it in a 68.4.1 where we could do a slow rollout (which would be slightly problematic for a 68.4.0).

Comment on attachment 9112514 [details] [diff] [review]
bug1599054.patch

Let's take this on 68 only after 68.4
Comment on attachment 9112514 [details] [diff] [review]
bug1599054.patch

Let's take this now. Bug 1597933 changesets need to land on esr first.
Attachment #9112514 - Flags: approval-comm-esr68? → approval-comm-esr68+
You need to log in before you can comment on or make changes to this bug.