Package: emacs;
Reported by: Xiyue Deng <manphiz <at> gmail.com>
Date: Tue, 30 Jul 2024 02:20:01 UTC
Severity: normal
Found in version 29.4
Done: Philip Kaludercic <philipk <at> posteo.net>
Bug is archived. No further changes may be made.
Message #139 received at 72358-done <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: Xiyue Deng <manphiz <at> gmail.com> Cc: 72358-done <at> debbugs.gnu.org Subject: Re: bug#72358: 29.4; oauth2.el improvements Date: Fri, 30 Aug 2024 07:09:21 +0000
Xiyue Deng <manphiz <at> gmail.com> writes: > Hi Philip, > > Thanks for the review! Please see my replies below inline. > [...] >>> From 2b9e50cb0948e0b4f28883042109994ffa295d3d Mon Sep 17 00:00:00 2001 >>> From: Xiyue Deng <manphiz <at> 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. 1+ >>> --- >>> 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 <at> 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. I think that double or single quotes are both fine. >>> 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. 1+ >>> (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 <at> 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. I am glad to hear that this was possible. >>> >>> >>> From e801af578e63c7e333e668bdfef05e4cf0802582 Mon Sep 17 00:00:00 2001 >>> From: Xiyue Deng <manphiz <at> 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 <at> 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. No, some packages use user options, others regular variables so I guess it is up to the person with the strongest opinion on the topic to decide (which isn't me in this case). >>> + >>> +(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. No complains from my side, I find it rather elegant! >> 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__'. I know of nothing. >>> + >>> (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. 1+ >>> data)))) >>> >>> (cl-defstruct oauth2-token >>> -- >>> 2.39.2 >>> >>> >>> From e8735da21ac82b0698edad1796ddf4a1b8eb4bb2 Mon Sep 17 00:00:00 2001 >>> From: Xiyue Deng <manphiz <at> 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 <at> gmail.com> writes: >> >> [...] >> >>> Friendly ping. >> >> Friendly pong. > > The updated patches are attached. Thanks again! OK, this looks very good. I'll apply the changes and am closing the report. Thanks, -- Philip Kaludercic on peregrine
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.