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: Thu, 29 Aug 2024 14:14:17 +0000 Message-ID: <87le0fquva.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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="2270"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 72358@debbugs.gnu.org To: Xiyue Deng Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Aug 29 16:15:35 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 1sjfvy-0000OI-F2 for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 29 Aug 2024 16:15:34 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sjfvZ-0004So-7a; Thu, 29 Aug 2024 10:15:09 -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 1sjfvW-0004SR-Px for bug-gnu-emacs@gnu.org; Thu, 29 Aug 2024 10:15: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 1sjfvW-0001Mj-F0 for bug-gnu-emacs@gnu.org; Thu, 29 Aug 2024 10:15: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=/D3cER1QvWUT7zuwBeS1dAkLEZXdqmL4BB23Z6bswHo=; b=YNyvynSer/Yozuhsbr0mreJRgXGeScPDBptVyMy+hwkDpOh+e8tMde6ZrSADGPTu/RIUFsBLtnBSets4ZK/xUKxiGTKq7gmAej5WSmlD8vmOjLxSTnHojXxrC/dij3ovOEIHH5Qv5VaFVpQbZPN+94jM+9r/pBmUz1La3wotIouV26cH6jmtjt6xxcq6ywdmZcS6pj1aimA3EvkIrrzMUugjITSQr7QiLSkOm4RWreL24V97DLabXP5dhdkuOP3olkzrBg+AybtSF8TUym27bZRoTV1HnJQbzVay3b97VFRtNemySU78eHGcKUxpI7OBqq5ttWHYR4tyruKQWpF28Q==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1sjfwP-0000wj-NW for bug-gnu-emacs@gnu.org; Thu, 29 Aug 2024 10:16:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Philip Kaludercic Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 29 Aug 2024 14:16:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 72358 X-GNU-PR-Package: emacs Original-Received: via spool by 72358-submit@debbugs.gnu.org id=B72358.17249409283575 (code B ref 72358); Thu, 29 Aug 2024 14:16:01 +0000 Original-Received: (at 72358) by debbugs.gnu.org; 29 Aug 2024 14:15:28 +0000 Original-Received: from localhost ([127.0.0.1]:51803 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1sjfvr-0000va-NC for submit@debbugs.gnu.org; Thu, 29 Aug 2024 10:15:28 -0400 Original-Received: from mout02.posteo.de ([185.67.36.66]:38659) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1sjfvn-0000vG-50 for 72358@debbugs.gnu.org; Thu, 29 Aug 2024 10:15:26 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 7F994240101 for <72358@debbugs.gnu.org>; Thu, 29 Aug 2024 16:14:19 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1724940859; bh=xvHsiK2DtUpFMLLzq4S2rjfZA4VOvl4lCxzX1d0v7uc=; h=From:To:Cc:Subject:Autocrypt:OpenPGP:Date:Message-ID:MIME-Version: Content-Type:Content-Transfer-Encoding:From; b=Oo+/gciln3m+ZE/0oKO6P1sURM1aErArJn1JmpACkphscPKIuENGNWF4ZROMIoz5D TaKhkCli8OEWLEgHcxnO4tJ+MbiHE8tuPtMISnCVFeCyAhcvEBCs1Lxg4yibN4Z9ls Fc1ngGVEyaFW/lcKsvnwK11KYa2+CGS+N1y8ltKKUs9/cr5AZGYMsRE8I1GpBKwkaR JL1HHIoViCa/acZaU222V0blzwZHQSsHIG0m6LexNDnaxgfnlH7HM6POba2pl2EpHj ldxYXJeQcu6mHs/KnP3L8X2AbHHCSGoZLiQMWbH3OSf6ob17kKKuxCGgIKY7GTxXW8 wQ54Lm+vtBlng== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4WvjvZ3yn6z9rxB; Thu, 29 Aug 2024 16:14:18 +0200 (CEST) In-Reply-To: <87seuxbk6f.fsf@debian-hx90.lan> (Xiyue Deng's message of "Wed, 21 Aug 2024 15:11:36 -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=7126E1DE2F0CE35C770BED01F2C3CC513DB89F66; url="https://keys.openpgp.org/vks/v1/by-fingerprint/7126E1DE2F0CE35C770BED01F2C3CC513DB89F66"; 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:290934 Archived-At: Xiyue Deng writes: > Hi Philip, > > Philip Kaludercic writes: > >> Xiyue Deng writes: >> >>> Eli Zaretskii writes: >>> >>>>> From: Xiyue Deng >>>>> Cc: Thomas Fitzsimmons , Bj=C3=B6rn Bidar >>>>> , rpluim@gmail.com, 72358@debbugs.gnu.org >>>>> Date: Wed, 14 Aug 2024 01:23:19 -0700 >>>>>=20 >>>>> >> 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 w= hen >>>>> >> 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; >>>>>=20 >>>>> 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.) >>>>>=20 >>>>> > please describe your conclusion and post the patch that you-all agr= ee >>>>> > would solve the issues, and let's take it from there. >>>>>=20 >>>>> I actually only received comments from Robert and I have updated my >>>>> patches according in [1][2] (also attached in EOM). >>>>>=20 >>>>> [1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=3D72358#20 >>>>> [2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=3D72358#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=3D72358#20 >> >> ? > > Almost with a later update for patch 5. I am now attaching the latest > patches here to avoid any confusions. Thanks! > > --=20 > Xiyue Deng > > 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) > --- > 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=3D" (url-hexify-string client-id) > - "&response_type=3Dcode" > - "&redirect_uri=3D" (url-hexify-string (or redirect= -uri "urn:ietf:wg:oauth:2.0:oob")) > - (if scope (concat "&scope=3D" (url-hexify-string s= cope)) "") > - (if state (concat "&state=3D" (url-hexify-string s= tate)) ""))) > - (read-string "Enter the code your browser displayed: ")) > + (let ((url (concat auth-url > + (if (string-match-p "\?" auth-url) "&" "?") > + "client_id=3D" (url-hexify-string client-id) > + "&response_type=3Dcode" > + "&redirect_uri=3D" (url-hexify-string (or redirect-= uri "urn:ietf:wg:oauth:2.0:oob")) > + (if scope (concat "&scope=3D" (url-hexify-string sc= ope)) "") > + (if state (concat "&state=3D" (url-hexify-string st= ate)) "")))) > + (browse-url url) > + (read-string (concat "Follow the instruction on your default browser= , or " > + "visit:\n" url > + "\nEnter the code your browser displayed: ")))) LGTM. >=20=20 > (defun oauth2-request-access-parse () > "Parse the result of an OAuth request." > --=20 > 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=3Doffline' and `prompt=3Dconsent' 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=3Dcode" > "&redirect_uri=3D" (url-hexify-string (or redirect-= uri "urn:ietf:wg:oauth:2.0:oob")) > (if scope (concat "&scope=3D" (url-hexify-string sc= ope)) "") > - (if state (concat "&state=3D" (url-hexify-string st= ate)) "")))) > + (if state (concat "&state=3D" (url-hexify-string st= ate)) "") > + "&access_type=3Doffline" > + "&prompt=3Dconsent"))) 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 > --=20 > 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=3D" client-id > + "client_id=3D" (url-hexify-string client-id) > (when client-secret > - (concat "&client_secret=3D" client-secret)) > - "&code=3D" code > + (concat "&client_secret=3D" (url-hexify-string client-se= cret))) > + "&code=3D" (url-hexify-string code) > "&redirect_uri=3D" (url-hexify-string (or redirect-uri "urn= :ietf:wg:oauth:2.0:oob")) > "&grant_type=3Dauthorization_code")))) > (make-oauth2-token :client-id client-id > --=20 > 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 > Date: Sun, 28 Jul 2024 03:00:04 -0700 > Subject: [PATCH 4/6] Support storing data for multiple accounts of the sa= me > 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-acces= s'." > :group 'oauth2 > :type 'file) >=20=20 > -(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))) >=20=20 > ;;;###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 > --=20 > 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) >=20=20 > (defvar url-http-data) > (defvar url-http-method) > @@ -53,6 +54,14 @@ > :link '(url-link :tag "Savannah" "https://git.savannah.gnu.org/cgit/em= acs/elpa.git/tree/?h=3Dexternals/oauth2") > :link '(url-link :tag "ELPA" "https://elpa.gnu.org/packages/oauth2.htm= l")) >=20=20 > +(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.=20=20 > + (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." >=20=20 > (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)))) >=20=20 > (cl-defstruct oauth2-token > --=20 > 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? 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. --=20 Philip Kaludercic on peregrine