Subject: Re: bug#52476: Fwd: [ELPA/oauth2] Request for patches review

From: Xiyue Deng <[email-masked]>
Date: Sun, 04 May 2025 17:10:04 -0700

Stefan Kangas <[email-masked]> writes:

> Xiyue Deng <[email-masked]> writes:
>
>> Hi Stefan,
>>
>> Stefan Kangas <[email-masked]> writes:
>>
>>> Xiyue, since you have looked into oauth2 recently, perhaps you would be
>>> willing to take a look at this?  I'm not sure how relevant any of this
>>> is with your recent changes.
>>>
>>> You can find the patches here:
>>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=52476
>>
>> Thanks for letting me know of this patch set - I wasn't when working on
>> the last changes on oauth2.  I'll try to take a look on each patch and
>> try to summarize which parts are still good additions.  I'll do one
>> patch per mail so that it doesn't become too long.  This will take a
>> while, so please allow me some time.
>
> Thanks!  There's no rush, so please take your time.

Sorry for the unexpected long delay.  It turns out what I proposed was a
bit too time consuming and it's easier to just give an overview with all
the patches and propose what I think may be beneficial to be submitted.

In general, the propose patch set has a few types of changes:
* Small clean ups
* Doc improvements
* Change of interface and implementation behavior

I think the first 2 can be incorporated, though for the last one I'd
refrain from adding at this time as they could break clients (like my
plugin :P)

I'll prepare a patch set with other improvements under a different bug.

Please see below for more detailed comments.  Please do correct me if
any of my assumptions is wrong.

> diff --git a/oauth2.el b/oauth2.el
> index 87e0c39c5c..ae0d7c3174 100644
> --- a/oauth2.el
> +++ b/oauth2.el
> @@ -1,11 +1,13 @@
>  ;;; oauth2.el --- OAuth 2.0 Authorization Protocol  -*- lexical-binding:t -*-
>  
> +;; Copyright © 2021 Aleksandr Vityazev <[email-masked]>
>  ;; Copyright (C) 2011-2021 Free Software Foundation, Inc
>  
>  ;; Author: Julien Danjou <[email-masked]>
>  ;; Version: 0.17
>  ;; Keywords: comm
> -;; Package-Requires: ((cl-lib "0.5") (nadvice "0.3"))
> +;; Package-Requires: ((emacs "27.1"))
> +;; Homepage: https://git.sr.ht/~akagi/oauth2
>

Changing the copyright holder and homepage should not be necessary.

