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 #128 received at 72358 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: Xiyue Deng <manphiz <at> gmail.com> Cc: 72358 <at> debbugs.gnu.org Subject: Re: bug#72358: 29.4; oauth2.el improvements Date: Thu, 29 Aug 2024 14:14:17 +0000
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)
> ---
> 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.
> 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?
> (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.
>
>
> 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'. Also, why is this not a user option?
> +
> +(defun oauth2--do-debug (&rest msg)
> + "Output debug messages when `oauth2-debug' is enabled."
> + (if oauth2-debug
I'd use `when' here.
> + (apply #'message msg)))
... and perhaps prefix the message with a string like "[oauth2] ".
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.)
> +
> (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.
> 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?
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.
--
Philip Kaludercic on peregrine
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.