Safe refresh

[jwoehr]

Context:
It's possible to save bad refresh tokens by accidentally including non-printable characters when copying them.

Description of the Change:
Settings.save() does a sanity check and raises on unprintables in the refresh token.

Benefits:
Less surprise and mystery on a user error in performing a token refresh

Possible Drawbacks:
This works, but though I wrote a test, I have not been able to make the test work, could use some help on test_settings.py test test_save_unprintable

Related GitHub Issues:

[Mandrenkov]

Thank you very much for the contribution, @jwoehr! 🎉 It's always appreciated when someone takes the time to improve a project based on their own experience with the software! 👍

I left several comments (mostly related to formatting and style) but overall this is quite nice!

[jwoehr]

@Mandrenkov I'm not sure the rest of the test test_save_unprintable() is correct.
The test is to make sure an exception is thrown on an unprintable character.
Does the test as modified really prove that this is so?

[Mandrenkov]

@Mandrenkov I'm not sure the rest of the test test_save_unprintable() is correct. The test is to make sure an exception is thrown on an unprintable character. Does the test as modified really prove that this is so?

Hi @jwoehr, I think it depends what the intention of the sanity check is. Consider:

settings = xcc.Settings(
            REFRESH_TOKEN="j.w.t\n", HOST="example.com", PORT=80, TLS=False
)

match = r"The REFRESH_TOKEN setting contains non-printable character(s)\."

# Check that a ValueError is thrown since "\n" is not printable.
with pytest.raises(ValueError, match=match):
    settings.save()

# Check that the .env file was not modified since there was a "\n" in the refresh token.
assert dotenv_values(env_file.name) == {
    "XANADU_CLOUD_REFRESH_TOKEN": "j.w.t",  # Note that the space at the end is deleted here.
    "XANADU_CLOUD_HOST": "example.com",
    "XANADU_CLOUD_PORT": "80",
    "XANADU_CLOUD_TLS": "False",
}

Here, we verify that the newline character is caught by the sanity check and the .env file is not modified. Of course, we can also expand the coverage of the test by parameterizing it:

@pytest.mark.parametrize("refresh_token", [chr(0x0a), chr(0x20)])

Note, however, that chr(0x0a) (i.e., a newline) is not printable while chr(0x20) (i.e., a space) is printable.

What do you think about increasing the strictness of the sanity check by matching the JWT format exactly?

[jwoehr]

What do you think about increasing the strictness of the sanity check by matching the JWT format exactly?

Yes, that sounds better than dealing with it piecemeal. Is there a quick reference on the format?
And are there other .env variables that should be sanity checked?
(And even so, should we leave this particular PR to focus just on sanity-checking REFRESH_TOKEN ?)

[Mandrenkov]

Thanks for the reply, @jwoehr!

Yes, that sounds better than dealing with it piecemeal.

🙌

Is there a quick reference on the format?

Yes! I would highly recommend taking a look at https://jwt.io/introduction (namely What is the JSON Web Token structure?). In a nutshell though, a JWT is three Base64-encoded strings concatenated with two periods (.).

And are there other .env variables that should be sanity checked? (And even so, should we leave this particular PR to focus just on sanity-checking REFRESH_TOKEN ?)

I think most of the settings in the .env file could benefit from additional validation (e.g., checking that PORT lies between 0 and 65535); however, REFRESH_TOKEN is the only field I would expect users to modify (and therefore would benefit the most from a sanity check). So let's keep this PR focused on just that field!

[jwoehr]

@Mandrenkov since the REFRESH_TOKEN is only manipulated by the user and by the affected code from the user side in its Base64Url form, then all that we have to check for is a character that's not legal in Base64URL, correct?

[jwoehr]

Given all of the above, a Base64URL value can be defined using the following regular expression:
^[A-Za-z0-9_-]+$
— Base64URL Guru

[jwoehr]

I had to add . (dot) to the set because it's used in JWT to separate sections.
But I think the code works now.

