bug#72358: 29.4; oauth2.el improvements

[Xiyue Deng <manphiz@gmail.com>]

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

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

[-- 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: 2055 bytes --]

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.
---
 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: 1264 bytes --]

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
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")))
     (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-parameters-when-requesting-access.patch --]
[-- Type: text/x-diff, Size: 1194 bytes --]

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


[-- 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: 2090 bytes --]

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


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

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.")
+
+(defun oauth2--do-debug (&rest msg)
+  "Output debug messages when `oauth2-debug' is enabled."
+  (if oauth2-debug
+    (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.
@@ -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))
         data))))
 
 (cl-defstruct oauth2-token
-- 
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: 1350 bytes --]

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.
-- 
2.39.2