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.

Sign up for GitHub

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
Closed

Added support for self-signed certificates #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/onepasswordconnectsdk/async_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,16 @@
class AsyncClient:
"""Python Async Client Class"""

def __init__(self, url: str, token: str) -> None:
def __init__(self, url: str, token: str, cert: str = None) -> None:
"""Initialize async client"""
self.url = url
self.token = token
self.session = self.create_session(url, token)
self.session = self.create_session(url, token, cert)
self.serializer = Serializer()

def create_session(self, url: str, token: str) -> httpx.AsyncClient:
def create_session(self, url: str, token: str, cert: str = None) -> httpx.AsyncClient:
if cert:
return httpx.AsyncClient(base_url=url, headers=self.build_headers(token), verify=cert)
return httpx.AsyncClient(base_url=url, headers=self.build_headers(token))

def build_headers(self, token: str) -> Dict[str, str]:
Expand Down
20 changes: 14 additions & 6 deletions src/onepasswordconnectsdk/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,16 @@
class Client:
"""Python Client Class"""

def __init__(self, url: str, token: str) -> None:
def __init__(self, url: str, token: str, cert: str = None) -> None:
"""Initialize client"""
self.url = url
self.token = token
self.session = self.create_session(url, token)
self.session = self.create_session(url, token, cert)
self.serializer = Serializer()

def create_session(self, url: str, token: str) -> httpx.Client:
def create_session(self, url: str, token: str, cert: str = None) -> httpx.Client:
if cert:
return httpx.Client(base_url=url, headers=self.build_headers(token), verify=cert)
return httpx.Client(base_url=url, headers=self.build_headers(token))

def build_headers(self, token: str) -> Dict[str, str]:
Expand Down Expand Up @@ -381,19 +383,25 @@ def sanitize_for_serialization(self, obj):
return self.serializer.sanitize_for_serialization(obj)


def new_client(url: str, token: str, is_async: bool = False) -> Union[AsyncClient, Client]:
def new_client(url: str, token: str, is_async: bool = False, certificate: str = None) -> Union[AsyncClient, Client]:
"""Builds a new client for interacting with 1Password Connect
Parameters:
url: The url of the 1Password Connect API
token: The 1Password Service Account token
is_async: Initialize async or sync client
certificate: Optional path to custom certificate bundle for verification

Returns:
Client: The 1Password Connect client
"""
if is_async:
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)
Comment on lines +397 to +404
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 •
edited
Loading

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.



def new_client_from_environment(url: str = None) -> Union[AsyncClient, Client]:
Expand Down