[jwoehr]

I can't make the post-check in the test_settings.py routine test_save_bad_Base64URL work so I've commented it out.
If it's in, I get this:

____________________________________________________________ TestSettings.test_save_bad_Base64URL ____________________________________________________________

self = <test_settings.TestSettings object at 0x7f70cda37250>, env_file = <tempfile._TemporaryFileWrapper object at 0x7f70cdad0910>

    def test_save_bad_Base64URL(self, env_file):
        """Tests that a ValueError will be raised
        if REFRESH_TOKEN contains a character outside the Base64URL with
        the addition that the '.' dot character is part of the token as a
        section separator.
        """
        settings = xcc.Settings(
            REFRESH_TOKEN="j.w.t\n", HOST="example.com", PORT=80, TLS=False
        )
        match = r"REFRESH_TOKEN contains non-Base64URL character\(s\)"
        with pytest.raises(ValueError, match=match):
            settings.save()
    
        # Check that the .env file was not modified since there was a "\n" in the refresh token.
>       assert dotenv_values(env_file.name) == {
            "XANADU_CLOUD_REFRESH_TOKEN": "j.w.t",
            "XANADU_CLOUD_HOST": "example.com",
            "XANADU_CLOUD_PORT": "80",
            "XANADU_CLOUD_TLS": "False",
        }
E       AssertionError: assert OrderedDict() == {'XANADU_CLOU...TLS': 'False'}
E         Right contains 4 more items:
E         {'XANADU_CLOUD_HOST': 'example.com',
E          'XANADU_CLOUD_PORT': '80',
E          'XANADU_CLOUD_REFRESH_TOKEN': 'j.w.t',
E          'XANADU_CLOUD_TLS': 'False'}
E         Use -v to get more diff

tests/test_settings.py:83: AssertionError
================================================================== short test summary info ===================================================================
FAILED tests/test_settings.py::TestSettings::test_save_bad_Base64URL - AssertionError: assert OrderedDict() == {'XANADU_CLOU...TLS': 'False'}

[Mandrenkov]

@Mandrenkov since the REFRESH_TOKEN is only manipulated by the user and by the affected code from the user side in its Base64Url form, then all that we have to check for is a character that's not legal in Base64URL, correct?

Yeah, almost! We also need to account for the . characters which delimit each part of the JWT.

I had to add . (dot) to the set because it's used in JWT to separate sections.
But I think the code works now.

Exactly!

I can't make the post-check in the test_settings.py routine test_save_bad_Base64URL work so I've commented it out. If it's in, I get this:

____________________________________________________________ TestSettings.test_save_bad_Base64URL ____________________________________________________________

self = <test_settings.TestSettings object at 0x7f70cda37250>, env_file = <tempfile._TemporaryFileWrapper object at 0x7f70cdad0910>

    def test_save_bad_Base64URL(self, env_file):
        """Tests that a ValueError will be raised
        if REFRESH_TOKEN contains a character outside the Base64URL with
        the addition that the '.' dot character is part of the token as a
        section separator.
        """
        settings = xcc.Settings(
            REFRESH_TOKEN="j.w.t\n", HOST="example.com", PORT=80, TLS=False
        )
        match = r"REFRESH_TOKEN contains non-Base64URL character\(s\)"
        with pytest.raises(ValueError, match=match):
            settings.save()
    
        # Check that the .env file was not modified since there was a "\n" in the refresh token.
>       assert dotenv_values(env_file.name) == {
            "XANADU_CLOUD_REFRESH_TOKEN": "j.w.t",
            "XANADU_CLOUD_HOST": "example.com",
            "XANADU_CLOUD_PORT": "80",
            "XANADU_CLOUD_TLS": "False",
        }
