bug#72358: 29.4; oauth2.el improvements

[Xiyue Deng <manphiz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 16503 bytes --]

Hi Philip,

Thanks for the review!  Please see my replies below inline.

Philip Kaludercic <philipk@posteo.net> writes:

> Xiyue Deng <manphiz@gmail.com> writes:
>
>> Hi Philip,
>>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Xiyue Deng <manphiz@gmail.com> writes:
>>>
>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>
>>>>>> From: Xiyue Deng <manphiz@gmail.com>
>>>>>> Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org>,  Björn Bidar
>>>>>>  <bjorn.bidar@thaodan.de>,  rpluim@gmail.com,  72358@debbugs.gnu.org
>>>>>> Date: Wed, 14 Aug 2024 01:23:19 -0700
>>>>>> 
>>>>>> >> It's been a few days since the last time I received feedback for
>>>>>> >> improvements regarding my patches.  Is there any other
>>>>>> >> feedbacks/reviews
>>>>>> >> I am expecting from the co-maintainers?  Please also let me know when
>>>>>> >> it's time to ask for merging and requesting a new tagged release.
>>>>>> >
>>>>>> > ?? The last message in this discussion was just yesterday evening, and
>>>>>> > my understanding is that you are still discussing the issues and did
>>>>>> > not reach the final conclusion.  If I'm mistaken, my apologies;
>>>>>> 
>>>>>> The recent communication was not related to my patches but to check
>>>>>> whether it is possible to support outlook.com OAuth2 login (and the
>>>>>> conclusion was no because refreshing access token was disabled as
>>>>>> confirmed by MS representative during an online chat.)
>>>>>> 
>>>>>> > please describe your conclusion and post the patch that you-all agree
>>>>>> > would solve the issues, and let's take it from there.
>>>>>> 
>>>>>> I actually only received comments from Robert and I have updated my
>>>>>> patches according in [1][2] (also attached in EOM).
>>>>>> 
>>>>>> [1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=72358#20
>>>>>> [2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=72358#44
>>>>>
>>>>> Thanks.
>>>>>
>>>>> Philip, could you please DTRT here?  This seems to be an ELPA package.
>>>>
>>>> Friendly ping.  Please also let me know if there are more review
>>>> comments.
>>>
>>> I'm sorry, this was a rather long thread and I didn't have the time yet
>>> to follow up on it.  Can you confirm that you want me to review the
>>> patches attached to this message:
>>>
>>>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=72358#20
>>>
>>> ?
>>
>> Almost with a later update for patch 5.  I am now attaching the latest
>> patches here to avoid any confusions.  Thanks!
>>
>> -- 
>> Xiyue Deng
>>
>> From 2b9e50cb0948e0b4f28883042109994ffa295d3d Mon Sep 17 00:00:00 2001
>> From: Xiyue Deng <manphiz@gmail.com>
>> Date: Sun, 21 Jul 2024 14:50:56 -0700
>> Subject: [PATCH 1/6] Show full authentication URL to let user choose how to
>>  visit it
>>
>> * packages/oauth2/oauth2.el (oauth2-request-authorization): show full
>> authentication URL in user prompt.
>
> Generally it would be nice if you could attach a "(Bug#72358)" to the
> end of each commit.  Just add it like a new sentence.
>
>     [...]
>     authentication URL in user prompt.  (Bug#72358)
>

Done for all patches.

>> ---
>>  oauth2.el | 19 +++++++++++--------
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/oauth2.el b/oauth2.el
>> index 7da9702004..3a3e50ad2b 100644
>> --- a/oauth2.el
>> +++ b/oauth2.el
>> @@ -57,14 +57,17 @@
>>    "Request OAuth authorization at AUTH-URL by launching `browse-url'.
>>  CLIENT-ID is the client id provided by the provider.
>>  It returns the code provided by the service."
>> -  (browse-url (concat auth-url
>> -                      (if (string-match-p "\?" auth-url) "&" "?")
>> -                      "client_id=" (url-hexify-string client-id)
>> -                      "&response_type=code"
>> -                      "&redirect_uri=" (url-hexify-string (or redirect-uri "urn:ietf:wg:oauth:2.0:oob"))
>> -                      (if scope (concat "&scope=" (url-hexify-string scope)) "")
>> -                      (if state (concat "&state=" (url-hexify-string state)) "")))
>> -  (read-string "Enter the code your browser displayed: "))
>> +  (let ((url (concat auth-url
>> +                     (if (string-match-p "\?" auth-url) "&" "?")
>> +                     "client_id=" (url-hexify-string client-id)
>> +                     "&response_type=code"
>> +                     "&redirect_uri=" (url-hexify-string (or redirect-uri "urn:ietf:wg:oauth:2.0:oob"))
>> +                     (if scope (concat "&scope=" (url-hexify-string scope)) "")
>> +                     (if state (concat "&state=" (url-hexify-string state)) ""))))
>> +    (browse-url url)
>> +    (read-string (concat "Follow the instruction on your default browser, or "
>> +                         "visit:\n" url
>> +                         "\nEnter the code your browser displayed: "))))
>
> LGTM.
>
>>  
>>  (defun oauth2-request-access-parse ()
>>    "Parse the result of an OAuth request."
>> -- 
>> 2.39.2
>>
>>
>> From 26ed9886bd9d3970d55cf76e4269cef3998503a7 Mon Sep 17 00:00:00 2001
>> From: Xiyue Deng <manphiz@gmail.com>
>> Date: Sun, 21 Jul 2024 14:52:02 -0700
>> Subject: [PATCH 2/6] Add parameters required by Google OAuth2 to get
>>  refresh_token
>>
>> * packages/oauth2/oauth2.el (oauth2-request-authorization): add
>> `access_type=offline' and `prompt=consent' when requesting token to
>
> I wouldn't use `...' syntax in commit ChangeLog messages.
>

Ah I saw this used in the texinfo doc extensively so I thought it was
preferred.  I have changed all these to double quotes, though do let me
know if there is a better preference.

>> receive refresh_token.
>> ---
>>  oauth2.el | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/oauth2.el b/oauth2.el
>> index 3a3e50ad2b..9780ac3a1d 100644
>> --- a/oauth2.el
>> +++ b/oauth2.el
>> @@ -63,7 +63,9 @@ It returns the code provided by the service."
>>                       "&response_type=code"
>>                       "&redirect_uri=" (url-hexify-string (or redirect-uri "urn:ietf:wg:oauth:2.0:oob"))
>>                       (if scope (concat "&scope=" (url-hexify-string scope)) "")
>> -                     (if state (concat "&state=" (url-hexify-string state)) ""))))
>> +                     (if state (concat "&state=" (url-hexify-string state)) "")
>> +                     "&access_type=offline"
>> +                     "&prompt=consent")))
>
> You do what you say, but perhaps a comment explaining why you have these
> queries might be useful?
>

Added a comment above to explain the reasons.

>>      (browse-url url)
>>      (read-string (concat "Follow the instruction on your default browser, or "
>>                           "visit:\n" url
>> -- 
>> 2.39.2
>>
>>
>> From 59225412e1d06ae9e165cfde6a4a985cee4fc569 Mon Sep 17 00:00:00 2001
>> From: Xiyue Deng <manphiz@gmail.com>
>> Date: Sun, 21 Jul 2024 14:54:08 -0700
>> Subject: [PATCH 3/6] Encode parameters when requesting access
>>
>> * packages/oauth2/oauth2.el (oauth2-request-access): encode all
>> parameters which may contain characters that breaks URL.
>> ---
>>  oauth2.el | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/oauth2.el b/oauth2.el
>> index 9780ac3a1d..b035742fc1 100644
>> --- a/oauth2.el
>> +++ b/oauth2.el
>> @@ -107,10 +107,10 @@ Return an `oauth2-token' structure."
>>             (oauth2-make-access-request
>>              token-url
>>              (concat
>> -             "client_id=" client-id
>> +             "client_id=" (url-hexify-string client-id)
>>  	     (when client-secret
>> -               (concat  "&client_secret=" client-secret))
>> -             "&code=" code
>> +               (concat  "&client_secret=" (url-hexify-string client-secret)))
>> +             "&code=" (url-hexify-string code)
>>               "&redirect_uri=" (url-hexify-string (or redirect-uri "urn:ietf:wg:oauth:2.0:oob"))
>>               "&grant_type=authorization_code"))))
>>        (make-oauth2-token :client-id client-id
>> -- 
>> 2.39.2
>
> 1+  Though one might also consider using something like `url-encode-url'
> to generate the entire URL.
>

It is indeed much cleaner.  Changed accordingly.

>>
>>
>> From e801af578e63c7e333e668bdfef05e4cf0802582 Mon Sep 17 00:00:00 2001
>> From: Xiyue Deng <manphiz@gmail.com>
>> Date: Sun, 28 Jul 2024 03:00:04 -0700
>> Subject: [PATCH 4/6] Support storing data for multiple accounts of the same
>>  provider
>>
>> Currently the plstore id computed by `oauth2-compute-id' only takes
>> `auth-url', `token-url', and `scope' into account, which could be the
>> same for the same provider (e.g. Gmail).  This prevents storing
>> information for multiple accounts of the same service for some
>> providers.
>>
>> This patch adds `client-id' to the calculation of plstore id to make
>> sure that it is unique for different accounts of the same provider.
>>
>> It also changes the hash function to sha512 to be more secure.
>>
>> * packages/oauth2/oauth2.el (oauth2-compute-id): add `client-id' as a
>> parameter of `oauth2-compute-id' to ensure unique id amount multiple
>> accounts of the same provider, and change hash function to sha512.
>> ---
>>  oauth2.el | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/oauth2.el b/oauth2.el
>> index b035742fc1..035971ac85 100644
>> --- a/oauth2.el
>> +++ b/oauth2.el
>> @@ -163,17 +163,17 @@ TOKEN should be obtained with `oauth2-request-access'."
>>    :group 'oauth2
>>    :type 'file)
>>  
>> -(defun oauth2-compute-id (auth-url token-url scope)
>> +(defun oauth2-compute-id (auth-url token-url scope client-id)
>>    "Compute an unique id based on URLs.
>>  This allows to store the token in an unique way."
>> -  (secure-hash 'md5 (concat auth-url token-url scope)))
>> +  (secure-hash 'sha512 (concat auth-url token-url scope client-id)))
>>  
>>  ;;;###autoload
>>  (defun oauth2-auth-and-store (auth-url token-url scope client-id client-secret &optional redirect-uri state)
>>    "Request access to a resource and store it using `plstore'."
>>    ;; We store a MD5 sum of all URL
>>    (let* ((plstore (plstore-open oauth2-token-file))
>> -         (id (oauth2-compute-id auth-url token-url scope))
>> +         (id (oauth2-compute-id auth-url token-url scope client-id))
>>           (plist (cdr (plstore-get plstore id))))
>>      ;; Check if we found something matching this access
>>      (if plist
>> -- 
>> 2.39.2
>
> LGTM
>
>>
>> From 55417ec61c91f6b4d8e16a0c9933fb178d7bb657 Mon Sep 17 00:00:00 2001
>> From: Xiyue Deng <manphiz@gmail.com>
>> Date: Sun, 28 Jul 2024 03:41:20 -0700
>> Subject: [PATCH 5/6] Add debug messages and provide a switch variable for
>>  enabling
>>
>> This helps debugging whether the authorization and refresh requests
>> were successful and inspecting the responses.
>>
>> * packages/oauth2/oauth2.el: add support for debug messages and a
>> switch variable for enabling.
>> ---
>>  oauth2.el | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/oauth2.el b/oauth2.el
>> index 035971ac85..ce7a835100 100644
>> --- a/oauth2.el
>> +++ b/oauth2.el
>> @@ -40,6 +40,7 @@
>>  (require 'plstore)
>>  (require 'json)
>>  (require 'url-http)
>> +(require 'pp)
>>  
>>  (defvar url-http-data)
>>  (defvar url-http-method)
>> @@ -53,6 +54,14 @@
>>    :link '(url-link :tag "Savannah" "https://git.savannah.gnu.org/cgit/emacs/elpa.git/tree/?h=externals/oauth2")
>>    :link '(url-link :tag "ELPA" "https://elpa.gnu.org/packages/oauth2.html"))
>>  
>> +(defvar oauth2-debug nil
>> +  "Enable debug messages.")
>
> I'd slightly expand this comment in case someone wants to use something
> like `apropos-documentation'.

Expanded the doc string to explain the purpose.

> Also, why is this not a user option?
>

As Robert mentioned in the follow-up mail it was his suggestion (thanks
Robert!)  Do let me know if `defcustom' is preferred.

>> +
>> +(defun oauth2--do-debug (&rest msg)
>> +  "Output debug messages when `oauth2-debug' is enabled."
>> +  (if oauth2-debug
>
> I'd use `when' here.  
>

Done

>> +    (apply #'message msg)))
>
> ... and perhaps prefix the message with a string like "[oauth2] ".
>

Done.  I used `setcar' but let me know if there is a more elegant way to
do this.

> I also notice that you manually mention the function issuing the debug
> message below.  It is possible to infer this using `backtrace-frame'
> (but that is really something that should be added to the core, instead
> of packages having to re-implement it themselves.)
>

Added a let-bounded `func-name' by getting function name from
backtrace-frame.  And it would be great if there is a built-in macro
like `__PRETTY_FUNCTION__' and `__LINE__'.

>> +
>>  (defun oauth2-request-authorization (auth-url client-id &optional scope state redirect-uri)
>>    "Request OAuth authorization at AUTH-URL by launching `browse-url'.
>>  CLIENT-ID is the client id provided by the provider.
>> @@ -79,6 +88,8 @@ It returns the code provided by the service."
>>  
>>  (defun oauth2-make-access-request (url data)
>>    "Make an access request to URL using DATA in POST."
>> +  (oauth2--do-debug "oauth2-make-access-request: url: %s" url)
>> +  (oauth2--do-debug "oauth2-make-access-request: data: %s" data)
>>    (let ((url-request-method "POST")
>>          (url-request-data data)
>>          (url-request-extra-headers
>> @@ -86,6 +97,8 @@ It returns the code provided by the service."
>>      (with-current-buffer (url-retrieve-synchronously url)
>>        (let ((data (oauth2-request-access-parse)))
>>          (kill-buffer (current-buffer))
>> +        (oauth2--do-debug "oauth2-make-access-request: response: %s"
>> +                          (pp-to-string data))
>
> Is pp-to-string here really much better than prin1-to-string?  Note that
> 'oauth2--do-debug' is a function, so the arguments are always evaluated.
> I have experienced pp being significantly slower than the core print
> functions, so this is something that is worth thinking about IMO.
>

It's just a personal preference.  And it look like the values are mostly
key-value pairs so they are not that hard to read through
`prin1-to-string' so I changed to it.

>>          data))))
>>  
>>  (cl-defstruct oauth2-token
>> -- 
>> 2.39.2
>>
>>
>> From e8735da21ac82b0698edad1796ddf4a1b8eb4bb2 Mon Sep 17 00:00:00 2001
>> From: Xiyue Deng <manphiz@gmail.com>
>> Date: Tue, 30 Jul 2024 03:46:57 -0700
>> Subject: [PATCH 6/6] Add NEWS file to document the changes to plstore id
>>  generation
>>
>> ---
>>  NEWS | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>  create mode 100644 NEWS
>>
>> diff --git a/NEWS b/NEWS
>> new file mode 100644
>> index 0000000000..6715a1914a
>> --- /dev/null
>> +++ b/NEWS
>> @@ -0,0 +1,23 @@
>> +Summary of changes to oauth2.el
>> +-------------------------------
>> +
>> +For changes of 0.16 and older or full changes please check the git
>> +history of the repository of oauth2.el.
>> +
>> +* 0.17
>> +
>> +** Changes to plstore id generation and needs to reacquire refresh_token
>> +
>> +The generation of plstore id used to include `auth-url', `token-url',
>> +and `scope'.  Now `client-id' is also included.  This is required to
>> +support multiple accounts of some providers which use the same
>> +`auth-url', `token-url', and `scope' (e.g. Gmail), and hence the
>> +generated plstore id is not unique amount accounts.  Adding
>> +`client-id' solves this problem.
>> +
>> +The hash function of calculating the plstore id has also changed from
>> +MD5 to SHA512 to be more secure.
>> +
>> +As a result, users of oauth2.el will need to redo the authentication
>> +process to get a new refresh_token when upgrading from older version
>> +to 0.17.
>
> Perhaps you can add a file local variable to enable outline-mode?
>

Done.  Also improved the wording.

> All in all the patches look more than fine though, nothing I mentioned
> is pressing.  If you don't have the time and the changes are urgent,
> then I can also apply them as such.
>
> Xiyue Deng <manphiz@gmail.com> writes:
>
> [...]
>
>> Friendly ping.
>
> Friendly pong.

The updated patches are attached.  Thanks again!

-- 
Xiyue Deng

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Show-full-authentication-URL-to-let-user-choose-how-.patch --]
[-- Type: text/x-diff, Size: 2068 bytes --]

From dc1930f5b4828eb285cb28ad0138e67158003c22 Mon Sep 17 00:00:00 2001
From: Xiyue Deng <manphiz@gmail.com>
Date: Sun, 21 Jul 2024 14:50:56 -0700
Subject: [PATCH 1/6] Show full authentication URL to let user choose how to
 visit it

* packages/oauth2/oauth2.el (oauth2-request-authorization): show full
authentication URL in user prompt.  (Bug#72358)
---
 oauth2.el | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/oauth2.el b/oauth2.el
index 7da9702004..3a3e50ad2b 100644
--- a/oauth2.el
+++ b/oauth2.el
@@ -57,14 +57,17 @@
   "Request OAuth authorization at AUTH-URL by launching `browse-url'.
 CLIENT-ID is the client id provided by the provider.
 It returns the code provided by the service."
-  (browse-url (concat auth-url
-                      (if (string-match-p "\?" auth-url) "&" "?")
-                      "client_id=" (url-hexify-string client-id)
-                      "&response_type=code"
-                      "&redirect_uri=" (url-hexify-string (or redirect-uri "urn:ietf:wg:oauth:2.0:oob"))
-                      (if scope (concat "&scope=" (url-hexify-string scope)) "")
-                      (if state (concat "&state=" (url-hexify-string state)) "")))
-  (read-string "Enter the code your browser displayed: "))
+  (let ((url (concat auth-url
+                     (if (string-match-p "\?" auth-url) "&" "?")
+                     "client_id=" (url-hexify-string client-id)
+                     "&response_type=code"
+                     "&redirect_uri=" (url-hexify-string (or redirect-uri "urn:ietf:wg:oauth:2.0:oob"))
+                     (if scope (concat "&scope=" (url-hexify-string scope)) "")
+                     (if state (concat "&state=" (url-hexify-string state)) ""))))
+    (browse-url url)
+    (read-string (concat "Follow the instruction on your default browser, or "
+                         "visit:\n" url
+                         "\nEnter the code your browser displayed: "))))
 
 (defun oauth2-request-access-parse ()
   "Parse the result of an OAuth request."
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-parameters-required-by-Google-OAuth2-to-get-refr.patch --]
[-- Type: text/x-diff, Size: 1419 bytes --]

From 4f6628439bdee67025719314bab5b824c86d19e9 Mon Sep 17 00:00:00 2001
From: Xiyue Deng <manphiz@gmail.com>
Date: Sun, 21 Jul 2024 14:52:02 -0700
Subject: [PATCH 2/6] Add parameters required by Google OAuth2 to get
 refresh_token

* packages/oauth2/oauth2.el (oauth2-request-authorization): add
"access_type=offline" and "prompt=consent" when requesting token to
receive refresh_token.  (Bug#72358)
---
 oauth2.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/oauth2.el b/oauth2.el
index 3a3e50ad2b..8371e73ffb 100644
--- a/oauth2.el
+++ b/oauth2.el
@@ -63,7 +63,11 @@ It returns the code provided by the service."
                      "&response_type=code"
                      "&redirect_uri=" (url-hexify-string (or redirect-uri "urn:ietf:wg:oauth:2.0:oob"))
                      (if scope (concat "&scope=" (url-hexify-string scope)) "")
-                     (if state (concat "&state=" (url-hexify-string state)) ""))))
+                     (if state (concat "&state=" (url-hexify-string state)) "")
+                     ;; The following two parameters are required for Gmail
+                     ;; OAuth2 to generate the refresh token
+                     "&access_type=offline"
+                     "&prompt=consent")))
     (browse-url url)
     (read-string (concat "Follow the instruction on your default browser, or "
                          "visit:\n" url
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Encode-URL-when-requesting-access.patch --]
[-- Type: text/x-diff, Size: 1524 bytes --]

From 5f2877930ac69bef64b2d53e7ded4913e13c50b6 Mon Sep 17 00:00:00 2001
From: Xiyue Deng <manphiz@gmail.com>
Date: Sun, 21 Jul 2024 14:54:08 -0700
Subject: [PATCH 3/6] Encode URL when requesting access

* packages/oauth2/oauth2.el (oauth2-request-access): encode the whole
URL to avoid having characters that breaks URL parsing.  (Bug#72358)
---
 oauth2.el | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/oauth2.el b/oauth2.el
index 8371e73ffb..54ca61e1cf 100644
--- a/oauth2.el
+++ b/oauth2.el
@@ -108,13 +108,14 @@ Return an `oauth2-token' structure."
     (let ((result
            (oauth2-make-access-request
             token-url
-            (concat
-             "client_id=" client-id
-	     (when client-secret
-               (concat  "&client_secret=" client-secret))
-             "&code=" code
-             "&redirect_uri=" (url-hexify-string (or redirect-uri "urn:ietf:wg:oauth:2.0:oob"))
-             "&grant_type=authorization_code"))))
+            (url-encode-url
+             (concat
+              "client_id=" client-id
+              (when client-secret
+                (concat  "&client_secret=" client-secret))
+              "&code=" code
+              "&redirect_uri=" (or redirect-uri "urn:ietf:wg:oauth:2.0:oob")
+              "&grant_type=authorization_code")))))
       (make-oauth2-token :client-id client-id
                          :client-secret client-secret
                          :access-token (cdr (assoc 'access_token result))
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Support-storing-data-for-multiple-accounts-of-the-sa.patch --]
[-- Type: text/x-diff, Size: 2103 bytes --]

From a5241f369f5ea255fcb6f36c0b52829745d71ca8 Mon Sep 17 00:00:00 2001
From: Xiyue Deng <manphiz@gmail.com>
Date: Sun, 28 Jul 2024 03:00:04 -0700
Subject: [PATCH 4/6] Support storing data for multiple accounts of the same
 provider

Currently the plstore id computed by "oauth2-compute-id" only takes
"auth-url", "token-url", and "scope" into account, which could be the
same for the same provider (e.g. Gmail).  This prevents storing
information for multiple accounts of the same service for some
providers.

This patch adds "client-id" to the calculation of plstore id to make
sure that it is unique for different accounts of the same provider.

It also changes the hash function to sha512 to be more secure.

* packages/oauth2/oauth2.el (oauth2-compute-id): add "client-id" as a
parameter of "oauth2-compute-id" to ensure unique id amount multiple
accounts of the same provider, and change hash function to
sha512.  (Bug#72358)
---
 oauth2.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/oauth2.el b/oauth2.el
index 54ca61e1cf..c8011ebf9b 100644
--- a/oauth2.el
+++ b/oauth2.el
@@ -166,17 +166,17 @@ TOKEN should be obtained with `oauth2-request-access'."
   :group 'oauth2
   :type 'file)
 
-(defun oauth2-compute-id (auth-url token-url scope)
+(defun oauth2-compute-id (auth-url token-url scope client-id)
   "Compute an unique id based on URLs.
 This allows to store the token in an unique way."
-  (secure-hash 'md5 (concat auth-url token-url scope)))
+  (secure-hash 'sha512 (concat auth-url token-url scope client-id)))
 
 ;;;###autoload
 (defun oauth2-auth-and-store (auth-url token-url scope client-id client-secret &optional redirect-uri state)
   "Request access to a resource and store it using `plstore'."
   ;; We store a MD5 sum of all URL
   (let* ((plstore (plstore-open oauth2-token-file))
-         (id (oauth2-compute-id auth-url token-url scope))
+         (id (oauth2-compute-id auth-url token-url scope client-id))
          (plist (cdr (plstore-get plstore id))))
     ;; Check if we found something matching this access
     (if plist
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Add-debug-messages-and-provide-an-option-variable-fo.patch --]
[-- Type: text/x-diff, Size: 2540 bytes --]

From e2ece9aee7f43ec35789823576dc5390d3384dc6 Mon Sep 17 00:00:00 2001
From: Xiyue Deng <manphiz@gmail.com>
Date: Sun, 28 Jul 2024 03:41:20 -0700
Subject: [PATCH 5/6] Add debug messages and provide an option variable for
 enabling

This helps debugging whether the authorization and refresh requests
were successful and inspecting the responses.

* packages/oauth2/oauth2.el: add support for debug messages and an
option variable for enabling.  (Bug#72358)
---
 oauth2.el | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/oauth2.el b/oauth2.el
index c8011ebf9b..9c91bb08a1 100644
--- a/oauth2.el
+++ b/oauth2.el
@@ -53,6 +53,15 @@
   :link '(url-link :tag "Savannah" "https://git.savannah.gnu.org/cgit/emacs/elpa.git/tree/?h=externals/oauth2")
   :link '(url-link :tag "ELPA" "https://elpa.gnu.org/packages/oauth2.html"))
 
+(defvar oauth2-debug nil
+  "Enable verbose logging in oauth2 to help debugging.")
+
+(defun oauth2--do-debug (&rest msg)
+  "Output debug messages when `oauth2-debug' is enabled."
+  (when oauth2-debug
+    (setcar msg (concat "[oauth2] " (car msg)))
+    (apply #'message msg)))
+
 (defun oauth2-request-authorization (auth-url client-id &optional scope state redirect-uri)
   "Request OAuth authorization at AUTH-URL by launching `browse-url'.
 CLIENT-ID is the client id provided by the provider.
@@ -81,14 +90,18 @@ It returns the code provided by the service."
 
 (defun oauth2-make-access-request (url data)
   "Make an access request to URL using DATA in POST."
-  (let ((url-request-method "POST")
-        (url-request-data data)
-        (url-request-extra-headers
-         '(("Content-Type" . "application/x-www-form-urlencoded"))))
-    (with-current-buffer (url-retrieve-synchronously url)
-      (let ((data (oauth2-request-access-parse)))
-        (kill-buffer (current-buffer))
-        data))))
+  (let ((func-name (nth 1 (backtrace-frame 3))))
+    (oauth2--do-debug "%s: url: %s" func-name url)
+    (oauth2--do-debug "%s: data: %s" func-name data)
+    (let ((url-request-method "POST")
+          (url-request-data data)
+          (url-request-extra-headers
+           '(("Content-Type" . "application/x-www-form-urlencoded"))))
+      (with-current-buffer (url-retrieve-synchronously url)
+        (let ((data (oauth2-request-access-parse)))
+          (kill-buffer (current-buffer))
+          (oauth2--do-debug "%s: response: %s" func-name (prin1-to-string data))
+          data)))))
 
 (cl-defstruct oauth2-token
   plstore
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #7: 0006-Add-NEWS-file-to-document-the-changes-to-plstore-id-.patch --]
[-- Type: text/x-diff, Size: 1444 bytes --]

From e99ec9b3f209312bfa0048c5ae94876e03654c18 Mon Sep 17 00:00:00 2001
From: Xiyue Deng <manphiz@gmail.com>
Date: Tue, 30 Jul 2024 03:46:57 -0700
Subject: [PATCH 6/6] Add NEWS file to document the changes to plstore id
 generation

* Also add local variables to enable outline-mode.  (Bug#72358)
---
 NEWS | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 NEWS

diff --git a/NEWS b/NEWS
new file mode 100644
index 0000000000..72b0606659
--- /dev/null
+++ b/NEWS
@@ -0,0 +1,27 @@
+Summary of changes to oauth2.el
+-------------------------------
+
+(For changes of 0.16 and older please check the git history of the
+repository of oauth2.el.)
+
+* 0.17
+
+** Changes to plstore id generation and needs to reacquire refresh_token
+
+The generation of plstore id used to include `auth-url', `token-url',
+and `scope'.  Now `client-id' is also included.  This is required to
+support multiple accounts of some providers which use the same
+`auth-url', `token-url', and `scope' (e.g. Gmail), or the generated
+plstore id is not unique amount accounts.  Adding `client-id' solves
+this problem.
+
+The hash function of calculating the plstore id has also changed from
+MD5 to SHA512 to be more secure.
+
+As a result, users of oauth2.el will need to redo the authentication
+process to get a new refresh_token when upgrading from older version
+to 0.17.
+
+# Local variables:
+# mode: outline
+# End:
-- 
2.39.2