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 #41 received at 72992 <at> debbugs.gnu.org (full text, mbox):
From: Xiyue Deng <manphiz <at> gmail.com> To: Philip Kaludercic <philipk <at> posteo.net> Cc: 72992 <at> debbugs.gnu.org Subject: Re: bug#72992: 29.4; towards xoauth2 support in Emacs Date: Sun, 22 Sep 2024 00:06:29 -0700
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. >> 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. > > [...] > >>>> (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. > > [...] > >>>> #'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. >> 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! >>>> >>>> (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 -- Xiyue Deng
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.