1597933 - clean up OAuth2 code

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

There is some dead code to remove and other things that could be modernized in the OAuth2 code to make it less confusing.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1597933_oauth_remove_implicit_flow.patch

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: bug1597933_oauth_parse_params.patch

From bug 1592407 comment 31. We don't need the hash, since we don't do the implicit flow now, which was the only thing that could use id.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1597933_oauth_grant.patch

Constants are overrated... all they do is make you jump back and forwards in code, when what you really want to do is verify it's correct as per the rfc.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: bug1597933_oauth_urlparams.patch

Comment 5

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 9110213 [details] [diff] [review]
bug1597933_oauth_parse_params.patch

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

::: mailnews/base/util/OAuth2.jsm
@@ +173,5 @@
>  
> +  // @see RFC 6749 section 4.1.2: Authorization Response
> +  onAuthorizationReceived(aURL) {
> +    this.log.info("OAuth2 authorization received: url=" + aURL);
> +    let params = new URLSearchParams(aURL.split("?", 2)[1]);

Would it make sense to use new URL() on `aURL` and then use `.search.substr(1)` ?

Comment 6

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 9110214 [details] [diff] [review]
bug1597933_oauth_grant.patch

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

::: mailnews/base/util/OAuth2.jsm
@@ +172,5 @@
>    onAuthorizationReceived(aURL) {
>      this.log.info("OAuth2 authorization received: url=" + aURL);
>      let params = new URLSearchParams(aURL.split("?", 2)[1]);
>      if (params.has("code")) {
> +      this.requestAccessToken(params.get("code"), false);

I find boolean args hard to read and you always have to remember the function signature. I'm not overly opposed though, so I'm going to r+

@@ +198,3 @@
>      ];
>  
> +    if (!aRefresh) {

Can you switch around these blocks as per eslint no-negated-condition?

@@ +202,2 @@
>        params.push(["code", aCode]);
>        params.push(["redirect_uri", this.completionURI]);

If you like you could also pass multiple args to one params.push call.

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #5)

Would it make sense to use new URL() on aURL and then use .search.substr(1) ?

That was the original suggestion, but it's still just looking at a string, so I didn't really see too much point in it.

Comment 8

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 9110215 [details] [diff] [review]
bug1597933_oauth_urlparams.patch

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

::: mailnews/base/util/OAuth2.jsm
@@ +64,5 @@
>    requestAuthorization() {
> +    let params = new URLSearchParams();
> +    params.append("response_type", "code");
> +    params.append("client_id", this.consumerKey);
> +    params.append("redirect_uri", this.completionURI);

You can shorten this to passing an object to the constructor of URLSearchParams.

@@ +73,5 @@
>      }
>  
> +    for (let [name, value] of this.extraAuthParams) {
> +      params.append(name, value);
> +    }

Together with the object approach you could do:

let params = new URLSearchParams({
  response_type: "code",
  client_id: this.consumerKey,
  redirect_uri: this.completionURI,
  ...this.extraAuthParams
});

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4c34261831ce
clean up OAuth2 code: remove responseType which is always "code". r=Fallen
https://hg.mozilla.org/comm-central/rev/ff646df74684
improve OAuth2 params parsing. r=Fallen
https://hg.mozilla.org/comm-central/rev/9b0f8cb7ffc1
don't pass string constants to determine OAuth refresh token or not. r=Fallen
https://hg.mozilla.org/comm-central/rev/42ac954abcd0
use fetch + URLSearchParms instead of Http.jsm to request OAuth2 access token. r=Fallen
https://hg.mozilla.org/comm-central/rev/f962ffd01192
Use URLSearchParams for setting params for OAuth2 authorization request. r=Fallen

Comment 10

[Jorg K (CEST = GMT+2)]

Looks like you broke
TEST-UNEXPECTED-FAIL | comm/mail/components/enterprisepolicies/tests/browser/browser_policy_extensions.js | Uncaught exception - at chrome://mochitests/content/browser/comm/mail/components/enterprisepolicies/tests/browser/browser_policy_extensions.js:23 - TypeError: disableBtn is null

Comment 11

[Jorg K (CEST = GMT+2)]

Sorry, my mistake, that was already broken in the Daily run.

Comment 12

[Geoff Lankow (:darktrojan)]

(In reply to Pulsebot from comment #9)

https://hg.mozilla.org/comm-central/rev/42ac954abcd0
use fetch + URLSearchParms instead of Http.jsm to request OAuth2 access token. r=Fallen

This patch wasn't attached and reviewed. Looks like it was just forgotten. I'm going to uplift it anyway because it's fine.

Comment 13

[Geoff Lankow (:darktrojan)]

68.4.2:
https://hg.mozilla.org/releases/comm-esr68/rev/344da96fabbfe4f5900769442745afc355f12ab0
https://hg.mozilla.org/releases/comm-esr68/rev/8c31b90e567bb0425eb644f13680e039e2496d63
https://hg.mozilla.org/releases/comm-esr68/rev/b5ba333d8eddea59a57ccb2900cba068bb77e796
https://hg.mozilla.org/releases/comm-esr68/rev/b7971f56a0e92dbfa6057ba4aa616d9e584a7967
https://hg.mozilla.org/releases/comm-esr68/rev/e2debb9918bb39aa210e704db9c2dc6d8d086eaa