bug#72992: 29.4; towards xoauth2 support in Emacs

[Xiyue Deng <manphiz@gmail.com>]

Hi Philip,

Philip Kaludercic <philipk@posteo.net> writes:

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

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.

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

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

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

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

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

-- 
Xiyue Deng