Fixes tfa_host incorrectly splitting scheme

[katzenbaer]

The current implementation returns https rather than the scheme + host

[katzenbaer]

It's weird. This change is required for me when I run locally on my M1 mac, but it doesn't fix the auth issues when I run inside a docker container.

[twrecked]

Can you update and see if this is still needed?

Your request made me realise the code was wrong in several places so I fixed them all and added some unit tests to make sure I keep handling this correctly.

[katzenbaer]

@twrecked Your changes wouldn't fix this issue because the Arlo2FARestAPI module still uses the tfa_host property which doesn't include the scheme.

https://github.com/twrecked/pyaarlo/blob/master/pyaarlo/tfa.py#L183C32-L183C40

Having a URL with a scheme is required for requests to work properly.

[katzenbaer]

@twrecked I just pushed a proposed change to this PR that would fix the issue. Although, I'm not sure implying https as the scheme is correct. It might be better to have a method to extract the scheme from self._kw.get("tfa_host", TFA_DEFAULT_HOST) and then restore it. Or just have a method that doesn't remove the scheme to begin with. But I'll leave that decision to you.

[twrecked]

I noticed this the other day as well but have been too busy to fix so thanks for the PR. One comment, in Python the _ at the front is a user/community convention to indicate the function is use internally but this isn't enforced by the interpreter. PyCharm will complain about using it.

Maybe a new function? tfa_host_with_scheme?

[twrecked]

Can you try the master again?

[katzenbaer]

Ah, that would work. I'm not sure if this is an edge-case you want to solve, but this test looks funny. I wouldn't imagine that it prepends https:// to a host specified with an IMAP port, but that's what it does from this test:

    def test_host_20(self):
        arlo = tests.arlo.PyArlo(tfa_host="imap.gmail.com:998")
        self.assertEqual(arlo.cfg.tfa_host, "imap.gmail.com")
        self.assertEqual(arlo.cfg.tfa_host_with_scheme, "https://imap.gmail.com")
        self.assertEqual(arlo.cfg.tfa_port, 998)

[katzenbaer]

But since tfa_host_with_scheme isn't used outside the 2FA REST API code, it might be okay.

[katzenbaer]

What about if tfa_host_with_scheme was a method instead with a scheme parameter? It would then pass this down to _add_scheme. I think that would make more sense since the caller knows the context (i.e. REST API) and can default it to https or imap or whatever.

I think inferring the scheme from ports could introduce more edge cases since some people might want to use port obfuscation.

[twrecked]

I think converting it to a method and adding the scheme is a good idea. And I'm always using odd ports so trying to pick the right scheme from the port might get complicated.

And the tests don't mean much I was just hoping it would do the right thing even if the results aren't meaningful.