Improving mutt_oauth2.py

[flatcap]

What can we do to improve mutt_oauth2.py?

[holmanb]

I'll summarize ideas that have been floated in recent pull requests:

1. Unit tests (this would probably add a test-time dependancy such as pytest)
2. CI - depends on 1.
3. the name mutt_oauth2.py seems redundant / unnecessary, what about oauth2.py or similar? I don't feel strongly about this, but figured it's worth a mention.
4. Refactor:

• subcommands: this script has multiple roles (registration, test, connection)
• make it generally more maintainable & readable

5. Input validation
6. Debugging: the script currently has various failure modes: tokens expired, no token file, etc. A simple solution would be to add logging to a file. Alternative solutions?

I think it might make sense to order this work as:

{subcommands,name change} -> unit tests -> {refactor,debug logging,input validation} -> end-to-end tests??

Changing the user interface should probably be done prior to the next release (eta?), and unit tests would be nice guardrails prior to an internal refactor.

Thoughts?

edit 1: added bullet points 5 and 6

[the-x-at]

Thanks for kicking this off. The script is rather short, some 450 lines of code, and not too hard to understand. Reorganizing the command line sounds like the most important thing to do. Using sub-commands appears as a very reasonable way to make this script more intuitive to work with.
As for testing, I feel there could arise some difficulties, as it involves interactive steps. At some point, one has to copy a link that is output from the script to the browser. Testing also needs active Microsoft/Google Mail accounts to check the script's full functionality. These accounts would be open to the public and could possibly be hijacked by spammers or other abusers.
A full refactoring of the code seems to be equal to a rewrite. Is it worth the effort?
Renaming the script is a good idea as people would do that, too - it's already called oauth2.py in resides in ~/.mutt on my machine.

[holmanb]

@the-x-at Thanks for the feedback.

As for testing, I feel there could arise some difficulties, as it involves interactive steps.

Agreed, end-to-end testing would be tricky. In my experience signal to noise ratio typically goes down with these types of tests. I consider end-to-end testing a lower priority.

These accounts would be open to the public and could possibly be hijacked by spammers or other abusers.

I suspect Github Actions' encrypted secrets could maybe help us on the security front. I've put zero effort into evaluating this as an option - I may be wrong. This discussion can probably wait, since I think end-to-end is the lowest priority imo.

At some point, one has to copy a link that is output from the script to the browser. Testing also needs active Microsoft/Google Mail accounts to check the script's full functionality.

For simplicity we could add unit tests that exclude the interactive bits. I'd like to see tests that verify that for a collection of cli args the script produces expected outputs (links that the users copy/paste, etc). This approach would help our confidence that 1. this code produces the same outputs across a range of different python versions as supported python versions change over time and 2. future changes (such as refactoring, adding new providers, etc) don't break existing functionality.

Is it worth the effort?

I haven't seen any positive statements about current state besides "it works". I suspect this would be an easy win. As you said, it's only a few hundred lines of code. If there's a preference to just leave it be and not touch it since it isn't broken, I'm fine with that too. I don't feel strongly about this one.

note: I'm going to add another item to my original list above regarding input validation and debugging.

[the-x-at]

Sounds like a reasonable approach. Quickly browsed the section on encrypted secrets and found:

• To create secrets for a personal account repository, you must be the repository owner. To create secrets for an organization repository, you must have admin access.
• Note: With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

The first bullet point indicates that @flatcap needs to get involved here as the repo owner. The second point makes forking and testing on a fork a bit more complicated, as forkers would need to implement their own secrets (and possibly obtain them from the main repo through the maintainer). I'm not at all a forking expert, but could this severely hamper forking the project? Unless fixed with new secrets, the tests would not run on the fork. Maybe turn off the OAUTH2 testing in forks? Just a warning here.

What I still see inevitable is to create test mail accounts. We need to see if the communication with the OAUTH2 server infrastructure is working as expected. As suggested by the encrypted secrets section, minimal access should be granted by these accounts. I could try to get one on the M$ side, as I have done that already for work. Hope it also works for free accounts.

Logging sounds good, the script is potentially called many times over a day. Could we write to the system log? This however introduces OS dependencies. Otherwise, one needs to take care that the log file can be created and written to and does not get too big as most likely users tend to forget about them.

[holmanb]

Logging sounds good, the script is potentially called many times over a day. Could we write to the system log? This however introduces OS dependencies.

We could, but I don't see the benefit. I was thinking this would be a flag (--debug-file <path> or whatever) that user enables temporarily if they are having issues. I can imagine ways to do this in non-os-dependent ways if really needed (pipe to a user-configured subprocess), but I'm not convinced it's worth it.

Otherwise, one needs to take care that the log file can be created and written to and does not get too big as most likely users tend to forget about them.

These logs probably don't matter unless the user is having issues, right? I don't think this is an issue to worry about for a temporarily-enabled debug flag.

+1 to everything else above. Again, I'm more inclined to add some mocked unittests (in preference of simplicity) before working out end to end test details, but you've shared some good thoughts that will be helpful.

[flatcap]

Has anyone heard of python-o365
See: #3521

[holmanb]

I haven't. It looks maintained and popular though.

[the-x-at]

Me neither. Quick thought when reading their home page: Solid. Maintained as @holmanb has already said. Drags in one additional library. Does not handle Google authentication (already by name *g*).

[dkwo]

maybe fix #3550 ?