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#72992: 29.4; towards xoauth2 support in Emacs Date: Sun, 22 Sep 2024 15:00:29 -0700 Message-ID: <87ikun4ade.fsf@debian-hx90.lan> References: <87h6ayfo87.fsf_-_@debian-hx90.lan> <87r09i14cm.fsf@posteo.net> <87msk5pji2.fsf@debian-hx90.lan> <87o74ldpby.fsf@posteo.net> <87sets416y.fsf@debian-hx90.lan> <87tte89gmn.fsf@posteo.net> 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="1482"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Notmuch/0.38.3 (https://notmuchmail.org) Emacs/29.4 (x86_64-pc-linux-gnu) Cc: 72992@debbugs.gnu.org To: Philip Kaludercic Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Sep 23 00:03:01 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 1ssUfV-0000Cr-2R for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 23 Sep 2024 00:03:01 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ssUfD-0006rz-Ph; Sun, 22 Sep 2024 18:02: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 1ssUfB-0006ra-Os for bug-gnu-emacs@gnu.org; Sun, 22 Sep 2024 18:02:42 -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 1ssUfB-0001Ub-EQ for bug-gnu-emacs@gnu.org; Sun, 22 Sep 2024 18:02:41 -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=nQ1BKqPuq58NZdQuHiYZf6Pq9eNhQ0dSBFI/fEM316Y=; b=IoJTiDIypbW6vLx25xJlrMqamcsD7sTPc2CqJZVYodlXPYiGnP57KqWFkcyk/yikxqG9c9zDjBoYS+3/q0IxdWk4EufyLbsjJM+Ui4abLiYP2iPuiRAulxl+l3+RVhUhUovfbRf7CEn6v7tOWN16NzJY/1ui8Z4DQGp2b9itOtAwNmJgXZu5ADInjad5dbzLVKPIeWaVrWz9fovHqXPCQP2pm/Y2YBFZuQwrfz19ZVrek0A5t+vuPtgbz2UkCqkGEioQ4ba2mOkoNqIOstWpBB2Sf+gbc+xp5RWoTBU1sc+oYvszKP3essSxOLEsoSeV3MhkGF7JzJ1UMWfTNY1zRw==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1ssUfW-0005Ps-7m for bug-gnu-emacs@gnu.org; Sun, 22 Sep 2024 18:03:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Xiyue Deng Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 22 Sep 2024 22:03:02 +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.172704252320691 (code B ref 72992); Sun, 22 Sep 2024 22:03:02 +0000 Original-Received: (at 72992) by debbugs.gnu.org; 22 Sep 2024 22:02:03 +0000 Original-Received: from localhost ([127.0.0.1]:42868 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ssUeY-0005Nc-Vp for submit@debbugs.gnu.org; Sun, 22 Sep 2024 18:02:03 -0400 Original-Received: from mail-pl1-f177.google.com ([209.85.214.177]:55339) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ssUeV-0005My-OX for 72992@debbugs.gnu.org; Sun, 22 Sep 2024 18:02:02 -0400 Original-Received: by mail-pl1-f177.google.com with SMTP id d9443c01a7336-20551e2f1f8so42914185ad.2 for <72992@debbugs.gnu.org>; Sun, 22 Sep 2024 15:01:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1727042432; x=1727647232; darn=debbugs.gnu.org; h=content-transfer-encoding:mime-version:message-id:date:user-agent :references:in-reply-to:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=nQ1BKqPuq58NZdQuHiYZf6Pq9eNhQ0dSBFI/fEM316Y=; b=Rb+jyzVa96APCYdQxUp4vPBCPlSj2kTimty5kPc2k510xHTpvgsEzEhQV0zRvE3QEd USG/Rx+FFoidGnl8fu/XUy4KWWcO3TaZd0pM3/H6ZpokqS3P9xbLM1m0/M0UoGrL80Dh PTmiMW0iX4vSXriOP9O3OGEF6W/OnLSZYs6cm74M3W4nOffg9dNa41GteIC+L9Soqemh OkO1RVdT4pzI0zz3N5E/D7Btf50rJLhgHXjCKHXWyYvvhAhoS6zGLg9iBCalAxJ1ZljK +fVXVdQPeMsszV65hM+X8D79roZUG55egyz3CI1xNNgnPEESFq6X7kG6VGnKbVpRCVaP Ld1Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727042432; x=1727647232; h=content-transfer-encoding:mime-version:message-id:date:user-agent :references:in-reply-to:subject:cc:to:from:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=nQ1BKqPuq58NZdQuHiYZf6Pq9eNhQ0dSBFI/fEM316Y=; b=BpfMgqXvuJNjWxemHs0z2jw4zqBXBEBlQUcAGca0UplWT+cMvZdYRErYxXb3+MquU4 dHaiM820p3zXMGC+n4q+kaMWYbtIbddoyDJt+ccoaJgIaRwBrH4DqPZhPKHQeOIHmrir fkXbXpqlNgY/CDIb/eYxr8Eej5KLYkitVbY2ty+ls3fcLH4UzZFVT+MKFl9u8TxROuG8 udpMLW28Ib0vsyoR0gRj17sczweg4QvZ+LdXQ8B+QDpDOdzkR1Rf7YwV1XFL13ynLQ6Q pvhITtRDXaYqpmYQz+IraC3EQZBzgHJRUT7hN/V8dcMZ4bRP1gi3ETWA3Eo8fpO6HKOi LaoA== X-Gm-Message-State: AOJu0Ywz70X6HgTXqhdsP4ERIHrbOHXMPxjJxQ2o1y8aIpPcXVnYhGYU v8cSTnyQliu+ruk0H5WfqZJ5LC83ed3vJbgWC3RO1ZWmLiMVdfOtjO0F6CIw X-Google-Smtp-Source: AGHT+IFqR1Hx0Ek4tm1KYwpPdrC0G2MF1L60gbxoxBFbFB2elX4TNcnj7fBBB4VkeJKqVTqMPT+3Aw== X-Received: by 2002:a17:902:da8c:b0:206:c486:4c4c with SMTP id d9443c01a7336-208d8415d4dmr161401515ad.57.1727042431321; Sun, 22 Sep 2024 15:00:31 -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-207945da67asm123446135ad.4.2024.09.22.15.00.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 22 Sep 2024 15:00:30 -0700 (PDT) In-Reply-To: <87tte89gmn.fsf@posteo.net> 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:292257 Archived-At: Philip Kaludercic writes: > 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 wi= th >>>> 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-sour= ce >>>> 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 t= his >>>>> 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 clien= t-secret >>>>>> redirect-uri state))) >>>>>> (auth-source-do-trivia "oauth2 token: %s" (pp-to-str= ing 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-t= oken) >>>>>> (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. > Ack. >>> >>> [...] >>> >>>>>> #'auth-source-xoauth2-plugin--search-backends)) >>>>> >>>>> I would recommend turning this into a global minor mode instead, so t= hat >>>>> 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/a= uth-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: > > 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: >=20=20 > ;; (use-package auth-source-xoauth2-plugin > -;; :config > +;; :custom > ;; (auth-source-xoauth2-plugin-mode t)) > Quick question: I wonder whether customizing the minor mode variable is preferred over toggling through the minor mode function? (I assume both will run the minor mode body.) > ;; After enabling, smtpmail should be supported. To enable this in Gnus > @@ -107,13 +107,13 @@ expected that `token_url', `client_id', `client_sec= ret', 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 toke= n...") > (let ((token (oauth2-auth-and-store > @@ -138,8 +138,7 @@ expected that `token_url', `client_id', `client_secre= t', and > res))) >=20=20 > (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.") >=20=20 > (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)) >=20=20 > (advice-remove #'auth-source-search-backends > #'auth-source-xoauth2-plugin--search-backends)) >=20=20 > +;;;###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) > >> >>>>>> >>>>>> (provide 'auth-source-xoauth2-plugin) >>>>>> >>>>>> ;;; auth-source-xoauth2-plugin.el ends here >>>>> >>>>> --=20 >>>>> Philip Kaludercic on siskin >>> >>> --=20 >>> Philip Kaludercic on siskin >> >> [1] https://gitlab.com/xiyueden/auth-source-xoauth2-plugin > > --=20 > Philip Kaludercic on siskin --=20 Xiyue Deng