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.
View this message in rfc822 format
From: Xiyue Deng <manphiz <at> gmail.com> To: Philip Kaludercic <philipk <at> posteo.net> Cc: 72358 <at> debbugs.gnu.org Subject: bug#72358: 29.4; oauth2.el improvements Date: Thu, 29 Aug 2024 16:54:41 -0700
[Message part 1 (text/plain, inline)]
Hi Philip, Thanks for the review! Please see my replies below inline. Philip Kaludercic <philipk <at> posteo.net> writes: > Xiyue Deng <manphiz <at> gmail.com> writes: > >> Hi Philip, >> >> Philip Kaludercic <philipk <at> posteo.net> writes: >> >>> Xiyue Deng <manphiz <at> gmail.com> writes: >>> >>>> Eli Zaretskii <eliz <at> gnu.org> writes: >>>> >>>>>> From: Xiyue Deng <manphiz <at> gmail.com> >>>>>> Cc: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>, Björn Bidar >>>>>> <bjorn.bidar <at> thaodan.de>, rpluim <at> gmail.com, 72358 <at> 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 >> >> 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. >> --- >> 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. >> 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. >> (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. >> >> >> 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. >> + >> +(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. > 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__'. >> + >> (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. >> 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! -- Xiyue Deng
[0001-Show-full-authentication-URL-to-let-user-choose-how-.patch (text/x-diff, attachment)]
[0002-Add-parameters-required-by-Google-OAuth2-to-get-refr.patch (text/x-diff, attachment)]
[0003-Encode-URL-when-requesting-access.patch (text/x-diff, attachment)]
[0004-Support-storing-data-for-multiple-accounts-of-the-sa.patch (text/x-diff, attachment)]
[0005-Add-debug-messages-and-provide-an-option-variable-fo.patch (text/x-diff, attachment)]
[0006-Add-NEWS-file-to-document-the-changes-to-plstore-id-.patch (text/x-diff, attachment)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.