Skip to content
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

Removing SecretStr from RemoteRepository model #75

Merged
merged 3 commits into from
Dec 15, 2020
Merged

Removing SecretStr from RemoteRepository model #75

merged 3 commits into from
Dec 15, 2020

Conversation

ajrpeggio
Copy link
Contributor

SecretStr is not JSON serializable and is throwing exceptions for RemoteRepositories that require a password.

Description

I've been using this library to create repositories for my Artifactory Instance. I started adding a RemoteRepository for docker and came upon this error that states: "Object of type SecretStr is not JSON serializable"

To Reproduce

Steps to reproduce the behavior:

from pyartifactory import Artifactory
from pyartifactory.models import RemoteRepository

url = <url to artifactory instance>
auth = (<username>, <password>)
remote_docker_repo_cfg = {
    "key": "docker-remote",
    "url": "https://registry-1.docker.io/",
    "packageType": "docker",
    "username": <dockerhub username>,
    "password": <dockerhub_password>,
}
rt = Artifactory(url=url, auth=auth)
remote_repo = RemoteRepository(**remote_docker_repo_cfg)
rt.repositories.create_repo(remote_repo)

Fixes # #74

SecretStr is not JSON serializable and is throwing exceptions for RemoteRepositories that require a password.
@anancarv anancarv added this to To do in Kanban board Dec 3, 2020
@anancarv anancarv moved this from To do to In progress in Kanban board Dec 3, 2020
@anancarv
Copy link
Owner

anancarv commented Dec 3, 2020

Hi @ajrpeggio,
After running some tests. It appears that it's better to keep the SecretStr type in the RemoteRepository model. Indeed, changing it to str will decrease the level of security and your password will be able to be printed.

A better solution is t create a custom encoder in a utils.py file for example with the following content:

import json
from pydantic import SecretStr
from pydantic.json import pydantic_encoder

def custom_encoder(obj):
      if isinstance(obj, SecretStr):
          return obj.get_secret_value()
      else:
          return pydantic_encoder(obj)

Then in the objects.py file, in the file 466 you can replace:

self._put(f"api/{self._uri}/{repo_name}", json=repo.dict())

with

data= json.dumps(repo, default=custom_encoder)
self._put(f"api/{self._uri}/{repo_name}", headers={"Content-Type": "application/json"}, data=data)

Comment on lines 1 to 9
from pydantic import SecretStr
from pydantic.json import pydantic_encoder


def custom_encoder(obj):
if isinstance(obj, SecretStr):
return obj.get_secret_value()
else:
return pydantic_encoder(obj)
Copy link
Owner

@anancarv anancarv Dec 4, 2020

Choose a reason for hiding this comment

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

It seems that you haven't run the pre-commits (Please read the CONTRIBUTING.md file). If you had, you could have seen the following errors:

pyartifactory/utils.py:1:0: C0114: Missing module docstring (missing-module-docstring)
pyartifactory/utils.py:5:0: C0116: Missing function or method docstring (missing-function-docstring)
pyartifactory/utils.py:6:4: R1705: Unnecessary "else" after "return" (no-else-return)

Can be fixed with:

"""
Definition of all utils.
"""

from typing import Any

from pydantic import SecretStr
from pydantic.json import pydantic_encoder


def custom_encoder(obj: Any) -> Any:
    """
    Custom encoder function to be passed to the default argument of json.dumps()
    :param obj: A pydantic object
    :return: An encoded pydantic object
    """
    if isinstance(obj, SecretStr):
        return obj.get_secret_value()
    return pydantic_encoder(obj)

f"Repository {repo_name} already exists"
)
except RepositoryNotFoundException:
data= json.dumps(repo, default=custom_encoder)
Copy link
Owner

Choose a reason for hiding this comment

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

The pre-commits also highlight formatting issue in this line. Indeed data= should be data =

@anancarv
Copy link
Owner

anancarv commented Dec 4, 2020

Also, there are conflicts with the master branch. Indeed, a new release has been merged a few days ago. You will also need to fix the conflicts before merging. We're almost done. A few changes and we are good :)

@anancarv anancarv linked an issue Dec 4, 2020 that may be closed by this pull request
@anancarv
Copy link
Owner

anancarv commented Dec 10, 2020

Hi @ajrpeggio ,
Are you facing troubles with the changes requested ? Would you prefer me to resume it ?

Copy link
Collaborator

@nymous nymous left a comment

Choose a reason for hiding this comment

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

Let's go!

@anancarv anancarv merged commit a6dda91 into anancarv:master Dec 15, 2020
Kanban board automation moved this from In progress to Done Dec 15, 2020
@ajrpeggio
Copy link
Contributor Author

Apologies for getting back to this so late. Thank you for fixing these!

@anancarv
Copy link
Owner

No worries. You're welcome !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Kanban board
  
Done
Development

Successfully merging this pull request may close these issues.

RemoteRepository attr "password" is not JSON Serializable
3 participants