Subject: bug#72992: 29.4; towards xoauth2 support in Emacs

From: Xiyue Deng <[email-masked]>
Date: Sun, 22 Sep 2024 15:00:29 -0700

Philip Kaludercic <[email-masked]> writes:

> Xiyue Deng <[email-masked]> writes:
>
>> Hi Philip,
>>
>> Philip Kaludercic <[email-masked]> writes:
>>
>>> Xiyue Deng <[email-masked]> writes:
>>>
>>>> Philip Kaludercic <[email-masked]> writes:
>>>>
>>>>> Xiyue Deng <[email-masked]> 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.
>

Ack.

>>>
>>> [...]
>>>
>>>>>>               #'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:
>
> 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))
>

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

-- 
Xiyue Deng