bug#72358: 29.4; oauth2.el improvements

[Philip Kaludercic <philipk@posteo.net>]

Xiyue Deng <manphiz@gmail.com> writes:

> Hi Philip,
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Xiyue Deng <manphiz@gmail.com> writes:
>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>>>> From: Xiyue Deng <manphiz@gmail.com>
>>>>> Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org>,  Björn Bidar
>>>>>  <bjorn.bidar@thaodan.de>,  rpluim@gmail.com,  72358@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@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@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@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@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@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@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@gmail.com> writes:

[...]

> Friendly ping.

Friendly pong.

-- 
	Philip Kaludercic on peregrine