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
View this message in rfc822 format
From: Philip Kaludercic <philipk <at> posteo.net> To: Xiyue Deng <manphiz <at> gmail.com> Cc: 72992 <at> debbugs.gnu.org Subject: bug#72992: 29.4; towards xoauth2 support in Emacs Date: Tue, 17 Sep 2024 19:12:57 +0000
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? You are proposing to add this to GNU ELPA?
[...]
>
> 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!
> [1] https://gitlab.com/xiyueden/auth-source-xoauth2-plugin
> ;; Copyright (C) 2024 Xiyue Deng <manphiz <at> gmail.com>
>
> ;; Author: Xiyue Deng <manphiz <at> gmail.com>
> ;; 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".
> (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).
> (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.
> (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)
>
> (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').
> 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.
>
> (advice-add 'auth-source-search-backends :around
There's no reason not to sharp-quote the function here as well?
> #'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.
>
> (provide 'auth-source-xoauth2-plugin)
>
> ;;; auth-source-xoauth2-plugin.el ends here
--
Philip Kaludercic on siskin
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.