bug#72358: 29.4; oauth2.el improvements

[Xiyue Deng <manphiz@gmail.com>]

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

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Mon, 29 Jul 2024 14:25:01 -0700, Xiyue Deng <manphiz@gmail.com> said:
>
>     Xiyue> Hi,
>     Xiyue> I have been trying out using oauth2.el to enable OAuth2-based
>     Xiyue> authentication for email service providers and had some success for
>     Xiyue> Gmail.  During this process, I have made a few changes to oauth2.el that
>     Xiyue> enables it to use with Gmail OAuth2 as well as some usability and
>     Xiyue> debugging improvements, which I'm sharing below.
>
> Thank you for this. This support is becoming more necessary as time
> goes on. I even wonder if we should bring oauth2.el into emacs instead
> of it being a package.
>

I would also love that to happen.  In fact, I have another addon
inspired by auth-source-xoauth2 and makes use of oauth2.el and
auth-source backend, though it requires advising the
auth-source-search-backends and kind of awkward.  I'll post that through
another bug report for comments after this one gets submitted.

>     Xiyue> This is a series of five patches, which are attached.
>
>     Xiyue> The first patch shows the authentication URL in the minibuffer window
>     Xiyue> alongside the prompt accepting the authorization code.  This helps when
>     Xiyue> a user has multiple accounts from the same provider but is logged into a
>     Xiyue> different account than the one that the user is trying to set up.  If
>     Xiyue> the user use the link (or through `browse-url') it will use the active
>     Xiyue> account instead of the one intended.  By showing the URL in the
>     Xiyue> minibuffer, the user can choose other ways to get the authorization code
>     Xiyue> (e.g. using another browser, using private/encognito mode, etc.)
>
> OK. This fixes one of my irritations with oauth2.el 🙂
>

Glad to hear I'm not the only one.

>     Xiyue> The second patch adds the parameters `access_type=offline' and
>     Xiyue> `prompt=consent' to the authorization URL, which is required for Gmail
>     Xiyue> OAuth2 to get the refresh token.  Without these 2 parameters, Gmail
>     Xiyue> response will only contain the access token which expires in one hour.
>     Xiyue> They should also be compatible with other OAuth2 authentication process.
>     Xiyue> (Though I am currently having trouble to get outlook.com to work
>     Xiyue> regardless of these parameters, which I'll ask in a separate thread.)
>
>     Xiyue> Note that the second patch depends on the first patch as they modify the same
>     Xiyue> part of the code.
>
> OK. Iʼm assuming oauth2.el can use the refresh token next time it
> needs to authorize? (Iʼve been avoiding actually using oauth2.el in
> anger, since app passwords still work)
>

Yes, with a valid refresh token you can login without any manual steps.
Unless the refresh_token itself is expired, in which case you'll have to
re-authorize.

>     Xiyue> The third patch encodes the parameters for requesting refreshing access
>     Xiyue> token, which is recommended because the client secret and other
>     Xiyue> parameters may contain characters that may break parameter parsing.
>
> OK
>
>     Xiyue> The fourth patch may need a bit of background: oauth2.el (optionally)
>     Xiyue> uses plstore to save authentication data for future reuse, and the
>     Xiyue> plstore id for an account is computed using a combination of `auth-url',
>     Xiyue> `token-url', and `scope'.  However, this combination of data doesn't
>     Xiyue> guarantee uniqueness for accounts for a same provider, e.g. for Gmail,
>     Xiyue> the three parameters are the same for different accounts, and hence
>     Xiyue> storing a second account information will override the first one.
>
>     Xiyue> This fourth patch adds `client-id' to the calculation of plstore id to
>     Xiyue> ensure its uniqueness.  This may cause a few concerns:
>
>     Xiyue> - This will invalidate all existing entries and a user will have to redo
>     Xiyue>   the authorization process again to get a new refresh token.  However,
>     Xiyue>   I think it's more important to ensure that oauth2.el works correctly
>     Xiyue>   for multiple accounts of the same provider, or a user may suffer from
>     Xiyue>   confusion when adding a new account invalidates a previous account.
>
> I donʼt think thatʼs too big a concern. 'modern' authentication flows
> regularly re-prompt, so this will not be too surprising (although
> maybe call it out in the packageʼs NEWS or README).
>

I have added a NEWS file in patch 6 documenting this for 0.17 (which
should be the version of the next release.)

>     Xiyue> - Adding `client-id' to the calculation of plstore id may provoke
>     Xiyue>   suspicion of leaking it as the hash calculation uses md5.  In most
>     Xiyue>   cases, requesting a refresh token requires both `client-id' and
>     Xiyue>   `client-secret', so without including the latter it should be safe.
>     Xiyue>   There are cases when requesting only the access token may work with
>     Xiyue>   `client-id' along.  Still, I think this should not be a big concern as
>     Xiyue>   the data is combined with `auth-url', `token-url', and `scope' which
>     Xiyue>   provides sufficient salt.  Alternatively, we can also choose to use a
>     Xiyue>   more secure hash function, e.g. SHA2 or better, given that existing
>     Xiyue>   entries will be invalidated anyway.
>
> If the existing entries are going to become invalid anyway, you might
> as well take the opportunity to move away from md5 at the same
> time. git picked SHA-256, but that was a while ago, so maybe SHA-512?
>

Makes sense.  I have updated the hash function to use SHA-512 in patch 5.

>     Xiyue> The fifth patch adds debug messages when doing a URL query which records
>     Xiyue> the request URL, the request data, and the response data, and provide a
>     Xiyue> custom variable to enable this.  This provides a way to help debugging
>     Xiyue> the requests, and I find it handy when testing oauth2 against different
>     Xiyue> providers.
>
> OK (although perhaps make it a defvar rather than a defcustom, to
> avoid people accidentally enabling it).
>

Done also in patch 5.

> Robert

-- 
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: 2751 bytes --]

From 08b04a07aa399bc756f1a8a00ae17cf333be02d9 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 | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/oauth2.el b/oauth2.el
index 035971ac85..aca48f9ce1 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,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"))
 
+(defcustom oauth2-debug nil
+  "Enable debug messages."
+  :type 'boolean)
+
+(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 +89,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 +98,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
@@ -158,10 +172,8 @@ TOKEN should be obtained with `oauth2-request-access'."
     auth-url client-id scope state redirect-uri)
    redirect-uri))
 
-(defcustom oauth2-token-file (concat user-emacs-directory "oauth2.plstore")
-  "File path where store OAuth tokens."
-  :group 'oauth2
-  :type 'file)
+(defvar oauth2-token-file (concat user-emacs-directory "oauth2.plstore")
+  "File path where store OAuth tokens.")
 
 (defun oauth2-compute-id (auth-url token-url scope client-id)
   "Compute an unique id based on URLs.
-- 
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 2fbc85ee128cfdf7a1521bbc9554424e9ba510da 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