bug#72358: 29.4; oauth2.el improvements

[Philip Kaludercic <philipk@posteo.net>]

Xiyue Deng <manphiz@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@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@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@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@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'.
>
> 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@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@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