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. >> >> 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: