Package: emacs;
Reported by: Stefan Kangas <stefan <at> marxist.se>
Date: Tue, 14 Dec 2021 02:40:01 UTC
Severity: wishlist
Tags: patch
Message #30 received at 52476 <at> debbugs.gnu.org (full text, mbox):
From: Xiyue Deng <manphiz <at> gmail.com> To: Stefan Kangas <stefankangas <at> gmail.com>, 52476 <at> debbugs.gnu.org Cc: julien <at> danjou.info, Aleksandr Vityazev <avityazev <at> posteo.org> Subject: Re: bug#52476: Fwd: [ELPA/oauth2] Request for patches review Date: Sun, 04 May 2025 17:10:04 -0700
[Message part 1 (text/plain, inline)]
Stefan Kangas <stefankangas <at> gmail.com> writes: > Xiyue Deng <manphiz <at> gmail.com> writes: > >> Hi Stefan, >> >> Stefan Kangas <stefankangas <at> gmail.com> 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 <avityazev <at> posteo.org> > ;; Copyright (C) 2011-2021 Free Software Foundation, Inc > > ;; Author: Julien Danjou <julien <at> danjou.info> > ;; 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
[signature.asc (application/pgp-signature, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.