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: Xiyue Deng <manphiz <at> gmail.com> To: Philip Kaludercic <philipk <at> posteo.net> Cc: 72992 <at> debbugs.gnu.org Subject: bug#72992: 29.4; towards xoauth2 support in Emacs Date: Tue, 17 Sep 2024 23:24:05 -0700
Philip Kaludercic <philipk <at> posteo.net> writes: > 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? 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 <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". > 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.