Skip to content

Conversation

@azdolinski
Copy link
Contributor

@azdolinski azdolinski commented Dec 4, 2023

#16086
proposal for fix conversion python obj -> dict when SecretStr exist (ie. password field) inside the object

PR checklist

  • Read the contribution guidelines.

  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.

  • Run the following to build the project and update samples:

    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    

    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.

  • File the PR against the correct branch: master (upcoming 7.1.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)

  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@azdolinski azdolinski changed the title Fix problem in sanitize_for_serialization for Python (pydantic type SecretStr ) #16086 Fix problem in sanitize_for_serialization for Python (pydantic type SecretStr ) BUG#16086 Dec 4, 2023
@wing328
Copy link
Member

wing328 commented Dec 6, 2023

cc @cbornet (2017/09) @tomplus (2018/10) @krjakbrjak (2023/02) @fa0311 (2023/10) @multani (2023/10)

@wing328
Copy link
Member

wing328 commented Dec 6, 2023

can you please follow step 3 to update the samples so that CI can verify the change?

@multani
Copy link
Contributor

multani commented Dec 6, 2023

@azdolinski do you think you can also write a test for this issue?

@azdolinski
Copy link
Contributor Author

@wing328 -

can you please follow step 3 to update the samples so that CI can verify the change?

done

@azdolinski
Copy link
Contributor Author

@multani - unfortunately I don't have such skill and time to do that...
for my project - implementation of this change resolves the error:
Error: 'SecretStr' object has no attribute 'to_dict'
From now on - before "if" statement hits the last section which is doing obj.to_dict() on SecretStr - it stops on the section which recognizes and decodes SecretStr -> string.

        elif isinstance(obj, SecretStr):
            return obj.get_secret_value()

(with pydantic2 - passwords are mapped to SecretStr which causes a problem with decoding in serialization when we want to send this password as clear text)

2nd change is just a small tune and for safety in the case of object/class will not have to_dict() function (I'll try to use py3 build in __dict__)

           if hasattr(obj, 'to_dict') and callable(getattr(obj, 'to_dict')):
               obj_dict = obj.to_dict()
           else:
               obj_dict = obj.__dict__

@wing328
Copy link
Member

wing328 commented Dec 10, 2023

I've filed #17364 to add a test for properties using SecretStr but it seems to be working fine with sanitize_for_serialization.

Have you tested with the latest master to confirm it's still an issue?

@azdolinski
Copy link
Contributor Author

@wing328
in your test, you didn't use SecretStr so probably that is why it was working fine
I modified your tests where you first import from pydantic import SecretStr

and put password inside SecretStr password

p = SecretStr('secretpassword')
self.assertEquals(petstore_api.ApiClient().sanitize_for_serialization(a), {'byte': b'string', 'date': '2013-09-17', 'number': 123.45, 'password': p, 'pattern_with_digits_and_delimiter': 'image_123'})

@wing328
Copy link
Member

wing328 commented Dec 13, 2023

thanks for the explanation and merging the test into your branch.

the CI tests failed: https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/27086/workflows/740b360d-6115-483e-9db5-2192ab97cecf/jobs/85120

can you please take a look?

if hasattr(obj, 'to_dict') and callable(getattr(obj, 'to_dict')):
obj_dict = obj.to_dict()
else:
obj_dict = obj.__dict__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this is fixing another issue, right?

is there a minimal spec to reproduce the issue? sorry I might have missed it

@azdolinski
Copy link
Contributor Author

the CI tests failed: https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/27086/workflows/740b360d-6115-483e-9db5-2192ab97cecf/jobs/85120

can you please take a look?

@wing328
As this demo is generated based on petstore.swagger - it doesn't contain a good example/model to test SecretStr.
password field becomes SecretStr only in case when it is properly marked based on the 'format' attribute:

- "password":{"type":"string"}
+ "password":{"type":"string","format":"password"}

I added in the test file, a dedicated class which pretends the password as SecretStr so now it passes the test

        # test sanitize for serializaation with SecretStr (format: password)
        from typing import  Optional
        from pydantic import BaseModel, SecretStr, StrictStr, Field
        from typing_extensions import Annotated
        class LoginTest(BaseModel):
            username: StrictStr = Field(min_length=2, strict=True, max_length=64)
            password: SecretStr
            
        l = LoginTest(username="admin", password="testing09876")
        self.assertEquals(petstore_api.ApiClient().sanitize_for_serialization(l), {'username': "admin", 'password': "testing09876"})

@wing328 wing328 removed this from the 7.2.0 milestone Dec 22, 2023
@wing328
Copy link
Member

wing328 commented Feb 28, 2024

is this still an issue with the latest master or stable version?

if yes, can you please resolve the merge conflicts and we will take another look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants