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

From: Xiyue Deng <[email-masked]>
Date: Tue, 17 Sep 2024 23:24:05 -0700

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.

> 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.  I think
Stefan's reply gave some suggestions in this regard and I'll follow-up
in a reply there.

Meanwhile, it may still worth adding this package to ELPA to support
older Emacs versions if desired.

> [...]
>
>>
>> Thanks for reading, and any comments are appreciated.
>
> I'll be honest, I have no idea about OAuth, so my comments are just
> related to Elisp for now.  Hope that's OK!
>

Your comments are greatly helpful and much appreciated!  Please see my
replies below.

>> [1] https://gitlab.com/xiyueden/auth-source-xoauth2-plugin
>
>> ;; Copyright (C) 2024 Xiyue Deng <[email-masked]>
>>
>> ;; Author: Xiyue Deng <[email-masked]>
>> ;; Version: 0.1-git
>> ;; Package-Requires: ((emacs "28.1") (oauth2 "0.17"))
>>
>> ;; This file is not part of GNU Emacs.
>>
>> ;; GNU Emacs is free software: you can redistribute it and/or modify
>> ;; it under the terms of the GNU General Public License as published by
>> ;; the Free Software Foundation, either version 3 of the License, or
>> ;; (at your option) any later version.
>>
>> ;; GNU Emacs is distributed in the hope that it will be useful,
>> ;; but WITHOUT ANY WARRANTY; without even the implied warranty of
>> ;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> ;; GNU General Public License for more details.
>>
>> ;; You should have received a copy of the GNU General Public License
>> ;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
>>
>> ;;; Commentary:
>>
>> ;; This package enables support for xoauth2 authentication with
>> ;; auth-source.  To set up, please put this file in the `load-path' of
>> ;; Emacs, and add the following lines in your Emacs configuration:
>>
>> ;;     (require 'auth-source-xoauth2-plugin)
>> ;;     (auth-source-xoauth2-plugin-enable)
>>
>> ;; or with use-package:
>>
>> ;;     (use-package auth-source-xoauth2-plugin
>> ;;       :config
>> ;;       (auth-source-xoauth2-plugin-enable))
>>
>> ;; After enabling, smtpmail should be supported.  To enable this in Gnus
>> ;; nnimap, you should also set `(nnimap-authenticator xoauth2)' in the
>> ;; corresponding account settings in `gnus-secondary-select-methods'
>>
>> ;; auth-source uses the `secret' field in auth-source file as password
>> ;; for authentication, including xoauth2.  To decide which
>> ;; authentication method to use (e.g. plain password vs xoauth2), it
>> ;; inspects the `auth' field from the auth-source entry, and if the
>> ;; value is `xaouth2', it will try to gather data and get the access
>> ;; token for use of xoauth2 authentication; otherwise, it will fallback
>> ;; to the default authentication method.
>>
>> ;; When xoauth2 authentication is enabled, it will try to get the
>> ;; following data from the auth-source entry: `auth-url', `token-url',
>> ;; `scope', `client-id', `client-secret', `redirect-uri', and optionally
>> ;; `state'.  These information will be used by oauth2 to retrieve the
>> ;; access-token.  This package uses an advice to switch the auth-source
>> ;; search result from the `password' to the `access-token' it got, which
>> ;; in turn will be used to construct the xoauth2 authentication string,
>> ;; currently in nnimap-login and smtpmail-try-auth-method.  To really
>> ;; enable xoauth2 in smtpmail, it will add 'xoauth2 to
>> ;; 'smtpmail-auth-supported (if it is not already in the list) using
>> ;; `add-to-list' so that xoauth2 is tried first.
>>
>> ;; Note that currently the auth-source requires the searched entry must
>> ;; have `secret' field set in the entry, which is not necessary when
>> ;; using xoauth2.  Therefore in the advice it temporarily disables
>> ;; checking for `:secret' if set and perform the search, and check the
>> ;; result before returning.
>>
>> ;;; Code:
>>
>> (require 'auth-source)
>> (require 'cl-lib)
>> (require 'oauth2)
>> (require 'smtpmail)
>>
>> (defun auth-source-xoauth2-plugin--search-backends (orig-fun &rest args)
>>   "Perform auth-source-search and set password as access-token when requested.
>> The substitution only happens if one sets `auth' to `xoauth2' in
>> your auth-source-entry.  It is expected that `token_url',
>> `client_id', `client_secret', and `refresh_token' are properly
>> set along `host', `user', and `port' (note the snake_case)."
>>   (auth-source-do-trivia "Advising auth-source-search")
>>   (let (check-secret)
>>     (when (memq :secret (nth 5 args))
>>       (auth-source-do-trivia
>>        "Required fields include :secret.  As we are requesting access token to replace the secret, we'll temporary remove :secret from the require list and check that it's properly set to a valid access token later.")
>>       (setf (nth 5 args) (remove :secret (nth 5 args)))
>>       (setq check-secret t))
>>     (let ((orig-res (apply orig-fun args))
>>           res)
>>       (dolist (auth-data orig-res)
>>         (auth-source-do-trivia "Matched auth data: %s" (pp-to-string auth-data))
>
> Is "trivia" the right debug level for all the messages in this function?
> I feel like some of them should at least be "debug".
>

Indeed.  I have changed logs without token to be "debug"-level.

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

>>             (auth-source-do-debug
>>              ":auth set to `xoauth2'.  Will get access token.")
>>             (let ((auth-url (plist-get auth-data :auth-url))
>>                   (token-url (plist-get auth-data :token-url))
>>                   (scope (plist-get auth-data :scope))
>>                   (client-id (plist-get auth-data :client-id))
>>                   (client-secret (plist-get auth-data :client-secret))
>>                   (redirect-uri (plist-get auth-data :redirect-uri))
>>                   (state (plist-get auth-data :state)))
>
> I feel like this could be simplified with a map-let expression.  
>
>   (map-let (:auth-url :token-url ...) auth-data
>     ...)
>                                                                  
> You should be able to require 'map at compile time, so that the
> expansions don't occur an overhead during run-time.
>

Another nice TIL!

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

>>
>>         (unless (and check-secret
>>                      (not (plist-get auth-data :secret)))
>>           (auth-source-do-trivia "Updating auth-source-search results.")
>>           (add-to-list 'res auth-data t)))
>
> This should definitely be a `push', as `res' is lexically scoped (see
> docstring for `add-to-list').
>

Indeed.  Guess I got lucky here that it behaves the same.

>>       res)))
>>
>> ;;;###autoload
>> (defun auth-source-xoauth2-plugin-enable ()
>>   "Enable auth-source-xoauth2-plugin."
>>   (unless (memq 'xoauth2 smtpmail-auth-supported)
>>     (add-to-list 'smtpmail-auth-supported 'xoauth2))
>
> In functions, it would be more conventional to use push.  Especially
> because `add-to-list' checks for duplicates, which you have already
> done.
>

Ack.

>>
>>   (advice-add 'auth-source-search-backends :around
>
> There's no reason not to sharp-quote the function here as well?
>

Indeed.  Added.

>>               #'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.  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.

I have updated the source code on GitLab[1] based on your review.
Please check it out.  Thanks very much!

>>
>> (provide 'auth-source-xoauth2-plugin)
>>
>> ;;; auth-source-xoauth2-plugin.el ends here
>
> -- 
> 	Philip Kaludercic on siskin

-- 
Xiyue Deng