>  ;; This file is part of GNU Emacs.
>  
> @@ -36,11 +38,19 @@
>  
>  ;;; Code:
>  
> -(eval-when-compile (require 'cl-lib))
> +(eval-when-compile
> +  (require 'cl-lib)
> +  (require 'subr-x))
> +

I don't find any actual use of subr-x.

>  (require 'plstore)
>  (require 'json)
>  (require 'url-http)
>  
> +(defcustom oauth2-token-file (concat user-emacs-directory "oauth2.plstore")
> +  "File path where store OAuth tokens."
> +  :type 'file
> +  :group 'oauth2)
> +

This moves the custom definition to the beginning alongside other
defvar/defcustom, which I think is good.

>  (defvar url-http-data)
>  (defvar url-http-method)
>  (defvar url-http-extra-headers)
> @@ -62,25 +72,29 @@
>      (setcar msg (concat "[oauth2] " (car msg)))
>      (apply #'message msg)))
>  
> -(defun oauth2-request-authorization (auth-url client-id &optional scope state redirect-uri)
> +(cl-defun oauth2-request-authorization (auth-url client-id &optional scope state
> +                                           (redirect-uri "urn:ietf:wg:oauth:2.0:oob"))
>    "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."
> -  (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)) "")
> -                     ;; 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
> -                         "\nEnter the code your browser displayed: "))))
> +The REDIRECT-URI is the registered redirect_uri for your CLIENT-ID,
> +the default for the desktop applications is \"urn:ietf:wg:oauth:2.0:oob\".
> +SCOPE identify the resources that your application could access
> +on the user's behalf.  STATE it is a string that your application
> +uses to maintain state between the request and redirect response.
> +
> +Return the code provided by the service."
> +  (browse-url (concat auth-url
> +                      (if (string-match-p (rx "?") auth-url) "&" "?")
> +                      "client_id=" (url-hexify-string client-id)
> +                      "&response_type=code"
> +                      "&redirect_uri=" (url-hexify-string redirect-uri)
> +                      (if scope (concat "&scope=" (url-hexify-string scope)) "")
> +                      (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"))
> +  (read-string "Enter the code your browser displayed: "))
>

This part uses cl-defun to give `redirect-uri' a default value at the
declaration.  I'm not sure whether this could change the interface.  The
documentation improvements are good though.

>  (defun oauth2-request-access-parse ()
>    "Parse the result of an OAuth request."
> @@ -104,182 +118,213 @@ It returns the code provided by the service."
>            data)))))
>  
>  (cl-defstruct oauth2-token
> -  plstore
> -  plstore-id
> -  client-id
> -  client-secret
> -  access-token
> -  refresh-token
> -  token-url
> -  access-response)
> -
> -(defun oauth2-request-access (token-url client-id client-secret code &optional redirect-uri)
> +  (plstore         "" :read-only t)
> +  (plstore-id      "" :read-only t)
> +  (client-id       "" :read-only t)
> +  (client-secret   "" :read-only t)
> +  (access-token    "" :read-only t)
> +  (refresh-token   "" :read-only t)
> +  (token-url       "" :read-only t)
> +  (expires-in      "" :read-only t)
> +  (created-at      "" :read-only t)
> +  (access-response "" :read-only t))

This makes the aouth2-token readonly.  Though it could be a good thing,
it would require some changes to the implementation and hence some
public interfaces, so I'd suggest against it for now.

> +
> +(defun oauth2--epoch-time ()
> +  "Return seconds since the Epoch."
> +  (time-convert (current-time) 'integer))
> +
> +(defun oauth2--created-at (response)
> +  "Return epoch time from RESPONSE.
> +If there no created_at key in RESPONSE, add
> +\(cons 'created_at . epoch-time\) to RESPONSE."
> +  (if-let ((it (cdr (assoc 'created_at response))))
> +      it
> +    (prog1 (oauth2--epoch-time)
> +      (cl-pushnew (cons 'created_at (oauth2--epoch-time)) response))))
> +

This is part of the implementation to add support for reusing cached
access token during the time it's valid.  I am also thinking about
implementing this, but in a slightly different way.

> +(cl-defun oauth2-request-access (token-url client-id client-secret code &optional
> +                                     (redirect-uri "urn:ietf:wg:oauth:2.0:oob"))
>    "Request OAuth access at TOKEN-URL.
> +CLIENT-ID is the client id provided by the provider.
> +CLIENT-SECRET is the client secret associated with your CLIENT-ID.
> +The REDIRECT-URI is the registered redirect_uri for your CLIENT-ID,
> +the default for the desktop applications is \"urn:ietf:wg:oauth:2.0:oob\".
>  The CODE should be obtained with `oauth2-request-authorization'.
> -Return an `oauth2-token' structure."
> +
> +Return an response from requested service."
>    (when code
> -    (let ((result
> +    (let ((response
>             (oauth2-make-access-request
>              token-url
>              (url-encode-url
>               (concat
>                "client_id=" client-id
> -              (when client-secret
> +	      (when client-secret
>                  (concat  "&client_secret=" client-secret))
>                "&code=" code
> -              "&redirect_uri=" (or redirect-uri "urn:ietf:wg:oauth:2.0:oob")
> +              "&redirect_uri=" (redirect-uri)
>                "&grant_type=authorization_code")))))
> -      (make-oauth2-token :client-id client-id
> -                         :client-secret client-secret
> -                         :access-token (cdr (assoc 'access_token result))
> -                         :refresh-token (cdr (assoc 'refresh_token result))
> -                         :token-url token-url
> -                         :access-response result))))
> +      response)))
>

Similar to the other `cl-defun' change.  It also make use of a new
interface added, so probably better not to change much here.  The doc
improvements are good though.

>  ;;;###autoload
>  (defun oauth2-refresh-access (token)
>    "Refresh OAuth access TOKEN.
>  TOKEN should be obtained with `oauth2-request-access'."
> -  (setf (oauth2-token-access-token token)
> -        (cdr (assoc 'access_token
> -                    (oauth2-make-access-request
> -                     (oauth2-token-token-url token)
> -                     (concat "client_id=" (oauth2-token-client-id token)
> -			     (when (oauth2-token-client-secret token)
> -                               (concat "&client_secret=" (oauth2-token-client-secret token)))
> -                             "&refresh_token=" (oauth2-token-refresh-token token)
> -                             "&grant_type=refresh_token")))))
> -  ;; If the token has a plstore, update it
> -  (let ((plstore (oauth2-token-plstore token)))
> -    (when plstore
> -      (plstore-put plstore (oauth2-token-plstore-id token)
> -                   nil `(:access-token
> -                         ,(oauth2-token-access-token token)
> -                         :refresh-token
> -                         ,(oauth2-token-refresh-token token)
> -                         :access-response
> -                         ,(oauth2-token-access-response token)
> -                         ))
> -      (plstore-save plstore)))
> -  token)
> +  (let ((plstore (oauth2-token-plstore token))
> +        (plstore-id (oauth2-token-plstore-id token))
> +        (client-id (oauth2-token-client-id token))
> +        (client-secret (oauth2-token-client-secret token))
> +        (token-url (oauth2-token-token-url token)))
> +    (let* ((response (oauth2-make-access-request
> +                      (oauth2-token-token-url token)
> +                      (concat "client_id=" client-id
> +			      (when client-secret
> +                                (concat "&client_secret=" client-secret))
> +                              "&refresh_token=" (oauth2-token-refresh-token token)
> +                              "&grant_type=refresh_token")))
> +           (new-token (make-oauth2-token :plstore plstore
> +                                         :plstore-id plstore-id
> +                                         :client-id client-id
> +                                         :client-secret client-secret
> +                                         :access-token (cdr (assoc 'access_token response))
> +                                         :refresh-token (cdr (assoc 'refresh_token response))
> +                                         :expires-in (cdr (assoc 'expires_in response))
> +                                         :created-at (oauth2--created-at response)
> +                                         :token-url token-url
> +                                         :access-response response)))
> +      (when plstore
> +        (plstore-put plstore plstore-id
> +                     nil `(:access-token
> +                           ,(oauth2-token-access-token new-token)
> +                           :refresh-token
> +                           ,(oauth2-token-refresh-token new-token)
> +                           :access-response
> +                           ,(oauth2-token-access-response new-token)))
> +        (plstore-save plstore))
> +      new-token)))
>

This part is because of oauth2-token is now readonly.  I tend to
consider this as not strictly necessary.

>  ;;;###autoload
> -(defun oauth2-auth (auth-url token-url client-id client-secret &optional scope state redirect-uri)
> -  "Authenticate application via OAuth2."
> -  (oauth2-request-access
> -   token-url
> -   client-id
> -   client-secret
> -   (oauth2-request-authorization
> -    auth-url client-id scope state redirect-uri)
> -   redirect-uri))
> +(defun oauth2-auth
> +    (auth-url token-url client-id client-secret &optional scope state redirect-uri)
> +  "Authenticate application via OAuth2 at AUTH-URL and TOKEN-URL.
> +CLIENT-ID is the client id provided by the provider.
> +CLIENT-SECRET is the client secret associated with your CLIENT-ID.
> +The REDIRECT-URI is the registered redirect_uri for your CLIENT-ID.
> +SCOPE identify the resources that your application could access
> +on the user's behalf.  STATE it is a string that your application
> +uses to maintain state between the request and redirect response.
>  
> -(defcustom oauth2-token-file (concat user-emacs-directory "oauth2.plstore")
> -  "File path where store OAuth tokens."
> -  :group 'oauth2
> -  :type 'file)
> +Return an `oauth2-token' structure."
>  
> -(defun oauth2-compute-id (auth-url token-url scope client-id)
> -  "Compute an unique id based on URLs.
> +  (let ((plstore (plstore-open oauth2-token-file))
> +        (id (oauth2-compute-id auth-url token-url scope))
> +        (response (oauth2-request-access
> +                   token-url
> +                   client-id
> +                   client-secret
> +                   (oauth2-request-authorization
> +                    auth-url client-id scope state redirect-uri)
> +                   redirect-uri)))
> +    (make-oauth2-token :plstore plstore
> +                       :plstore-id id
> +                       :client-id client-id
> +                       :client-secret client-secret
> +                       :access-token (cdr (assoc 'access_token response))
> +                       :refresh-token (cdr (assoc 'refresh_token response))
> +                       :expires-in (cdr (assoc 'expires_in response))
> +                       :created-at (oauth2--created-at response)
> +                       :token-url token-url
> +                       :access-response response)))
> +
> +(defun oauth2-compute-id (auth-url token-url scope)
> +  "Compute an unique id based on AUTH-URL, TOKEN-URL and SCOPE.
>  This allows to store the token in an unique way."
>    (secure-hash 'sha512 (concat auth-url token-url scope client-id)))
>

Similarly due to oauth2-token being readonly.  Not strictly necessary.

>  ;;;###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 client-id))
> -         (plist (cdr (plstore-get plstore id))))
> -    ;; Check if we found something matching this access
> -    (if plist
> -        ;; We did, return the token object
> -        (make-oauth2-token :plstore plstore
> -                           :plstore-id id
> -                           :client-id client-id
> -                           :client-secret client-secret
> -                           :access-token (plist-get plist :access-token)
> -                           :refresh-token (plist-get plist :refresh-token)
> -                           :token-url token-url
> -                           :access-response (plist-get plist :access-response))
> -      (let ((token (oauth2-auth auth-url token-url
> -                                client-id client-secret scope state redirect-uri)))
> -        ;; Set the plstore
> -        (setf (oauth2-token-plstore token) plstore)
> -        (setf (oauth2-token-plstore-id token) id)
> -        (plstore-put plstore id nil `(:access-token
> -                                      ,(oauth2-token-access-token token)
> -                                      :refresh-token
> -                                      ,(oauth2-token-refresh-token token)
> -                                      :access-response
> -                                      ,(oauth2-token-access-response token)))
> -        (plstore-save plstore)
> -        token))))
> +(defun oauth2-auth-and-store
> +    (auth-url token-url client-id client-secret scope &optional state redirect-uri)
> +  "Request access to a resource and store it using `plstore'.
> +If the token has not yet been saved in plsote, then authenticate
> +the application via OAuth2 using the AUTH-URL, TOKEN-URL and after that
> +save the token in the `oauth2-token-file'.
> +CLIENT-ID is the client id provided by the provider.
> +CLIENT-SECRET is the client secret associated with your CLIENT-ID.
> +The REDIRECT-URI is the registered redirect_uri for your CLIENT-ID.
> +SCOPE identify the resources that your application could access
> +on the user's behalf.  STATE it is a string that your application
> +uses to maintain state between the request and redirect response.
> +
> +Return an `oauth2-token' structure."
> +  (if-let ((plstore (plstore-open oauth2-token-file))
> +           (id (oauth2-compute-id auth-url token-url scope))
> +           (plist (cdr (plstore-get plstore id)))
> +           (expires (cdr (assoc 'expires_in (plist-get plist :access-response))))
> +           (created (cdr (assoc 'created_at (plist-get plist :access-response))))
> +           (current-time (oauth2--epoch-time))
> +           (token (make-oauth2-token :plstore plstore
> +                                     :plstore-id id
> +                                     :client-id client-id
> +                                     :client-secret client-secret
> +                                     :access-token (plist-get plist :access-token)
> +                                     :refresh-token (plist-get plist :refresh-token)
> +                                     :expires-in expires
> +                                     :created-at created
> +                                     :token-url token-url
> +                                     :access-response (plist-get plist :access-response))))
> +      (if (< current-time (+ expires created))
> +          token
> +        (oauth2-refresh-access token))
> +    (let ((token (oauth2-auth auth-url token-url
> +                        client-id client-secret scope state redirect-uri)))
> +      (plstore-put plstore id nil `(:access-token
> +                                    ,(oauth2-token-access-token token)
> +                                    :refresh-token
> +                                    ,(oauth2-token-refresh-token token)
> +                                    :access-response
> +                                    ,(oauth2-token-access-response token)))
> +      (plstore-save plstore)
> +      token)))
>

Ditto.

>  (defun oauth2-url-append-access-token (token url)
> -  "Append access token to URL."
> +  "Append access TOKEN to URL."
>    (concat url
> -          (if (string-match-p "\?" url) "&" "?")
> +          (if (string-match-p (rx "?") url) "&" "?")
>            "access_token=" (oauth2-token-access-token token)))
>

Slight improvement to doc and regexp.  Not strictly required.

> -(defvar oauth--url-advice nil)
> -(defvar oauth--token-data)
> -
> -(defun oauth2-authz-bearer-header (token)
> -  "Return `Authoriztions: Bearer' header with TOKEN."
> -  (cons "Authorization" (format "Bearer %s" token)))
> +(defun oauth2-authz-bearer-header (access-token)
> +  "Return `Authoriztions: Bearer' header with ACCESS-TOKEN."
> +  (cons "Authorization" (format "Bearer %s" access-token)))
>

Slight improvements.  Not really necessary.

> -(defun oauth2-extra-headers (extra-headers)
> -  "Return EXTRA-HEADERS with `Authorization: Bearer' added."
> -  (cons (oauth2-authz-bearer-header (oauth2-token-access-token (car oauth--token-data)))
> +(defun oauth2-extra-headers (token extra-headers)
> +  "Return EXTRA-HEADERS with `Authorization: Bearer' header with TOKEN added."
> +  (cons (oauth2-authz-bearer-header (oauth2-token-access-token token))
>          extra-headers))
>

This changes the interface, so probably not good.  Could probably
implement under a new name.

> -
> -;; FIXME: We should change URL so that this can be done without an advice.
> -(defun oauth2--url-http-handle-authentication-hack (orig-fun &rest args)
> -  (if (not oauth--url-advice)
> -      (apply orig-fun args)
> -    (let ((url-request-method url-http-method)
> -          (url-request-data url-http-data)
> -          (url-request-extra-headers
> -           (oauth2-extra-headers url-http-extra-headers)))
> -      (oauth2-refresh-access (car oauth--token-data))
> -      (url-retrieve-internal (cdr oauth--token-data)
> -                             url-callback-function
> -                             url-callback-arguments)
> -      ;; This is to make `url' think it's done.
> -      (when (boundp 'success) (setq success t)) ;For URL library in Emacs<24.4.
> -      t)))                                      ;For URL library in Emacs≥24.4.
> -(advice-add 'url-http-handle-authentication :around
> -            #'oauth2--url-http-handle-authentication-hack)
> -
>  ;;;###autoload
> -(defun oauth2-url-retrieve-synchronously (token url &optional request-method request-data request-extra-headers)
> +(defun oauth2-url-retrieve-synchronously
> +    (token url &optional request-method request-data request-extra-headers)
>    "Retrieve an URL synchronously using TOKEN to access it.
>  TOKEN can be obtained with `oauth2-auth'."
> -  (let* ((oauth--token-data (cons token url)))
> -    (let ((oauth--url-advice t)         ;Activate our advice.
> -          (url-request-method request-method)
> -          (url-request-data request-data)
> -          (url-request-extra-headers
> -           (oauth2-extra-headers request-extra-headers)))
> -      (url-retrieve-synchronously url))))
> +  (let ((url-request-method request-method)
> +        (url-request-data request-data)
> +        (url-request-extra-headers
> +         (oauth2-extra-headers token request-extra-headers)))
> +    (url-retrieve-synchronously url)))
>

This seems to change the implementation based on the tweaked function
above.  So also not necessary.

>  ;;;###autoload
>  (defun oauth2-url-retrieve (token url callback &optional
> -                                  cbargs
> -                                  request-method request-data request-extra-headers)
> +                            cbargs
> +                            request-method request-data request-extra-headers)
>    "Retrieve an URL asynchronously using TOKEN to access it.
>  TOKEN can be obtained with `oauth2-auth'.  CALLBACK gets called with CBARGS
>  when finished.  See `url-retrieve'."
>    ;; TODO add support for SILENT and INHIBIT-COOKIES.  How to handle this in `url-http-handle-authentication'.
> -  (let* ((oauth--token-data (cons token url)))
> -    (let ((oauth--url-advice t)         ;Activate our advice.
> -          (url-request-method request-method)
> -          (url-request-data request-data)
> -          (url-request-extra-headers
> -           (oauth2-extra-headers request-extra-headers)))
> -      (url-retrieve url callback cbargs))))
> +  (let ((url-request-method request-method)
> +        (url-request-data request-data)
> +        (url-request-extra-headers
> +         (oauth2-extra-headers token request-extra-headers)))
> +    (url-retrieve url callback cbargs)))

Ditto.

>  (provide 'oauth2)

-- 
Regards,
Xiyue Deng