From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Xiyue Deng Newsgroups: gmane.emacs.bugs Subject: bug#72358: 29.4; oauth2.el improvements Date: Wed, 21 Aug 2024 15:11:36 -0700 Message-ID: <87seuxbk6f.fsf@debian-hx90.lan> 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> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="5037"; mail-complaints-to="usenet@ciao.gmane.io" Cc: bjorn.bidar@thaodan.de, Eli Zaretskii , fitzsim@fitzsim.org, rpluim@gmail.com, 72358@debbugs.gnu.org To: Philip Kaludercic Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Aug 22 00:13:43 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 1sgtaI-0001C1-4T for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 22 Aug 2024 00:13:42 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sgtZz-0004lt-Bn; Wed, 21 Aug 2024 18:13:26 -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 1sgtZt-0004lW-8z for bug-gnu-emacs@gnu.org; Wed, 21 Aug 2024 18:13:17 -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 1sgtZs-0002md-VK for bug-gnu-emacs@gnu.org; Wed, 21 Aug 2024 18:13:17 -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=gTJMdzjRa+awU9XAORHbEMTssSlSkDPO07WtwlsVIio=; b=hdgfS4Hgfl7qzFLu02keayQMaSp+GYKBKmX3dhEtbFf1gf/SIyiN6Tb//M+5Zu084iL1Jir0Vc+ruNnkGa6iXyuCLU/6pMvxOBw9Gu7BDLszFJ9WAWk2u384MW5YoJs5bzsKPOZ0svxJUfeTYJ4CXPBNc2O7IVD4qO4iXNK7AboLIdDT8/0rT1mgrlFlKtpBgGLD/A68u8NciIhcNfOrVb8KydsZ9Q6vbEotN19B/4fD1VyhWGlzzPbEFXkSXjOH76sUmJvTDiOkqZ7KUp1pC8MFN3QEGgZdpkRjvATZOwkyvoYRX0mIs0e0MelYz2I7UDpxuNqlt/ScamDtb0WcnQ==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1sgtab-0001ds-RQ for bug-gnu-emacs@gnu.org; Wed, 21 Aug 2024 18:14:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Xiyue Deng Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 21 Aug 2024 22:14: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.17242784176269 (code B ref 72358); Wed, 21 Aug 2024 22:14:01 +0000 Original-Received: (at 72358) by debbugs.gnu.org; 21 Aug 2024 22:13:37 +0000 Original-Received: from localhost ([127.0.0.1]:36427 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1sgtaC-0001d2-8K for submit@debbugs.gnu.org; Wed, 21 Aug 2024 18:13:37 -0400 Original-Received: from mail-pl1-f173.google.com ([209.85.214.173]:48580) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1sgta8-0001ch-12 for 72358@debbugs.gnu.org; Wed, 21 Aug 2024 18:13:35 -0400 Original-Received: by mail-pl1-f173.google.com with SMTP id d9443c01a7336-20203988f37so1548975ad.1 for <72358@debbugs.gnu.org>; Wed, 21 Aug 2024 15:12:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1724278301; x=1724883101; darn=debbugs.gnu.org; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=gTJMdzjRa+awU9XAORHbEMTssSlSkDPO07WtwlsVIio=; b=jwSKWNv/DhRBif+jX2HI1Msfd/SgcI1LzxqAXg1I701N8JAkKmT5Aq98LbH48yliNY G3irWZHMdRlZ6RxJyvn3jAZ5vXBfucwsOcVT4Jo8jA4Q0F33IbsQh1PnP74SM5bRkpzy zKy10sir2hUjuaQiYSOG7s9QgotRwIPdfv0lLCg2oTA9Z+ZKw4valFS4nsU24q3679h5 qJO9NrOR3EkNeIhRb0DNkVXo4RFcX1ZqTAJeBeaPErza+czfSw9/yTOzm2Vg+ATmCfYR Jlnn1kEYqYgSULKncd6zogqwIU0s1lVtsfZ8zn091ZQXK9uVdK47uQmYPoFv3LUi+aEM 4RBg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724278301; x=1724883101; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=gTJMdzjRa+awU9XAORHbEMTssSlSkDPO07WtwlsVIio=; b=MyQs4rZZoYKqWtuuBgiv2c7OOrlwVkA7LsdNgwGn7l+SLYUkJ/qi9R7e03mrmbLE55 A9CH/781On2IyTbFdAg7cM8o6RVBrGrlUU6ew7HkDo29e5UOr5mfkridylWW9s8rikQ1 1bM22qtrKAdYdJnYwyI+FfNtbfEbnrpoOPY1p023qhMAmxD9Tf/slqSILK7OPPLNywfq nYmN+4qRMAz+Hy0a12tMP7c5VPGd4uPf2LIVxjGPvV6mMhUY//965CXpA0vf4hQeIxWO 9IL8kFjPoNlKDu9gv5bpiNpReZTaAtCtXQ9A0WqMMpIfSU2xgSLML3nNeeyF65O3shRp 853A== X-Forwarded-Encrypted: i=1; AJvYcCXd9AMNq1p6gSwA0b7BWi6YdQIpiRMA0GADYZGhyNEqbObIyyUnBNSnlBoYfYGlzj5RoPQZLw==@debbugs.gnu.org X-Gm-Message-State: AOJu0YwFhJuIfd4ew69JseBdRKB+7TZ8mQLs5faE5I5TqEW/BN3BOLL3 DJRGxt5Q95aDmyeJeIuvdp3cWxiWaSz8Xx/T9c2nVqdk/zNZXRADEAJMYMA5 X-Google-Smtp-Source: AGHT+IHne1TcTvnM1iRyJ0ykjsD9LcS/hE/cGEarU1RV52d0j2A2kpmBzA0f0wEtMx9d3T7BYX3q+g== X-Received: by 2002:a17:902:ce86:b0:201:f83e:c267 with SMTP id d9443c01a7336-20367d32b6cmr39322905ad.5.1724278300226; Wed, 21 Aug 2024 15:11:40 -0700 (PDT) Original-Received: from debian-hx90 (syn-076-094-249-045.res.spectrum.com. [76.94.249.45]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2038557ee79sm798415ad.82.2024.08.21.15.11.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Aug 2024 15:11:39 -0700 (PDT) In-Reply-To: <87seuxacij.fsf@posteo.net> (Philip Kaludercic's message of "Wed, 21 Aug 2024 19:42:28 +0000") 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:290505 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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/re= views >>>> >> I am expecting from the co-maintainers? Please also let me know wh= en >>>> >> it's time to ask for merging and requesting a new tagged release. >>>> > >>>> > ?? The last message in this discussion was just yesterday evening, a= nd >>>> > 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 agree >>>> > 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 --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0001-Show-full-authentication-URL-to-let-user-choose-how-.patch >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. --- 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: ")))) (defun oauth2-request-access-parse () "Parse the result of an OAuth request." -- 2.39.2 --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0002-Add-parameters-required-by-Google-OAuth2-to-get-refr.patch >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 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"))) (browse-url url) (read-string (concat "Follow the instruction on your default browser, or " "visit:\n" url -- 2.39.2 --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0003-Encode-parameters-when-requesting-access.patch >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 --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0004-Support-storing-data-for-multiple-accounts-of-the-sa.patch >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 --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0005-Add-debug-messages-and-provide-a-switch-variable-for.patch >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.") + +(defun oauth2--do-debug (&rest msg) + "Output debug messages when `oauth2-debug' is enabled." + (if oauth2-debug + (apply #'message msg))) + (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)) data)))) (cl-defstruct oauth2-token -- 2.39.2 --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0006-Add-NEWS-file-to-document-the-changes-to-plstore-id-.patch >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. -- 2.39.2 --=-=-=--