mutt_oauth2: offer to return SASL method and string

[mjg]

In order to use oauth tokens for IMAP/POP/SMTP connections, one needs to wrap them in a way which depends on the method (bearer/xoauth2) and - possibly - the host and protocol. OAUTH2 aware clients typically know how to do that, but if you want to debug using a direct connection you need the wrapped token.

We compute the wrapped token anyways for --test, so offer to return it instead of the plain access token if --sasl is given.

Submitted upstream as https://gitlab.com/muttmua/mutt/-/merge_requests/179 but we know the uptake in contrib there.

[gahr]

I don't claim familiarity with this file, and I won't stand in the way of enhancements, but:

1. I don't like the first commit, it makes build_sasl_string less functional and more dependent on global state
2. I don't understand why the second commit is necessary, as opposed to, say, emit the SASL string in --debug or --verbose mode. Perhaps with something that allows you to grep, if you need to programmatically access the string from another string.

[mjg]

I don't claim familiarity with this file, and I won't stand in the way of enhancements, but:

1. I don't like the first commit, it makes build_sasl_string less functional and more dependent on global state

Yes. My observation was that build_sasl_string used global state already, and that the different call sites did similar things in different places. The refactoring puts this into one place and makes each call shorter. In hindsight, one may want to be flexible about "user" for the case of shared outlook mailboxes where you use one token for several "users", but I'm not 100% sure about what outlook wants yet.

2. I don't understand why the second commit is necessary, as opposed to, say, emit the SASL string in --debug or --verbose mode. Perhaps with something that allows you to grep, if you need to programmatically access the string from another string.

Well, so far that info is not output at all. That's why a second commit is necessary. If you think of that string as debug information then by all means put it there. I see it as the same type of "helper information" as the token itself - probably because I use it in a mailbox idle watcher script the same way as I let mbsync/msmtp use the token.

[gahr]

Yeah, I think I'd prefer if any extra info is dumped as debug info... otherwise we'll end up adding a switch for every little piece of output we want. Are you fine with i) reverting the first commit, and ii) adding the SASL string to debug info?

[gahr]

Yeah, I think I'd prefer if any extra info is dumped as debug info... otherwise we'll end up adding a switch for every little piece of output we want. Are you fine with i) reverting the first commit, and ii) adding the SASL string to debug info?

@mjg ping - are you willing to do the changes?

[mjg]

Yeah, I think I'd prefer if any extra info is dumped as debug info... otherwise we'll end up adding a switch for every little piece of output we want. Are you fine with i) reverting the first commit, and ii) adding the SASL string to debug info?

@mjg ping - are you willing to do the changes?

I got side-tracked, sorry. I'll look into it.

[gahr]

@mjg ping - are you willing to do the changes?

[mjg]

@mjg ping - are you willing to do the changes?

Yes, but after reviewing what I did and why I'm not sure which changes:

• --debug sends its output to stdout rather than stderr. So it's useless for parsing unless you specify --verbose, in which case you can match for the prefix and have to remove it afterwards. [connection debugging is sent to stderr by the library, but the script's own debug info goes to stdout.]
• The SASL string depends on protocol/port (for some providers), so we would anyways need an extra argument such as --protocol to be able to compute the string. (I"m not sure whether you suggest unconditional debug output of the SASL string, just saying it's not possible without a protocol.)
• Computing the string adds yet another call-site which (without refactoring) would call build_sasl_string() with global variables as arguments, but then still use the global registration(with other bits of registration and token passed as arguments. This is bad practice which should be resolved one way (pass in all used globals) or the other (do not pass in globals).
• I have to re-check what SASL string MS expects for shared mailboxes, i.e. where you access the mailbox of userB with credentails for userA. We may need an extra --user parameter for that.

I don't mind changing my own (working) setup, but I want the changes to be useful.

[gahr]

• I think I don't have a strong opinion on refactoring build_sasl_string, I'm fine with your version
• Let's forget about --verbose
• Your suggested --sasl would make the script dump the sasl string instead of the access token. I don't get this part. Why can't you just dump the sasl string after the access token on a new line?

[mjg]

• I think I don't have a strong opinion on refactoring build_sasl_string, I'm fine with your version
• Let's forget about --verbose
• Your suggested --sasl would make the script dump the sasl string instead of the access token. I don't get this part. Why can't you just dump the sasl string after the access token on a new line?

I guess the last two are connected: callers (I mean someone calling mutt_oauth2.py from a script) typically either need the access token (and build a SASL string themselves) or the SASL string. Outputting both would mean the caller has to skip the first line, which is doable of course. But when would both lines be useful? I see --sasl as a switch which determines the type/form of the output. (One can view it differently, of course.)

[gahr]

I think I get it now. Sorry it took so long. How about something like a --format arguments that could be raw (the default, the current behaviour), or sasl?

This way, if new methods of using/printing the token come up, we could support them more easily in an enum kind of choice.

[mjg]

So, here's a version which implements the formats as suggested. Two formats indeed, which I don't mind reducing to one if you prefer. Also, I checked shared mailboxes and implemented that. Let me try out for a day or two in RL, though. [Also rebased to upstream/main.]

[mjg]

Also, I've tested "live" with office365 shared mailboxes (imap), they do work as expected so far. Should I add docs to the README in contrib?

[flatcap]

Should I add docs to the README in contrib?

Yes please