From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.bugs Subject: bug#72358: 29.4; oauth2.el improvements Date: Fri, 30 Aug 2024 07:09:21 +0000 Message-ID: <87o75av65a.fsf@posteo.net> References: <87mslz8yzk.fsf@debian-hx90.lan> <87frrr725m.fsf@gmail.com> <87ed7a8ye6.fsf@debian-hx90.lan> <87jzh2vt3y.fsf@gmail.com> <87frrp6ch8.fsf@debian-hx90.lan> <87ttg349yd.fsf@debian-hx90.lan> <86r0b6up8p.fsf@gnu.org> <87ikw4qdvx.fsf@debian-hx90.lan> <86plqbfzat.fsf@gnu.org> <877ccjecnc.fsf@debian-hx90.lan> <86wmkj8o29.fsf@gnu.org> <87wmk9busk.fsf@debian-hx90.lan> <87seuxacij.fsf@posteo.net> <87seuxbk6f.fsf@debian-hx90.lan> <87le0fquva.fsf@posteo.net> <87r0a64y98.fsf@debian-hx90.lan> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="2610"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 72358-done@debbugs.gnu.org To: Xiyue Deng Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Aug 30 09:10:29 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1sjvm8-0000V7-ET for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 30 Aug 2024 09:10:28 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sjvlo-0006IT-K9; Fri, 30 Aug 2024 03:10:08 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sjvlm-0006IC-Fk for bug-gnu-emacs@gnu.org; Fri, 30 Aug 2024 03:10:07 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1sjvlm-0006bf-4g for bug-gnu-emacs@gnu.org; Fri, 30 Aug 2024 03:10:06 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=MIME-Version:Date:References:In-Reply-To:From:To:Subject; bh=EruJMNfb3LScs5cLQwAuDk1T2YrTYEmPCQqR4Qsf6ow=; b=Kq8HBVdc+yx2SFKrwCamgZ9lYPMNA1BSbxNtzj3fG/2B36LcalmKWjDPzmOhss5YFUB6FDmmy79pRdjqEQ/5i8rC9tV/oPkKZr9sbMIeFYN7b54aTyuvj6zfrdmjs6m1x/4wUTC6+3oCIsao9TWW1D6k23R37qjdS6MXSVm34LBtNmkS8gvubjzauNVXGI1GH9bzv7q0f2fsON9UZCiK6y1ZlFxOrU7tJUH0yoRnPf00q6ZksAdifivtKAb6zMWcz5jOfrN/6GAFfw+szp65jn1vL6WaK64csyXhNzGjeJlYeLGCu+8TbWBW239SE4AB8RWzUWZRhxQIJX6QFiPKNg==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1sjvmg-0002PN-GC for bug-gnu-emacs@gnu.org; Fri, 30 Aug 2024 03:11:02 -0400 Resent-From: Philip Kaludercic Original-Sender: "Debbugs-submit" Resent-To: bug-gnu-emacs@gnu.org Resent-Date: Fri, 30 Aug 2024 07:11:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: cc-closed 72358 X-GNU-PR-Package: emacs Mail-Followup-To: 72358@debbugs.gnu.org, philipk@posteo.net, manphiz@gmail.com Original-Received: via spool by 72358-done@debbugs.gnu.org id=D72358.17250018329206 (code D ref 72358); Fri, 30 Aug 2024 07:11:02 +0000 Original-Received: (at 72358-done) by debbugs.gnu.org; 30 Aug 2024 07:10:32 +0000 Original-Received: from localhost ([127.0.0.1]:52412 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1sjvmB-0002OP-AP for submit@debbugs.gnu.org; Fri, 30 Aug 2024 03:10:32 -0400 Original-Received: from mout02.posteo.de ([185.67.36.66]:58437) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1sjvm8-0002O7-0s for 72358-done@debbugs.gnu.org; Fri, 30 Aug 2024 03:10:29 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id A8DF7240101 for <72358-done@debbugs.gnu.org>; Fri, 30 Aug 2024 09:09:23 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1725001763; bh=yucEnRqggPNlWi3sklagg8KS00B9rWSWmCGdh8b/LMM=; h=From:To:Cc:Subject:Autocrypt:OpenPGP:Date:Message-ID:MIME-Version: Content-Type:From; b=VKOmY96uCBSne5wvQ+zf+qTQihvSdjL5EMpZAIP5rXdS2UNJrqjxhedvFnK9oNe35 Md1TPPnWKaVG+590yE/8in9oP7/lVjU73FX9BCyFi8fLGT11x0yBGuRfMk80VhTVBr UOi5VarXxDvtm4xoyBftQWt3WPNQG9v6T8SuikrIajuv3Z8i4VvQ9adHqB1TCIvHZe tkp6gjXfNU2YbM2Rr80hKRCkKqbx6PEf2SMwRn5YKTThIQ/Y0RheJcloxJ56WmVKoe OmGJwkKqbOtUEmmmbQMs1dh2iSheZ9vDpYH/2tSQEnEI8twTpWT47+VP0UZ6FbkCdW TSRA2++iQe2sw== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4Ww8Qp4fJJz9rxF; Fri, 30 Aug 2024 09:09:22 +0200 (CEST) In-Reply-To: <87r0a64y98.fsf@debian-hx90.lan> (Xiyue Deng's message of "Thu, 29 Aug 2024 16:54:41 -0700") Autocrypt: addr=philipk@posteo.net; keydata= mDMEZBBQQhYJKwYBBAHaRw8BAQdAHJuofBrfqFh12uQu0Yi7mrl525F28eTmwUDflFNmdui0QlBo aWxpcCBLYWx1ZGVyY2ljIChnZW5lcmF0ZWQgYnkgYXV0b2NyeXB0LmVsKSA8cGhpbGlwa0Bwb3N0 ZW8ubmV0PoiWBBMWCAA+FiEEDg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwMFCQHhM4AFCwkI BwIGFQoJCAsCBBYCAwECHgECF4AACgkQ8xYDWXahwulikAEA77hloUiSrXgFkUVJhlKBpLCHUjA0 mWZ9j9w5d08+jVwBAK6c4iGP7j+/PhbkxaEKa4V3MzIl7zJkcNNjHCXmvFcEuDgEZBBQQhIKKwYB BAGXVQEFAQEHQI5NLiLRjZy3OfSt1dhCmFyn+fN/QKELUYQetiaoe+MMAwEIB4h+BBgWCAAmFiEE Dg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwwFCQHhM4AACgkQ8xYDWXahwukm+wEA8cml4JpK NeAu65rg+auKrPOP6TP/4YWRCTIvuYDm0joBALw98AMz7/qMHvSCeU/hw9PL6u6R2EScxtpKnWof z4oM OpenPGP: id=philipk@posteo.net; url="https://keys.openpgp.org/vks/v1/by-email/philipk@posteo.net"; preference=signencrypt X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:290956 Archived-At: Xiyue Deng 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 >>> 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 >>> 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 >>> 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 >>> 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 >>> 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 >>> 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 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