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

Added support for self-signed certificates #96

Closed
wants to merge 1 commit into from

Conversation

kwilsonmg
Copy link

Support added for self-signed certificates. This addresses issue #95. I have tested and confirmed working with the synchronous client.

Support added for self-signed certificates. This addresses issue 1Password#95
Comment on lines +397 to +404
if is_async and certificate:
return AsyncClient(url, token, certificate)
elif is_async:
return AsyncClient(url, token)
return Client(url, token)
elif certificate:
return Client(url, token, certificate)
else:
return Client(url, token)
Copy link
Contributor

Choose a reason for hiding this comment

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

As the constructor params starts to grow, to avoid growing the amount of the params more in the future, I think it would be good to pass clientConfig: ClientConfig instead of passing a specific certificate param.

So, the idea is that ClientConfig class will have the certificate: str param, and in the future we can easily extend it with more, if we need them. In addition to that, we can make ClientConfig inherit the httpx.Client class, so we will be able to pass all the properties that httpx.Client accepts, and not only certificate.

Does it make sense?

Copy link
Author

@kwilsonmg kwilsonmg Mar 9, 2024

Choose a reason for hiding this comment

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

@volodymyrZotov That does make sense. I just did a search of this repo and don't see a "ClientConfig". Would that would be a new class created? If so, what sorts of parameters would you see it take? Certificate, url, token?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that should be a new class. Ideally, having the same parameters as in httpx.Client and httpx.AsyncClient classes.

@volodymyrZotov
Copy link
Contributor

Created issue #99 to address this more broadly, so the client can be configured with different options.

@kwilsonmg I'll appreciate your input there. I close this PR.

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