E       AssertionError: assert OrderedDict() == {'XANADU_CLOU...TLS': 'False'}
E         Right contains 4 more items:
E         {'XANADU_CLOUD_HOST': 'example.com',
E          'XANADU_CLOUD_PORT': '80',
E          'XANADU_CLOUD_REFRESH_TOKEN': 'j.w.t',
E          'XANADU_CLOUD_TLS': 'False'}
E         Use -v to get more diff

tests/test_settings.py:83: AssertionError
================================================================== short test summary info ===================================================================
FAILED tests/test_settings.py::TestSettings::test_save_bad_Base64URL - AssertionError: assert OrderedDict() == {'XANADU_CLOU...TLS': 'False'}

Ah, so if you take a look at the implementation of env_file, you will see that the fixture just configures the Settings class to use an empty, temporary file as the .env file. This is why dotenv_values(env_file.name) yields an empty dictionary in your test. What you probably want to use instead is the settings fixture which applies some default settings and commits them to disk.

[jwoehr]

Hmm now I get a different error in the test:

self = <test_settings.TestSettings object at 0x7f659d1e12a0>
settings = Settings(REFRESH_TOKEN='j.w.t\n', ACCESS_TOKEN=None, HOST='example.com', PORT=80, TLS=False)

    def test_save_bad_Base64URL(self, settings):
        """Tests that a ValueError will be raised
        if REFRESH_TOKEN contains a character outside the Base64URL with
        the addition that the '.' dot character is part of the token as a
        section separator.
        """
        settings.REFRESH_TOKEN = "j.w.t\n"
    
        match = r"REFRESH_TOKEN contains non-JWT character\(s\)"
        with pytest.raises(ValueError, match=match):
            settings.save()
    
        # Check that the .env file was not modified since there was a "\n" in the refresh token.
        assert dotenv_values(env_file.name) == {
            "XANADU_CLOUD_REFRESH_TOKEN": "j.w.t",
            "XANADU_CLOUD_HOST": "example.com",
            "XANADU_CLOUD_PORT": "80",
            "XANADU_CLOUD_TLS": "False",
>       }
E       AttributeError: 'function' object has no attribute 'name'

tests/test_settings.py:87: AttributeError

[Mandrenkov]

Hmm now I get a different error in the test:

self = <test_settings.TestSettings object at 0x7f659d1e12a0>
settings = Settings(REFRESH_TOKEN='j.w.t\n', ACCESS_TOKEN=None, HOST='example.com', PORT=80, TLS=False)

    def test_save_bad_Base64URL(self, settings):
        """Tests that a ValueError will be raised
        if REFRESH_TOKEN contains a character outside the Base64URL with
        the addition that the '.' dot character is part of the token as a
        section separator.
        """
        settings.REFRESH_TOKEN = "j.w.t\n"
    
        match = r"REFRESH_TOKEN contains non-JWT character\(s\)"
        with pytest.raises(ValueError, match=match):
            settings.save()
    
        # Check that the .env file was not modified since there was a "\n" in the refresh token.
        assert dotenv_values(env_file.name) == {
            "XANADU_CLOUD_REFRESH_TOKEN": "j.w.t",
            "XANADU_CLOUD_HOST": "example.com",
            "XANADU_CLOUD_PORT": "80",
            "XANADU_CLOUD_TLS": "False",
>       }
E       AttributeError: 'function' object has no attribute 'name'

tests/test_settings.py:87: AttributeError

Right! We're no longer using the env_file fixture and so we'll need to replace

assert dotenv_values(env_file.name) ...

with

assert dotenv_values(xcc.Settings.Config.env_file) ...

[jwoehr]

Excellent everything works now.

[Mandrenkov]

Looks like you still need to run make format lint. 🤔

[jwoehr]

Ah, I did run black but apparently the linter has settings other than default.

[jwoehr]

I fixed lint's complaints.
Opened a new PR to add a pyproject.toml file so developers running black get the project-mandated format.

[Mandrenkov]

Thanks, @jwoehr! I think this PR is ready to merge. 🟢