Re: [Nmh-workers] Big patch: Add XOAUTH2 support for SMTP and POP

[David Levine]

Eric wrote:

> OAuth is a very modern thing to bring into nmh.

Wow, cool, thanks!

> I enable a libcurl dependency only when configured with
> --with-oauth which is off by default.  But practically no one has
> jsmn installed, so I'm suggesting we include it directly.
> I think that might be unprecedented for nmh.  But I hope it's not
> too controversial.

I think that's fine.

> - mhlogin name / flag names
>
>   Naming is hard :).  I picked this on the theory that it's not
>   terribly confusing as is, and if there were to be some other
>   kind of system users might need to login to, expanding mhlogin
>   to have more than just -oauth would make sense.

I like it.

> - Repeating -user for each command is possibly odd.  Maybe put
>   -user on mhlogin and save it in the cred file.  Arguably easier
>   -for the user this way, arguably not.  Changing it would
>   -complicate the code slightly.  I don't really care either way.

Would it be easier with multiple accounts to use -user everywhere?
I'm thinking shell aliases/scripts.  It could also be done with
the cred file and $MH, but I think that's marginally messier.

> - I have a lot of test cases in only a few broadly categorized
>   test scripts, and they print descriptions as they go so it's
>   easy to see what broke.  This messes up the test suite output.
>   Does this make sense, should I change this only to print only
>   if some environment variable is set,

Or just if something goes wrong?

test-mhlogin failed for me because each line of my actual output
ends with a Ctrl-M.  On Linux, so I don't know why.

Any idea with test-inc and test-msgcheck would fail with:

    'inc: POP server does not support SASL'
    'msgchk: POP server does not support SASL'

I did build with sasl, of course, and confirmed with mhparam.

> Of course, I welcome criticism on all other aspects too:  API,
> documentation, organization, whatever.

I didn't dig in to any of that, but just noticed when applying the
patch:

warning: squelched 5 whitespace errors
warning: 10 lines add whitespace errors.

You chose not to use Cyrus SASL for XOAUTH2.  I wouldn't
hesitate to use it:  nmh already can be configured Cyrus SASL
and some of us do use it.  It is configured in by the Fedora
package.

To do:

* add "mhparam oauth" support
* add libcurl and libcurl-devel (on Linux) to MACHINES
* add reference to jsmn LICENSE to COPYRIGHT

David