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#72992: 29.4; towards xoauth2 support in Emacs Date: Sun, 22 Sep 2024 09:34:08 +0000 Message-ID: <87tte89gmn.fsf@posteo.net> References: <87h6ayfo87.fsf_-_@debian-hx90.lan> <87r09i14cm.fsf@posteo.net> <87msk5pji2.fsf@debian-hx90.lan> <87o74ldpby.fsf@posteo.net> <87sets416y.fsf@debian-hx90.lan> 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="34957"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 72992@debbugs.gnu.org To: Xiyue Deng Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Sep 22 11:34:55 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 1ssIzX-0008wJ-AX for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 22 Sep 2024 11:34:55 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ssIzL-00073U-6l; Sun, 22 Sep 2024 05:34:43 -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 1ssIzI-000731-OB for bug-gnu-emacs@gnu.org; Sun, 22 Sep 2024 05:34:40 -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 1ssIzI-0004cV-F1 for bug-gnu-emacs@gnu.org; Sun, 22 Sep 2024 05:34:40 -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=nRdON2igO65BVlxpP1/z2bs3PCHoH9DBUY6KkcWLFzk=; b=Ll74ytp5PrViuKlU0i7Xn5rbOgm/JRDJhhJe+v1K94eI9a/6ajFn8Oq4OnMNJGOaSZcS+3EVz15lz2fUmHaW2zMy7QOhd3atAV/cO4QwuessE1LCEa3+D+kx4J7wy6w0DrUPAHKdRgXcECH+/aCkIiP0Uag9cJCJNnDckcaoQCl5NpU9LijY+deER4sk3uLuxpto4NLQMS4E+o0hl3Psl4rLqTc6CLJMakw8BIy2dLNLtnbLr7YcZU8hoHYQnPNLXl/MX0YisnKkqZxWR9+ZsgbOAk5WlYOUVEqxtytkG110YQyeXRqMGq4d2VDvVJas0MuKF+XVAl9rf/18c7aNAw==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1ssIzd-0006eR-LX for bug-gnu-emacs@gnu.org; Sun, 22 Sep 2024 05:35: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: Sun, 22 Sep 2024 09:35:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 72992 X-GNU-PR-Package: emacs Original-Received: via spool by 72992-submit@debbugs.gnu.org id=B72992.172699768225531 (code B ref 72992); Sun, 22 Sep 2024 09:35:01 +0000 Original-Received: (at 72992) by debbugs.gnu.org; 22 Sep 2024 09:34:42 +0000 Original-Received: from localhost ([127.0.0.1]:40884 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ssIzJ-0006dj-Mk for submit@debbugs.gnu.org; Sun, 22 Sep 2024 05:34:42 -0400 Original-Received: from mout02.posteo.de ([185.67.36.66]:59519) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ssIzF-0006dO-Dm for 72992@debbugs.gnu.org; Sun, 22 Sep 2024 05:34:40 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 26E15240101 for <72992@debbugs.gnu.org>; Sun, 22 Sep 2024 11:34:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1726997649; bh=wAH9PYKp/F8EPUjpKxGwWindYErqMCWVS4d99oifBiQ=; h=From:To:Cc:Subject:Autocrypt:OpenPGP:Date:Message-ID:MIME-Version: Content-Type:From; b=pTikAvXL7y2ZGkSpHUnEf1ge29tW8dzwGV3tEiM26fJ/m4srgjxc964Djr7FHL3b5 A8vL7WcetlX93z9K8afZDkOFiAFXmBePTSLy+mMsUjOuhjgQ7ZpgvCht9ivYrMivjH 7DHARAr0r2H5PsfGJeB33ulCwHswO7DIHUGWLks9OTQVIDJNn9APf87WDcnqIa39mi w/cCdUPEVeK9SVnVb5gkicGOo3p3aShhybqsthXyZeHMYgROswfl0HSLSR+fc/p9Wl dxyjZdLWLDvtyugdtLZcB/yfJRcX5vj9m7eiz/3bEBE1pQQ+OVz5l/wRvu0Yf13nff BRLxvnMCk1hxA== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4XBLYD2sFVz9rxK; Sun, 22 Sep 2024 11:34:08 +0200 (CEST) In-Reply-To: <87sets416y.fsf@debian-hx90.lan> (Xiyue Deng's message of "Sun, 22 Sep 2024 00:06:29 -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:292227 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Xiyue Deng writes: > Hi Philip, > > Philip Kaludercic writes: > >> Xiyue Deng writes: >> >>> Philip Kaludercic writes: >>> >>>> Xiyue Deng writes: >>>> >>>>> Now that bug#72358 is done, as promised, I'm posting my plugin for >>>>> auth-sources that enables oauth2 handling which you can find on >>>>> Gitlab[1] (also attached). >>>> >>>> Once again I just want to be sure: When you say "plugin", you mean >>>> package, right? >>> >>> Yes, though it's not really an independent package but a "plugin" for >>> auth-source, a.k.a. a hack (the advice) to make auth-source to work with >>> xoauth2. >> >> Just to clarify: When I say package, I mean something to add to ELPA. >> > > Ah in that regard yes. > >>>> You are proposing to add this to GNU ELPA? >>> >>> Actually I would like to see which of my proposed changes to auth-source >>> is acceptable and update auth-source in core accordingly.=20=20 >> >> Sure it's acceptable, but in that case it would better to submit a patch >> modifying. auth-source.el >> >>> I think >>> Stefan's reply gave some suggestions in this regard and I'll follow-up >>> in a reply there.=20=20 >> >> I just want to second Stefan's point that some clarification as to what >> xoauth2 is. >> > > Updated the comments section with this info. Great, that explains it well! >>> Meanwhile, it may still worth adding this package >>> to ELPA to support older Emacs versions if desired. >> >> In that case it might be better not to merge your changes into >> auth-source.el directly, as that would make it more difficult to >> automatically pull your changes out of the core to ELPA. >> >> An alternative is that ELPA mirrors your repository, and then we >> manually synchronise the changes into the core, whenever there is a new >> release. >> > > I was thinking making it only for Emacs <30 if the auth-source side > changes are upstreamed for 31. Similar to "docker-tramp" which is only > for EMacs <28. The issue here is that tramp is developed outside of Emacs and synchronised manually back into the core/automatically on ELPA, while auth-source is currently only in the core and not distributed on ELPA. If this remains a separate file, we could easily add it to ELPA, but I don't know what the preference is there. >> >> [...] >> >>>>> (let ((auth (plist-get auth-data :auth))) >>>>> (when (and auth >>>>> (stringp auth) >>>>> (string=3D auth "xoauth2")) >>>> >>>> You can simplify the check by just doing (equal auth "xoauth2"), as th= is >>>> implies all of the above (if it is `equal' to a string, it must be a >>>> string and hence also non-nil). >>>> >>> >>> Done. Nice tip! Coming from strong-typed languages I always want to do >>> type-checks first in fear of any aborting error :) >> >> If you want strong typing, then string=3D is the right thing to use, >> because if you want to assume that auth is always a string, then an >> error will be signalled. That being said, if auth has the type "Maybe >> String", then checking the values explicitly or implicitly using equal >> is the right approach. >> > > Ack. Thanks for the tip! > >> >> [...] >> >>>>> (auth-source-do-trivia "Using oauth2 to auth and store = token...") >>>>> (let ((token (oauth2-auth-and-store >>>>> auth-url token-url scope client-id client= -secret >>>>> redirect-uri state))) >>>>> (auth-source-do-trivia "oauth2 token: %s" (pp-to-stri= ng token)) >>>>> (auth-source-do-trivia "Refreshing token...") >>>>> (oauth2-refresh-access token) >>>>> (auth-source-do-trivia "oauth2 token after refresh: %= s" >>>>> (pp-to-string token)) >>>>> (let ((access-token (oauth2-token-access-token token)= )) >>>>> (auth-source-do-trivia >>>>> "Updating :secret with access-token: %s" access-to= ken) >>>>> (plist-put auth-data :secret access-token)))))) >>>> >>>> The documentation for plist-put warns: >>>> >>>> The new plist is returned; >>>> use =E2=80=98(setq x (plist-put x prop val))=E2=80=99 to be sure to = use the new value. >>>> The PLIST is modified by side effects. >>>> >>>> Alternatively, you should also be able to do: >>>> >>>> (setf (plist-get auth-data :secret) access-token) >>>> >>> >>> Ah didn't know this as I learned the usage of plist-put from searching. >>> Changed to your `setq' version. Though I'd also expect that the side >>> effect is not going away anytime soon either ;) >> >> I am not sure what you mean? The crux of the issue is demonstrated >> here: >> >> (let (plist) >> (list (plist-put plist :foo 1) plist)) >> ;; ((:foo 1) nil) >> >> I.e. the plist was not modified, because there was no cons-cell to >> modify. >> > > I see. Thanks for the explanation. Looks like the side effect worked > for me because auth-data already had data in it. Probably, but that's not the kind of thing I want to rely on. >> >> [...] >> >>>>> #'auth-source-xoauth2-plugin--search-backends)) >>>> >>>> I would recommend turning this into a global minor mode instead, so th= at >>>> it is easy to disable, if a user just wants to try it out. >>>> >>> >>> This is an interesting suggestion and sounds like a good idea. Though >>> as a matter of fact the oauth2 support in auth-source in Emacs core >>> actually doesn't work without those hack as of now, so I don't think >>> it's of interest to support turning off.=20=20 >> >> I regard it as a matter of good style to allow the user to disable >> anything then can enable, if anything then just to allow better >> experimentation. >> > > You actually convinced me. Making it a minor mode also enables a user > to disable it temporarily if it causes any issues. It took me a while > to convert it. Please help take another look. Looks good. >>> But of course it would be >>> great if auth-source can be changed to support all this out-of-the-box. >>> Will continue the discussion in my reply to Stefan. >> >> Ack. >> >>> I have updated the source code on GitLab[1] based on your review. >>> Please check it out. Thanks very much! >> >> For anyone following the thread, it seem the footnote was missing: >> >> [1]https://gitlab.com/xiyueden/auth-source-xoauth2-plugin/-/blob/main/au= th-source-xoauth2-plugin.el >> >> Watch out, in >> >> (unless (memq 'xoauth2 smtpmail-auth-supported) >> (push 'smtpmail-auth-supported 'xoauth2)) >> >> the push expression is malformed, as 'xoauth2 is not a place. I'm >> guessing that you want to write >> >> (... (push 'xoauth2 smtpmail-auth-supported)) >> > > Thanks! Fixed. > >> Also, checkdoc complains about >> `auth-source-xoauth2-plugin--search-backends's docstring. I'd try to >> address the issues it mentions. >> > > Also fixed. Thanks! > >> The (and auth (equal auth "xoauth2")) can be further simplified to just >> (equal auth "xauth2"), as if auth is equal to "xauth2" is cannot be nil. >> > > Ack and simplified. > > The GitLab repo[1] is updated accordingly. PTAL. TIA! Looks good, just a few "soft" comments I can find: --=-=-= Content-Type: text/plain Content-Disposition: inline diff --git a/auth-source-xoauth2-plugin.el b/auth-source-xoauth2-plugin.el index cdcc9e7..caf5baf 100644 --- a/auth-source-xoauth2-plugin.el +++ b/auth-source-xoauth2-plugin.el @@ -41,7 +41,7 @@ ;; or with use-package: ;; (use-package auth-source-xoauth2-plugin -;; :config +;; :custom ;; (auth-source-xoauth2-plugin-mode t)) ;; After enabling, smtpmail should be supported. To enable this in Gnus @@ -107,13 +107,13 @@ expected that `token_url', `client_id', `client_secret', and (when (equal auth "xoauth2") (auth-source-do-debug ":auth set to `xoauth2'. Will get access token.") - (map-let ((:auth-url auth-url) - (:token-url token-url) - (:scope scope) - (:client-id client-id) - (:client-secret client-secret) - (:redirect-uri redirect-uri) - (:state state)) + (map-let (:auth-url ;You can simplify the `map-let' + :token-url ;expression if they keys match + :scope ;the bindings like they do here. + :client-id ;Perhaps you can use the additional + :client-secret ;space to document what the keys + :redirect-uri ;are for? + :state) auth-data (auth-source-do-debug "Using oauth2 to auth and store token...") (let ((token (oauth2-auth-and-store @@ -138,8 +138,7 @@ expected that `token_url', `client_id', `client_secret', and res))) (defvar auth-source-xoauth2-plugin--enabled-xoauth2-by-us nil - "Used for tracking whether xoauth2 in smtpmail-auth-supported is -set by us.") + "Non-nil means `smtpmail-auth-supported' was set by us.") (defun auth-source-xoauth2-plugin--enable () "Enable auth-source-xoauth2-plugin." @@ -154,17 +153,17 @@ set by us.") "Disable auth-source-xoauth2-plugin." (when (and auth-source-xoauth2-plugin--enabled-xoauth2-by-us (memq 'xoauth2 smtpmail-auth-supported)) - (delete 'xoauth2 smtpmail-auth-supported) + (setq smtpmail-auth-supported (delq 'xoauth2 smtpmail-auth-supported)) (setq auth-source-xoauth2-plugin--enabled-xoauth2-by-us nil)) (advice-remove #'auth-source-search-backends #'auth-source-xoauth2-plugin--search-backends)) +;;;###autoload (define-minor-mode auth-source-xoauth2-plugin-mode "Toggle auth-source-xoauth2-plugin-mode. Enable auth-source-xoauth2-plugin-mode to use xoauth2 authentications for emails." - :lighter nil :global t (if auth-source-xoauth2-plugin-mode (auth-source-xoauth2-plugin--enable) --=-=-= Content-Type: text/plain > >>>>> >>>>> (provide 'auth-source-xoauth2-plugin) >>>>> >>>>> ;;; auth-source-xoauth2-plugin.el ends here >>>> >>>> -- >>>> Philip Kaludercic on siskin >> >> -- >> Philip Kaludercic on siskin > > [1] https://gitlab.com/xiyueden/auth-source-xoauth2-plugin -- Philip Kaludercic on siskin --=-=-=--