bug#72992: 29.4; towards xoauth2 support in Emacs

[Philip Kaludercic <philipk@posteo.net>]

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.

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

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


[...]

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


[...]

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


[...]

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

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

Also, checkdoc complains about
`auth-source-xoauth2-plugin--search-backends's docstring.  I'd try to
address the issues it mentions.

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.

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

-- 
	Philip Kaludercic on siskin