Skip to content

Azure Key Vault: Don't load disabled secret #578

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 7, 2025

Conversation

AndreuCodina
Copy link
Contributor

No description provided.

@hramezani
Copy link
Member

Thanks @AndreuCodina for the PR.

Could you please add a test for enabled=False?

@AndreuCodina
Copy link
Contributor Author

Thanks @AndreuCodina for the PR.

Could you please add a test for enabled=False?

If I don't mock .get_secret, I never get a value in settings, and if I mock it, I always get the mocked value. I don't know how it can be tested.

@hramezani
Copy link
Member

Thanks @AndreuCodina for the PR.
Could you please add a test for enabled=False?

If I don't mock .get_secret, I never get a value in settings, and if I mock it, I always get the mocked value. I don't know how it can be tested.

You are mocking list_properties_of_secrets return values like:

        expected_secrets = [
            type('', (), {'name': 'SqlServerUser', 'enabled': True}),
            type('', (), {'name': 'SqlServer--Password', 'enabled': True}),
        ]

You can add a new test and change the enabled value to False, then we expect a validation error because the value is not enabled. right?

@AndreuCodina
Copy link
Contributor Author

Thanks @AndreuCodina for the PR.
Could you please add a test for enabled=False?

If I don't mock .get_secret, I never get a value in settings, and if I mock it, I always get the mocked value. I don't know how it can be tested.

You are mocking list_properties_of_secrets return values like:

        expected_secrets = [
            type('', (), {'name': 'SqlServerUser', 'enabled': True}),
            type('', (), {'name': 'SqlServer--Password', 'enabled': True}),
        ]

You can add a new test and change the enabled value to False, then we expect a validation error because the value is not enabled. right?

I tried that, but I get this:

{'SqlServerPassword': 'SecretValue', 'DisabledSqlServerPassword': 'SecretValue'}

This is the test:

    def test_do_not_load_disabled_secrets(self, mocker: MockerFixture) -> None:
        class AzureKeyVaultSettings(BaseSettings):
            """AzureKeyVault settings."""

            SqlServerPassword: str
            DisabledSqlServerPassword: str

        expected_secrets = [
            type('', (), {'name': 'SqlServerPassword', 'enabled': True}),
            type('', (), {'name': 'DisabledSqlServerPassword', 'enabled': False}),
        ]
        mocker.patch(
            f'{AzureKeyVaultSettingsSource.__module__}.{SecretClient.list_properties_of_secrets.__qualname__}',
            return_value=expected_secrets,
        )
        mocker.patch(
            f'{AzureKeyVaultSettingsSource.__module__}.{SecretClient.get_secret.__qualname__}',
            return_value=KeyVaultSecret(SecretProperties(), 'SecretValue'),
        )
        obj = AzureKeyVaultSettingsSource(
            AzureKeyVaultSettings, 'https://my-resource.vault.azure.net/', DefaultAzureCredential()
        )

        settings = obj()

        assert 'SqlServerPassword' in settings
        assert 'DisabledSqlServerPassword' not in settings

@hramezani
Copy link
Member

You need to change

if key not in self._loaded_secrets:

to if key not in self._loaded_secrets and key in self._secret_names:

@hramezani
Copy link
Member

Thanks @AndreuCodina

@hramezani hramezani merged commit d54d146 into pydantic:main Apr 7, 2025
18 checks passed
@AndreuCodina AndreuCodina deleted the bugfix/read-disabled-secret branch April 7, 2025 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants