bug#72992: 29.4; towards xoauth2 support in Emacs

[Philip Kaludercic <philipk@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 7600 bytes --]

Xiyue Deng <manphiz@gmail.com> writes:

> Hi Philip,
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Xiyue Deng <manphiz@gmail.com> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>> Xiyue Deng <manphiz@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:


[-- Attachment #2: Type: text/plain, Size: 2746 bytes --]

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)

[-- Attachment #3: Type: text/plain, Size: 296 bytes --]


>
>>>>>
>>>>> (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