Package: emacs;
Reported by: Xiyue Deng <manphiz <at> gmail.com>
Date: Tue, 3 Sep 2024 00:00:02 UTC
Severity: wishlist
Found in version 29.4
Message #44 received at 72992 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: Xiyue Deng <manphiz <at> gmail.com> Cc: 72992 <at> debbugs.gnu.org Subject: Re: bug#72992: 29.4; towards xoauth2 support in Emacs Date: Sun, 22 Sep 2024 09:34:08 +0000
[Message part 1 (text/plain, inline)]
Xiyue Deng <manphiz <at> gmail.com> writes: > Hi Philip, > > Philip Kaludercic <philipk <at> posteo.net> writes: > >> Xiyue Deng <manphiz <at> gmail.com> writes: >> >>> Philip Kaludercic <philipk <at> posteo.net> writes: >>> >>>> Xiyue Deng <manphiz <at> gmail.com> 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. >> >> 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. >> >> 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= auth "xoauth2")) >>>> >>>> You can simplify the check by just doing (equal auth "xoauth2"), as this >>>> 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= 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-string 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-token) >>>>> (plist-put auth-data :secret access-token)))))) >>>> >>>> The documentation for plist-put warns: >>>> >>>> The new plist is returned; >>>> use ‘(setq x (plist-put x prop val))’ 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 that >>>> 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. >> >> 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/auth-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:
[Message part 2 (text/plain, 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)
[Message part 3 (text/plain, inline)]
> >>>>> >>>>> (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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.