Re: mailgw argument parsing

On Wed, Nov 23, 2022 at 01:06:49PM -0500, John P. Rouillard wrote:
> Hi Ralf:
> 
> I just checked your commit. Sorry I didn't look at the branch you
> pushed.

I hadn't pushed a branch (mostly for fear of not being able to do this
with mercurial which is a separate issue :-)

> When I said I removed my objection, I meant it was to the
> removal of -C. But I also gave approval for the merge, which I
> shouldn't have. I can make the changes if you agree.

Sorry.

> So a few notes:
> 
> user_guide.txt has this change:
> 
> 
> @@ -217,7 +217,7 @@
>    <Range from 2003-01-01.00:00:00 to 2003-12-31.23:59:59>
>  - "2003-04" means
>    <Range from 2003-04-01.00:00:00 to 2003-04-30.23:59:59>
> -
> +

This is only a whitespace change in the last line. The '- "2003-04" means'
ways not removed, it is literal text there.

> Does the usage string returned by roundup-mailgw when the source is
> unrecognized need to be updated to include imaps_oauth?
>     return usage(argv, _('Error: The source must be either "mailbox",'
>                          ' "pop", "pops", "apop", "imap", "imaps" or'
>                          ' "imaps_cram'))

Yes. And the usage function is obsolete. Should be converted to just
returning a string and call to cmd.print_help(sys.stderr) as in another
instance. There is another call to usage which must also be fixed.

> When a user specifies imaps_oauth but doesn't have requests installed,
> I see a logger error. Does that go the user if run interactively and
> logging is set to a file in config.ini? If not, should we detect this
> failure and both log it as you have, but report to the user from
> roundup-mailgw?

Yes in my observation the logs go to the user, seen this work for other
errors. I have not tested uninstalling requests to see what happens.

> 
> Regarding the command line setting of oauth parameters (-IOT) Also it
> looks like -I is required and -T defaults to
> 'https://login.microsoftonline.com/'organizations/oauth2/v2.0/token if
> you are using imaps_oauth? Is this correct? If so where is the -T
> default and argument format documented.

Yes the default for -T is correct and -I is required.
The -T (token endpoint) is something you get from your cloud provider.
Since I implemented this for MS it's the default. For google (who also
offer OAuth) you'd need a different URL. I *think* that someone who has
jumpt through all the hoops to get the OAuth tokens, this should be
obious. And I *think* that a howto would be out of scope (and be very
cloud provider specific I don't want to feature a specific provider
here)

> Also I am not sure that's the right place to set those options.
> Specifying -I, -O, or -T don't turn on oauth. We don't have any
> protocol specific command line options otherwise.

Yes, I also thought about that. Maybe that should go into the
configuration (but see below we might change this per invocation)?
What do you suggest? On the other hand the mailgw is usually set up once
and then forgotten, so I thought it a good idea to keep in in one place.

> 
> If you used:
> 
>   roundup-mailgw -I ... imaps username:ignored_password@server

> that would be different in my view.

You mean to distinguish "normal" imaps from imaps_oauth by the -I
option? Would this also apply to imaps_cram?

> (As an aside, is pops with OAuth a thing?).

I think it exists. I've not bothered to implement it.

> As another example, adding a -P <password file> option to
> roundup-mailgw so the password isn't exposed on the cli would be a
> good multi-protocol cli option.

The roundup mailgw supports .netrc files. That's more or less a password
file indexed by the hostname (and also containing the username). I've
updated the mailgw docs to mention this when merging the info from the
docs with the mailgw help (it was only in the mailgw help).
I've been using that feature for years now, you don't want to have the
password in the crontab.

> 
> I wonder if modifying the imaps_oauth command line would be better.
> Rather than -I client_id and -O directory and -T endpoint something
> like:
> 
>   roundup-mailgw imaps_oauth username@client_id:server [mailbox] [https://path/to/oauth_endpoint] [oauth dir]

Hmm, this provides a number of positional arguments that I wanted to
avoid. I think that the mailgw already has too many of those. I'd rather
convert the mailbox to an option were it not for backward compatibility.

> where oauth dir is "relative_path" or "/absolute/path". I am flexible
> about the order of the optional params. Ideally we would put them in
> the order of most likely to be changed.

The suggestion of a relative vs. absolute path is a good idea.

> I agree it's a bit wierd for the client_id which is given to the oauth
> endpoint (IIUC) to be associated with the mailbox server. Any missing
> parameter is replaced with a '' or "" ("" only on windows) to make
> parsing easier.

As noted above I'd rather not introduce more positional args.

> An alternative idea is to put these options into the tracker's
> config.ini file under [mailgw]?

Yes I thought about that (see above) but the mailgw is usually a
configure-once, forget-about-it activity :-)

> The downside of this is that we are
> limited to only one oauth server, so maybe this is not a good idea.

Yes. The OAuth server might be specific to the issue-class we are
polling for. Probably not a very common use-case, though, maybe when
you're hosting a tracker for multiple parties, they might have their own
email. Or other use-cases where emails can come from multiple sources.

> In the test code I see changes froma tuple to an array:
> 
> -    def _create_mailgw(self, message, args=()):
> +    def _create_mailgw(self, message, args=[]):
> 
> Might I ask why? IIUC you want an immutable default value for a method
> argument.

Yes, should probably be changed back. I was under the impression that
argv needs to be a list but it *does* work with a tuple (we're testing
this now, the args are parsed by the same routine that is used in the
mailgw script).

> Thoughts? Again apologies for not looking at the code.
Sorry, as said above you didn't have the chance, I didn't push to a
branch.

> Also I pushed a reference to the requests library to the
> installation.txt doc for this feature.

Thanks, I had put this only into the mailgw docs.

So I see two changes to do
- Fix calls to usage in roundup_mailgw
- make args a tuple again

And further discuss positional args vs. options.

Thanks for the review!
Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [email protected]